cluster: fix race condition setting suicide prop

lib/cluster.js

@@ -434,7 +434,7 @@ function masterInit() {
     else if (message.act === 'listening')
       listening(worker, message);
     else if (message.act === 'suicide')
-      worker.suicide = true;
+      suicide(worker, message);
     else if (message.act === 'close')
       close(worker, message);
   }
@@ -445,6 +445,11 @@ function masterInit() {
     cluster.emit('online', worker);
   }
 
+  function suicide(worker, message) {
+    worker.suicide = true;
+    send(worker, { ack: message.seq });
+  }
+
   function queryServer(worker, message) {
     // Stop processing if worker already disconnecting
     if (worker.suicide)
@@ -541,7 +546,7 @@ function workerInit() {
       if (message.act === 'newconn')
         onconnection(message, handle);
       else if (message.act === 'disconnect')
-        worker.disconnect();
+        _disconnect.call(worker, true);
     }
   };
 
@@ -662,14 +667,36 @@ function workerInit() {
   }
 
   Worker.prototype.disconnect = function() {
+    _disconnect.call(this);
+  };
+
+  Worker.prototype.destroy = function() {
+    this.suicide = true;
+    if (!this.isConnected()) process.exit(0);
+    var exit = process.exit.bind(null, 0);
+    send({ act: 'suicide' }, () => process.disconnect());
+    process.once('disconnect', exit);
+  };
+
+  function send(message, cb) {
+    sendHelper(process, message, null, cb);
+  }
+
+  function _disconnect(masterInitiated) {
     this.suicide = true;
     let waitingCount = 1;
 
     function checkWaitingCount() {
       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());
+        }
       }
     }
 
@@ -681,19 +708,6 @@ function workerInit() {
     }
 
     checkWaitingCount();
-  };
-
-  Worker.prototype.destroy = function() {
-    this.suicide = true;
-    if (!this.isConnected()) process.exit(0);
-    var exit = process.exit.bind(null, 0);
-    send({ act: 'suicide' }, exit);
-    process.once('disconnect', exit);
-    process.disconnect();
-  };
-
-  function send(message, cb) {
-    sendHelper(process, message, null, cb);
   }
 }
 

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

@@ -4,18 +4,19 @@ const assert = require('assert');
 const cluster = require('cluster');
 
 if (cluster.isMaster) {
-  const worker = cluster.fork();
-  let disconnected = false;
+  function forkWorker(action) {
+    const worker = 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';
+const common = require('../common');
+const assert = require('assert');
+const cluster = require('cluster');
+const os = require('os');
+
+if (cluster.isMaster) {
+  function forkWorker(action) {
+    const worker = cluster.fork({ action });
+    worker.on('disconnect', common.mustCall(() => {
+      assert.strictEqual(worker.suicide, true);
+    }));
+
+    worker.on('exit', common.mustCall(() => {
+      assert.strictEqual(worker.suicide, true);
+    }));
+  }
+
+  const cpus = os.cpus().length;
+  const tries = cpus > 8 ? 64 : cpus * 8;
+
+  cluster.on('exit', common.mustCall((worker, code) => {
+    assert.strictEqual(code, 0, 'worker exited with error');
+  }, tries * 2));
+
+  for (let i = 0; i < tries; ++i) {
+    forkWorker('disconnect');
+    forkWorker('kill');
+  }
+} else {
+  cluster.worker[process.env.action]();
+}