Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Signal handling improvements #10958

Merged
merged 2 commits into from Jul 21, 2022
Merged

Signal handling improvements #10958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a4f217
Add seld/signal-handler dependency
Seldaek Jul 20, 2022
07645b9
Capture signals and wait until child process exits to also exit, fixe…
Seldaek Jul 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Expand Up @@ -40,7 +40,8 @@
"react/promise": "^2.8",
"composer/pcre": "^2 || ^3",
"symfony/polyfill-php73": "^1.24",
"symfony/polyfill-php80": "^1.24"
"symfony/polyfill-php80": "^1.24",
"seld/signal-handler": "^2.0"
},
"require-dev": {
"symfony/phpunit-bridge": "^6.0",
Expand Down
63 changes: 62 additions & 1 deletion composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 15 additions & 22 deletions src/Composer/Command/CreateProjectCommand.php
View file
Open in desktop
Expand Up @@ -35,6 +35,7 @@
use Composer\Script\ScriptEvents;
use Composer\Util\Silencer;
use Composer\Console\Input\InputArgument;
use Seld\Signal\SignalHandler;
use Symfony\Component\Console\Input\InputInterface;
use Composer\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
Expand Down Expand Up @@ -452,28 +453,15 @@ protected function installRootPackage(IOInterface $io, Config $config, string $p
throw new \InvalidArgumentException($errorMessage .'.');
}

// handler Ctrl+C for unix-like systems
if (function_exists('pcntl_async_signals') && function_exists('pcntl_signal')) {
@mkdir($directory, 0777, true);
if ($realDir = realpath($directory)) {
pcntl_async_signals(true);
pcntl_signal(SIGINT, static function () use ($realDir): void {
$fs = new Filesystem();
$fs->removeDirectory($realDir);
exit(130);
});
}
}
// handler Ctrl+C for Windows on PHP 7.4+
if (function_exists('sapi_windows_set_ctrl_handler') && PHP_SAPI === 'cli') {
@mkdir($directory, 0777, true);
if ($realDir = realpath($directory)) {
sapi_windows_set_ctrl_handler(static function () use ($realDir): void {
$fs = new Filesystem();
$fs->removeDirectory($realDir);
exit(130);
});
}
// handler Ctrl+C aborts gracefully
@mkdir($directory, 0777, true);
if (false !== ($realDir = realpath($directory))) {
$signalHandler = SignalHandler::create([SignalHandler::SIGINT, SignalHandler::SIGTERM, SignalHandler::SIGHUP], function (string $signal, SignalHandler $handler) use ($realDir) {
$this->getIO()->writeError('Received '.$signal.', aborting', true, IOInterface::DEBUG);
$fs = new Filesystem();
$fs->removeDirectory($realDir);
$handler->exitWithLastSignal();
});
}

// avoid displaying 9999999-dev as version if default-branch was selected
Expand Down Expand Up @@ -512,6 +500,11 @@ protected function installRootPackage(IOInterface $io, Config $config, string $p

Platform::putEnv('COMPOSER_ROOT_VERSION', $package->getPrettyVersion());

// once the root project is fully initialized, we do not need to wipe everything on user abort anymore even if it happens during deps install
if (isset($signalHandler)) {
$signalHandler->unregister();
}

return $installedFromVcs;
}
}
30 changes: 12 additions & 18 deletions src/Composer/Command/RequireCommand.php
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@

use Composer\DependencyResolver\Request;
use Composer\Util\Filesystem;
use Seld\Signal\SignalHandler;
use Symfony\Component\Console\Input\InputInterface;
use Composer\Console\Input\InputArgument;
use Composer\Console\Input\InputOption;
Expand Down Expand Up @@ -148,12 +149,11 @@ protected function execute(InputInterface $input, OutputInterface $output)
$this->composerBackup = file_get_contents($this->json->getPath());
$this->lockBackup = file_exists($this->lock) ? file_get_contents($this->lock) : null;

if (function_exists('pcntl_async_signals') && function_exists('pcntl_signal')) {
pcntl_async_signals(true);
pcntl_signal(SIGINT, function () { $this->revertComposerFile(); });
pcntl_signal(SIGTERM, function () { $this->revertComposerFile(); });
pcntl_signal(SIGHUP, function () { $this->revertComposerFile(); });
}
$signalHandler = SignalHandler::create([SignalHandler::SIGINT, SignalHandler::SIGTERM, SignalHandler::SIGHUP], function (string $signal, SignalHandler $handler) {
$this->getIO()->writeError('Received '.$signal.', aborting', true, IOInterface::DEBUG);
$this->revertComposerFile();
$handler->exitWithLastSignal();
});

// check for writability by writing to the file as is_writable can not be trusted on network-mounts
// see https://github.com/composer/composer/issues/8231 and https://bugs.php.net/bug.php?id=68926
Expand Down Expand Up @@ -210,7 +210,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
);
} catch (\Exception $e) {
if ($this->newlyCreated) {
$this->revertComposerFile(false);
$this->revertComposerFile();

throw new \RuntimeException('No composer.json present in the current directory ('.$this->file.'), this may be the cause of the following exception.', 0, $e);
}
Expand Down Expand Up @@ -293,9 +293,11 @@ protected function execute(InputInterface $input, OutputInterface $output)
return $this->doUpdate($input, $output, $io, $requirements, $requireKey, $removeKey);
} catch (\Exception $e) {
if (!$this->dependencyResolutionCompleted) {
$this->revertComposerFile(false);
$this->revertComposerFile();
}
throw $e;
} finally {
$signalHandler->unregister();
}
}

Expand Down Expand Up @@ -439,7 +441,7 @@ private function doUpdate(InputInterface $input, OutputInterface $output, IOInte
}
}
}
$this->revertComposerFile(false);
$this->revertComposerFile();
}

return $status;
Expand Down Expand Up @@ -479,11 +481,7 @@ protected function interact(InputInterface $input, OutputInterface $output): voi

}

/**
* @param bool $hardExit
* @return void
*/
public function revertComposerFile(bool $hardExit = true): void
private function revertComposerFile(): void
{
$io = $this->getIO();

Expand All @@ -504,9 +502,5 @@ public function revertComposerFile(bool $hardExit = true): void
file_put_contents($this->lock, $this->lockBackup);
}
}

if ($hardExit) {
exit(1);
}
}
}
16 changes: 8 additions & 8 deletions src/Composer/Console/Application.php
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@
use Composer\Util\Platform;
use Composer\Util\Silencer;
use LogicException;
use Seld\Signal\SignalHandler;
use Symfony\Component\Console\Application as BaseApplication;
use Symfony\Component\Console\Exception\CommandNotFoundException;
use Symfony\Component\Console\Helper\HelperSet;
Expand Down Expand Up @@ -93,13 +94,14 @@ public function __construct()
date_default_timezone_set(Silencer::call('date_default_timezone_get'));
}

$this->io = new NullIO();

if (!$shutdownRegistered) {
if (function_exists('pcntl_async_signals') && function_exists('pcntl_signal')) {
pcntl_async_signals(true);
pcntl_signal(SIGINT, static function ($sig): void {
exit(130);
});
}
$signalHandler = SignalHandler::create([SignalHandler::SIGINT, SignalHandler::SIGTERM, SignalHandler::SIGHUP], function (string $signal, SignalHandler $handler) {
$this->io->writeError('Received '.$signal.', aborting', true, IOInterface::DEBUG);

$handler->exitWithLastSignal();
});

$shutdownRegistered = true;

Expand All @@ -114,8 +116,6 @@ public function __construct()
});
}

$this->io = new NullIO();

$this->initialWorkingDirectory = getcwd();

parent::__construct('Composer', Composer::getVersion());
Expand Down
2 changes: 2 additions & 0 deletions src/Composer/EventDispatcher/ScriptExecutionException.php
View file
Open in desktop
Expand Up @@ -13,6 +13,8 @@
namespace Composer\EventDispatcher;

/**
* Thrown when a script running an external process exits with a non-0 status code
*
* @author Jordi Boggiano <j.boggiano@seld.be>
*/
class ScriptExecutionException extends \RuntimeException
Expand Down
52 changes: 8 additions & 44 deletions src/Composer/Installer/InstallationManager.php
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@
use Composer\Util\Loop;
use Composer\Util\Platform;
use React\Promise\PromiseInterface;
use Seld\Signal\SignalHandler;

/**
* Package operation manager.
Expand Down Expand Up @@ -222,36 +223,11 @@ public function execute(InstalledRepositoryInterface $repo, array $operations, b
}
};

$handleInterruptsUnix = function_exists('pcntl_async_signals') && function_exists('pcntl_signal');
$handleInterruptsWindows = PHP_VERSION_ID >= 70400 && function_exists('sapi_windows_set_ctrl_handler') && PHP_SAPI === 'cli';
$prevHandler = null;
$windowsHandler = null;
if ($handleInterruptsUnix) {
pcntl_async_signals(true);
$prevHandler = pcntl_signal_get_handler(SIGINT);
pcntl_signal(SIGINT, static function ($sig) use ($runCleanup, $prevHandler, $io): void {
$io->writeError('Received SIGINT, aborting', true, IOInterface::DEBUG);
$runCleanup();

if (!in_array($prevHandler, array(SIG_DFL, SIG_IGN), true)) {
call_user_func($prevHandler, $sig);
}

exit(130);
});
}
if ($handleInterruptsWindows) {
$windowsHandler = static function ($event) use ($runCleanup, $io): void {
if ($event !== PHP_WINDOWS_EVENT_CTRL_C) {
return;
}
$io->writeError('Received CTRL+C, aborting', true, IOInterface::DEBUG);
$runCleanup();

exit(130);
};
sapi_windows_set_ctrl_handler($windowsHandler);
}
$signalHandler = SignalHandler::create([SignalHandler::SIGINT, SignalHandler::SIGTERM, SignalHandler::SIGHUP], static function (string $signal, SignalHandler $handler) use ($io, $runCleanup) {
$io->writeError('Received '.$signal.', aborting', true, IOInterface::DEBUG);
$runCleanup();
$handler->exitWithLastSignal();
});

try {
// execute operations in batches to make sure download-modifying-plugins are installed
Expand Down Expand Up @@ -284,21 +260,9 @@ public function execute(InstalledRepositoryInterface $repo, array $operations, b
} catch (\Exception $e) {
$runCleanup();

if ($handleInterruptsUnix) {
pcntl_signal(SIGINT, $prevHandler);
}
if ($handleInterruptsWindows) {
sapi_windows_set_ctrl_handler($windowsHandler, false);
}

throw $e;
}

if ($handleInterruptsUnix) {
pcntl_signal(SIGINT, $prevHandler);
}
if ($handleInterruptsWindows) {
sapi_windows_set_ctrl_handler($windowsHandler, false);
} finally {
$signalHandler->unregister();
}

// do a last write so that we write the repository even if nothing changed
Expand Down
27 changes: 22 additions & 5 deletions src/Composer/Util/ProcessExecutor.php
View file
Open in desktop
Expand Up @@ -14,6 +14,8 @@

use Composer\IO\IOInterface;
use Composer\Pcre\Preg;
use Seld\Signal\SignalHandler;
use Symfony\Component\Process\Exception\ProcessSignaledException;
use Symfony\Component\Process\Process;
use Symfony\Component\Process\Exception\RuntimeException;
use React\Promise\Promise;
Expand Down Expand Up @@ -125,13 +127,28 @@ private function doExecute($command, ?string $cwd, bool $tty, &$output = null):
$this->outputHandler($type, $buffer);
};

$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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@Seldaek Seldaek Jul 20, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
});

if ($this->captureOutput && !is_callable($output)) {
$output = $process->getOutput();
}
try {
$process->run($callback);

$this->errorOutput = $process->getErrorOutput();
if ($this->captureOutput && !is_callable($output)) {
$output = $process->getOutput();
}

$this->errorOutput = $process->getErrorOutput();
} catch (ProcessSignaledException $e) {
if ($signalHandler->isTriggered()) {
// exiting as we were signaled and the child process exited too due to the signal
$signalHandler->exitWithLastSignal();
}
} finally {
$signalHandler->unregister();
}

return $process->getExitCode();
}
Expand Down