mysticatea / npm-run-all

A CLI tool to run multiple npm-scripts in parallel or sequential.

MIT License

5.72k stars 240 forks source link

Handle Signals #171

Closed wyattjoh closed 5 years ago

wyattjoh commented 5 years ago

We discovered an issue where npm-run-all swallows errors caused by the v8 runtime, specifically, the SIGABRT signal sent when the node process experiences a JavaScript heap out of memory error.

From the documentation available https://nodejs.org/api/child_process.html, it indicates that when the process closes, if the code provided is null, then the subprocess terminated due to a signal.

The standard behaviour that NodeJS implements when running code that causes these errors directly is to return an exit code of 128 plus the value of the signal code.

This PR does one thing, checks to see if the code is null, and a signal was provided, convert the signal to it's number version, and then mutates the return code used by the rest of npm-run-all by adding 128 to the signal value.

codecov[bot] commented 5 years ago

Codecov Report

Merging #171 into master will decrease coverage by 0.36%. The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   92.94%   92.58%   -0.37%     
==========================================
  Files          22       22              
  Lines         468      472       +4     
==========================================
+ Hits          435      437       +2     
- Misses         33       35       +2

Impacted Files	Coverage Δ
lib/run-task.js	`86.76% <100%> (ø)`	:arrow_up:
lib/run-tasks.js	`83.09% <50%> (-1.98%)`	:arrow_down:

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 5017a88...d293933. Read the comment docs.

mysticatea commented 5 years ago

Thank you for this PR. I had not noticed the second parameter.

Generally LGTM, but the signal-numbers package has been deprecated. Would you know another option?

wyattjoh commented 5 years ago

Good point about the signal-numbers package, but thankfully, those codes won't change for a long time, so I'd think this would be good for now as I couldn't find another package that did the same thing.

mysticatea commented 5 years ago

Well, in that case, I want to have that numbers in itself instead of using the package.

Because I have experienced malicious code injection via inactive packages (https://github.com/mysticatea/npm-run-all/issues/149), I want not to use deprecated packages.

wyattjoh commented 5 years ago

Not a problem! I've addressed the fix @mysticatea!

n9niwas commented 3 years ago

is there a plan to release this?

latest version in npm doesn't have this fix