Open dimikot opened 4 months ago
Thanks for the comprehensive bug report! We've had this behaviour for a long while (#150 + #164), which was probably pragmatic back then.
I think it's straightforward to fix this for unix-based systems; for Windows, it's more complicated due to it always using cmd.exe even on WSL.
Here is a longread about best practices in shell-like programs (and programs launching other programs) on how to handle SIGINT: https://www.cons.org/cracauer/sigint.html
So if I understood you correctly, exiting cleanly with code 0 was intentionally implemented in https://github.com/open-cli-tools/concurrently/pull/164, as a work-around, to not let npm print a nasty error message when ^C is pressed. If so, then I suspect that the implementation should've been different: instead of exiting with zero exitcode when a child process received a SIGINT (and thus making npm happy), concurrently
should've terminated itself with SIGINT in the end (instead of exiting with any exit code, 0 or not 0). In this case, I believe npm would behave nicely. This is all explained in details in the above article.
In short, there are 3 ways to terminate a process:
process.removeAllListeners("SIGINT"); process.kill(process.pid, "SIGINT")
(or any other signal, but SIGINT is the most interesting here). This is what a shell-like program (like concurrently
) should do when it detects that one of its children finished with SIGINT. Notice that this method has nothing to do with exit codes (although technically, the exit code is returned as 130, but the caller like npm or bash uses some special API to detect whether a child exited because of a signal or not).
MacOS and Linux at least.
Sounds like
concurrently
tool reaction on receiving SIGINT is exiting with code 0 (instead of exiting with e.g. code 130 as the best-practice reaction on Unix processes). This breaks scripts like:When ^C is pressed in the above script, it still prints "still there", although it should not.
The reason why proper SIGINT handling is important is also described here: https://mywiki.wooledge.org/SignalTrap#When_is_the_signal_handled.3F, section "Special Note On SIGINT and SIGQUIT".
Repro
Console 1:
Console 2:
Then ^C in console 3:
When ^C is pressed, SIGINT is sent to the entire process group (which is -78069 in this example). I.e. SIGINT is sent to all 4 processes on the screenshot. The bug is that, when
concurrently
itself receives that SIGINT, it exits with exitcode=0, i.e. it tells the caller that it terminated successfully, although it's not true. According to default unix practices, the process killed by SIGINT should exit with a nonzero exit code (ideally with code=130 which is 128+SIGINT).We can reproduce the same behavior by not pressing ^C, but by:
concurrently
pid itself.kill -SIGINT -78069
in the above example.Interestingly enough, this happens only when receiving SIGINT. On e.g. SIGTERM or SIGHUP it behaves properly.
P.S. There is an ugly work-around for this:
Since the shell itself also receives that ^C SIGINT (it's a member of the process group), it fails in
wait
call, so the message is not printed. But again, this is not a good practice (it is based on a side effect, e.g. it still doesn't help when there is no ^C involved, and onlyconcurrently
tool is sent with a SIGINT).