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

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

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`.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 21, 2018
@joyeecheung
Copy link
Member Author

@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
Copy link
Member Author

joyeecheung commented Dec 22, 2018

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

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joyeecheung
Copy link
Member Author

Landed in bcea74f...457603e, thanks!

joyeecheung added a commit that referenced this pull request Dec 24, 2018
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
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
Copy link
Contributor

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

refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#25163
Refs: nodejs#24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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 pushed a commit that referenced this pull request Jan 15, 2019
PR-URL: #25163
Refs: #24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax pushed a commit that referenced this pull request Jan 15, 2019
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
Copy link
Member

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

@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
PR-URL: nodejs#25163
Refs: nodejs#24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
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 pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
PR-URL: nodejs#25163
Refs: nodejs#24961
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
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 mentioned this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants