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

lib: fix error name #26738

Closed
wants to merge 6 commits into from

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Mar 18, 2019

This makes sure Node.js errors have a "regular" error name while keeping the error code in the stack trace. Please have a look at the individual commits for a more detailed description.

Fixes: #26669
Fixes: #20253

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
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@gabrielschulhof I am not familar with the n-api code and I went ahead to remove the error code from the n-api error names. What I do in JS is to add the code only to the stack trace instead of the name property. Is there a chance to do the same for n-api as well?

@BridgeAR BridgeAR force-pushed the BridgeAR:fix-error-name branch from f61ffc0 to 455b8e6 Mar 18, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Mar 18, 2019

Note that Error.prototype.toString() is specified

Return the string-concatenation of name, the code unit 0x003A (COLON), the code unit 0x0020 (SPACE), and msg.

If we are going semver-major anyway, I'd prefer we just move the code to the message, and follow the behavior of native errors, that way we don't need to hack around Error.captureStackTrace.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@joyeecheung I am aware of the spec but my goal here was to keep the current behavior how the stack is visible while making the sure the name is set to the expected one.

I personally like that the code is not part of the error message.

we don't need to hack around Error.captureStackTrace

I personally would not call it a hack. To me it seems like the hideStackFrame function is easy to use and understand and it has the benefit that the code is a) smaller b) more straight forward (we are able to throw immediately) and c) we always make sure the stack is not calculated twice.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 18, 2019

I'm -1 on moving the code to the message

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@jasnell with my implementation the output is exactly as it was before (the code is not in the message). The only difference is that the name property does not contain the code anymore.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 18, 2019

@@ -15,6 +15,8 @@ rules:
# Config specific to lib
- selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])"
message: "Use an error exported by the internal/errors module."
- selector: "CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace']"

This comment has been minimized.

Copy link
@Trott

Trott Mar 19, 2019

Member

Given that there are 17 exceptions to this rule in lib, are we sure this is a good rule to have?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Mar 19, 2019

Author Member

It might be difficult to identify the cases which should not use Error.captureStackTrace without this rule.

I count 16 exceptions while 7 of those are all inside internal/errors and I would normally just add a file wide exception explicitly for this case but that would require to write an individual rule instead of using the no-restricted-syntax rule.

Three more cases would ideally also switch to using hideStackFrames() but C++ errors still work quite differently than our JS errors (besides N-API errors which try to mirror the JS side) and handling errors inside callbacks is not yet possible. I already have an idea to improve those further if this PR lands.

That leaves six other cases of which five seem to be exceptions to the rule in a way that they are neither directly related to our errors nor should they ever use hideStackFrames().

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Mar 20, 2019

Author Member

I updated the hideStackFrames() function and now there is one are two exception less than before.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 19, 2019

@joyeecheung I am aware of the spec but my goal here was to keep the current behavior how the stack is visible while making the sure the name is set to the expected one.

I'm inclined to agree with Joyee that moving the code to the message property would be best. First, message is clearly intended to be something human-readable and mutable over time. So that's the place to put custom stuff. Second, the custom error stuff is new enough that a semver major change like that probably won't be catastrophic on the ecosystem. But if we wait a few more releases, it is likely to become impossible to make such a change. So my inclination is: Let's do it now while there's a chance that we still can.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 19, 2019

Thanks for doing this. It's way more modified files than I would have expected. Yikes!

Show resolved Hide resolved lib/internal/errors.js Outdated
Show resolved Hide resolved lib/util.js Outdated
@@ -15,6 +15,8 @@ rules:
# Config specific to lib
- selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])"
message: "Use an error exported by the internal/errors module."
- selector: "CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace']"

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Mar 19, 2019

Author Member

It might be difficult to identify the cases which should not use Error.captureStackTrace without this rule.

I count 16 exceptions while 7 of those are all inside internal/errors and I would normally just add a file wide exception explicitly for this case but that would require to write an individual rule instead of using the no-restricted-syntax rule.

Three more cases would ideally also switch to using hideStackFrames() but C++ errors still work quite differently than our JS errors (besides N-API errors which try to mirror the JS side) and handling errors inside callbacks is not yet possible. I already have an idea to improve those further if this PR lands.

That leaves six other cases of which five seem to be exceptions to the rule in a way that they are neither directly related to our errors nor should they ever use hideStackFrames().

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 19, 2019

@joyeecheung @Trott even if we plan on moving the code inside the error message, I would still like to land this as is instead of working on an completely different approach which would have to go to the TSC as @jasnell pointed out that he's against that approach. This does seem to improve the situation for everyone already by at least removing the code property from the name. Is there a reason not to accept this as is and then to look further into this?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 20, 2019

Is there a reason not to accept this as is and then to look further into this?

No objection from me on that approach. It's not my first choice, but yeah, sometimes you have to do things in phases and hope it doesn't stall.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 20, 2019

Just as a side note: using the hideStackFrames() function has another benefit over the former implementation: if such functions are stacked (e.g. some validation functions call other validation functions inside of them), then the most outer function is the one used to hide the stack frame instead of the most inner one as it is the case so far.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 20, 2019

I just updated this PR with two more commits: the first one improves the hideStackFrames functions (now only Node.js core error frames will be hidden. E.g., programming errors have the full stack trace; if more than a single error is created inside of the wrapped function, all errors hide the internal frames; the function will now work on arbitrary code inside of the wrapped function and does not rely anymore on the return value or thrown errors). The other commit improves the performance to create AssertionErrors (I could move that into a separate PR but it felt natural to this one).

CI https://ci.nodejs.org/job/node-test-pull-request/21687/

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 21, 2019

If we want to remove the code from the name, we should likely do that for v12. Otherwise more people might rely on it and it becomes a more significant breaking change.

@BridgeAR BridgeAR requested review from targos, apapirovski, ChALkeR and danbev Mar 21, 2019

@BridgeAR BridgeAR requested a review from mcollina Mar 21, 2019

@mcollina
Copy link
Member

mcollina left a comment

LGTM

@targos

targos approved these changes Mar 21, 2019

configurable: true
});
} else {
delete err.name;

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Mar 21, 2019

Member

Why do we need this? Can't we just reset err.name = name?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Mar 21, 2019

Author Member

Errors use their prototypes name and should not have a instances name. I add the instances name only to create the error stack. Removing it afterwards will fall back to the prototypes error name again.
SystemError works a bit different and that's why I set the name as non-enumerable property. I plan on refactoring that at some point but that seemed out of scope for this PR.

Show resolved Hide resolved lib/internal/errors.js Outdated
@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Mar 21, 2019

I am ok with this landing before the v12 cutoff, but would still like to see we revisit the stack manipulation afterwards.

Also cc @devsnek does this affect #23926 ?

@ChALkeR

This comment has been minimized.

Copy link
Member

ChALkeR commented Mar 21, 2019

Definitely +1 on this approach.
I have not yet reviewed the code, but I don't see any problems at the first glance.

BridgeAR added some commits Mar 16, 2019

errors: update error name
This updates all Node.js errors by removing the `code` being part
of the `name` property. Instead, the name is just changed once on
instantiation, the stack is accessed to create the stack as expected
and then the `name` property is set back to it's original form.
lib: refactor Error.captureStackTrace() usage
When using `Errors.captureStackFrames` the error's stack property
is set again. This adds a helper function that wraps this functionality
in a simple API that does not only set the stack including the `code`
property but it also improves the performance to create the error.
The helper works for thrown errors and errors returned from wrapped
functions in case they are Node.js core errors.
lib: validate Error.captureStackTrace() calls
This adds a custom eslint rule to verify that
`Error.captureStackTrace()` is only called if necessary. In most
cases the helper function should be used instead.
test: remove redundant common.mustCall
All calls verify the cb is called, thus using `common.mustCall` is
not required inside of this function.
n-api: remove code from error name
This is a first step to align the n-api errors towards errors created
in JS. The stack still has to be updated to add the error code.
assert: improve performance to instantiate errors
This improves the performance for AssertionError by deactivating
duplicated stack frame creation.

@BridgeAR BridgeAR force-pushed the BridgeAR:fix-error-name branch from 9dae150 to e6514c8 Mar 21, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 21, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 22, 2019

@joyeecheung

does this affect #23926

AFAIC this has no other impact than the code before. I just remove the name property before calling Error.prepareStackTrace.

@BridgeAR BridgeAR referenced this pull request Mar 22, 2019

Closed

lib: refactor validators #26809

4 of 4 tasks complete
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 22, 2019

It would be nice if someone would just confirm their LG right quick before landing.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 23, 2019

errors: update error name
This updates all Node.js errors by removing the `code` being part
of the `name` property. Instead, the name is just changed once on
instantiation, the stack is accessed to create the stack as expected
and then the `name` property is set back to it's original form.

PR-URL: nodejs#26738
Fixes: nodejs#26669
Fixes: nodejs#20253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 23, 2019

lib: refactor Error.captureStackTrace() usage
When using `Errors.captureStackFrames` the error's stack property
is set again. This adds a helper function that wraps this functionality
in a simple API that does not only set the stack including the `code`
property but it also improves the performance to create the error.
The helper works for thrown errors and errors returned from wrapped
functions in case they are Node.js core errors.

PR-URL: nodejs#26738
Fixes: nodejs#26669
Fixes: nodejs#20253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 23, 2019

lib: validate Error.captureStackTrace() calls
This adds a custom eslint rule to verify that
`Error.captureStackTrace()` is only called if necessary. In most
cases the helper function should be used instead.

PR-URL: nodejs#26738
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 23, 2019

test: remove redundant common.mustCall
All calls verify the cb is called, thus using `common.mustCall` is
not required inside of this function.

PR-URL: nodejs#26738
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 23, 2019

n-api: remove code from error name
This is a first step to align the n-api errors towards errors created
in JS. The stack still has to be updated to add the error code.

PR-URL: nodejs#26738
Fixes: nodejs#26669
Fixes: nodejs#20253
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 23, 2019

assert: improve performance to instantiate errors
This improves the performance for AssertionError by deactivating
duplicated stack frame creation.

PR-URL: nodejs#26738
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 23, 2019

Thanks a lot for the reviews.

Landed in c757cb1...afce912 🎉

@BridgeAR BridgeAR closed this Mar 23, 2019

@felixfbecker

This comment has been minimized.

Copy link
Contributor

felixfbecker commented Mar 25, 2019

Thank you so much for fixing this

@BethGriggs BethGriggs referenced this pull request Mar 26, 2019

Draft

v12.0.0 proposal #26930

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.