Send "disconnecting" message to workers
sandinmyjoints opened this issue · 3 comments
Another one from me. How about when timeout > 0, sending a "disconnecting" message to workers so they have a chance to gracefully cleanup and close out existing connections. I understand that once disconnected they will not receive new connections from master, but their servers may continue to serve existing long-lived connections for a while before abruptly exiting when timeout is over.
Then in my app, I can do process.on("message", function(msg) { if(msg == "disconnecting") cleanUp(); });
. As-is, the app knows nothing about what is going on until it receives SIGTERM from worker.kill
and by then existing clients may have been getting served by old workers for as long as the timeout -- in production, you default to 1 hour for this.
What do you think? I an opening an issue rather than a PR because I am not sure I am thinking about this the right way and not missing something. But for code, I think it could be as simple as:
diff --git a/index.js b/index.js
index 42c3fbc..bc52213 100644
--- a/index.js
+++ b/index.js
@@ -157,6 +157,7 @@ module.exports = function(file, opt) {
if (opt.timeout > 0) {
var timeout = setTimeout(killfn, opt.timeout * 1000);
worker.on('exit', clearTimeout.bind(this, timeout));
+ worker.send('disconnecting');
} else {
killfn();
}
Looks good to me, and I wonder why I haven't done it yet :)
Since we already use process.send({cmd: 'ready'})
, I think that the best way to go about this is to use {cmd: 'disconnect'}
as a message. In the future, the key cmd can be made configurable - in case other IPC activity between the cluster and the workers also uses that message format (resulting with conflicts)
Ah, now I remember why.
If you're using recluster for a listening server worker, its best to simply use:
server.listen(...);
server.on('close', function() {
// disconnected from master, no more clients. clean up.
});
But since I added the option process.send{cmd: 'ready'})
for clustering non-server workers, something like worker.send({cmd: 'disconnect'})
became useful for those. So yeah, I would merge your PR :)
Makes sense to me. Opened #10.