cluster: fix race condition setting suicide prop

There is no guarantee that the `suicide` property of a worker in the
master process is going to be set when the `disconnect` and `exit`
events are emitted.
To fix it, wait for the ACK of the suicide message from the master
before disconnecting the worker. Also, there's no need to send the
suicide message from the worker if the disconnection has been
initiated in the master.
Add `test-cluster-disconnect-suicide-race` that forks a lot of workers
to consistently reproduce the issue this patch tries to solve.
Modify `test-regress-GH-3238` so it checks both the `kill` and
`disconnect` cases. Also take into account that the `disconnect` event
may be received after the `exit` event.
PR-URL: #4349
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

Author: santigimeno

lib/cluster.js

@@ -428,7 +428,7 @@ function masterInit() {
 elseif(message.act==='listening')
 listening(worker,message);
 elseif(message.act==='suicide')
-worker.suicide=true;
+suicide(worker,message);
 elseif(message.act==='close')
 close(worker,message);
 }
@@ -439,6 +439,11 @@ function masterInit() {
 cluster.emit('online',worker);
 }
 
+functionsuicide(worker,message){
+worker.suicide=true;
+send(worker,{ack: message.seq});
+}
+
 functionqueryServer(worker,message){
 varargs=[message.address,
 message.port,
@@ -532,7 +537,7 @@ function workerInit() {
 if(message.act==='newconn')
 onconnection(message,handle);
 elseif(message.act==='disconnect')
-worker.disconnect();
+_disconnect.call(worker,true);
 }
 };
 
@@ -653,14 +658,36 @@ function workerInit() {
 }
 
 Worker.prototype.disconnect=function(){
+_disconnect.call(this);
+};
+
+Worker.prototype.destroy=function(){
+this.suicide=true;
+if(!this.isConnected())process.exit(0);
+varexit=process.exit.bind(null,0);
+send({act: 'suicide'},()=>process.disconnect());
+process.once('disconnect',exit);
+};
+
+functionsend(message,cb){
+sendHelper(process,message,null,cb);
+}
+
+function_disconnect(masterInitiated){
 this.suicide=true;
 letwaitingCount=1;
 
 functioncheckWaitingCount(){
 waitingCount--;
 if(waitingCount===0){
-send({act: 'suicide'});
-process.disconnect();
+// If disconnect is worker initiated, wait for ack to be sure suicide
+// is properly set in the master, otherwise, if it's master initiated
+// there's no need to send the suicide message
+if(masterInitiated){
+process.disconnect();
+}else{
+send({act: 'suicide'},()=>process.disconnect());
+}
 }
 }
 
@@ -672,19 +699,6 @@ function workerInit() {
 }
 
 checkWaitingCount();
-};
-
-Worker.prototype.destroy=function(){
-this.suicide=true;
-if(!this.isConnected())process.exit(0);
-varexit=process.exit.bind(null,0);
-send({act: 'suicide'},exit);
-process.once('disconnect',exit);
-process.disconnect();
-};
-
-functionsend(message,cb){
-sendHelper(process,message,null,cb);
 }
 }
 

test/parallel/test-regress-GH-3238.js

@@ -4,18 +4,19 @@ const assert = require('assert');
 constcluster=require('cluster');
 
 if(cluster.isMaster){
-constworker=cluster.fork();
-letdisconnected=false;
+functionforkWorker(action){
+constworker=cluster.fork({ action });
+worker.on('disconnect',common.mustCall(()=>{
+assert.strictEqual(worker.suicide,true);
+}));
 
-worker.on('disconnect',common.mustCall(function(){
-assert.strictEqual(worker.suicide,true);
-disconnected=true;
-}));
+worker.on('exit',common.mustCall(()=>{
+assert.strictEqual(worker.suicide,true);
+}));
+}
 
-worker.on('exit',common.mustCall(function(){
-assert.strictEqual(worker.suicide,true);
-assert.strictEqual(disconnected,true);
-}));
+forkWorker('disconnect');
+forkWorker('kill');
 }else{
-cluster.worker.disconnect();
+cluster.worker[process.env.action]();
 }

test/sequential/test-cluster-disconnect-suicide-race.js

@@ -0,0 +1,32 @@
+'use strict';
+constcommon=require('../common');
+constassert=require('assert');
+constcluster=require('cluster');
+constos=require('os');
+
+if(cluster.isMaster){
+functionforkWorker(action){
+constworker=cluster.fork({ action });
+worker.on('disconnect',common.mustCall(()=>{
+assert.strictEqual(worker.suicide,true);
+}));
+
+worker.on('exit',common.mustCall(()=>{
+assert.strictEqual(worker.suicide,true);
+}));
+}
+
+constcpus=os.cpus().length;
+consttries=cpus>8 ? 64 : cpus*8;
+
+cluster.on('exit',common.mustCall((worker,code)=>{
+assert.strictEqual(code,0,'worker exited with error');
+},tries*2));
+
+for(leti=0;i<tries;++i){
+forkWorker('disconnect');
+forkWorker('kill');
+}
+}else{
+cluster.worker[process.env.action]();
+}