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

promise: better stack traces for --trace-warnings #9525

Closed

Conversation

addaleax
Copy link
Member

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

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
@addaleax addaleax added promises Issues and PRs related to ECMAScript promises. dont-land-on-v4.x labels Nov 9, 2016
@@ -0,0 +1,4 @@
// Flags: --trace-warnings
'use strict';
const p = Promise.reject(new Error('This was rejected'));
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@danbev right, done

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 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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
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.

@jasnell
Copy link
Member

jasnell commented Nov 18, 2016

No objections to landing as a patch. Good change

@addaleax
Copy link
Member Author

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

Copy link
Member

@targos targos left a 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.

@Fishrock123
Copy link
Contributor

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
Copy link
Member Author

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

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.

Copy link
Member Author

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')) {
Copy link
Contributor

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?

Copy link
Member Author

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.

@addaleax
Copy link
Member Author

addaleax commented Dec 5, 2016

Landed in 196d27d

@addaleax addaleax closed this Dec 5, 2016
@addaleax addaleax deleted the unhandled-promise-rejection-stack-trace branch December 5, 2016 17:30
addaleax added a commit that referenced this pull request Dec 5, 2016
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
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
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
    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 pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
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
Copy link
Contributor

@addaleax should we land this on v6.x

addaleax added a commit to addaleax/node that referenced this pull request Dec 21, 2016
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
Copy link
Member Author

backport @ #10378, we can discuss that there

addaleax added a commit that referenced this pull request Jan 20, 2017
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 pushed a commit that referenced this pull request Jan 24, 2017
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 mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
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
Labels
promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants