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: https://github.com/nodejs/node/pull/4349
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

diff --git a/lib/cluster.js b/lib/cluster.js
index f202f25cdd522ab438d9161ca9eee260303bb4c0..0900b22ca592a48e6d63155c7b5c15c345c0b6b3 100644 (file)
--- a/lib/cluster.js
+++ b/lib/cluster.js
@@ -428,7 +428,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);
   }
@@ -439,6 +439,11 @@ function masterInit() {
     cluster.emit('online', worker);
   }
 
+  function suicide(worker, message) {
+    worker.suicide = true;
+    send(worker, { ack: message.seq });
+  }
+
   function queryServer(worker, message) {
     var args = [message.address,
                 message.port,
@@ -532,7 +537,7 @@ function workerInit() {
       if (message.act === 'newconn')
         onconnection(message, handle);
       else if (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);
+    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());
+        }
       }
     }
 
@@ -672,19 +699,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);
   }
 }
 

diff --git a/test/parallel/test-regress-GH-3238.js b/test/parallel/test-regress-GH-3238.js
index a92a09db5fe4b15181ed5d8a11273e01c735b2c3..e6fe030bda9a104c9343a3f8721f730276c77446 100644 (file)
--- a/test/parallel/test-regress-GH-3238.js
+++ b/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]();
 }

diff --git a/test/sequential/test-cluster-disconnect-suicide-race.js b/test/sequential/test-cluster-disconnect-suicide-race.js
new file mode 100644 (file)
index 0000000..e05c420
--- /dev/null
+++ b/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]();
+}