Skip to content

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

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

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