fix: signal handling #227
fix: signal handling #227
Conversation
Signals should delegate to the child process to determine what to do as cross-env is a facade to spawning them cross platform. SIGINT, in particular, can decide swallow the signal and continue on. cross-env needs to wait for the child to decide when it's time to exit. fixed leaking `process.on` listeners.
Codecov Report
@@ Coverage Diff @@
## master #227 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 4 4
Lines 93 93
Branches 19 19
=====================================
Hits 93 93Continue to review full report at Codecov.
|
| if (signal !== null) { | ||
| // Pass through child's signal to parent. | ||
| // SIGINT should not be transformed into a 0 exit code | ||
| process.kill(process.pid, signal) |
There was a problem hiding this comment.
Do I understand correctly that the code does the following?
- when the parent process (
cross-env) receives aSIGTERMsignal, it will handle it by sending it to the child process - then when the child process exits and its exit status says it's been killed with a signal, the parent sends that signal to itself?
- that means that the parent process receives the signal two times: first from external source, handled by a custom handler that forwards it to the child, second time from itself, and handled by the default handler, as the custom handler was removed a moment ago?
- the second
SIGTERM, with custom handler removed, will cause the parent process to terminate, as that's what the default handler does? - the parent process will end with an exit status that says it was terminated with
SIGTERM?
The SIGINT handling seems to be a bit special: the parent process' custom handler simply ignores it, and relies on the fact that the child process received the SIGINT signal, too. Then in the child.on('exit') callback, it will uninstall the "ignore" handler and will send SIGINT to itself. Then it will finally terminate with SIGINT signal exit status.
One case that looks a bit suspicious: if I kill the parent process explicitly, with kill -s SIGINT <parent-pid>, then it will ignore the signal and do nothing. Both processes continue to run.
There was a problem hiding this comment.
Yes your understanding is correct for the example using SIGTERM and all other non-SIGINT signals that are listened for.
For the example of SIGINT, all I can say is the documentation I found indicates this is what happens on Unix systems. I haven't tested this on that platform. If you can find a definitive guide to signal handling I"m happy to adjust the code.
On Windows there is no such thing as signals, they are emulated by Node Signal Events
Did you try the kill -s SIGINT <parent-pid>? As I'm on windows I dont have those utilities.
| } | ||
| } | ||
|
|
||
| const sigtermHandler = delegateSignalToChild('SIGTERM') |
There was a problem hiding this comment.
The signal event receives the signal name as the argument. That means you don't need to create a special handler for each signal:
const delegateSignalToChild = signal => {
process.kill(child.pid, signal);
}
process.on('SIGTERM', delegateSignalToChild);
process.on('SIGBREAK', delegateSignalToChild);There was a problem hiding this comment.
Sure. I was following other examples around the place to get the correct handling working.
As the original code created inline arrow functions to handle the individual events I just kept a similar code flow.
I wasn't sure whether the listeners would vary or be removed independently so went with the more verbose style.
|
🎉 This PR is included in version 7.0.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
FYI, I see processes hanging after hitting
|
This reverts commit 8a9cf0e.
|
This PR has been reverted. Sorry for the trouble @XhmikosR. Thanks for letting us know of the problem. Let's ensure that we address the core issues and handle cases like this. |
|
Thanks @kentcdodds! Not sure if you had CI run on Windows would help. Maybe it's time to try GitHub Actions CI? :) PS. I opened a new issue this morning, because I wasn't sure if you'd notice my comment here #230. Feel free to close it. |
|
We do have CI for windows set up. |
|
Indeed, I missed appveyor.yml. Oh, well, not sure what else could be done, then, assuming your tests cover this case. |
What:
Signals should delegate to the child process to determine what to
do as cross-env is a facade to spawning them cross platform.
SIGINT, in particular, can decide swallow the signal and continue on.
cross-env needs to wait for the child to decide when it's time to exit.
fixed leaking
process.onlisteners.Why:
See https:github.com/jtlapp/node-cleanup
The current implementation delegates the SIGINT received by the parent to child, so the child receives two signals. And the process terminates abruptly.
How:
Created a
delegateSignalToChildthat checked to see if child still exists before delegating signals. Doesn't delegate SIGINT.Removed process listeners when child exits.
Sets process.exitCode as per https://nodejs.org/api/process.html#process_process_exit_code instead of called
process.exit(errorCode).Checklist:
I've only tested it on Windows...