GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — process: refactor bootstrap of worker/main thread stdio, fatalException, and script evaluation by joyeecheung · Pull Request #25199 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Dec 23, 2018

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@joyeecheung
joyeecheung requested a review from addaleax December 23, 2018 23:44
@nodejs-github-bot

nodejs-github-bot commented Dec 23, 2018

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Dec 23, 2018
joyeecheung
joyeecheung commented Dec 23, 2018
View reviewed changes
Comment thread lib/internal/process/execution.js Outdated

@joyeecheung joyeecheung Dec 23, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@joyeecheung

joyeecheung commented Dec 23, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/19771/
Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/19776/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung
joyeecheung force-pushed the worker-stdio branch from 7173692 to 70245bc Compare December 24, 2018 00:22
joyeecheung added 3 commits December 24, 2018 08:23
@joyeecheung
process: move eval and exception bootstrap ito process/execution.js
bcd78cd 
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.
@joyeecheung
process: split worker IO into internal/worker/io.js
4765b70 
- 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.
@joyeecheung
process: move worker bootstrap code into worker_thread_only.js
1c52afd 
Move worker bootstrap code into worker_thread_only.js from
internal/worker.js since they are only run once during bootstrap.
@joyeecheung
joyeecheung force-pushed the worker-stdio branch from 70245bc to 1c52afd Compare December 24, 2018 00:23
@Trott

Trott commented Dec 24, 2018

Copy link
Copy Markdown
Member

Post force-push CI: https://ci.nodejs.org/job/node-test-pull-request/19781/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Dec 24, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Dec 27, 2018

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Dec 27, 2018
View reviewed changes
@joyeecheung

joyeecheung commented Dec 31, 2018

Copy link
Copy Markdown
Member Author

Landed in da13c44...00babd3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung closed this Dec 31, 2018
joyeecheung added a commit that referenced this pull request Dec 31, 2018
@joyeecheung
process: move eval and exception bootstrap ito process/execution.js
7163fbf 
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 <[email protected]>
joyeecheung added a commit that referenced this pull request Dec 31, 2018
@joyeecheung
process: split worker IO into internal/worker/io.js
af23715 
- 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 <[email protected]>
joyeecheung added a commit that referenced this pull request Dec 31, 2018
@joyeecheung
process: move worker bootstrap code into worker_thread_only.js
00babd3 
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 <[email protected]>
@addaleax addaleax added the backport-requested-v11.x label Jan 5, 2019
@addaleax

addaleax commented Jan 5, 2019

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax mentioned this pull request Jan 5, 2019
worker: improve JS-side debugging #25312
Closed
2 tasks done
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung @refack
process: move eval and exception bootstrap ito process/execution.js
950cf41 
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 <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung @refack
process: split worker IO into internal/worker/io.js
53b6344 
- 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 <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung @refack
process: move worker bootstrap code into worker_thread_only.js
4746fdf 
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 <[email protected]>
@addaleax addaleax removed the backport-requested-v11.x label Jan 15, 2019
@addaleax

addaleax commented Jan 15, 2019

Copy link
Copy Markdown
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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@joyeecheung @addaleax
process: move eval and exception bootstrap ito process/execution.js
1c4d7eb 
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 <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@joyeecheung @addaleax
process: split worker IO into internal/worker/io.js
020a9ab 
- 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 <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@joyeecheung @addaleax
process: move worker bootstrap code into worker_thread_only.js
5b1a987 
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 <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
v11.7.0 proposal #25537
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung @BridgeAR
process: move eval and exception bootstrap ito process/execution.js
c51670d 
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 <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung @BridgeAR
process: split worker IO into internal/worker/io.js
84b95ff 
- 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 <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung @BridgeAR
process: move worker bootstrap code into worker_thread_only.js
3e6d1c2 
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 <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
@joyeecheung @BridgeAR
process: move eval and exception bootstrap ito process/execution.js
456b1b5 
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 <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
@joyeecheung @BridgeAR
process: split worker IO into internal/worker/io.js
9480e1b 
- 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 <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
@joyeecheung @BridgeAR
process: move worker bootstrap code into worker_thread_only.js
655c1c9 
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 <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
v11.8.0 proposal #25687
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax Awaiting requested review from addaleax

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@joyeecheung @nodejs-github-bot @Trott @addaleax @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.