Skip to content

process: move child process IPC setup condition into node.js#25130

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.

Sign up for GitHub

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

Jump to bottom
Closed
joyeecheung wants to merge 1 commit into from
Closed

process: move child process IPC setup condition into node.js #25130

joyeecheung wants to merge 1 commit into from

Conversation

@joyeecheung
Copy link
Member

@joyeecheungjoyeecheung commented Dec 19, 2018

Instead of branching in main_thread_only.js, move the branch on
process.env.NODE_CHANNEL_FD in node.js so it's easier to tell when
this needs to happen. Also added comments about what side effect
this causes, and lazy load assert.

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

@joyeecheung
process: move child process IPC setup condition into node.js
b607c1c
Instead of branching in main_thread_only.js, move the branch on process.env.NODE_CHANNEL_FD in node.js so it's easier to tell when this needs to happen. Also added comments about what side effect this causes, and lazy load `assert`.
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 19, 2018

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

@nodejs-github-botnodejs-github-bot added the process Issues and PRs related to the process subsystem. label Dec 19, 2018
@joyeecheung
Copy link
MemberAuthor

joyeecheung commented Dec 19, 2018

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

@Fishrock123
Copy link
Contributor

Fishrock123 commented Dec 19, 2018

CI on MacOS 10.11:

11:13:16 not ok 1898 parallel/test-tls-net-socket-keepalive 11:13:16 --- 11:13:16 duration_ms: 0.274 11:13:16 severity: fail 11:13:16 exitcode: 1 11:13:16 stack: |- 11:13:16 Mismatched noop function calls. Expected exactly 1, actual 0. 11:13:16 at Object.mustCall (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/test/common/index.js:339:10) 11:13:16 at Server.tls.createServer.listen.common.mustCall (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/test/parallel/test-tls-net-socket-keepalive.js:45:27) 11:13:16 at Server.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1011/test/common/index.js:373:15) 11:13:16 at Object.onceWrapper (events.js:277:13) 11:13:16 at Server.emit (events.js:189:13) 11:13:16 at emitListeningNT (net.js:1289:10) 11:13:16 at internalTickCallback (internal/process/next_tick.js:72:19) 11:13:16 at process._tickCallback (internal/process/next_tick.js:47:5) 11:13:16 at Function.Module.runMain (internal/modules/cjs/loader.js:774:11)

@joyeecheungjoyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 20, 2018
BridgeAR
BridgeAR approved these changes Dec 20, 2018
View reviewed changes
lib/internal/process/main_thread_only.js
if(process.env.NODE_CHANNEL_FD){
constfd=parseInt(process.env.NODE_CHANNEL_FD,10);
assert(fd>=0);
constassert=require('assert').ok;
Copy link
Member

@BridgeARBridgeARDec 20, 2018

Choose a reason for hiding this comment

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

Nit: strict was correct. Otherwise there's no need to access ok as it's the same property as the one exported by default.

@joyeecheung
Copy link
MemberAuthor

joyeecheung commented Dec 20, 2018

Resume: https://ci.nodejs.org/job/node-test-pull-request/19703/

@joyeecheung
Copy link
MemberAuthor

joyeecheung commented Dec 22, 2018

Landed in 0c1a388

joyeecheung added a commit that referenced this pull request Dec 22, 2018
@joyeecheung
process: move child process IPC setup condition into node.js
0c1a388
Instead of branching in main_thread_only.js, move the branch on process.env.NODE_CHANNEL_FD in node.js so it's easier to tell when this needs to happen. Also added comments about what side effect this causes, and lazy load `assert`. PR-URL: #25130 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Dec 25, 2018

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
@joyeecheung@refack
process: move child process IPC setup condition into node.js
29102b2
Instead of branching in main_thread_only.js, move the branch on process.env.NODE_CHANNEL_FD in node.js so it's easier to tell when this needs to happen. Also added comments about what side effect this causes, and lazy load `assert`. PR-URL: nodejs#25130 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
@joyeecheung@addaleax
process: move child process IPC setup condition into node.js
6b5c962
Instead of branching in main_thread_only.js, move the branch on process.env.NODE_CHANNEL_FD in node.js so it's easier to tell when this needs to happen. Also added comments about what side effect this causes, and lazy load `assert`. PR-URL: #25130 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@addaleax
Copy link
Member

addaleax commented Jan 14, 2019

This applies cleanly now.

@BridgeARBridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung@BridgeAR
process: move child process IPC setup condition into node.js
32fcc6e
Instead of branching in main_thread_only.js, move the branch on process.env.NODE_CHANNEL_FD in node.js so it's easier to tell when this needs to happen. Also added comments about what side effect this causes, and lazy load `assert`. PR-URL: nodejs#25130 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@lpincalpincalpinca approved these changes

@BridgeARBridgeARBridgeAR approved these changes

+1 more reviewer

@Fishrock123Fishrock123Fishrock123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.processIssues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@joyeecheung@nodejs-github-bot@Fishrock123@MylesBorins@addaleax@jasnell@lpinca@BridgeAR