Skip to content

Commit

Permalink
cluster: migrate from worker.suicide
Browse files Browse the repository at this point in the history
Replace it with worker.exitedAfterDisconnect. Print deprecation
message when getting or setting until it is removed.

PR-URL: #3743
Fixes: #3721
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Π‘ΠΊΠΎΠ²ΠΎΡ€ΠΎΠ΄Π° Никита АндрССвич <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
  • Loading branch information
evanlucas committed Apr 26, 2016
1 parent 9bb5a5e commit 4f619bd
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 34 deletions.
46 changes: 37 additions & 9 deletions doc/api/cluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ those servers, and then disconnect the IPC channel.
In the master, an internal message is sent to the worker causing it to call
`.disconnect()` on itself.

Causes `.suicide` to be set.
Causes `.exitedAfterDisconnect` to be set.

Note that after a server is closed, it will no longer accept new connections,
but connections may be accepted by any other listening worker. Existing
Expand Down Expand Up @@ -292,6 +292,27 @@ if (cluster.isMaster) {
}
```

### worker.exitedAfterDisconnect

* {Boolean}

Set by calling `.kill()` or `.disconnect()`. Until then, it is `undefined`.

The boolean `worker.exitedAfterDisconnect` lets you distinguish between voluntary
and accidental exit, the master may choose not to respawn a worker based on
this value.

```js
cluster.on('exit', (worker, code, signal) => {
if (worker.exitedAfterDisconnect === true) {
console.log('Oh, it was just voluntary\' – no need to worry').
}
});

// kill worker
worker.kill();
```

### worker.id

* {Number}
Expand Down Expand Up @@ -322,7 +343,7 @@ This function will kill the worker. In the master, it does this by disconnecting
the `worker.process`, and once disconnected, killing with `signal`. In the
worker, it does it by disconnecting the channel, and then exiting with code `0`.

Causes `.suicide` to be set.
Causes `.exitedAfterDisconnect` to be set.

This method is aliased as `worker.destroy()` for backwards compatibility.

Expand All @@ -340,8 +361,8 @@ is stored.
See: [Child Process module][]

Note that workers will call `process.exit(0)` if the `'disconnect'` event occurs
on `process` and `.suicide` is not `true`. This protects against accidental
disconnection.
on `process` and `.exitedAfterDisconnect` is not `true`. This protects against
accidental disconnection.

### worker.send(message[, sendHandle][, callback])

Expand Down Expand Up @@ -374,24 +395,30 @@ if (cluster.isMaster) {

### worker.suicide

* {Boolean}
Stability: 0 - Deprecated: Use [`worker.exitedAfterDisconnect`][] instead.

Set by calling `.kill()` or `.disconnect()`, until then it is `undefined`.
An alias to [`worker.exitedAfterDisconnect`][].

The boolean `worker.suicide` lets you distinguish between voluntary and accidental
exit, the master may choose not to respawn a worker based on this value.
Set by calling `.kill()` or `.disconnect()`. Until then, it is `undefined`.

The boolean `worker.suicide` lets you distinguish between voluntary
and accidental exit, the master may choose not to respawn a worker based on
this value.

```js
cluster.on('exit', (worker, code, signal) => {
if (worker.suicide === true) {
console.log('Oh, it was just suicide\' – no need to worry').
console.log('Oh, it was just voluntary\' – no need to worry').
}
});

// kill worker
worker.kill();
```

This API only exists for backwards compatibility and will be removed in the
future.

## Event: 'disconnect'

* `worker` {cluster.Worker}
Expand Down Expand Up @@ -707,6 +734,7 @@ socket.on('data', (id) => {
[`disconnect`]: child_process.html#child_process_child_disconnect
[`kill`]: process.html#process_process_kill_pid_signal
[`server.close()`]: net.html#net_event_close
[`worker.exitedAfterDisconnect`]: #cluster_worker_exitedafterdisconnect
[Child Process module]: child_process.html#child_process_child_process_fork_modulepath_args_options
[child_process event: 'exit']: child_process.html#child_process_event_exit
[child_process event: 'message']: child_process.html#child_process_event_message
48 changes: 31 additions & 17 deletions lib/cluster.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,20 @@ function Worker(options) {
if (options === null || typeof options !== 'object')
options = {};

this.suicide = undefined;
this.exitedAfterDisconnect = undefined;

Object.defineProperty(this, 'suicide', {
get: function() {
// TODO: Print deprecation message.
return this.exitedAfterDisconnect;
},
set: function(val) {
// TODO: Print deprecation message.
this.exitedAfterDisconnect = val;
},
enumerable: true
});

this.state = options.state || 'none';
this.id = options.id | 0;

Expand Down Expand Up @@ -355,7 +368,7 @@ function masterInit() {
removeWorker(worker);
}

worker.suicide = !!worker.suicide;
worker.exitedAfterDisconnect = !!worker.exitedAfterDisconnect;
worker.state = 'dead';
worker.emit('exit', exitCode, signalCode);
cluster.emit('exit', worker, exitCode, signalCode);
Expand All @@ -376,7 +389,7 @@ function masterInit() {
*/
if (worker.isDead()) removeWorker(worker);

worker.suicide = !!worker.suicide;
worker.exitedAfterDisconnect = !!worker.exitedAfterDisconnect;
worker.state = 'disconnected';
worker.emit('disconnect');
cluster.emit('disconnect', worker);
Expand Down Expand Up @@ -407,7 +420,7 @@ function masterInit() {
};

Worker.prototype.disconnect = function() {
this.suicide = true;
this.exitedAfterDisconnect = true;
send(this, { act: 'disconnect' });
removeHandlesForWorker(this);
removeWorker(this);
Expand All @@ -432,8 +445,8 @@ function masterInit() {
queryServer(worker, message);
else if (message.act === 'listening')
listening(worker, message);
else if (message.act === 'suicide')
suicide(worker, message);
else if (message.act === 'exitedAfterDisconnect')
exitedAfterDisconnect(worker, message);
else if (message.act === 'close')
close(worker, message);
}
Expand All @@ -444,14 +457,14 @@ function masterInit() {
cluster.emit('online', worker);
}

function suicide(worker, message) {
worker.suicide = true;
function exitedAfterDisconnect(worker, message) {
worker.exitedAfterDisconnect = true;
send(worker, { ack: message.seq });
}

function queryServer(worker, message) {
// Stop processing if worker already disconnecting
if (worker.suicide)
if (worker.exitedAfterDisconnect)
return;
var args = [message.address,
message.port,
Expand Down Expand Up @@ -533,7 +546,7 @@ function workerInit() {
cluster.worker = worker;
process.once('disconnect', function() {
worker.emit('disconnect');
if (!worker.suicide) {
if (!worker.exitedAfterDisconnect) {
// Unexpected disconnect, master exited, or some such nastiness, so
// worker exits immediately.
process.exit(0);
Expand Down Expand Up @@ -670,10 +683,10 @@ function workerInit() {
};

Worker.prototype.destroy = function() {
this.suicide = true;
this.exitedAfterDisconnect = true;
if (!this.isConnected()) process.exit(0);
var exit = process.exit.bind(null, 0);
send({ act: 'suicide' }, () => process.disconnect());
send({ act: 'exitedAfterDisconnect' }, () => process.disconnect());
process.once('disconnect', exit);
};

Expand All @@ -682,19 +695,20 @@ function workerInit() {
}

function _disconnect(masterInitiated) {
this.suicide = true;
this.exitedAfterDisconnect = true;
let waitingCount = 1;

function checkWaitingCount() {
waitingCount--;
if (waitingCount === 0) {
// 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 disconnect is worker initiated, wait for ack to be sure
// exitedAfterDisconnect is properly set in the master, otherwise, if
// it's master initiated there's no need to send the
// exitedAfterDisconnect message
if (masterInitiated) {
process.disconnect();
} else {
send({ act: 'suicide' }, () => process.disconnect());
send({ act: 'exitedAfterDisconnect' }, () => process.disconnect());
}
}
}
Expand Down
10 changes: 7 additions & 3 deletions test/parallel/test-cluster-worker-disconnect.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@ if (cluster.isWorker) {
http.Server(function() {

}).listen(common.PORT, '127.0.0.1');
const worker = cluster.worker;
assert.strictEqual(worker.exitedAfterDisconnect, worker.suicide);

cluster.worker.on('disconnect', function() {
assert.strictEqual(cluster.worker.exitedAfterDisconnect,
cluster.worker.suicide);
process.exit(42);
});

Expand All @@ -26,7 +30,7 @@ if (cluster.isWorker) {
emitDisconnectInsideWorker: false,
emitExit: false,
state: false,
suicideMode: false,
voluntaryMode: false,
died: false
}
};
Expand Down Expand Up @@ -60,7 +64,7 @@ if (cluster.isWorker) {
// Check worker events and properties
worker.once('disconnect', function() {
checks.worker.emitDisconnect = true;
checks.worker.suicideMode = worker.suicide;
checks.worker.voluntaryMode = worker.exitedAfterDisconnect;
checks.worker.state = worker.state;
});

Expand All @@ -86,7 +90,7 @@ if (cluster.isWorker) {

// flags
assert.equal(w.state, 'disconnected', 'The state property was not set');
assert.equal(w.suicideMode, true, 'Suicide mode was not set');
assert.equal(w.voluntaryMode, true, 'Voluntary exit mode was not set');

// is process alive
assert.ok(w.died, 'The worker did not die');
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-cluster-worker-exit.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// verifies that, when a child process exits (by calling `process.exit(code)`)
// - the parent receives the proper events in the proper order, no duplicates
// - the exitCode and signalCode are correct in the 'exit' event
// - the worker.suicide flag, and worker.state are correct
// - the worker.exitedAfterDisconnect flag, and worker.state are correct
// - the worker process actually goes away

var common = require('../common');
Expand Down Expand Up @@ -32,6 +32,8 @@ if (cluster.isWorker) {
worker_emitExit: [1, "the worker did not emit 'exit'"],
worker_state: ['disconnected', 'the worker state is incorrect'],
worker_suicideMode: [false, 'the worker.suicide flag is incorrect'],
worker_exitedAfterDisconnect: [false,
'the .exitedAfterDisconnect flag is incorrect'],
worker_died: [true, 'the worker is still running'],
worker_exitCode: [EXIT_CODE, 'the worker exited w/ incorrect exitCode'],
worker_signalCode: [null, 'the worker exited w/ incorrect signalCode']
Expand Down Expand Up @@ -66,6 +68,7 @@ if (cluster.isWorker) {
worker.on('disconnect', function() {
results.worker_emitDisconnect += 1;
results.worker_suicideMode = worker.suicide;
results.worker_exitedAfterDisconnect = worker.exitedAfterDisconnect;
results.worker_state = worker.state;
if (results.worker_emitExit > 0) {
process.nextTick(function() { finish_test(); });
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-cluster-worker-forced-exit.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ var cluster = require('cluster');
var SENTINEL = 42;

// workers forcibly exit when control channel is disconnected, if
// their .suicide flag isn't set
// their .exitedAfterDisconnect flag isn't set
//
// test this by:
//
Expand Down
7 changes: 4 additions & 3 deletions test/parallel/test-cluster-worker-kill.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// verifies that, when a child process is killed (we use SIGKILL)
// - the parent receives the proper events in the proper order, no duplicates
// - the exitCode and signalCode are correct in the 'exit' event
// - the worker.suicide flag, and worker.state are correct
// - the worker.exitedAfterDisconnect flag, and worker.state are correct
// - the worker process actually goes away

var common = require('../common');
Expand All @@ -29,7 +29,8 @@ if (cluster.isWorker) {
worker_emitDisconnect: [1, "the worker did not emit 'disconnect'"],
worker_emitExit: [1, "the worker did not emit 'exit'"],
worker_state: ['disconnected', 'the worker state is incorrect'],
worker_suicideMode: [false, 'the worker.suicide flag is incorrect'],
worker_exitedAfter: [false,
'the .exitedAfterDisconnect flag is incorrect'],
worker_died: [true, 'the worker is still running'],
worker_exitCode: [null, 'the worker exited w/ incorrect exitCode'],
worker_signalCode: [KILL_SIGNAL,
Expand Down Expand Up @@ -65,7 +66,7 @@ if (cluster.isWorker) {
// Check worker events and properties
worker.on('disconnect', function() {
results.worker_emitDisconnect += 1;
results.worker_suicideMode = worker.suicide;
results.worker_exitedAfter = worker.exitedAfterDisconnect;
results.worker_state = worker.state;
});

Expand Down

2 comments on commit 4f619bd

@Kreijstal
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's way more verbose.

@addaleax
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rainbowdangerdash That is not necessarily a bad thing (and I don’t know if you are implying that it might be); the new name is arguably better at reflecting what is actually represented by the property.

Please sign in to comment.