Skip to content
This repository has been archived by the owner. It is now read-only.

fix: signal handling #227

Merged
merged 2 commits into from Mar 3, 2020
Merged

Conversation

@baerrach
Copy link
Contributor

@baerrach baerrach commented Feb 13, 2020

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.on listeners.

Why:

See https:github.com/jtlapp/node-cleanup

When you hit Ctrl-C, you send a SIGINT signal to each process in the
current process group. A process group is set of processes that are
all supposed to end together as a group instead of persisting
independently. However, some programs, such as Emacs, intercept and
repurpose SIGINT so that it does not end the process. In such cases,
SIGINT should not end any processes of the group.

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 delegateSignalToChild that 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:

  • Documentation
  • Tests
  • Ready to be merged

I've only tested it on Windows...

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.
@baerrach
Copy link
Contributor Author

@baerrach baerrach commented Feb 13, 2020

The issues #178 #180 don't fix the problem deep enough.

@codecov
Copy link

@codecov codecov bot commented Feb 13, 2020

Codecov Report

Merging #227 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #227   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          93     93           
  Branches       19     19           
=====================================
  Hits           93     93

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f730e45...03e2cf7. Read the comment docs.

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)

This comment has been minimized.

@jsnajdr

jsnajdr Feb 20, 2020

Do I understand correctly that the code does the following?

  • when the parent process (cross-env) receives a SIGTERM signal, 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.

This comment has been minimized.

@baerrach

baerrach Feb 20, 2020
Author Contributor

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

This comment has been minimized.

@jsnajdr

jsnajdr Feb 20, 2020

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

This comment has been minimized.

@baerrach

baerrach Feb 20, 2020
Author Contributor

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.

Copy link
Owner

@kentcdodds kentcdodds left a comment

Thank you for this. It looks good to me. Merging!

@kentcdodds kentcdodds merged commit 8a9cf0e into kentcdodds:master Mar 3, 2020
2 checks passed
2 checks passed
@codecov
codecov/patch Coverage not affected when comparing f730e45...03e2cf7
Details
@codecov
codecov/project 100% remains the same compared to f730e45
Details
@kentcdodds
Copy link
Owner

@kentcdodds kentcdodds commented Mar 3, 2020

🎉 This PR is included in version 7.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@XhmikosR
Copy link

@XhmikosR XhmikosR commented Mar 5, 2020

FYI, I see processes hanging after hitting Ctrl+C on Windows (also one ctrl+C does not close all processes like before). Going back to 7.0.0 works fine.

2020-03-05_10-34-20

kentcdodds added a commit that referenced this pull request Mar 5, 2020
@kentcdodds
Copy link
Owner

@kentcdodds kentcdodds commented Mar 5, 2020

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.

@XhmikosR
Copy link

@XhmikosR XhmikosR commented Mar 5, 2020

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.

@kentcdodds
Copy link
Owner

@kentcdodds kentcdodds commented Mar 5, 2020

We do have CI for windows set up.

@XhmikosR
Copy link

@XhmikosR XhmikosR commented Mar 5, 2020

Indeed, I missed appveyor.yml. Oh, well, not sure what else could be done, then, assuming your tests cover this case.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants