doxout/recluster

timeout does not work as documented

joshribakoff opened this issue · 5 comments

joshribakoff commented

I am running NODE_ENV=development node backend-cluster.js

backend-cluster.js

var recluster = require('recluster'),
    path = require('path');

process.title = 'backend'

// With relative path, it will only follow the symlink 1x.
// use absolute path to symlink. cuz we want it to follow the symlink every restart
var cluster = recluster(path.join('/var/www/html/app/current/repos/backend.js'), {
    timeout: 30
});
cluster.run();

backend.js

setInterval(() => console.log('foo'),1000)

After editing backend.js and removing the code / changing the log message, I send the kill signal, recluster spawns replacement workers but the old workers do not die & "foo" keeps getting sent to console. I expect the old workers to be killed after 1m since NODE_ENV is dev. I also added the explicit timeout opt for 30 seconds, but "foo" keeps getting sent to console for a long time longer (maybe 1hour?)

joshribakoff commented

Added this to backend.js

process.on("message", function(m) {
  if (m.cmd == "disconnect") {
    process.exit()
  }
});

Still doesn't kill old workers:

 NODE_ENV=development node backend-cluster.js 
spawned cluster, kill -s SIGUSR2 29921 to reload
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
foo1
Got SIGUSR2, reloading cluster...
foo1
foo1
foo1
foo1
foo2
foo2
foo2
foo2
foo1
foo1
foo1
foo1
foo2
foo2
foo2
foo2
foo1
foo1
foo1
foo1
foo2
foo2
foo2
foo2
foo1
foo1
foo1
foo1
foo2
foo2
foo2
foo2
spion commented

recluster doesn't know if the replacements are ready if they don't signal they are. The default is when they start listening on any port. In your case {readyWhen: 'started'} will work, however if async initialisation is necessary, use {readyWhen: 'ready'} from the cluster and process.send({cmd: 'ready'}) from the worker to signal readiness.

There should, however be readiness timeout as well which will kill the replacements if they don't activate quickly enough. Unfortunately I don't have time to work on this project at the moment, and the code could use a bit of an overhaul (using promises will make things much simpler) and better tests before making any additions (side A / side B restarts, rolling restarts and killing in-progress replacements due to timeout or another reload would all be nice). But I will happily review any PRs.

  • 👍1
joshribakoff commented
spion commented

Right, sorry - I edited the response to address that, but it didn't get sent via email:

There should, however be readiness timeout as well which will kill the replacements if they don't activate quickly enough. Unfortunately I don't have time to work on this project at the moment, and the code could use a bit of an overhaul (using promises will make things much simpler) and better tests before making any additions (side A / side B restarts, rolling restarts and killing in-progress replacements due to timeout or another reload would all be nice). But I will happily review any PRs.

joshribakoff commented

Great thanks for your support so far. Yup async/await or promises would be nice, maybe I'll send a PR if I end up making any improvements. Thanks again for clarifying about the "on ready" event that I missed. Marking this as closed.