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 bootstrap of worker/main thread stdio, fatalException, and script evaluation #25199

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

process: move eval and exception bootstrap ito process/execution.js

This patch:

  • Moves tryGetCwd, evalScript and fatalException from
    bootstrap/node.js into process/execution.js so that
    they do have to be passed into the worker thread
    setup function, instead the worker code can require them
    when necessary.
  • Moves setUncaughtExceptionCaptureCallback and
    hasUncaughtExceptionCaptureCallback along with the two
    global state exceptionHandlerState and
    shouldAbortOnUncaughtToggle info process.execution.js
    as those are only used by the fatalException and these
    two accessors as one self-contained unit.

process: split worker IO into internal/worker/io.js

  • Move setupProcessStdio which contains write access to
    the process object into bootstrap/node.js
  • Move MessagePort, MessageChannel, ReadableWorkerStdio,
    and WritableWorkerStdio into internal/worker/io.js
  • Move more worker-specific bootstrap code into
    internal/process/worker_thread_only from setupChild
    in internal/worker.js, and move the process._fatalException
    overwrite into bootstrap/node.js for clarity.

process: move worker bootstrap code into worker_thread_only.js

Move worker bootstrap code into worker_thread_only.js from
internal/worker.js since they are only run once during bootstrap.

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

@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 23, 2018
// this may indicate that node::FatalException should fix up the callback scope
// before calling into process._fatalException, or this function should
// take extra care of the async hooks before it schedules a setImmediate.
function createFatalException() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Fun fact: you can fail the test on master with this diff

diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
index 0f207ab660..a07b1e5657 100644
--- a/lib/internal/bootstrap/node.js
+++ b/lib/internal/bootstrap/node.js
@@ -645,7 +645,7 @@ function setupProcessFatal() {
     emitAfter
   } = NativeModule.require('internal/async_hooks');

-  process._fatalException = (er) => {
+  process._fatalException = function foo(er) {
     // It's possible that defaultTriggerAsyncId was set for a constructor
     // call that threw and was never cleared. So clear it now.
     clearDefaultTriggerAsyncId();

@joyeecheung
Copy link
Member Author

joyeecheung commented Dec 23, 2018

This patch:

- Moves `tryGetCwd`, `evalScript` and `fatalException` from
  `bootstrap/node.js` into `process/execution.js` so that
  they do have to be passed into the worker thread
  setup function, instead the worker code can require them
  when necessary.
- Moves `setUncaughtExceptionCaptureCallback` and
  `hasUncaughtExceptionCaptureCallback` along with the two
  global state `exceptionHandlerState` and
  `shouldAbortOnUncaughtToggle` info `process.execution.js`
  as those are only used by the fatalException and these
  two accessors as one self-contained unit.
- Move `setupProcessStdio` which contains write access to
  the process object into `bootstrap/node.js`
- Move `MessagePort`, `MessageChannel`, `ReadableWorkerStdio`,
  and `WritableWorkerStdio` into `internal/worker/io.js`
- Move more worker-specific bootstrap code into
  `internal/process/worker_thread_only` from `setupChild`
  in `internal/worker.js`, and move the `process._fatalException`
  overwrite into `bootstrap/node.js` for clarity.
Move worker bootstrap code into worker_thread_only.js from
internal/worker.js since they are only run once during bootstrap.
@Trott
Copy link
Member

Trott commented Dec 24, 2018

@Trott
Copy link
Member

Trott commented Dec 24, 2018

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

@joyeecheung
Copy link
Member Author

Ping @addaleax @nodejs/process can I have some review please?

@joyeecheung
Copy link
Member Author

Landed in da13c44...00babd3

joyeecheung added a commit that referenced this pull request Dec 31, 2018
This patch:

- Moves `tryGetCwd`, `evalScript` and `fatalException` from
  `bootstrap/node.js` into `process/execution.js` so that
  they do have to be passed into the worker thread
  setup function, instead the worker code can require them
  when necessary.
- Moves `setUncaughtExceptionCaptureCallback` and
  `hasUncaughtExceptionCaptureCallback` along with the two
  global state `exceptionHandlerState` and
  `shouldAbortOnUncaughtToggle` info `process.execution.js`
  as those are only used by the fatalException and these
  two accessors as one self-contained unit.

PR-URL: #25199
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Dec 31, 2018
- Move `setupProcessStdio` which contains write access to
  the process object into `bootstrap/node.js`
- Move `MessagePort`, `MessageChannel`, `ReadableWorkerStdio`,
  and `WritableWorkerStdio` into `internal/worker/io.js`
- Move more worker-specific bootstrap code into
  `internal/process/worker_thread_only` from `setupChild`
  in `internal/worker.js`, and move the `process._fatalException`
  overwrite into `bootstrap/node.js` for clarity.

PR-URL: #25199
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Dec 31, 2018
Move worker bootstrap code into worker_thread_only.js from
internal/worker.js since they are only run once during bootstrap.

PR-URL: #25199
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member

addaleax commented Jan 5, 2019

Should this be backported to v11.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@addaleax addaleax mentioned this pull request Jan 5, 2019
2 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
This patch:

- Moves `tryGetCwd`, `evalScript` and `fatalException` from
  `bootstrap/node.js` into `process/execution.js` so that
  they do have to be passed into the worker thread
  setup function, instead the worker code can require them
  when necessary.
- Moves `setUncaughtExceptionCaptureCallback` and
  `hasUncaughtExceptionCaptureCallback` along with the two
  global state `exceptionHandlerState` and
  `shouldAbortOnUncaughtToggle` info `process.execution.js`
  as those are only used by the fatalException and these
  two accessors as one self-contained unit.

PR-URL: nodejs#25199
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
- Move `setupProcessStdio` which contains write access to
  the process object into `bootstrap/node.js`
- Move `MessagePort`, `MessageChannel`, `ReadableWorkerStdio`,
  and `WritableWorkerStdio` into `internal/worker/io.js`
- Move more worker-specific bootstrap code into
  `internal/process/worker_thread_only` from `setupChild`
  in `internal/worker.js`, and move the `process._fatalException`
  overwrite into `bootstrap/node.js` for clarity.

PR-URL: nodejs#25199
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Move worker bootstrap code into worker_thread_only.js from
internal/worker.js since they are only run once during bootstrap.

PR-URL: nodejs#25199
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member

Applies cleanly now, apart from the usual hickup in parallel/test-bootstrap-modules and a trivial linter fix (the noop function removed here is still in use in v11.x).

addaleax pushed a commit that referenced this pull request Jan 15, 2019
This patch:

- Moves `tryGetCwd`, `evalScript` and `fatalException` from
  `bootstrap/node.js` into `process/execution.js` so that
  they do have to be passed into the worker thread
  setup function, instead the worker code can require them
  when necessary.
- Moves `setUncaughtExceptionCaptureCallback` and
  `hasUncaughtExceptionCaptureCallback` along with the two
  global state `exceptionHandlerState` and
  `shouldAbortOnUncaughtToggle` info `process.execution.js`
  as those are only used by the fatalException and these
  two accessors as one self-contained unit.

PR-URL: #25199
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 15, 2019
- Move `setupProcessStdio` which contains write access to
  the process object into `bootstrap/node.js`
- Move `MessagePort`, `MessageChannel`, `ReadableWorkerStdio`,
  and `WritableWorkerStdio` into `internal/worker/io.js`
- Move more worker-specific bootstrap code into
  `internal/process/worker_thread_only` from `setupChild`
  in `internal/worker.js`, and move the `process._fatalException`
  overwrite into `bootstrap/node.js` for clarity.

PR-URL: #25199
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 15, 2019
Move worker bootstrap code into worker_thread_only.js from
internal/worker.js since they are only run once during bootstrap.

PR-URL: #25199
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
This patch:

- Moves `tryGetCwd`, `evalScript` and `fatalException` from
  `bootstrap/node.js` into `process/execution.js` so that
  they do have to be passed into the worker thread
  setup function, instead the worker code can require them
  when necessary.
- Moves `setUncaughtExceptionCaptureCallback` and
  `hasUncaughtExceptionCaptureCallback` along with the two
  global state `exceptionHandlerState` and
  `shouldAbortOnUncaughtToggle` info `process.execution.js`
  as those are only used by the fatalException and these
  two accessors as one self-contained unit.

PR-URL: nodejs#25199
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
- Move `setupProcessStdio` which contains write access to
  the process object into `bootstrap/node.js`
- Move `MessagePort`, `MessageChannel`, `ReadableWorkerStdio`,
  and `WritableWorkerStdio` into `internal/worker/io.js`
- Move more worker-specific bootstrap code into
  `internal/process/worker_thread_only` from `setupChild`
  in `internal/worker.js`, and move the `process._fatalException`
  overwrite into `bootstrap/node.js` for clarity.

PR-URL: nodejs#25199
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Move worker bootstrap code into worker_thread_only.js from
internal/worker.js since they are only run once during bootstrap.

PR-URL: nodejs#25199
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
This patch:

- Moves `tryGetCwd`, `evalScript` and `fatalException` from
  `bootstrap/node.js` into `process/execution.js` so that
  they do have to be passed into the worker thread
  setup function, instead the worker code can require them
  when necessary.
- Moves `setUncaughtExceptionCaptureCallback` and
  `hasUncaughtExceptionCaptureCallback` along with the two
  global state `exceptionHandlerState` and
  `shouldAbortOnUncaughtToggle` info `process.execution.js`
  as those are only used by the fatalException and these
  two accessors as one self-contained unit.

PR-URL: nodejs#25199
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
- Move `setupProcessStdio` which contains write access to
  the process object into `bootstrap/node.js`
- Move `MessagePort`, `MessageChannel`, `ReadableWorkerStdio`,
  and `WritableWorkerStdio` into `internal/worker/io.js`
- Move more worker-specific bootstrap code into
  `internal/process/worker_thread_only` from `setupChild`
  in `internal/worker.js`, and move the `process._fatalException`
  overwrite into `bootstrap/node.js` for clarity.

PR-URL: nodejs#25199
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
Move worker bootstrap code into worker_thread_only.js from
internal/worker.js since they are only run once during bootstrap.

PR-URL: nodejs#25199
Reviewed-By: James M Snell <jasnell@gmail.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.

5 participants