Skip to content

exit when child has exited, not earlier (mh) #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
binarykitchen wants to merge 1 commit into
base: master
Choose a base branch
from
Open

exit when child has exited, not earlier (mh) #132

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions lib/supervisor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ var debug = true;
var verbose = false;
var ignoredPaths = {};
var forceWatchFlag = false;
var log = console.log
var log = console.log;

exports.run = run;

Expand Down Expand Up @@ -94,7 +94,7 @@ function run (args) {
}
if (executor === "coffee" && (debugFlag || debugBrkFlag)) {
// coffee does not understand debug or debug-brk, make coffee pass options to node
program.unshift("--nodejs")
program.unshift("--nodejs");
}

try {
Expand All @@ -103,18 +103,30 @@ 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');
Copy link
Collaborator

@iangreenleaf iangreenleaf Oct 12, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@binarykitchen binarykitchen Oct 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@iangreenleaf iangreenleaf Oct 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@binarykitchen binarykitchen Oct 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

@iangreenleaf iangreenleaf Oct 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@binarykitchen binarykitchen Oct 16, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, no time right now and I do not have the commit/branch anymore. Also have switched to nodemon :/


log("Sending "+signal+" to child ...");

child.on('exit', function(code) {
log("Program " + executor + " " + program.join(" ") + " exited with code " + code);
exports.child = null;
Copy link
Collaborator

@iangreenleaf iangreenleaf Oct 12, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to clear the property here?

Copy link
Author

@binarykitchen binarykitchen Oct 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scroll down to line 254 of this commit. There the property is cleared too (written by Ben Schüttler). So since this logic is already there, I have respected this pattern and copied that line.

Really, it makes sense to me. Why holding a reference to the child process when it has exited anyway?

Furthermore look further down at the crash() function, it will kill the process when the child is set. That's what I want to avoid when a crash occurs while really exiting the child.

Copy link
Collaborator

@iangreenleaf iangreenleaf Oct 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fair enough. I'll take it on faith that there is a good reason for that line existing, in which case it does make sense to copy it to here.

// Only here, exit supervisor when child process has exited as well
process.exit();
});

child.kill(signal);
} else {
process.exit();
}
process.exit();
});
});
} catch(e) {
// Windows doesn't support signals yet, so they simply don't get this handling.
// https://github.com/joyent/node/issues/1553
}

log("")
log("");
log("Running node-supervisor with");
log(" program '" + program.join(" ") + "'");
log(" --watch '" + watch + "'");
Expand Down