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

Closed
wants to merge 2 commits into
base: master
from

Conversation

@addaleax
Member

addaleax commented Nov 9, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

promises

Description of change

Give better stack traces for PromiseRejectionHandledWarningand UnhandledPromiseRejectionWarnings.

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

promise: better stack traces for --trace-warnings
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
@danbev

danbev approved these changes Nov 9, 2016

@mhdawson

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 ?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2016

Member

@mhdawson Well… only if the application doesn’t use the unhandledRejection and rejectionHandled events, which are the programmatic API for these events. It is true that process.on('warning') handlers will receive different stack traces but I don’t know if that really counts as semver-major.

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…?

Member

addaleax commented Nov 9, 2016

@mhdawson Well… only if the application doesn’t use the unhandledRejection and rejectionHandled events, which are the programmatic API for these events. It is true that process.on('warning') handlers will receive different stack traces but I don’t know if that really counts as semver-major.

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…?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 16, 2016

Member

If nobody objects, I’d like to land this by Friday as a semver-patch change.

CI: https://ci.nodejs.org/job/node-test-commit/6020/

Member

addaleax commented Nov 16, 2016

If nobody objects, I’d like to land this by Friday as a semver-patch change.

CI: https://ci.nodejs.org/job/node-test-commit/6020/

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Nov 17, 2016

Member

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.

Member

mhdawson commented Nov 17, 2016

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.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Nov 18, 2016

Member

No objections to landing as a patch. Good change

Member

jasnell commented Nov 18, 2016

No objections to landing as a patch. Good change

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 26, 2016

Member

@mhdawson Would you be okay with this landing as a patch change? Is there any particular scenario that you are worried about?

Member

addaleax commented Nov 26, 2016

@mhdawson Would you be okay with this landing as a patch change? Is there any particular scenario that you are worried about?

@targos

targos approved these changes Nov 27, 2016

LGTM. +1 for considering it as a patch.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Dec 5, 2016

Member

Oh wow, I just used trace --trace-warnings and rejected a promise and... that is definitely not useful currently.

This should be considered a bug fix, can we land it in v7.2.1 tomorrow?

Member

Fishrock123 commented Dec 5, 2016

Oh wow, I just used trace --trace-warnings and rejected a promise and... that is definitely not useful currently.

This should be considered a bug fix, can we land it in v7.2.1 tomorrow?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Dec 5, 2016

Member

I’ll land it now, I think we basically have consensus on considering this semver-patch.

Member

addaleax commented Dec 5, 2016

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})`);
}

This comment has been minimized.

@Fishrock123

Fishrock123 Dec 5, 2016

Member

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.

@Fishrock123

Fishrock123 Dec 5, 2016

Member

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.

This comment has been minimized.

@addaleax

addaleax Dec 5, 2016

Member

Maybe, but I think it’s okay. This should not be a hot path anyway.

@addaleax

addaleax Dec 5, 2016

Member

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')) {

This comment has been minimized.

@Fishrock123

Fishrock123 Dec 5, 2016

Member

Won't this always be hit because of the default listener?

@Fishrock123

Fishrock123 Dec 5, 2016

Member

Won't this always be hit because of the default listener?

This comment has been minimized.

@addaleax

addaleax Dec 5, 2016

Member

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

@addaleax

addaleax Dec 5, 2016

Member

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

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Dec 5, 2016

Member

Landed in 196d27d

Member

addaleax commented Dec 5, 2016

Landed in 196d27d

@addaleax addaleax closed this Dec 5, 2016

@addaleax addaleax deleted the addaleax:unhandled-promise-rejection-stack-trace branch Dec 5, 2016

addaleax added a commit that referenced this pull request Dec 5, 2016

promise: better stack traces for --trace-warnings
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

addaleax added a commit that referenced this pull request Dec 5, 2016

promise: better stack traces for --trace-warnings
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

Fishrock123 added a commit that referenced this pull request Dec 6, 2016

2016-12-06, Version 7.2.1 (Current)
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

imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 7, 2016

2016-12-06, Version 7.2.1 (Current)
    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 <ilkka.myller@nodefield.com>

jmdarling added a commit to jmdarling/node that referenced this pull request Dec 8, 2016

promise: better stack traces for --trace-warnings
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Dec 20, 2016

Member

@addaleax should we land this on v6.x

Member

MylesBorins commented Dec 20, 2016

@addaleax should we land this on v6.x

addaleax added a commit to addaleax/node that referenced this pull request Dec 21, 2016

promise: better stack traces for --trace-warnings
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Dec 21, 2016

Member

backport @ #10378, we can discuss that there

Member

addaleax commented Dec 21, 2016

backport @ #10378, we can discuss that there

addaleax added a commit that referenced this pull request Jan 20, 2017

promise: better stack traces for --trace-warnings
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

MylesBorins added a commit that referenced this pull request Jan 24, 2017

promise: better stack traces for --trace-warnings
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

@MylesBorins MylesBorins referenced this pull request Jan 24, 2017

Merged

v6.10.0 proposal #10974

MylesBorins added a commit that referenced this pull request Jan 31, 2017

promise: better stack traces for --trace-warnings
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 <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment