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: group main thread execution preparation code #26000

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 8, 2019

This patch groups the preparation of main thread execution into
prepareMainThreadExecution() and removes the cluster IPC
setup in worker thread bootstrap since clusters do not use
worker threads for its implementation and it's unlikely to change.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • 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 Feb 8, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good!

@joyeecheung
Copy link
Member Author

Added another commit with a test to fix #25967 on master. @addaleax Can you take another look?

CI: https://ci.nodejs.org/job/node-test-pull-request/20655/

@@ -0,0 +1,34 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

Thank you @joyeecheung!

This patch groups the preparation of main thread execution into
`prepareMainThreadExecution()` and removes the cluster IPC
setup in worker thread bootstrap since clusters do not use
worker threads for its implementation and it's unlikely to change.
And make sure that `process.argv` from the preloaded modules
is the same as the one in the main module.

Refs: nodejs#25967
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Skip the test in workers since we can't chdir there. CI: https://ci.nodejs.org/job/node-test-pull-request/20699/

@addaleax
Copy link
Member

addaleax commented Feb 9, 2019

@richardlau
Copy link
Member

Skip the test in workers since we can't chdir there.

You can set the cwd option on spawnsync.

@joyeecheung
Copy link
Member Author

Landed in 70f4f08...69714ab

joyeecheung added a commit that referenced this pull request Feb 10, 2019
This patch groups the preparation of main thread execution into
`prepareMainThreadExecution()` and removes the cluster IPC
setup in worker thread bootstrap since clusters do not use
worker threads for its implementation and it's unlikely to change.

PR-URL: #26000
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
joyeecheung added a commit that referenced this pull request Feb 10, 2019
And make sure that `process.argv` from the preloaded modules
is the same as the one in the main module.

Refs: #25967
PR-URL: #26000
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Feb 10, 2019
This patch groups the preparation of main thread execution into
`prepareMainThreadExecution()` and removes the cluster IPC
setup in worker thread bootstrap since clusters do not use
worker threads for its implementation and it's unlikely to change.

PR-URL: #26000
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Feb 10, 2019
And make sure that `process.argv` from the preloaded modules
is the same as the one in the main module.

Refs: #25967
PR-URL: #26000
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@targos targos mentioned this pull request Feb 14, 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