exit when child has exited, not earlier (mh) #132
Conversation
|
What happens to your app without this feature? |
|
Without this, the graceful shutdown procedure is interrupted. |
|
+1 would like to see this merged as well. Right now support for graceful shutdown for the child is not present in process managers for node.js. |
|
+1 I have been looking for graceful shutdown support. |
|
any news on graceful shutdown or workarrounds? this is a pain when combined with the file watcher and then having corrupt DB/Cache because my script is beheaded.. |
|
Ok, I can see how this could be useful when some of the child processes take a moment to finish dying. Are those of you who've +1'd this using it on your systems? I'd like to hear from some folks who have tested this branch in daily use, to see if it works well in practice and if there are any ill side effects that commonly show up (for example, child processes hanging or not signalling their exit correctly). |
| @@ -103,10 +103,22 @@ function run (args) { | |||
| process.on(signal, function () { | |||
| var child = exports.child; | |||
| if (child) { | |||
| log("Sending "+signal+" to child..."); | |||
| // Remove all exit handlers that might cause a restart (mh) | |||
| child.removeAllListeners('exit'); | |||
There was a problem hiding this comment.
I'm not sure I like this line. I don't understand exactly why it's here, which starts to feel superstitious. Could you give me a scenario in which the child would have an exit handler hanging around that would interfere?
There was a problem hiding this comment.
Yeah, two reasons: 1. there are other process managers out there with custom kill signals like SIGINT2 with their own exit handlers doing other stuff. 2. to avoid deadlocks when the upper process has spawned multiple child processes and these child processes have a graceful shutdown procedure (= having a setTimeout(cb, process.kill) in them).
There was a problem hiding this comment.
In those situations, wouldn't the best course of action be to leave the existing listeners alone? If the managed process has an exit handler in place to do graceful shutdown stuff, us removing it before killing the process seems like it would lead to bad things. I guess I'm still have trouble understanding in what situation we'd encounter an actual deadlock.
There was a problem hiding this comment.
Well, to be honest, I cannot remember perfectly. This commit of mine is pretty old. Five months is a long time for a busy hacker. I have tested it with my own apps and had this deadlock issue: the child process crashed during it's own graceful shutdown procedure and made the parent process wait forever. Hence the child.removeAllListeners('exit') to stop this. Something like that. I reckon I should have written more comments. But on the other hand, if you had asked me that question earlier I could have given you a much better answer ...
Also, the lack of unit tests here is another disadvantage here!
There was a problem hiding this comment.
Yeah, I know - sorry for letting this sit around. I would love to have tests for this package, but just haven't yet figured out a time-efficient way to set some up and have them be useful.
For now, let's please remove these lines, and I'll be happy to merge it. Then, if people encounter problems, we can come back to this with a better understanding of why it might be necessary.
There was a problem hiding this comment.
Sorry, no time right now and I do not have the commit/branch anymore. Also have switched to nodemon :/
With this little addition, the supervisor won't exit until the child process has really exited.
I need this feature because my app has a graceful shutdown.
Already tested it on my machine and works well. Also added few missing semicolons.