-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
promise: better stack traces for --trace-warnings #9525
promise: better stack traces for --trace-warnings #9525
Conversation
Give better stack traces for `PromiseRejectionHandledWarning` and `UnhandledPromiseRejectionWarning`s. For `PromiseRejectionHandledWarning`, when it is likely that there is an `Error` object generated, it is created early to provide a proper stack trace. For `UnhandledPromiseRejectionWarning`, the stack trace of the underlying error object is used, if possible. Fixes: nodejs#9523
@@ -0,0 +1,4 @@ | |||
// Flags: --trace-warnings | |||
'use strict'; | |||
const p = Promise.reject(new Error('This was rejected')); |
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 the common
module needs to be required here:
require('../common');
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.
@danbev right, done
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 as its seems like a good idea to me. Should it be tagged as a semver major though because it changes the stack trace that will be seen by the application ?
@mhdawson Well… only if the application doesn’t use the Marking this as a major might defeat the purpose of it, because I think the plan is to turn the warning into an error in the next (?) major release. I’m not sure about how exactly that would look like, though. @nodejs/ctc Thoughts…? |
If nobody objects, I’d like to land this by Friday as a |
It would be nice to have a bit more input from ctc members on whether its ok to handle as a patch. @jasnell @bnoordhuis any opinion here. |
No objections to landing as a patch. Good change |
@mhdawson Would you be okay with this landing as a patch change? Is there any particular scenario that you are worried about? |
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. +1 for considering it as a patch.
Oh wow, I just used trace This should be considered a bug fix, can we land it in v7.2.1 tomorrow? |
I’ll land it now, I think we basically have consensus on considering this semver-patch. |
function getAsynchronousRejectionWarningObject(uid) { | ||
return new Error('Promise rejection was handled ' + | ||
`asynchronously (rejection id: ${uid})`); | ||
} |
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.
Perhaps this could reduce late promise handling performance slightly but... oh well, maybe we can make that an option if people complain. ¯\_(ツ)_/¯ Good defaults are more important for this kind of thing.
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.
Maybe, but I think it’s okay. This should not be a hot path anyway.
@@ -31,10 +36,15 @@ function setupPromises(scheduleMicrotasks) { | |||
const uid = promiseToGuidProperty.get(promise); | |||
promiseToGuidProperty.delete(promise); | |||
if (hasBeenNotified === true) { | |||
let warning = null; | |||
if (!process.listenerCount('rejectionHandled')) { |
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.
Won't this always be hit because of the default listener?
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.
@Fishrock123 Are you referring to the code in the block below this one? That’s not technically a listener, so it isn’t counted here. Also, at least a quick check in the REPL yields process.listenerCount('rejectionHandled') === 0
by default.
Landed in 196d27d |
Give better stack traces for `PromiseRejectionHandledWarning` and `UnhandledPromiseRejectionWarning`s. For `PromiseRejectionHandledWarning`, when it is likely that there is an `Error` object generated, it is created early to provide a proper stack trace. For `UnhandledPromiseRejectionWarning`, the stack trace of the underlying error object is used, if possible. Fixes: #9523 PR-URL: #9525 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Give better stack traces for `PromiseRejectionHandledWarning` and `UnhandledPromiseRejectionWarning`s. For `PromiseRejectionHandledWarning`, when it is likely that there is an `Error` object generated, it is created early to provide a proper stack trace. For `UnhandledPromiseRejectionWarning`, the stack trace of the underlying error object is used, if possible. Fixes: #9523 PR-URL: #9525 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Notable changes: * buffer: - Reverted the runtime deprecation of calling `Buffer()` without `new`. (Anna Henningsen) #9529 - Fixed `buffer.transcode()` for single-byte character encodings to `UCS2`. (Anna Henningsen) #9838 * promise: `--trace-warnings` now produces useful stacktraces for Promise warnings. (Anna Henningsen) #9525 * repl: Fixed a bug preventing correct parsing of generator functions. (Teddy Katz) #9852 * V8: Fixed a significant `instanceof` performance regression. (Franziska Hinkelmann) #9730 PR-URL: #10127
Notable changes: * buffer: - Reverted the runtime deprecation of calling `Buffer()` without `new`. (Anna Henningsen) nodejs/node#9529 - Fixed `buffer.transcode()` for single-byte character encodings to `UCS2`. (Anna Henningsen) nodejs/node#9838 * promise: `--trace-warnings` now produces useful stacktraces for Promise warnings. (Anna Henningsen) nodejs/node#9525 * repl: Fixed a bug preventing correct parsing of generator functions. (Teddy Katz) nodejs/node#9852 * V8: Fixed a significant `instanceof` performance regression. (Franziska Hinkelmann) nodejs/node#9730 Signed-off-by: Ilkka Myller <[email protected]>
Give better stack traces for `PromiseRejectionHandledWarning` and `UnhandledPromiseRejectionWarning`s. For `PromiseRejectionHandledWarning`, when it is likely that there is an `Error` object generated, it is created early to provide a proper stack trace. For `UnhandledPromiseRejectionWarning`, the stack trace of the underlying error object is used, if possible. Fixes: nodejs#9523 PR-URL: nodejs#9525 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@addaleax should we land this on v6.x |
Give better stack traces for `PromiseRejectionHandledWarning` and `UnhandledPromiseRejectionWarning`s. For `PromiseRejectionHandledWarning`, when it is likely that there is an `Error` object generated, it is created early to provide a proper stack trace. For `UnhandledPromiseRejectionWarning`, the stack trace of the underlying error object is used, if possible. Fixes: nodejs#9523 PR-URL: nodejs#9525 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
backport @ #10378, we can discuss that there |
Give better stack traces for `PromiseRejectionHandledWarning` and `UnhandledPromiseRejectionWarning`s. For `PromiseRejectionHandledWarning`, when it is likely that there is an `Error` object generated, it is created early to provide a proper stack trace. For `UnhandledPromiseRejectionWarning`, the stack trace of the underlying error object is used, if possible. Fixes: #9523 PR-URL: #9525 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Give better stack traces for `PromiseRejectionHandledWarning` and `UnhandledPromiseRejectionWarning`s. For `PromiseRejectionHandledWarning`, when it is likely that there is an `Error` object generated, it is created early to provide a proper stack trace. For `UnhandledPromiseRejectionWarning`, the stack trace of the underlying error object is used, if possible. Fixes: #9523 PR-URL: #9525 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Give better stack traces for `PromiseRejectionHandledWarning` and `UnhandledPromiseRejectionWarning`s. For `PromiseRejectionHandledWarning`, when it is likely that there is an `Error` object generated, it is created early to provide a proper stack trace. For `UnhandledPromiseRejectionWarning`, the stack trace of the underlying error object is used, if possible. Fixes: #9523 PR-URL: #9525 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
promises
Description of change
Give better stack traces for
PromiseRejectionHandledWarning
andUnhandledPromiseRejectionWarning
s.For
PromiseRejectionHandledWarning
, when it is likely that there is anError
object generated, it is created early to provide a proper stack trace.For
UnhandledPromiseRejectionWarning
, the stack trace of theunderlying error object is used, if possible.
Fixes: #9523