promises: refactor rejection handling #18207

apapirovski · 2018-01-17T16:07:16Z

Remove the unnecessary microTasksTickObject for scheduling microtasks and instead use TickInfo to keep track of whether promise rejections exist that need to be emitted. Consequently allow the microtasks to execute on average fewer times, in more predictable manner than previously.

Simplify unhandled & handled rejection tracking to do more in C++ to avoid needing to expose additional info in JS. Unite emitting unhandledRejection and rejectionHandled into a single function: emitPromiseRejectionWarnings, which runs after all nextTicks have executed.

When new unhandledRejections are emitted within an unhandledRejection handler, allow the event loop to proceed first instead. This means that if the end-user code handles all promise rejections on nextTick, rejections within unhandledRejection now won't spiral into an infinite loop.

On the whole, this should hopefully make reasoning about nextTick, promises and promise rejections a whole lot simpler.

Fixes: #17913

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

process, promises, src

apapirovski · 2018-01-17T16:16:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12580/
~~CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1204/~~

Edit: weird failures in CitGM. Investigating...

apapirovski · 2018-01-17T16:33:45Z

CitGM failures unrelated to this PR but clearly something landed in the last 24 hours that completely destroyed CitGM.

benjamingr · 2018-01-17T17:13:44Z

I'll need a few days to digest this and run through the edge cases we had when we specified the hooks.

Pinging (no pressure to participate!) relevant parties @petkaantonov @addaleax @domenic

apapirovski · 2018-01-17T20:27:47Z

Just to distill what's going on in the current loop, here's an outline:

Check if any next ticks exist (C++)
a. If none exist, run Microtasks
b. If any ticks exist or unhandled/handled promise rejections were added, go to 2; otherwise exit
Execute all currently scheduled next ticks (if any)
Execute microtasks
Check if any next ticks exist, if they do go to 2.
Emit async handled rejections
Emit all currently existing unhandled promise rejections
a. if any unhandledRejection listeners exist or there are newly added unhandled promise rejections then go to 2
Next tick loop is done (back we go to C++)

addaleax · 2018-01-17T22:22:48Z

test/parallel/test-promises-unhandled-rejections.js

+  let didCall = false;
+  process.on('unhandledRejection', () => {
+    assert(!didCall);
+    didCall = true;


I’m not sure, but, common.mustCall()? ;)

Since it checks at exit, this would just keep looping forever until timeout is hit.

benjamingr

After reading the code and testing it - I like the behavior and changes. LGTM.

apapirovski · 2018-01-19T02:55:30Z

CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1213/

Remove the unnecessary microTasksTickObject for scheduling microtasks and instead use TickInfo to keep track of whether promise rejections exist that need to be emitted. Consequently allow the microtasks to execute on average fewer times, in more predictable manner than previously. Simplify unhandled & handled rejection tracking to do more in C++ to avoid needing to expose additional info in JS. When new unhandledRejections are emitted within an unhandledRejection handler, allow the event loop to proceed first instead. This means that if the end-user code handles all promise rejections on nextTick, rejections within unhandledRejection now won't spiral into an infinite loop.

BridgeAR · 2018-01-19T15:31:42Z

New CI due to some changed code (If I am not mistaken) https://ci.nodejs.org/job/node-test-pull-request/12619/

apapirovski · 2018-01-19T15:59:34Z

@BridgeAR It was just rebased to make it possible to run the CitGM. But no harm in extra CI :)

apapirovski · 2018-01-21T17:38:31Z

Landed in d62566e

Remove the unnecessary microTasksTickObject for scheduling microtasks and instead use TickInfo to keep track of whether promise rejections exist that need to be emitted. Consequently allow the microtasks to execute on average fewer times, in more predictable manner than previously. Simplify unhandled & handled rejection tracking to do more in C++ to avoid needing to expose additional info in JS. When new unhandledRejections are emitted within an unhandledRejection handler, allow the event loop to proceed first instead. This means that if the end-user code handles all promise rejections on nextTick, rejections within unhandledRejection now won't spiral into an infinite loop. PR-URL: #18207 Fixes: #17913 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

MylesBorins · 2018-02-20T22:20:26Z

This does not land cleanly on v9.x, should we backport?

apapirovski · 2018-02-20T22:26:56Z

@MylesBorins v9.x might be missing some preceding commits, I think. I'll have time to look into it at the end of the week or the weekend, swamped with a move right now. Sorry.

addaleax · 2018-02-27T16:41:34Z

This seem to apply cleanly on v9.x now, I’m removing the label.

Remove the unnecessary microTasksTickObject for scheduling microtasks and instead use TickInfo to keep track of whether promise rejections exist that need to be emitted. Consequently allow the microtasks to execute on average fewer times, in more predictable manner than previously. Simplify unhandled & handled rejection tracking to do more in C++ to avoid needing to expose additional info in JS. When new unhandledRejections are emitted within an unhandledRejection handler, allow the event loop to proceed first instead. This means that if the end-user code handles all promise rejections on nextTick, rejections within unhandledRejection now won't spiral into an infinite loop. PR-URL: nodejs#18207 Fixes: nodejs#17913 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

codebytere · 2018-08-02T15:48:53Z

@apapirovski this doesn't land cleanly on v8.x, do you think this makes sense to backport to v8.x?

apapirovski · 2018-08-02T15:51:51Z

@codebytere Yeah, probably wouldn't hurt. I have a backlog of things I'm supposed to backport anyway. I'll do them all this weekend.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 17, 2018

benjamingr self-assigned this Jan 17, 2018

joyeecheung mentioned this pull request Jan 17, 2018

CITGM seems to be broken on master nodejs/citgm#536

Closed

addaleax approved these changes Jan 17, 2018

View reviewed changes

benjamingr approved these changes Jan 17, 2018

View reviewed changes

benjamingr removed their assignment Jan 18, 2018

apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018

apapirovski force-pushed the patch-promise-reject-refactor branch from 0413ebb to 21a2220 Compare January 19, 2018 03:55

apapirovski closed this Jan 21, 2018

apapirovski deleted the patch-promise-reject-refactor branch January 21, 2018 17:38

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

addaleax removed backport-requested-v9.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 27, 2018

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

codebytere added the backport-requested-v8.x label Aug 2, 2018

alexkozy mentioned this pull request Nov 6, 2018

regression: UnhandledPromiseRejection is not fired until process is terminated #24209

Closed

hexpunk mentioned this pull request Dec 12, 2018

Performance issue with Promise rejection chaining #25000

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

promises: refactor rejection handling #18207

promises: refactor rejection handling #18207

apapirovski commented Jan 17, 2018

apapirovski commented Jan 17, 2018 •

edited

Loading

apapirovski commented Jan 17, 2018

benjamingr commented Jan 17, 2018 •

edited

Loading

apapirovski commented Jan 17, 2018 •

edited

Loading

addaleax Jan 17, 2018

apapirovski Jan 17, 2018

benjamingr left a comment

apapirovski commented Jan 19, 2018 •

edited

Loading

BridgeAR commented Jan 19, 2018

apapirovski commented Jan 19, 2018 •

edited

Loading

apapirovski commented Jan 21, 2018

MylesBorins commented Feb 20, 2018

apapirovski commented Feb 20, 2018

addaleax commented Feb 27, 2018

codebytere commented Aug 2, 2018

apapirovski commented Aug 2, 2018

promises: refactor rejection handling #18207

promises: refactor rejection handling #18207

Conversation

apapirovski commented Jan 17, 2018

Checklist

Affected core subsystem(s)

apapirovski commented Jan 17, 2018 • edited Loading

apapirovski commented Jan 17, 2018

benjamingr commented Jan 17, 2018 • edited Loading

apapirovski commented Jan 17, 2018 • edited Loading

addaleax Jan 17, 2018

Choose a reason for hiding this comment

apapirovski Jan 17, 2018

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

apapirovski commented Jan 19, 2018 • edited Loading

BridgeAR commented Jan 19, 2018

apapirovski commented Jan 19, 2018 • edited Loading

apapirovski commented Jan 21, 2018

MylesBorins commented Feb 20, 2018

apapirovski commented Feb 20, 2018

addaleax commented Feb 27, 2018

codebytere commented Aug 2, 2018

apapirovski commented Aug 2, 2018

apapirovski commented Jan 17, 2018 •

edited

Loading

benjamingr commented Jan 17, 2018 •

edited

Loading

apapirovski commented Jan 17, 2018 •

edited

Loading

apapirovski commented Jan 19, 2018 •

edited

Loading

apapirovski commented Jan 19, 2018 •

edited

Loading