doxout / recluster

Node clustering library with support for zero downtime reloading
522 stars 44 forks source link

Send "disconnecting" message to workers #9

Closed sandinmyjoints closed 11 years ago

sandinmyjoints commented 11 years ago

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();
                 }
spion commented 11 years ago

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)

spion commented 11 years ago

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 :)

sandinmyjoints commented 11 years ago

Makes sense to me. Opened #10.