kentcdodds / cross-env

🔀 Cross platform setting of environment scripts
https://www.npmjs.com/package/cross-env
MIT License
6.32k stars 243 forks source link

fix: signal handling #227

Closed baerrach closed 4 years ago

baerrach commented 4 years ago

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:

I've only tested it on Windows...

baerrach commented 4 years ago

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

codecov[bot] commented 4 years ago

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.

kentcdodds commented 4 years ago

:tada: This PR is included in version 7.0.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

XhmikosR commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

We do have CI for windows set up.

XhmikosR commented 4 years ago

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