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

process: refactor promise rejection handling #25200

Closed
wants to merge 4 commits into from

Conversation

@joyeecheung
Copy link
Member

commented 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
@joyeecheung

This comment has been minimized.

@starkwang starkwang added the promises label Dec 24, 2018

lib/internal/process/promises.js Outdated Show resolved Hide resolved
lib/internal/process/next_tick.js Outdated Show resolved Hide resolved
lib/internal/process/promises.js Show resolved Hide resolved

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

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented 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

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

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

This comment has been minimized.

Copy link
@addaleax

addaleax Dec 26, 2018

Member

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…

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Dec 27, 2018

Author Member

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

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

Getting closer...

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

lib/internal/process/promises.js Outdated Show resolved Hide resolved
lib/internal/process/promises.js Show resolved Hide resolved
lib/internal/process/promises.js Show resolved Hide resolved
joyeecheung added 4 commits Dec 22, 2018
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.

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

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2019

Landed in f6a1d88...d18b0a0, thanks!

@joyeecheung joyeecheung closed this Jan 6, 2019

joyeecheung added a commit that referenced this pull request Jan 6, 2019
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>
joyeecheung added a commit that referenced this pull request Jan 6, 2019
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>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 8, 2019
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>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 8, 2019
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

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

This needs to be backported to v11.x.

@BridgeAR 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
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>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
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

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

This applies cleanly now. :)

addaleax added a commit that referenced this pull request Jan 15, 2019
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>
addaleax added a commit that referenced this pull request Jan 15, 2019
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>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
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>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
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>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.