Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cluster: migrate from worker.suicide #3743

Merged
merged 1 commit into from
Apr 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if there is a reason to duplicate the exitedVoluntarily documentation here, just a link should do it IMO.
I don't have a strong opinion on this, though. It would perhaps be better to wait for more opinions here.

Copy link
Member

Choose a reason for hiding this comment

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

It's likely not necessary to duplicate but pulling out the redundant text can be done in a separate PR

Copy link
Member

@ChALkeR ChALkeR Apr 25, 2016

Choose a reason for hiding this comment

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

/cc @nodejs/collaborators — should we keep this part, or just replace it with the following:

### worker.suicide

Stability: 0 - Deprecated: Use [`worker.exitedVoluntarily`][] instead.

An alias to [`worker.exitedVoluntarily`][].

removing all the duplicate text?

Update: ah, sorry, missed the last comment by @jasnell.


```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');
Copy link
Member

Choose a reason for hiding this comment

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

This file still has voluntary exit and voluntary references. It's only a test file, though.

Copy link
Member

Choose a reason for hiding this comment

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

It'a actually fine there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good catch. Fixing now


// 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