Promises/better unhandled rejection message #17158

MadaraUchiha · 2017-11-20T19:30:26Z

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)

parallel, promises

Fixes #16768

#goodnessSquad

Fixes nodejs#16768

For unhandled rejection error message change.

refack · 2017-11-20T19:40:33Z

Thanks @MadaraUchiha for the contribution.

@nodejs/release I see no mention in the guides to semverity of process warning messages. I would assume this is "major" (to protect tools like pm2 etc.).

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

benjamingr

+1000 on this change.

evanlucas · 2017-11-20T21:28:18Z

lib/internal/process/promises.js

@@ -60,11 +58,23 @@ function setupPromises(scheduleMicrotasks) {
  }

  function emitWarning(uid, reason) {
+    try {
+      if (reason instanceof Error) {
+        process.emitWarning(reason.stack, 'UnhandledPromiseRejectionWarning');


So I guess the only downfall to doing it this way is it won't include any additional properties added on to the Error object. Any reason to do this vs using util.inspect()?

Just that I wasn't aware of util.inspect(). I went with what the rest of the code did in the same function.

Namely, Node doesn't show this information for synchronous errors:

// test.js var e = new Error("error"); e.foo = "bar"; throw e;

~/test/test.js:3 throw e; ^ Error: error at Object.<anonymous> (/Users/benjamin/Downloads/test.js:1:71) at Module._compile (module.js:573:30) at Object.Module._extensions..js (module.js:584:10) at Module.load (module.js:507:32) at tryModuleLoad (module.js:470:12) at Function.Module._load (module.js:462:3) at Function.Module.runMain (module.js:609:10) at startup (bootstrap_node.js:158:16) at bootstrap_node.js:598:3

refack · 2017-11-20T21:52:36Z

Optional suggestion: wrap the message

What do people think?

MadaraUchiha · 2017-11-20T21:53:44Z

@refack Initial iterations actually did that, but then it looked awkward with relations to the rest of the warnings, so I decided against. I have no strong opinion either way though. Maybe with an extra linebreak separating it from other output?

refack · 2017-11-20T21:57:11Z

@MadaraUchiha regarding the second line, it is only printed once, so after doing a more "realistic" test

IMHO this PR is good as is.

refack · 2017-11-20T22:02:34Z

lib/internal/process/promises.js

+      if (reason instanceof Error) {
+        process.emitWarning(reason.stack, 'UnhandledPromiseRejectionWarning');
+      } else {
+        process.emitWarning(reason, 'UnhandledPromiseRejectionWarning');


So this still should be safeToString(reason), notice that emitWarning will only emit strings

D:\code\4node$ promise-warning.exe\node.exe -e "Promise.reject('a');Promise.reject(100)" (node:18696) UnhandledPromiseRejectionWarning: a (node:18696) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1) (node:18696) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code. (node:18696) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)

P.S. could you add a test for a non string rejection?

What's the expected behavior when strings get rejected? I couldn't find a way to get the stack trace in that case.

I'd assume the stack has been destroyed when this is reached. IMHO just printing the reason (after stringifying) is the best we can do.
So if it's an Error reason.stack else safeToString(reason).

I think that test-promises-warning-on-unhandled-rejection.js implicitly checks for string rejections, please tell me if something else might be better.

I've pushed the commit to wrap the reason with safeToString().

Yeah it's an awkward test... You could add a line just before last:

const p = Promise.reject(100);

then move case (2) to (3) add a case (2):

case 2: assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning'); assert(/100/.test(warning.message)); break;

P.S. it's testing Strings because it checks the stringified output

Sorry, should be p1 since p is used for the async handling check

@refack Added the requested test, also added comments to make it clear what each state in the state machine means.

Also clean up the test a bit and add comments

Now checks correctly that Symbol() is outputted when rejection happens.

benjamingr

Still looks good to me, @refack you wanna land this today or should I?

jasnell · 2017-11-22T19:18:22Z

test/parallel/test-promises-warning-on-unhandled-rejection.js

-      assert.strictEqual(warning.name, 'DeprecationWarning');
+      // Warning about rejection not being handled (will be next tick)
+      assert.strictEqual(warning.name, 'UnhandledPromiseRejectionWarning');
+      assert(/Unhandled promise rejection/.test(warning.message), 'Expected warning message to contain "Unhandled promise rejection" but did not. Had "' + warning.message + '" instead.');


these lines need to be line-wrapped at <= 80 chars

How come make test's linter passes for me? I totally missed that.

refack · 2017-11-22T20:31:29Z

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

refack · 2017-11-22T22:35:01Z

Landed in 13db29b.

@MadaraUchiha, thank you very much 🍾

PR-URL: #17158 Refs: #16768 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-12-19T21:16:49Z

Should this be added to LTS?

As an aside, @refack asked if this should be considered Semver-Major and I didn't see any direct responses. Obviously this landed as a patch, but I am curious the reasoning for it not being major

benjamingr · 2017-12-20T09:49:13Z

@MylesBorins - it landed as patch because tools didn't and had no incentive to rely on the message (a direct event is exposed for unhandledRejection anyway).

This change should if possible be backported since it improves the developer experience a lot.

MylesBorins · 2018-01-19T18:37:56Z

This lands cleanly on v8.x but will require a manual backport to v6.x

PR-URL: #17158 Refs: #16768 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

MadaraUchiha added 2 commits November 20, 2017 21:27

Improved unhandled rejection message

a29fe91

Fixes nodejs#16768

Make tests pass

ba4adc5

For unhandled rejection error message change.

nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Nov 20, 2017

refack added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 20, 2017

refack approved these changes Nov 20, 2017

View reviewed changes

refack added the promises Issues and PRs related to ECMAScript promises. label Nov 20, 2017

benjamingr approved these changes Nov 20, 2017

View reviewed changes

evanlucas reviewed Nov 20, 2017

View reviewed changes

refack reviewed Nov 20, 2017

View reviewed changes

Wrap reason with unsafeToString() if it's a string

ab45e60

MadaraUchiha force-pushed the promises/better-unhandled-rejection-message branch from 331a932 to ab45e60 Compare November 20, 2017 22:39

evanlucas approved these changes Nov 21, 2017

View reviewed changes

MadaraUchiha added 2 commits November 21, 2017 21:41

Add test for checking warnings about rejected numbers.

bf06851

Also clean up the test a bit and add comments

Fix failing symbol rejection test.

9d29f0c

Now checks correctly that Symbol() is outputted when rejection happens.

MadaraUchiha force-pushed the promises/better-unhandled-rejection-message branch from cda4d30 to 9d29f0c Compare November 21, 2017 19:50

benjamingr approved these changes Nov 22, 2017

View reviewed changes

jasnell reviewed Nov 22, 2017

View reviewed changes

Make test lines wrap at 80 characters or less.

481c7f1

jasnell approved these changes Nov 22, 2017

View reviewed changes

refack closed this Nov 22, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

MylesBorins added lts-watch-v6.x labels Dec 19, 2017

MylesBorins added backport-requested-v6.x and removed lts-watch-v6.x labels Jan 19, 2018

MylesBorins added land-on-v8.x and removed lts-watch-v8.x labels Jan 19, 2018

simonua mentioned this pull request Mar 8, 2018

UnhandledPromiseRejectionWarning when testing with Jasmine axios/axios#1404

Closed

BridgeAR mentioned this pull request Mar 17, 2020

process: delay throwing an error using throwDeprecation #32312

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promises/better unhandled rejection message #17158

Promises/better unhandled rejection message #17158

MadaraUchiha commented Nov 20, 2017 •

edited

Loading

refack commented Nov 20, 2017

benjamingr left a comment

evanlucas Nov 20, 2017

MadaraUchiha Nov 20, 2017

benjamingr Nov 20, 2017

refack commented Nov 20, 2017

MadaraUchiha commented Nov 20, 2017

refack commented Nov 20, 2017 •

edited

Loading

refack Nov 20, 2017 •

edited

Loading

MadaraUchiha Nov 20, 2017

refack Nov 20, 2017

MadaraUchiha Nov 20, 2017

refack Nov 20, 2017 •

edited

Loading

refack Nov 20, 2017

MadaraUchiha Nov 21, 2017

refack Nov 22, 2017

benjamingr left a comment

jasnell Nov 22, 2017

MadaraUchiha Nov 22, 2017

refack commented Nov 22, 2017

refack commented Nov 22, 2017

MylesBorins commented Dec 19, 2017

benjamingr commented Dec 20, 2017

MylesBorins commented Jan 19, 2018

Promises/better unhandled rejection message #17158

Promises/better unhandled rejection message #17158

Conversation

MadaraUchiha commented Nov 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Nov 20, 2017

benjamingr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Nov 20, 2017

MadaraUchiha commented Nov 20, 2017

refack commented Nov 20, 2017 • edited Loading

refack Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Nov 22, 2017

refack commented Nov 22, 2017

MylesBorins commented Dec 19, 2017

benjamingr commented Dec 20, 2017

MylesBorins commented Jan 19, 2018

MadaraUchiha commented Nov 20, 2017 •

edited

Loading

refack commented Nov 20, 2017 •

edited

Loading

refack Nov 20, 2017 •

edited

Loading

refack Nov 20, 2017 •

edited

Loading