Skip to content

Commit 3924c80

Browse files
yoeunesyoeunes
committed
[Console] Fix signal handlers not being cleared after command termination
1 parent 8ea1b3a commit 3924c80

File tree

4 files changed

+312
-6
lines changed

4 files changed

+312
-6
lines changed

‎src/Symfony/Component/Console/Application.php‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -993,9 +993,14 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
993993
}
994994
}
995995

996+
$registeredSignals = false;
997+
996998
if (($commandSignals = $command->getSubscribedSignals()) || $this->dispatcher && $this->signalsToDispatchEvent) {
997999
$signalRegistry = $this->getSignalRegistry();
9981000

1001+
$registeredSignals = true;
1002+
$signalRegistry->pushCurrentHandlers();
1003+
9991004
if ($this->dispatcher) {
10001005
// We register application signals, so that we can dispatch the event
10011006
foreach ($this->signalsToDispatchEvent as $signal) {
@@ -1052,7 +1057,15 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
10521057
}
10531058

10541059
if (null === $this->dispatcher) {
1055-
return $command->run($input, $output);
1060+
try {
1061+
$exitCode = $command->run($input, $output);
1062+
} finally {
1063+
if ($registeredSignals) {
1064+
$this->getSignalRegistry()->popPreviousHandlers();
1065+
}
1066+
}
1067+
1068+
return $exitCode;
10561069
}
10571070

10581071
// bind before the console.command event, so the listeners have access to input options/arguments
@@ -1082,6 +1095,10 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
10821095
if (0 === $exitCode = $event->getExitCode()) {
10831096
$e = null;
10841097
}
1098+
} finally {
1099+
if ($registeredSignals) {
1100+
$this->getSignalRegistry()->popPreviousHandlers();
1101+
}
10851102
}
10861103

10871104
$event = new ConsoleTerminateEvent($command, $input, $output, $exitCode);

‎src/Symfony/Component/Console/SignalRegistry/SignalRegistry.php‎

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,21 @@
1313

1414
final class SignalRegistry
1515
{
16+
/**
17+
* @var array<int, array<callable>>
18+
*/
1619
private array $signalHandlers = [];
1720

21+
/**
22+
* @var array<array<int, array<callable>>>
23+
*/
24+
private array $stack = [];
25+
26+
/**
27+
* @var array<int, callable|int|string>
28+
*/
29+
private array $originalHandlers = [];
30+
1831
public function __construct()
1932
{
2033
if (\function_exists('pcntl_async_signals')) {
@@ -24,17 +37,21 @@ public function __construct()
2437

2538
public function register(int $signal, callable $signalHandler): void
2639
{
27-
if (!isset($this->signalHandlers[$signal])) {
28-
$previousCallback = pcntl_signal_get_handler($signal);
40+
$previous = pcntl_signal_get_handler($signal);
41+
42+
if (!isset($this->originalHandlers[$signal])) {
43+
$this->originalHandlers[$signal] = $previous;
44+
}
2945

30-
if (\is_callable($previousCallback)) {
31-
$this->signalHandlers[$signal][] = $previousCallback;
46+
if (!isset($this->signalHandlers[$signal])) {
47+
if (\is_callable($previous) && $previous !== [$this, 'handle']) {
48+
$this->signalHandlers[$signal][] = $previous;
3249
}
3350
}
3451

3552
$this->signalHandlers[$signal][] = $signalHandler;
3653

37-
pcntl_signal($signal, $this->handle(...));
54+
pcntl_signal($signal, [$this, 'handle']);
3855
}
3956

4057
public static function isSupported(): bool
@@ -55,6 +72,40 @@ public function handle(int $signal): void
5572
}
5673
}
5774

75+
/**
76+
* Pushes the current active handlers onto the stack and clears the active list.
77+
*
78+
* This prepares the registry for a new set of handlers within a specific scope.
79+
*
80+
* @internal
81+
*/
82+
public function pushCurrentHandlers(): void
83+
{
84+
$this->stack[] = $this->signalHandlers;
85+
$this->signalHandlers = [];
86+
}
87+
88+
/**
89+
* Restores the previous handlers from the stack, making them active.
90+
*
91+
* This also restores the original OS-level signal handler if no
92+
* more handlers are registered for a signal that was just popped.
93+
*
94+
* @internal
95+
*/
96+
public function popPreviousHandlers(): void
97+
{
98+
$popped = $this->signalHandlers;
99+
$this->signalHandlers = array_pop($this->stack) ?? [];
100+
101+
// Restore OS handler if no more Symfony handlers for this signal
102+
foreach (array_keys($popped) as $signal) {
103+
if (empty($this->signalHandlers[$signal]) && isset($this->originalHandlers[$signal])) {
104+
pcntl_signal($signal, $this->originalHandlers[$signal]);
105+
}
106+
}
107+
}
108+
58109
/**
59110
* @internal
60111
*/

‎src/Symfony/Component/Console/Tests/ApplicationTest.php‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2485,6 +2485,181 @@ public function onAlarm(ConsoleAlarmEvent $event): void
24852485
$this->assertSame([SignalEventSubscriber::class, AlarmEventSubscriber::class], $command->signalHandlers);
24862486
}
24872487

2488+
/**
2489+
* @requires extension pcntl
2490+
*/
2491+
public function testSignalHandlersAreCleanedUpAfterCommandRuns()
2492+
{
2493+
$application = new Application();
2494+
$application->setAutoExit(false);
2495+
$application->setCatchExceptions(false);
2496+
$application->add(new SignableCommand(false));
2497+
2498+
$signalRegistry = $application->getSignalRegistry();
2499+
$tester = new ApplicationTester($application);
2500+
2501+
$this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Registry should be empty initially.');
2502+
2503+
$tester->run(['command' => 'signal']);
2504+
$this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Registry should be empty after first run.');
2505+
2506+
$tester->run(['command' => 'signal']);
2507+
$this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Registry should still be empty after second run.');
2508+
}
2509+
2510+
/**
2511+
* @requires extension pcntl
2512+
*/
2513+
public function testSignalHandlersCleanupOnException()
2514+
{
2515+
$command = new class('signal:exception') extends Command implements SignalableCommandInterface {
2516+
public function getSubscribedSignals(): array
2517+
{
2518+
return [\SIGUSR1];
2519+
}
2520+
2521+
public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false
2522+
{
2523+
return false;
2524+
}
2525+
2526+
protected function execute(InputInterface $input, OutputInterface $output): int
2527+
{
2528+
throw new \RuntimeException('Test exception');
2529+
}
2530+
};
2531+
2532+
$application = new Application();
2533+
$application->setAutoExit(false);
2534+
$application->setCatchExceptions(true);
2535+
$application->add($command);
2536+
2537+
$signalRegistry = $application->getSignalRegistry();
2538+
$tester = new ApplicationTester($application);
2539+
2540+
$this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Pre-condition: Registry must be empty.');
2541+
2542+
$tester->run(['command' => 'signal:exception']);
2543+
$this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Signal handlers must be cleaned up even on exception.');
2544+
}
2545+
2546+
/**
2547+
* @requires extension pcntl
2548+
*/
2549+
public function testNestedCommandsIsolateSignalHandlers()
2550+
{
2551+
$application = new Application();
2552+
$application->setAutoExit(false);
2553+
$application->setCatchExceptions(false);
2554+
2555+
$signalRegistry = $application->getSignalRegistry();
2556+
$self = $this;
2557+
2558+
$innerCommand = new class('signal:inner') extends Command implements SignalableCommandInterface {
2559+
public $signalRegistry;
2560+
public $self;
2561+
2562+
public function getSubscribedSignals(): array
2563+
{
2564+
return [\SIGUSR1];
2565+
}
2566+
2567+
public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false
2568+
{
2569+
return false;
2570+
}
2571+
2572+
protected function execute(InputInterface $input, OutputInterface $output): int
2573+
{
2574+
$handlers = $this->self->getHandlersForSignal($this->signalRegistry, \SIGUSR1);
2575+
$this->self->assertCount(1, $handlers, 'Inner command should only see its own handler.');
2576+
$output->write('Inner execute.');
2577+
2578+
return 0;
2579+
}
2580+
};
2581+
2582+
$outerCommand = new class('signal:outer') extends Command implements SignalableCommandInterface {
2583+
public $signalRegistry;
2584+
public $self;
2585+
2586+
public function getSubscribedSignals(): array
2587+
{
2588+
return [\SIGUSR1];
2589+
}
2590+
2591+
public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false
2592+
{
2593+
return false;
2594+
}
2595+
2596+
protected function execute(InputInterface $input, OutputInterface $output): int
2597+
{
2598+
$handlersBefore = $this->self->getHandlersForSignal($this->signalRegistry, \SIGUSR1);
2599+
$this->self->assertCount(1, $handlersBefore, 'Outer command must have its handler registered.');
2600+
2601+
$output->write('Outer pre-run.');
2602+
2603+
$this->getApplication()->find('signal:inner')->run(new ArrayInput([]), $output);
2604+
2605+
$output->write('Outer post-run.');
2606+
2607+
$handlersAfter = $this->self->getHandlersForSignal($this->signalRegistry, \SIGUSR1);
2608+
$this->self->assertCount(1, $handlersAfter, 'Outer command\'s handler must be restored.');
2609+
$this->self->assertSame($handlersBefore, $handlersAfter, 'Handler stack must be identical after pop.');
2610+
2611+
return 0;
2612+
}
2613+
};
2614+
2615+
$innerCommand->self = $self;
2616+
$innerCommand->signalRegistry = $signalRegistry;
2617+
$outerCommand->self = $self;
2618+
$outerCommand->signalRegistry = $signalRegistry;
2619+
2620+
$application->add($innerCommand);
2621+
$application->add($outerCommand);
2622+
2623+
$tester = new ApplicationTester($application);
2624+
2625+
$this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Pre-condition: Registry must be empty.');
2626+
$tester->run(['command' => 'signal:outer']);
2627+
$this->assertStringContainsString('Outer pre-run.Inner execute.Outer post-run.', $tester->getDisplay());
2628+
2629+
$this->assertCount(0, $this->getHandlersForSignal($signalRegistry, \SIGUSR1), 'Registry must be empty after all commands are finished.');
2630+
}
2631+
2632+
/**
2633+
* @requires extension pcntl
2634+
*/
2635+
public function testOriginalHandlerRestoredAfterPop()
2636+
{
2637+
$this->assertSame(\SIG_DFL, pcntl_signal_get_handler(\SIGUSR1), 'Pre-condition: Original handler for SIGUSR1 must be SIG_DFL.');
2638+
2639+
$application = new Application();
2640+
$application->setAutoExit(false);
2641+
$application->setCatchExceptions(false);
2642+
$application->add(new SignableCommand(false));
2643+
2644+
$tester = new ApplicationTester($application);
2645+
$tester->run(['command' => 'signal']);
2646+
2647+
$this->assertSame(\SIG_DFL, pcntl_signal_get_handler(\SIGUSR1), 'OS-level handler for SIGUSR1 must be restored to SIG_DFL.');
2648+
2649+
$tester->run(['command' => 'signal']);
2650+
$this->assertSame(\SIG_DFL, pcntl_signal_get_handler(\SIGUSR1), 'OS-level handler must remain SIG_DFL after a second run.');
2651+
}
2652+
2653+
/**
2654+
* Reads the private "signalHandlers" property of the SignalRegistry for assertions.
2655+
*/
2656+
public function getHandlersForSignal(SignalRegistry $registry, int $signal): array
2657+
{
2658+
$handlers = (\Closure::bind(fn () => $this->signalHandlers, $registry, SignalRegistry::class))();
2659+
2660+
return $handlers[$signal] ?? [];
2661+
}
2662+
24882663
private function createSignalableApplication(Command $command, ?EventDispatcherInterface $dispatcher): Application
24892664
{
24902665
$application = new Application();

‎src/Symfony/Component/Console/Tests/SignalRegistry/SignalRegistryTest.php‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,4 +130,67 @@ public function testTwoCallbacksForASignalPreviousCallbackFromAnotherRegistry()
130130
$this->assertTrue($isHandled1);
131131
$this->assertTrue($isHandled2);
132132
}
133+
134+
public function testPushPopIsolatesHandlers()
135+
{
136+
$registry = new SignalRegistry();
137+
138+
$signal = \SIGUSR1;
139+
140+
$handler1 = static function () {};
141+
$handler2 = static function () {};
142+
143+
$registry->pushCurrentHandlers();
144+
$registry->register($signal, $handler1);
145+
146+
$this->assertCount(1, $this->getHandlersForSignal($registry, $signal));
147+
148+
$registry->pushCurrentHandlers();
149+
$registry->register($signal, $handler2);
150+
151+
$this->assertCount(1, $this->getHandlersForSignal($registry, $signal));
152+
$this->assertSame([$handler2], $this->getHandlersForSignal($registry, $signal));
153+
154+
$registry->popPreviousHandlers();
155+
156+
$this->assertCount(1, $this->getHandlersForSignal($registry, $signal));
157+
$this->assertSame([$handler1], $this->getHandlersForSignal($registry, $signal));
158+
159+
$registry->popPreviousHandlers();
160+
161+
$this->assertCount(0, $this->getHandlersForSignal($registry, $signal));
162+
}
163+
164+
public function testRestoreOriginalOnEmptyAfterPop()
165+
{
166+
if (!\extension_loaded('pcntl')) {
167+
$this->markTestSkipped('PCNTL extension required');
168+
}
169+
170+
$registry = new SignalRegistry();
171+
172+
$signal = \SIGUSR2;
173+
174+
$original = pcntl_signal_get_handler($signal);
175+
176+
$handler = static function () {};
177+
178+
$registry->pushCurrentHandlers();
179+
$registry->register($signal, $handler);
180+
181+
$this->assertNotEquals($original, pcntl_signal_get_handler($signal));
182+
183+
$registry->popPreviousHandlers();
184+
185+
$this->assertEquals($original, pcntl_signal_get_handler($signal));
186+
}
187+
188+
private function getHandlersForSignal(SignalRegistry $registry, int $signal): array
189+
{
190+
$ref = new \ReflectionClass($registry);
191+
$prop = $ref->getProperty('signalHandlers');
192+
$handlers = $prop->getValue($registry);
193+
194+
return $handlers[$signal] ?? [];
195+
}
133196
}

0 commit comments

Comments
 (0)