cluster: refine worker.destroy function#6502
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));
}
|
Generally LGTM with a nit and if CI is green: https://ci.nodejs.org/job/node-test-pull-request/2451/ |
|
@jasnell fixed the nit, could you help me run a new job, thank you :-) |
lib/cluster.js
Outdated
| var exit = process.exit.bind(null, 0); | ||
| send({ act: 'exitedAfterDisconnect' }, () => process.disconnect()); | ||
| process.once('disconnect', exit); | ||
| var code = 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.
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
|
The default is |
|
So why did we introduce the |
|
LGTM |
|
New CI: https://ci.nodejs.org/job/node-test-pull-request/2457/ |
|
Nice. Let's let this sit for another day or so in case anyone else wants to weigh in. |
|
Ping @cjihrig |
|
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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
:-) 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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This commit replaces process.exit.bind() with an arrow function in Worker.prototype.destroy(). PR-URL: nodejs#6502 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
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 :-)