process: refactor promise rejection handling #25200

joyeecheung · Dec 24, 2018

It's easier to review without the indentation changes: https://github.com/nodejs/node/pull/25200/files?w=1

src: refactor tickInfo access

Wrap access to tickInfo fields in functions
Rename kHasScheduled to kHasTickScheduled and
kHasPromiseRejections to kHasRejectionToWarn for clarity - note
the latter will be set to false if the rejection does not lead to
a warning so the previous description is not accurate.
Set kHasRejectionToWarn in JS land of relying on C++ to use
an implict contract (return value of the promise rejection handler)
to set it, as the decision is made entirely in JS land.
Destructure promise reject event constants.

process: make tick callback and promise rejection callback more robust

Rename internalTickCallback to processTicksAndRejections, make
sure it does not get called if it's not set in C++.
Rename emitPromiseRejectionWarnings to processPromiseRejections
since it also emit events that are not warnings.
Sets SetPromiseRejectCallback in the Environment constructor
to make sure it only gets called once per-isolate, and make
sure it does not get called if it's not set in C++.
Wrap promise rejection callback initialization into
listenForRejections().
Add comments.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · Dec 24, 2018

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2053/pipeline

joyeecheung · Dec 24, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/19779/ ✔️
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/283/

BridgeAR · 2018-12-24T09:26:02Z

BridgeAR reviewed

Dec 24, 2018

View changes

lib/internal/process/promises.js

lib/internal/process/next_tick.js

lib/internal/process/promises.js

joyeecheung · Dec 25, 2018

I left the more opinionated refactoring out of this PR and focused on renaming & improving robustness.

CI: https://ci.nodejs.org/job/node-test-pull-request/19808/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/284/

Trott · Dec 26, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19812/

addaleax · 2018-12-26T13:27:40Z

addaleax approved these changes

Dec 26, 2018

View changes

addaleax · Dec 26, 2018

src/env.cc

@@ -241,6 +241,8 @@ Environment::Environment(IsolateData* isolate_data,
  if (options_->no_force_async_hooks_checks) {
    async_hooks_.no_force_checks();
  }
+
+  isolate()->SetPromiseRejectCallback(task_queue::PromiseRejectCallback);


Can you leave a TODO comment for me? We’re letting a single Environment take control of per-Isolate state here – I know it’s been that way before, but it’s not what we should be doing…

Out of curiosity - when would multiple Environments correspond to an isolate?

Trott · Dec 26, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19824/

Trott · Dec 26, 2018

Getting closer...

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19825/

BridgeAR · 2018-12-27T00:43:42Z

BridgeAR approved these changes

Dec 27, 2018

View changes

lib/internal/process/promises.js

jasnell · 2018-12-27T17:11:22Z

jasnell approved these changes

Dec 27, 2018

View changes


      src: refactor tickInfo access

- Wrap access to tickInfo fields in functions
- Rename `kHasScheduled` to `kHasTickScheduled` and
  `kHasPromiseRejections` to `kHasRejectionToWarn` for clarity - note
  the latter will be set to false if the rejection does not lead to
  a warning so the previous description is not accurate.
- Set `kHasRejectionToWarn` in JS land of relying on C++ to use
  an implict contract (return value of the promise rejection handler)
  to set it, as the decision is made entirely in JS land.
- Destructure promise reject event constants.


      process: make tick callback and promise rejection callback more robust

- Rename `internalTickCallback` to `processTicksAndRejections`, make
  sure it does not get called if it's not set in C++.
- Rename `emitPromiseRejectionWarnings` to `processPromiseRejections`
  since it also emit events that are not warnings.
- Sets `SetPromiseRejectCallback` in the `Environment` constructor
  to make sure it only gets called once per-isolate, and make
  sure it does not get called if it's not set in C++.
- Wrap promise rejection callback initialization into
  `listenForRejections()`.
- Add comments.


      fixup! process: make tick callback and promise rejection callback mor…

…e robust


      fixup! src: refactor tickInfo access

joyeecheung · Jan 4, 2019

Addressed the reviews. CI: https://ci.nodejs.org/job/node-test-pull-request/19937/

joyeecheung · Jan 6, 2019

Landed in f6a1d88...d18b0a0, thanks!


      src: refactor tickInfo access

- Wrap access to tickInfo fields in functions
- Rename `kHasScheduled` to `kHasTickScheduled` and
  `kHasPromiseRejections` to `kHasRejectionToWarn` for clarity - note
  the latter will be set to false if the rejection does not lead to
  a warning so the previous description is not accurate.
- Set `kHasRejectionToWarn` in JS land of relying on C++ to use
  an implict contract (return value of the promise rejection handler)
  to set it, as the decision is made entirely in JS land.
- Destructure promise reject event constants.

PR-URL: #25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>


      process: make tick callback and promise rejection callback more robust

- Rename `internalTickCallback` to `processTicksAndRejections`, make
  sure it does not get called if it's not set in C++.
- Rename `emitPromiseRejectionWarnings` to `processPromiseRejections`
  since it also emit events that are not warnings.
- Sets `SetPromiseRejectCallback` in the `Environment` constructor
  to make sure it only gets called once per-isolate, and make
  sure it does not get called if it's not set in C++.
- Wrap promise rejection callback initialization into
  `listenForRejections()`.
- Add comments.

PR-URL: #25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>


      src: refactor tickInfo access

- Wrap access to tickInfo fields in functions
- Rename `kHasScheduled` to `kHasTickScheduled` and
  `kHasPromiseRejections` to `kHasRejectionToWarn` for clarity - note
  the latter will be set to false if the rejection does not lead to
  a warning so the previous description is not accurate.
- Set `kHasRejectionToWarn` in JS land of relying on C++ to use
  an implict contract (return value of the promise rejection handler)
  to set it, as the decision is made entirely in JS land.
- Destructure promise reject event constants.

PR-URL: nodejs#25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>


      process: make tick callback and promise rejection callback more robust

- Rename `internalTickCallback` to `processTicksAndRejections`, make
  sure it does not get called if it's not set in C++.
- Rename `emitPromiseRejectionWarnings` to `processPromiseRejections`
  since it also emit events that are not warnings.
- Sets `SetPromiseRejectCallback` in the `Environment` constructor
  to make sure it only gets called once per-isolate, and make
  sure it does not get called if it's not set in C++.
- Wrap promise rejection callback initialization into
  `listenForRejections()`.
- Add comments.

PR-URL: nodejs#25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · Jan 9, 2019

This needs to be backported to v11.x.


      src: refactor tickInfo access

- Wrap access to tickInfo fields in functions
- Rename `kHasScheduled` to `kHasTickScheduled` and
  `kHasPromiseRejections` to `kHasRejectionToWarn` for clarity - note
  the latter will be set to false if the rejection does not lead to
  a warning so the previous description is not accurate.
- Set `kHasRejectionToWarn` in JS land of relying on C++ to use
  an implict contract (return value of the promise rejection handler)
  to set it, as the decision is made entirely in JS land.
- Destructure promise reject event constants.

PR-URL: nodejs#25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>


      process: make tick callback and promise rejection callback more robust

- Rename `internalTickCallback` to `processTicksAndRejections`, make
  sure it does not get called if it's not set in C++.
- Rename `emitPromiseRejectionWarnings` to `processPromiseRejections`
  since it also emit events that are not warnings.
- Sets `SetPromiseRejectCallback` in the `Environment` constructor
  to make sure it only gets called once per-isolate, and make
  sure it does not get called if it's not set in C++.
- Wrap promise rejection callback initialization into
  `listenForRejections()`.
- Add comments.

PR-URL: nodejs#25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · Jan 15, 2019

This applies cleanly now. :)


      src: refactor tickInfo access

- Wrap access to tickInfo fields in functions
- Rename `kHasScheduled` to `kHasTickScheduled` and
  `kHasPromiseRejections` to `kHasRejectionToWarn` for clarity - note
  the latter will be set to false if the rejection does not lead to
  a warning so the previous description is not accurate.
- Set `kHasRejectionToWarn` in JS land of relying on C++ to use
  an implict contract (return value of the promise rejection handler)
  to set it, as the decision is made entirely in JS land.
- Destructure promise reject event constants.

PR-URL: #25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>


      process: make tick callback and promise rejection callback more robust

- Rename `internalTickCallback` to `processTicksAndRejections`, make
  sure it does not get called if it's not set in C++.
- Rename `emitPromiseRejectionWarnings` to `processPromiseRejections`
  since it also emit events that are not warnings.
- Sets `SetPromiseRejectCallback` in the `Environment` constructor
  to make sure it only gets called once per-isolate, and make
  sure it does not get called if it's not set in C++.
- Wrap promise rejection callback initialization into
  `listenForRejections()`.
- Add comments.

PR-URL: #25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>


      src: refactor tickInfo access

- Wrap access to tickInfo fields in functions
- Rename `kHasScheduled` to `kHasTickScheduled` and
  `kHasPromiseRejections` to `kHasRejectionToWarn` for clarity - note
  the latter will be set to false if the rejection does not lead to
  a warning so the previous description is not accurate.
- Set `kHasRejectionToWarn` in JS land of relying on C++ to use
  an implict contract (return value of the promise rejection handler)
  to set it, as the decision is made entirely in JS land.
- Destructure promise reject event constants.

PR-URL: nodejs#25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>


      process: make tick callback and promise rejection callback more robust

- Rename `internalTickCallback` to `processTicksAndRejections`, make
  sure it does not get called if it's not set in C++.
- Rename `emitPromiseRejectionWarnings` to `processPromiseRejections`
  since it also emit events that are not warnings.
- Sets `SetPromiseRejectCallback` in the `Environment` constructor
  to make sure it only gets called once per-isolate, and make
  sure it does not get called if it's not set in C++.
- Wrap promise rejection callback initialization into
  `listenForRejections()`.
- Add comments.

PR-URL: nodejs#25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>


      src: refactor tickInfo access

- Wrap access to tickInfo fields in functions
- Rename `kHasScheduled` to `kHasTickScheduled` and
  `kHasPromiseRejections` to `kHasRejectionToWarn` for clarity - note
  the latter will be set to false if the rejection does not lead to
  a warning so the previous description is not accurate.
- Set `kHasRejectionToWarn` in JS land of relying on C++ to use
  an implict contract (return value of the promise rejection handler)
  to set it, as the decision is made entirely in JS land.
- Destructure promise reject event constants.

PR-URL: nodejs#25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>


      process: make tick callback and promise rejection callback more robust

- Rename `internalTickCallback` to `processTicksAndRejections`, make
  sure it does not get called if it's not set in C++.
- Rename `emitPromiseRejectionWarnings` to `processPromiseRejections`
  since it also emit events that are not warnings.
- Sets `SetPromiseRejectCallback` in the `Environment` constructor
  to make sure it only gets called once per-isolate, and make
  sure it does not get called if it's not set in C++.
- Wrap promise rejection callback initialization into
  `listenForRejections()`.
- Add comments.

PR-URL: nodejs#25200
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added C++ lib / src labels Dec 24, 2018

starkwang added the promises label Dec 24, 2018

joyeecheung force-pushed the joyeecheung:refactor-tasks branch from 147e825 to 698cfad Dec 25, 2018

joyeecheung added 4 commits Dec 22, 2018

fixup! process: make tick callback and promise rejection callback mor…
…e robust

Verified

This commit was signed with a verified signature.

joyeecheung Joyee Cheung

GPG key ID: 92B78A53C8303B8D Learn about signing commits

bdad2db

fixup! src: refactor tickInfo access

Verified

This commit was signed with a verified signature.

joyeecheung Joyee Cheung

GPG key ID: 92B78A53C8303B8D Learn about signing commits

Loading status checks…

4a6765b

joyeecheung force-pushed the joyeecheung:refactor-tasks branch from 698cfad to 4a6765b Jan 4, 2019

joyeecheung added the author ready label Jan 4, 2019

joyeecheung closed this Jan 6, 2019

joyeecheung removed the author ready label Jan 6, 2019

joyeecheung added a commit that referenced this pull request Jan 6, 2019

joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 8, 2019

addaleax added the backport-requested-v11.x label Jan 9, 2019

BridgeAR added this to Backport requested in v11.x Jan 10, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

addaleax removed the backport-requested-v11.x label Jan 15, 2019

addaleax added a commit that referenced this pull request Jan 15, 2019

BridgeAR referenced this pull request Jan 16, 2019

v11.7.0 proposal #25537

Merged

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

MylesBorins referenced this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

targos removed this from Backport requested in v11.x Jan 30, 2019

nodejs/node

process: refactor promise rejection handling #25200

process: refactor promise rejection handling #25200

joyeecheung commented Dec 24, 2018 •

edited

This comment has been minimized.

nodejs-github-bot commented Dec 24, 2018

nodejs-github-bot added C++ lib / src labels Dec 24, 2018

This comment has been minimized.

joyeecheung commented Dec 24, 2018 •

edited by Trott

starkwang added the promises label Dec 24, 2018

joyeecheung force-pushed the joyeecheung:refactor-tasks branch from `147e825` to `698cfad` Dec 25, 2018

This comment has been minimized.

joyeecheung commented Dec 25, 2018

This comment has been minimized.

Trott commented Dec 26, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Trott commented Dec 26, 2018

This comment has been minimized.

Trott commented Dec 26, 2018

joyeecheung force-pushed the joyeecheung:refactor-tasks branch from `698cfad` to `4a6765b` Jan 4, 2019

This comment has been minimized.

joyeecheung commented Jan 4, 2019

joyeecheung added the author ready label Jan 4, 2019

This comment has been minimized.

joyeecheung commented Jan 6, 2019

joyeecheung closed this Jan 6, 2019

joyeecheung removed the author ready label Jan 6, 2019

This comment has been minimized.

addaleax commented Jan 9, 2019

addaleax added the backport-requested-v11.x label Jan 9, 2019

BridgeAR added this to Backport requested in v11.x Jan 10, 2019

addaleax removed the backport-requested-v11.x label Jan 15, 2019

This comment has been minimized.

addaleax commented Jan 15, 2019

targos removed this from Backport requested in v11.x Jan 30, 2019

nodejs/node

Join GitHub today

process: refactor promise rejection handling #25200

Conversation

joyeecheung commented Dec 24, 2018 • edited

src: refactor tickInfo access

process: make tick callback and promise rejection callback more robust

Checklist

This comment has been minimized.

nodejs-github-bot commented Dec 24, 2018

nodejs-github-bot added C++ lib / src labels Dec 24, 2018

This comment has been minimized.

joyeecheung commented Dec 24, 2018 • edited by Trott

starkwang added the promises label Dec 24, 2018

joyeecheung force-pushed the joyeecheung:refactor-tasks branch from 147e825 to 698cfad Dec 25, 2018

This comment has been minimized.

joyeecheung commented Dec 25, 2018

This comment has been minimized.

Trott commented Dec 26, 2018

This comment has been minimized.

addaleax Dec 26, 2018

This comment has been minimized.

joyeecheung Dec 27, 2018

This comment has been minimized.

Trott commented Dec 26, 2018

This comment has been minimized.

Trott commented Dec 26, 2018

joyeecheung force-pushed the joyeecheung:refactor-tasks branch from 698cfad to 4a6765b Jan 4, 2019

This comment has been minimized.

joyeecheung commented Jan 4, 2019

joyeecheung added the author ready label Jan 4, 2019

This comment has been minimized.

joyeecheung commented Jan 6, 2019

joyeecheung closed this Jan 6, 2019

joyeecheung removed the author ready label Jan 6, 2019

This comment has been minimized.

addaleax commented Jan 9, 2019

addaleax added the backport-requested-v11.x label Jan 9, 2019

BridgeAR added this to Backport requested in v11.x Jan 10, 2019

addaleax removed the backport-requested-v11.x label Jan 15, 2019

This comment has been minimized.

addaleax commented Jan 15, 2019

targos removed this from Backport requested in v11.x Jan 30, 2019

joyeecheung commented Dec 24, 2018 •

edited

joyeecheung commented Dec 24, 2018 •

edited by Trott

joyeecheung force-pushed the joyeecheung:refactor-tasks branch from `147e825` to `698cfad` Dec 25, 2018

joyeecheung force-pushed the joyeecheung:refactor-tasks branch from `698cfad` to `4a6765b` Jan 4, 2019