Skip to content
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

errors: improve error types #13857

Closed
wants to merge 5 commits into from
Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jun 21, 2017

While working on the internal errors a bit more I realized that there are multiple error types not yet documented and some have minor shortcomings. Here I mainly just document the missing ones. All changes are summarized in the commit messages.

I feel the change to always rely on util.format for the arguments passed to error types might actually be the wrong way and instead we should always use a function. The latter would likely be a tad faster and we could also validate the args in that case. Relying on util.format is nicer to read though and shorter.

Besides that I want to kick of a short discussion about the error types in general. I know I'm late to the party but maybe my two cents are still worth the effort:
For me the error codes are most useful when a bit more abstract and if they cover a range of more specific errors and are not bound to exactly one error (with exceptions) (i.e. a single bound error type with a optional lower and upper bound instead of a individual bounds error each time).
That way the user can handle some generic ones in his code if wanted and necessary. I think handling lots of very specific ones is often not doable or as useful.
So if there's still a possibility to do this, I would say let's try to always find more generic approaches and go so far to consolidate a few already existing error codes. As some were not yet documented, it's easier with them but all others would have to be changed as semver major and I'm not sure if it's worth the effort in that case.
Here's my view on what could still be improved:

  • ERR_INVALID_FD not yet documented and this could easily be a more generic one
  • ERR_INVALID_CALLBACK could be replaced with ERR_INVALID_ARG_TYPE. That essentially does the same, just not as specific to callbacks. If you want to outline the callbacks, it should stay as it is.
  • ERR_INVALID_REPL_EVAL_CONFIG could be generic with something similar to: "Receive two types that may not be set at the same time."
  • ERR_STDERR_CLOSE and ERR_STDOUT_CLOSE could be consolidated to ERR_STD_CLOSE
  • ERR_UNKNOWN_STREAM_TYPE and ERR_STDIN_STREAM_TYPE could be consolidated to ERR_UNKNOWN_UV_TYPE
  • ERR_SOCKET_BAD_PORT I think it would be better to use a generic range / bound error instead
  • ERR_SOCKET_BAD_TYPE could be converted to a more generic approach with predefined allowed entries.

Besides that it would recommend to try to limit adding new error codes to the bare minimum.

Summoning @jasnell

If all or some of my suggestions find acclaim, I'd open another PR to address these points.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

errors,dgram,process,util,v8,http

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jun 21, 2017
@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 21, 2017
@mscdex
Copy link
Contributor

mscdex commented Jun 21, 2017

Marking as semver-major due to error message change(s).

E('ERR_TRANSFORM_WITH_LENGTH_0',
'Calling transform done when ws.length != 0');
E('ERR_HTTP_TRAILER_INVALID',
'Trailers are invalid with this transfer encoding');
E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing this, perhaps it should instead be used to replace the manual/old Error creation in lib/internal/bootstrap_node.js ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do that because of the comment right above that error:

// Model the error off the internal/errors.js model, but
// do not use that module given that it could actually be
// the one causing the error if there's a bug in Node.js

Used when a callback is called more than once.

<a id="ERR_ASSERTION"></a>
### ERR_ASSERTION
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be in alphabetical order.

<a id="ERR_CPU_USAGE"></a>
### ERR_CPU_USAGE

Used when the native call from process.cpuUsage can't be processed properly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please avoid the use of contractions... s/can't/cannot

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: process.cpuUsage is code and should probably be process.cpuUsage

### ERR_HTTP_HEADERS_SENT

Used in case the headers were already sent and another attempt is made to add
more headers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used when headers have already been sent and another ...

<a id="ERR_INVALID_CHAR"></a>
### ERR_INVALID_CHAR

Used in e.g. http if invalid characters are detected in headers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used when invalid characters are detected in headers.

<a id="ERR_HTTP_INVALID_STATUS_CODE"></a>
### ERR_HTTP_INVALID_STATUS_CODE

Used for nonexisting status codes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite. non-existing would be non-registered. The code actually allows using valid but not-registered status codes (e.g. I could send 999). The error is thrown when the status code is < 100 or > 999

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware that the validation is less strict but personally I would keep the description like that as it feels more intuitive to me. With the range I would wonder why that range was chosen. As far as I know the current bounds were chosen to be "somewhat" near the regular status codes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see your point about non-existing as well though.

<a id="ERR_INVALID_ARRAY_LENGTH"></a>
### ERR_INVALID_ARRAY_LENGTH

Used in case a array is not of the expected length or in a valid range.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used when an Array is not ...

<a id="ERR_INVALID_FD"></a>
### ERR_INVALID_FD

Used in case a FD entry is not valid (i.e. negative).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used when a file descriptor ('fd') is not valid.

<a id="ERR_PARSE_HISTORY_DATA"></a>
### ERR_PARSE_HISTORY_DATA

Used in the `repl` if you use the old history file and an error occurred while
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid the use of informal personal pronouns like you in the docs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the old history file is used

### ERR_HTTP_TRAILER_INVALID

Used when the `Trailer` header is set even though the encoding does not support
that.
Copy link
Member

@jasnell jasnell Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is misleading. It's not the encoding that matters, it is the transfer encoding ...

<a id="ERR_V8BREAKITERATOR"></a>
### ERR_V8BREAKITERATOR

Used if the full ICU data is not installed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used when the V8 BreakIterator API is used but the full ICU data set is not installed.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this. Left some comments.

<a id="ERR_MULTIPLE_CALLBACK"></a>
### ERR_MULTIPLE_CALLBACK

Used when a callback is called more than once.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm not sure this is indicative enough for people new to node.

<a id="ERR_TRANSFORM_ALREADY_TRANSFORMING"></a>
### ERR_TRANSFORM_ALREADY_TRANSFORMING

Used when stream transform done is called even though it is still in process.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is indicative enough.

@benjamingr
Copy link
Member

Nice work, I think some examples for the errors would be really nice but I'm fine with landing it (after the requested fixes) without them and iterating.

@BridgeAR
Copy link
Member Author

BridgeAR commented Jun 21, 2017

Note: I explicitly do not document ERR_INVALID_REPL_HISTORY as it is not yet exposed to the users and I have another PR open that will fix that. I also did not document ERR_TRANSFORM_WITH_LENGTH_0 and ERR_TRANSFORM_ALREADY_TRANSFORMING. There is no test for these at the moment and I think it would be better to document them in another PR / investigate what to do with those.

@BridgeAR BridgeAR force-pushed the improve-error-types branch 2 times, most recently from 85920fb to 885a725 Compare June 21, 2017 23:58
@BridgeAR BridgeAR changed the title errors: improve error types WIP errors: improve error types Jun 22, 2017
@BridgeAR
Copy link
Member Author

Comments addressed. @jasnell would you be so kind and just drop a comment about my suggestions in the PR description? I could open a separate issue for these if preferred.

throw err;
}
if (err && err.message !== 'Channel closed') {
throw err;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the thrown error should never have that error code ECONNREFUSED as far as I can see.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#8950
I'll run a stress test

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM with one nit

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for all the hard work to refine/improve the errors.

1) Add missing lazy assert call
2) Remove obsolete error type
3) Name undocumented error type more appropriate
4) Consolidate error type style (rely on util.format
   instead of using a function)
5) Uppercase the first letter from error messages
6) Improve some internal error parameters
@BridgeAR
Copy link
Member Author

Nit addressed and rebased

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to test
test/parallel/test-child-process-fork-regr-gh-2847.js

throw err;
}
if (err && err.message !== 'Channel closed') {
throw err;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#8950
I'll run a stress test

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stress passed

@refack
Copy link
Contributor

refack commented Jun 27, 2017

@Trott
Copy link
Member

Trott commented Jun 27, 2017

After 4 hours of stalled build on the Raspberry Pi devices, I canceled the CI run on that platform. The devices need attention, I'm afraid. This will either have to land without a CI run on the Raspberry Pi's (which I think is probably OK) or else wait for the CI builds to start working again (shouldn't be too long before that happens, Rod mentioned in IRC he was looking at it).

@tniessen
Copy link
Member

tniessen commented Jun 28, 2017

CI on arm: https://ci.nodejs.org/job/node-test-commit-arm-fanned/9641/

Other failures appear to be unrelated / flaky tests.

@mhdawson
Copy link
Member

Given this: nodejs/build#774 (comment) I think its probably ok to land without arm results.

tniessen pushed a commit that referenced this pull request Jun 28, 2017
PR-URL: #13857
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
tniessen pushed a commit that referenced this pull request Jun 28, 2017
1) Add missing lazy assert call
2) Remove obsolete error type
3) Name undocumented error type more appropriate
4) Consolidate error type style (rely on util.format
   instead of using a function)
5) Uppercase the first letter from error messages
6) Improve some internal error parameters

PR-URL: #13857
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
tniessen pushed a commit that referenced this pull request Jun 28, 2017
PR-URL: #13857
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
tniessen pushed a commit that referenced this pull request Jun 28, 2017
PR-URL: #13857
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
tniessen pushed a commit that referenced this pull request Jun 28, 2017
PR-URL: #13857
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@tniessen
Copy link
Member

Landed in 8ca9338 ... 2fa2a60, thank you! 🎉 Post-merge (partial) CI: https://ci.nodejs.org/job/node-test-commit-linuxone/6913/

@tniessen tniessen closed this Jun 28, 2017
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
PR-URL: nodejs#13857
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
Backport-PR-URL: #22380
PR-URL: #13857
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
@BridgeAR BridgeAR deleted the improve-error-types branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants