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.

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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

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

@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-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Dec 19, 2018
@joyeecheung
Copy link
Member Author

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

@Fishrock123
Copy link
Contributor

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)

@joyeecheung joyeecheung 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) {
const fd = parseInt(process.env.NODE_CHANNEL_FD, 10);
assert(fd >= 0);
const assert = require('assert').ok;
Copy link
Member

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
Member Author

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

@joyeecheung
Copy link
Member Author

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

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

This applies cleanly now.

@BridgeAR BridgeAR 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]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues 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