This repository was archived by the owner on Apr 22, 2023. It is now read-only.
Repository navigation
This repository was archived by the owner on Apr 22, 2023. It is now read-only.
Cluster worker.kill(signal) does not pass the signal parameter #6042
Description
When calling worker.kill(signal) and passing a signal, the signal does not get sent to worker process as per documentation.
http://nodejs.org/api/cluster.html#cluster_worker_kill_signal_sigterm
Example:
var cluster = require('cluster');
if (cluster.isMaster) {
cluster.fork();
cluster.on('exit', function(worker, code, signal) {
console.log('Worker exited:', worker.process.pid);
});
setTimeout(function () {
for (var id in cluster.workers) {
var worker = cluster.workers[id];
worker.kill('SIGUSR2');
}
}, 2000);
} else {
process.on('SIGUSR2', function() {
//WHY DOES THIS NOT GET CALLED? http://nodejs.org/api/cluster.html#cluster_worker_kill_signal_sigterm
console.log('SIGUSR2 called', process.pid);
});
console.log('Worker listening for SIGUSR2', process.pid);
}
Activity
sam-github commentedon Aug 14, 2013
I think this is a bug.
worker.kill() sets .suicide in the master, but it doesn't send suicide to the worker, so master/worker opinion of suicide is not in sync.
Since master does a process.disconnect() before it does a process.kill(), the workers sees an unexpected disconnect, and exits before the signal gets a chance to be delivered.
Probably fix is for master to send a suicide message before doing a worker.process.disconnect(), similar to what the worker does when kill is called in the child.
bnoordhuis commentedon Oct 16, 2013
Been talking it over with Sam and I'm leaning towards removing Worker#kill(). It's an alias for Worker#destroy() right now but it:
Which is weird because e.g.
worker.kill('SIGUSR1')doesn't actually kill the process. I propose we retire it and make Worker#destroy() the officially sanctioned method.Other committers, thoughts?
trevnorris commentedon Oct 16, 2013
@bnoordhuis +1 from me.
sam-github commentedon Oct 16, 2013
@bnoordhuis, since worker.destroy === worker.kill right now, all the problems you enumerate exist for destroy as much as for kill. Did you intend to change it so it can only be used to terminate the worker?
I suggest changing worker.kill() to be an alias to worker.process.kill(), and removing worker.destroy(). This would parallel how worker.send() is an alias to worker.process.send().
I'm also OK with simply removing both .kill and .destroy, though I think leaving the documented .kill around is a bit friendlier to existing code.
Reasons:
I'm happy to implement this (yeah, its only 2 lines of code, but then there are all the docs and tests...), if you all want it.
sam-github commentedon Oct 28, 2013
/cc @isaacs cf. e428bb7
jasnell commentedon May 26, 2015
@sam-github @bnoordhuis ... is this resolved?
sam-github commentedon May 27, 2015
Not resolved.
cluster.destroy()getting renamed tokill()still makes no sense. It disconnects before sending the signal to its child, pretty much guaranteeing the signal won't get delivered, or at least will be racy.diegoperini commentedon Jan 4, 2016
Regarding a workaround, how safe is it to kill a child process via
worker.process.kill()?sam-github commentedon Jan 4, 2016
doing so just sends the signal. whether that is "safe" or not just depends on whether the signal does what you want, which is up to your code. worker.process and its methods are public APIs of worker/cluster, and can be used as doced
diegoperini commentedon Jan 4, 2016
Thank you for the quick response @sam-github