-
Notifications
You must be signed in to change notification settings - Fork 29.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib: finish assigning codes to errors thrown from JS #19373
Conversation
lib/net.js
Outdated
@@ -934,6 +935,7 @@ function internalConnect( | |||
localAddress = localAddress || '::'; | |||
err = self._handle.bind6(localAddress, localPort); | |||
} else { | |||
// eslint-disable-next-line no-restricted-syntax | |||
self.destroy(new TypeError('Invalid addressType: ' + addressType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error doesn't have a code but I don't know what to do with it: Assign a code? Which one? How can we reach this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can provide a custom lookup function to net.connect
that calls the callback with an invalid addressType
to trigger this. Something like
net.connect({
host: 'foo.bar'
port: 80,
lookup: (host, dnsopts, cb) => cb(null, '127.0.0.1', 100) // 100 is not a valid addressType
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we probably need to invent a error code for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I guess there is something broken to fix first, because this is what I get with your example:
$ node test/parallel/test-net-connect-options-invalid-address-type.js
node[15295]: ../src/tcp_wrap.cc:282:static void node::TCPWrap::Connect(const v8::FunctionCallbackInfo<v8::Value>&): Assertion `args[2]->IsUint32()' failed.
1: node::Abort() [node]
2: 0x8cebdb [node]
3: node::TCPWrap::Connect(v8::FunctionCallbackInfo<v8::Value> const&) [node]
4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
5: 0xb1444c [node]
6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
7: 0x1f18982042fd
[1] 15295 abort (core dumped) node test/parallel/test-net-connect-options-invalid-address-type.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens because localAddress
and localPort
are undefined
so we end up in another path:
https://github.com/targos/node/blob/244cdb1e033706756c0296d344cca5702dea261b/lib/net.js#L965-L971
I added a code for the net.connect error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
lib/.eslintrc.yaml
Outdated
no-restricted-syntax: | ||
- error | ||
- selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])" | ||
message: "Use an error exported by the internal/errors module" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a doubt now: does this override the no-restricted-syntax
rule from our main config? If so, can we merge them somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, it will. When looking at this I did not see that it was only for ./lib. Activating it for the hole code base is probably not the best idea though. We could of course copy the rules that are currently in the main file but that is also not ideal...
Is there any possibility to combine this rule? That would be really great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@not-an-aardvark Do you have a suggestion? We would like to add a new restricted syntax to lib
while not overriding the other ones configured in the main config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, there's not a good way to do that at the moment. As a workaround, you could copy-paste the configuration from the parent directory, add the new entry, and put the resulting config in lib/.eslintrc.yaml
. This would have the desired effect, but it could cause the two configs to get out of sync if someone updates .eslintrc.yaml
later on and forgets to update the copied version in lib/.eslintrc.yaml
.
There is some discussion on improving this in eslint/eslint#9192.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I copied the config and put a comment explaining that it should be kept in sync.
@@ -423,6 +423,9 @@ function tryCreateBuffer(size, fd, isUserFd) { | |||
var threw = true; | |||
var buffer; | |||
try { | |||
if (size > kMaxLength) { | |||
throw new ERR_FS_FILE_TOO_LARGE(size); | |||
} | |||
buffer = Buffer.allocUnsafe(size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we now check for kMaxLength
: is it still possible for the alloc to throw an error? If not, then the try / catch
should be obsolete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can still throw if there is not enough memory available to construct the buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, I thought it might crash in that case but it will actually throw Array buffer allocation failed
.
blocking until #19415 lands because I think it will affect the 3rd commit |
#19415 landed, so I removed the |
615de82
to
4c9274d
Compare
doc/api/errors.md
Outdated
@@ -1301,6 +1307,11 @@ multiple of the element size. | |||
While calling `napi_create_typedarray()`, `(length * size_of_element) + | |||
byte_offset` was larger than the length of given `buffer`. | |||
|
|||
<a id="ERR_NET_INVALID_ADDRESS_TYPE"></a> | |||
### ERR_NET_INVALID_ADDRESS_TYPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an existing ERR_INVALID_ADDRESS_FAMILY
that should serve the same purpose, if we want to create an error specific to the DNS lookup function, can we use another name specific to that to avoid confusion? For example, ERR_NET_INVALID_LOOKUP
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks. I meant to change this after #19415.
After this commit, all errors thrown from JS code in lib have an error code.
This adds a rule that forbids the use of native Error constructors in the `lib` directory. This is to encourage use of the `internal/errors` mechanism. The rule is disabled for errors that are not created with the `internal/errors` module but are still assigned an error code.
4c9274d
to
957c493
Compare
@@ -935,7 +936,7 @@ function internalConnect( | |||
localAddress = localAddress || '::'; | |||
err = self._handle.bind6(localAddress, localPort); | |||
} else { | |||
self.destroy(new TypeError('Invalid addressType: ' + addressType)); | |||
self.destroy(new ERR_INVALID_ADDRESS_FAMILY(addressType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unreachable now, so I removed the test. I'm going to replace it by a CHECK when #19503 is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -287,9 +288,9 @@ function masterProcessMain() { | |||
|
|||
cluster.setupMaster(clusterSettings); | |||
|
|||
assert.throws(() => { | |||
common.expectsError(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a recommendation for the future: assert.throws
is actually becoming more and more powerful and using it is probably better than using common.expectsError
(even though that is still useful as a callback function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should I do to make it work with assert.throws
?
This is what I get:
AssertionError [ERR_ASSERTION]: type: expected [Function: RangeError], not undefined
at masterProcessMain (/home/mzasso/git/nodejs/node/test/sequential/test-inspector-port-cluster.js:238:16)
at Object.<anonymous> (/home/mzasso/git/nodejs/node/test/sequential/test-inspector-port-cluster.js:347:3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now there are some special handled properties in common.expectsError
that do not exist in assert.throws
. One is the message
property: it is either a strict equal comparison or you can use a regular expression. Using regular expressions is not (yet) possible (I am still considering adding options to accept those as well) with assert.throws
.
The other difference is the type
property: it will actually check for the specific error type. That is not possible at all with assert.throws
and instead you can check for the name
property.
After this commit, all errors thrown from JS code in lib have an error code. PR-URL: #19373 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
This adds a rule that forbids the use of native Error constructors in the `lib` directory. This is to encourage use of the `internal/errors` mechanism. The rule is disabled for errors that are not created with the `internal/errors` module but are still assigned an error code. PR-URL: #19373 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in fddcd62...6a9f049 |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes