cluster: refine worker.destroy function#6502
Uh oh!
There was an error while loading. Please reload this page.
Conversation
| }else{ | ||
| send({act: 'exitedAfterDisconnect'},()=>process.disconnect()); | ||
| process.once('disconnect',exit); | ||
| } |
There was a problem hiding this comment.
Why not eliminate the bind() altogether?
if(!this.isConnected()){process.exit(0);}else{send({act: 'exitedAfterDisconnect'},()=>process.disconnect());process.once('disconnect',()=>process.exit(0));}
jasnell commented May 1, 2016
Generally LGTM with a nit and if CI is green: https://ci.nodejs.org/job/node-test-pull-request/2451/ |
yorkie commented May 1, 2016
@jasnell fixed the nit, could you help me run a new job, thank you :-) |
lib/cluster.js Outdated
| varexit=process.exit.bind(null,0); | ||
| send({act: 'exitedAfterDisconnect'},()=>process.disconnect()); | ||
| process.once('disconnect',exit); | ||
| varcode=0; |
There was a problem hiding this comment.
Can you drop this altogether or at least make it const.
There was a problem hiding this comment.
@cjihrig I choose to use your suggestion 1 because of the consistence with other functions in this module.
jasnell commented May 1, 2016
lib/cluster.js Outdated
| send({act: 'exitedAfterDisconnect'},()=>process.disconnect()); | ||
| process.once('disconnect',exit); | ||
| if(!this.isConnected()){ | ||
| process.exit(); |
There was a problem hiding this comment.
Sorry if there was confusion. In my last comment, I said to either get rid of the var completely, or make it const. By get rid of, I meant make the 0 exit code inline, as it was prior to this PR. Without specifying the 0, this is a very subtle breaking change.
There was a problem hiding this comment.
Ah, right... good point :-)
@yorkie ... this should be process.exit(0) and below
yorkie commented May 1, 2016
jasnell commented May 1, 2016
The default is |
yorkie commented May 1, 2016
So why did we introduce the |
cjihrig commented May 1, 2016
LGTM |
jasnell commented May 1, 2016
New CI: https://ci.nodejs.org/job/node-test-pull-request/2457/ |
yorkie commented May 2, 2016
cjihrig commented May 2, 2016
Nice. Let's let this sit for another day or so in case anyone else wants to weigh in. |
yorkie commented May 3, 2016
Ping @cjihrig |
cjihrig commented May 3, 2016
Thanks, landed in 4e905fa. |
This commit replaces process.exit.bind() with an arrow function in Worker.prototype.destroy(). PR-URL: #6502 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
yorkie commented May 3, 2016
:-) thanks for your advices too |
This commit replaces process.exit.bind() with an arrow function in Worker.prototype.destroy(). PR-URL: #6502 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
This commit replaces process.exit.bind() with an arrow function in Worker.prototype.destroy(). PR-URL: nodejs#6502 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins commented Jun 2, 2016
Added dont-land for lts. Please feel free to let me know if I am incorrect in making this assumption |
5 participants
Checklist
Affected core subsystem(s)
Description of change
This PR just refines a cluster function and make the duplicated exit functions to be shared, so there is no tests or benchmarks could be provided :-)