More cluster event consistency

Regarding discussion in #3198. Passing the worker as an argument to an event emitted on the worker is redundant, and an unnecessary break in consistency vs the events on the ChildProcess objects. It was removed from 'exit', but 'listening' and others were overlooked. This corrects that oversight.
nodejs · May 5, 2012 · 3d84c3d · 3d84c3d
1 parent 1930772
commit 3d84c3d
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 43 deletions.
diff --git a/doc/api/cluster.markdown b/doc/api/cluster.markdown
@@ -19,7 +19,7 @@ all share server ports.
         cluster.fork();
       }
 
-      cluster.on('exit', function(worker) {
+      cluster.on('exit', function(worker, code, signal) {
         console.log('worker ' + worker.pid + ' died');
       });
     } else {
@@ -77,17 +77,17 @@ When a new worker is forked the cluster module will emit a 'fork' event.
 This can be used to log worker activity, and create you own timeout.
 
     var timeouts = [];
-    var errorMsg = function () {
+    function errorMsg() {
       console.error("Something must be wrong with the connection ...");
-    });
+    }
 
-    cluster.on('fork', function (worker) {
+    cluster.on('fork', function(worker) {
       timeouts[worker.uniqueID] = setTimeout(errorMsg, 2000);
     });
-    cluster.on('listening', function (worker) {
+    cluster.on('listening', function(worker, address) {
       clearTimeout(timeouts[worker.uniqueID]);
     });
-    cluster.on('exit', function (worker) {
+    cluster.on('exit', function(worker, code, signal) {
       clearTimeout(timeouts[worker.uniqueID]);
       errorMsg();
     });
@@ -102,7 +102,7 @@ The difference between 'fork' and 'online' is that fork is emitted when the
 master tries to fork a worker, and 'online' is emitted when the worker is
 being executed.
 
-    cluster.on('online', function (worker) {
+    cluster.on('online', function(worker) {
       console.log("Yay, the worker responded after it was forked");
     });
 
@@ -120,7 +120,7 @@ object and the `address` object contains the following connection properties:
 `address`, `port` and `addressType`. This is very useful if the worker is listening
 on more than one address.
 
-    cluster.on('listening', function (worker, address) {
+    cluster.on('listening', function(worker, address) {
       console.log("A worker is now connected to " + address.address + ":" + address.port);
     });
 
@@ -143,11 +143,14 @@ connections.
 ## Event: 'exit'
 
 * `worker` {Worker object}
+* `code` {Number} the exit code, if it exited normally. 
+* `signal` {String} the name of the signal (eg. `'SIGHUP'`) that caused
+  the process to be killed.
 
 When any of the workers die the cluster module will emit the 'exit' event.
 This can be used to restart the worker by calling `fork()` again.
 
-    cluster.on('exit', function(worker) {
+    cluster.on('exit', function(worker, code, signal) {
       var exitCode = worker.process.exitCode;
       console.log('worker ' + worker.pid + ' died ('+exitCode+'). restarting...');
       cluster.fork();
@@ -225,14 +228,14 @@ In the cluster all living worker objects are stored in this object by there
         callback(cluster.workers[uniqueID]);
       }
     }
-    eachWorker(function (worker) {
+    eachWorker(function(worker) {
       worker.send('big announcement to all workers');
     });
 
 Should you wish to reference a worker over a communication channel, using
 the worker's uniqueID is the easiest way to find the worker.
 
-    socket.on('data', function (uniqueID) {
+    socket.on('data', function(uniqueID) {
       var worker = cluster.workers[uniqueID];
     });
 
@@ -285,7 +288,7 @@ This example will echo back all messages from the master:
       worker.send('hi there');
 
     } else if (cluster.isWorker) {
-      process.on('message', function (msg) {
+      process.on('message', function(msg) {
         process.send(msg);
       });
     }
@@ -296,7 +299,7 @@ This function will kill the worker, and inform the master to not spawn a
 new worker.  The boolean `suicide` lets you distinguish between voluntary
 and accidental exit.
 
-    cluster.on('exit', function (worker) {
+    cluster.on('exit', function(worker, code, signal) {
       if (worker.suicide === true) {
         console.log('Oh, it was just suicide\' – no need to worry').
       }
@@ -324,30 +327,30 @@ that would normally not allow the worker to do any cleanup if needed.
       var worker = cluser.fork();
       var timeout;
 
-      worker.on('listening', function () {
+      worker.on('listening', function(address) {
         worker.disconnect();
-        timeout = setTimeout(function () {
+        timeout = setTimeout(function() {
           worker.send('force kill');
         }, 2000);
       });
 
-      worker.on('disconnect', function () {
+      worker.on('disconnect', function() {
         clearTimeout(timeout);
       });
 
     } else if (cluster.isWorker) {
       var net = require('net');
-      var server = net.createServer(function (socket) {
+      var server = net.createServer(function(socket) {
         // connection never end
       });
 
       server.listen(8000);
 
-      server.on('close', function () {
+      server.on('close', function() {
         // cleanup
       });
 
-      process.on('message', function (msg) {
+      process.on('message', function(msg) {
         if (msg === 'force kill') {
           server.destroy();
         }
@@ -377,15 +380,15 @@ in the master process using the message system:
       }, 1000);
 
       // Count requestes
-      var messageHandler = function (msg) {
+      function messageHandler(msg) {
         if (msg.cmd && msg.cmd == 'notifyRequest') {
           numReqs += 1;
         }
-      };
+      }
 
       // Start workers and listen for messages containing notifyRequest
       cluster.autoFork();
-      Object.keys(cluster.workers).forEach(function (uniqueID) {
+      Object.keys(cluster.workers).forEach(function(uniqueID) {
         cluster.workers[uniqueID].on('message', messageHandler);
       });
 
@@ -403,35 +406,30 @@ in the master process using the message system:
 
 ### Event: 'online'
 
-* `worker` {Worker object}
-
 Same as the `cluster.on('online')` event, but emits only when the state change
 on the specified worker.
 
-    cluster.fork().on('online', function (worker) {
+    cluster.fork().on('online', function() {
       // Worker is online
     };
 
 ### Event: 'listening'
 
-* `worker` {Worker object}
 * `address` {Object}
 
 Same as the `cluster.on('listening')` event, but emits only when the state change
 on the specified worker.
 
-    cluster.fork().on('listening', function (worker, address) {
+    cluster.fork().on('listening', function(address) {
       // Worker is listening
     };
 
 ### Event: 'disconnect'
 
-* `worker` {Worker object}
-
 Same as the `cluster.on('disconnect')` event, but emits only when the state change
 on the specified worker.
 
-    cluster.fork().on('disconnect', function (worker) {
+    cluster.fork().on('disconnect', function() {
       // Worker has disconnected
     };
 
@@ -445,7 +443,7 @@ Emitted by the individual worker instance, when the underlying child process
 is terminated.  See [child_process event: 'exit'](child_process.html#child_process_event_exit). 
 
     var worker = cluster.fork();
-    worker.on('exit', function (code, signal) {
+    worker.on('exit', function(code, signal) {
       if( signal ) {
         console.log("worker was killed by signal: "+signal);
       } else if( code !== 0 ) {

diff --git a/lib/cluster.js b/lib/cluster.js
@@ -184,7 +184,7 @@ if (cluster.isMaster) {
   messageHandler.online = function(message, worker) {
     worker.state = 'online';
     debug('Worker ' + worker.process.pid + ' online');
-    worker.emit('online', worker);
+    worker.emit('online');
     cluster.emit('online', worker);
   };
 
@@ -213,7 +213,7 @@ if (cluster.isMaster) {
     worker.state = 'listening';
 
     // Emit listening, now that we know the worker is listening
-    worker.emit('listening', worker, {
+    worker.emit('listening', {
       address: message.address,
       port: message.port,
       addressType: message.addressType
@@ -297,7 +297,7 @@ function Worker(customEnv) {
   this.process.once('exit', function(exitCode, signalCode) {
     prepareExit(self, 'dead');
     self.emit('exit', exitCode, signalCode);
-    cluster.emit('exit', self);
+    cluster.emit('exit', self, exitCode, signalCode);
   });
   this.process.once('disconnect', function() {
     prepareExit(self, 'disconnected');

diff --git a/test/simple/test-cluster-basic.js b/test/simple/test-cluster-basic.js
@@ -122,14 +122,26 @@ else if (cluster.isMaster) {
       checks.worker.events[name] = true;
 
       //Check argument
-      if (name == 'exit') {
-        checks.worker.equal[name] = (
-          worker.process.exitCode === arguments[0] &&
-          worker.process.signalCode === arguments[1] &&
-          worker === this
-        );
-      } else {
-        checks.worker.equal[name] = worker === arguments[0];
+      checks.worker.equal[name] = (worker === this);
+
+      switch (name) {
+        case 'exit':
+          assert.equal(arguments[0], worker.process.exitCode);
+          assert.equal(arguments[1], worker.process.signalCode);
+          assert.equal(arguments.length, 2);
+          break;
+
+        case 'listening':
+          assert.equal(arguments.length, 1);
+          var expect = { address: '127.0.0.1',
+                         port: common.PORT,
+                         addressType: 4 };
+          assert.deepEqual(arguments[0], expect);
+          break;
+
+        default:
+          assert.equal(arguments.length, 0);
+          break;
       }
     });
   });
@@ -145,7 +157,7 @@ else if (cluster.isMaster) {
     //Check cluster event arguments
     forEach(checks.cluster.equal, function(check, name) {
       assert.ok(check, 'The cluster event "' + name + '" did not emit ' +
-                'with corrent argument');
+                'with correct argument');
     });
 
     //Check worker states