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

src: set up process.nextTick and promise handlers with internalBinding('task_queue') #25163

Closed
wants to merge 3 commits into from

Conversation

@joyeecheung
Copy link
Member

commented Dec 21, 2018

Please use https://github.com/nodejs/node/pull/25163/files?w=1 to skip indentation changes

src: move symbols binding into node_symbols.cc

src: move process.nextTick and promise setup into node_task_queue.cc

This patch:

  • Moves the process.nextTick and promise setup C++ code into
    node_task_queue.cc which is exposed as
    internalBinding('task_queue')
  • Makes lib/internal/process/promises.js and
    lib/internal/process/next_tick.js as side-effect-free
    as possible
  • Removes the bootstrapper object being passed into
    bootstrap/node.js, let next_tick.js and promises.js
    load whatever they need from internalBinding('task_queue')
    instead.
  • Rename process._tickCallback to runNextTicks internally
    for clarity but still expose it as process._tickCallback.

Refs: #24961

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
joyeecheung added 2 commits Dec 19, 2018
src: move process.nextTick and promise setup into node_task_queue.cc
This patch:

- Moves the process.nextTick and promise setup C++ code into
  node_task_queue.cc which is exposed as
  `internalBinding('task_queue')`
- Makes `lib/internal/process/promises.js` and
  `lib/internal/process/next_tick.js` as side-effect-free
  as possible
- Removes the bootstrapper object being passed into
  `bootstrap/node.js`, let `next_tick.js` and `promises.js`
  load whatever they need from `internalBinding('task_queue')`
  instead.
- Rename `process._tickCallback` to `runNextTicks` internally
  for clarity but still expose it as `process._tickCallback`.
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2018

@joyeecheung joyeecheung changed the title src: move process.nextTick and promise setup into node_task_queue.cc src: set up process.nextTick and promise handlers with internalBinding('task_queue') Dec 21, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 22, 2018

Fixed the test for --worker. CI: https://ci.nodejs.org/job/node-test-pull-request/19747/ ✔️

@apapirovski
Copy link
Member

left a comment

LGTM

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Dec 24, 2018

Landed in bcea74f...457603e, thanks!

joyeecheung added a commit that referenced this pull request Dec 24, 2018
src: move symbols binding into node_symbols.cc
PR-URL: #25163
Refs: #24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
joyeecheung added a commit that referenced this pull request Dec 24, 2018
src: move process.nextTick and promise setup into node_task_queue.cc
This patch:

- Moves the process.nextTick and promise setup C++ code into
  node_task_queue.cc which is exposed as
  `internalBinding('task_queue')`
- Makes `lib/internal/process/promises.js` and
  `lib/internal/process/next_tick.js` as side-effect-free
  as possible
- Removes the bootstrapper object being passed into
  `bootstrap/node.js`, let `next_tick.js` and `promises.js`
  load whatever they need from `internalBinding('task_queue')`
  instead.
- Rename `process._tickCallback` to `runNextTicks` internally
  for clarity but still expose it as `process._tickCallback`.

PR-URL: #25163
Refs: #24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

This doesn't land cleanly on v11.x, should it be backported?

@targos targos added this to Backport requested in v11.x Dec 28, 2018

refack added a commit to refack/node that referenced this pull request Jan 14, 2019
src: move symbols binding into node_symbols.cc
PR-URL: nodejs#25163
Refs: nodejs#24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
src: move process.nextTick and promise setup into node_task_queue.cc
This patch:

- Moves the process.nextTick and promise setup C++ code into
  node_task_queue.cc which is exposed as
  `internalBinding('task_queue')`
- Makes `lib/internal/process/promises.js` and
  `lib/internal/process/next_tick.js` as side-effect-free
  as possible
- Removes the bootstrapper object being passed into
  `bootstrap/node.js`, let `next_tick.js` and `promises.js`
  load whatever they need from `internalBinding('task_queue')`
  instead.
- Rename `process._tickCallback` to `runNextTicks` internally
  for clarity but still expose it as `process._tickCallback`.

PR-URL: nodejs#25163
Refs: nodejs#24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax added a commit that referenced this pull request Jan 15, 2019
src: move symbols binding into node_symbols.cc
PR-URL: #25163
Refs: #24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax added a commit that referenced this pull request Jan 15, 2019
src: move process.nextTick and promise setup into node_task_queue.cc
This patch:

- Moves the process.nextTick and promise setup C++ code into
  node_task_queue.cc which is exposed as
  `internalBinding('task_queue')`
- Makes `lib/internal/process/promises.js` and
  `lib/internal/process/next_tick.js` as side-effect-free
  as possible
- Removes the bootstrapper object being passed into
  `bootstrap/node.js`, let `next_tick.js` and `promises.js`
  load whatever they need from `internalBinding('task_queue')`
  instead.
- Rename `process._tickCallback` to `runNextTicks` internally
  for clarity but still expose it as `process._tickCallback`.

PR-URL: #25163
Refs: #24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

This applies cleanly now, apart from conflict resolution in parallel/test-bootstrap-modules (we should really do sth about that test…)

@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: move symbols binding into node_symbols.cc
PR-URL: nodejs#25163
Refs: nodejs#24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
src: move process.nextTick and promise setup into node_task_queue.cc
This patch:

- Moves the process.nextTick and promise setup C++ code into
  node_task_queue.cc which is exposed as
  `internalBinding('task_queue')`
- Makes `lib/internal/process/promises.js` and
  `lib/internal/process/next_tick.js` as side-effect-free
  as possible
- Removes the bootstrapper object being passed into
  `bootstrap/node.js`, let `next_tick.js` and `promises.js`
  load whatever they need from `internalBinding('task_queue')`
  instead.
- Rename `process._tickCallback` to `runNextTicks` internally
  for clarity but still expose it as `process._tickCallback`.

PR-URL: nodejs#25163
Refs: nodejs#24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
src: move symbols binding into node_symbols.cc
PR-URL: nodejs#25163
Refs: nodejs#24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
src: move process.nextTick and promise setup into node_task_queue.cc
This patch:

- Moves the process.nextTick and promise setup C++ code into
  node_task_queue.cc which is exposed as
  `internalBinding('task_queue')`
- Makes `lib/internal/process/promises.js` and
  `lib/internal/process/next_tick.js` as side-effect-free
  as possible
- Removes the bootstrapper object being passed into
  `bootstrap/node.js`, let `next_tick.js` and `promises.js`
  load whatever they need from `internalBinding('task_queue')`
  instead.
- Rename `process._tickCallback` to `runNextTicks` internally
  for clarity but still expose it as `process._tickCallback`.

PR-URL: nodejs#25163
Refs: nodejs#24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.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
6 participants
You can’t perform that action at this time.