Signal handling improvements #10958
Signal handling improvements #10958
Conversation
composer.lockPackage changes
Settings · Docs · Powered by Private Packagist |
| $process->run($callback); | ||
| $signalHandler = SignalHandler::create([SignalHandler::SIGINT, SignalHandler::SIGTERM, SignalHandler::SIGHUP], function (string $signal) { | ||
| if ($this->io !== null) { | ||
| $this->io->writeError('Received '.$signal.', aborting when child process is done', true, IOInterface::DEBUG); |
There was a problem hiding this comment.
Maybe I'm wrong about how this works, but I don't think anything here actually forwards the signals to the child processes? At least a sigint would likely correctly interrupt a child process too if it was to receive it as the user intended?
There was a problem hiding this comment.
When you signal (at least with a ctrl-C, but I can't imagine it's different with other signaling methods) the whole process tree gets signaled, so the child gets it even without forwarding.
There was a problem hiding this comment.
There is one issue I have found in symfony/process it's that even if the child process handles/suppresses the signal, and exit(0), Process will still throw a ProcessSignaledException. I need to investigate a bit more to see whether this can be fixed as it'd be nice for completeness but I am not sure this may be a PHP limitation, and anyway it can't happen in this repo so I'm ignoring this for now.
There was a problem hiding this comment.
I'm a bit late to the party here, forgive me.
I just want to clarify one important aspect: Ctrl+C is different from a SIGINT.
In a terminal, a Ctrl+C sends a SIGINT to every process that has the same process group ID as the foreground process group. This is the equivalent of doing kill -INT -$composerpid (negative process number means process group number, and the lead process of a process group's process ID defines the group ID).
If, however, you send a SIGINT to only the Composer process, the children will not receive the same signal (and neither will they for a SIGTERM, SIGKILL, etc). If Composer terminates and they're still running, they will become orphaned, and may receive a SIGHUP, which might cause them to exit, but YMMV.
IMO, the correct behavior for Composer on SIGINT in this particular situation is to do nothing. It's fine to ignore a SIGINT if the program chooses so, and if the SIGINT was sent because of a Ctrl+C keystroke, then the child process(es) will receive this signal and (probably) terminate themselves (quickly or slowly).
However, it's still important to detect the SIGINT, and, if the child exited due to a SIGINT as well, terminate Composer itself by signaling SIGINT to itself, rather than exiting normally.
The reason a child process terminated can be checked using pcntl_wifsignaled() and friends, to distinguish between "the child exited zero or non-zero" and "the child exited due to a signal". On the shell, this is represented using an exit code greater than 128 (128+$SIGNALNO), but that's just how the shell represents it.
Distinguishing these cases, and correctly terminating oneself using the right signal, becomes important in e.g. loops. http://mywiki.wooledge.org/SignalTrap#Special_Note_On_SIGINT_and_SIGQUIT (and the rest of the article) have some background info there, and https://www.cons.org/cracauer/sigint.html is even more detailed.
For SIGTERM, that signal should be passed on to the immediate child process, and, after a certain timeout, to all child processes, possibly followed by a SIGKILL, to ensure correct termination.
Welcome to the world of process management ;)
Fixes #6059
The main improvement is that when we get a SIGINT/SIGTERM/SIGHUP while an external process is running we now wait until that one terminates before terminating.
This means Composer should now show the full output of child processes before killing itself, and also it gives a chance to child processes to handle the signal more gracefully.