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

[Console] Fix ConsoleEvents::SIGNAL subscriber dispatch #45333

Merged
merged 1 commit into from Aug 2, 2022
Merged

[Console] Fix ConsoleEvents::SIGNAL subscriber dispatch #45333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 20 additions & 15 deletions src/Symfony/Component/Console/Application.php
View file
Open in desktop
Expand Up @@ -974,22 +974,31 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
}
}

if ($command instanceof SignalableCommandInterface && ($this->signalsToDispatchEvent || $command->getSubscribedSignals())) {
if (!$this->signalRegistry) {
throw new RuntimeException('Unable to subscribe to signal events. Make sure that the `pcntl` extension is installed and that "pcntl_*" functions are not disabled by your php.ini\'s "disable_functions" directive.');
}
if ($this->signalsToDispatchEvent) {
$commandSignals = $command instanceof SignalableCommandInterface ? $command->getSubscribedSignals() : [];
$dispatchSignals = $this->dispatcher && $this->dispatcher->hasListeners(ConsoleEvents::SIGNAL);
Copy link
Contributor

Choose a reason for hiding this comment

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

$this->dispatcher is typed to be a \Symfony\Contracts\EventDispatcher\EventDispatcherInterface, but hasListeners is not part of that contract.

This leads to our package breaking with the current branch version (5.4.x-dev in our case), since it does not fulfill the \Symfony\Component\EventDispatcher\EventDispatcherInterface


if (Terminal::hasSttyAvailable()) {
$sttyMode = shell_exec('stty -g');
if ($commandSignals || $dispatchSignals) {
if (!$this->signalRegistry) {
throw new RuntimeException('Unable to subscribe to signal events. Make sure that the `pcntl` extension is installed and that "pcntl_*" functions are not disabled by your php.ini\'s "disable_functions" directive.');
}

foreach ([\SIGINT, \SIGTERM] as $signal) {
$this->signalRegistry->register($signal, static function () use ($sttyMode) {
shell_exec('stty '.$sttyMode);
});
if (Terminal::hasSttyAvailable()) {
$sttyMode = shell_exec('stty -g');

foreach ([\SIGINT, \SIGTERM] as $signal) {
$this->signalRegistry->register($signal, static function () use ($sttyMode) {
shell_exec('stty '.$sttyMode);
});
}
}

foreach ($commandSignals as $signal) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any specific reason why the order of registration of signal handlers have been modified? This caused an issue #48205 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't remember my approach/reason at the time, but the test added in this PR should cover the use-case problem I had.

Your fix #48210 is most probably correct.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks you for reviewing.

$this->signalRegistry->register($signal, [$command, 'handleSignal']);
}
}

if ($this->dispatcher) {
if ($dispatchSignals) {
foreach ($this->signalsToDispatchEvent as $signal) {
$event = new ConsoleSignalEvent($command, $input, $output, $signal);

Expand All @@ -1005,10 +1014,6 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
});
}
}

foreach ($command->getSubscribedSignals() as $signal) {
lyrixx marked this conversation as resolved.
Show resolved Hide resolved
$this->signalRegistry->register($signal, [$command, 'handleSignal']);
}
}

if (null === $this->dispatcher) {
Expand Down
161 changes: 138 additions & 23 deletions src/Symfony/Component/Console/Tests/ApplicationTest.php
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@
use Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass;
use Symfony\Component\Console\Event\ConsoleCommandEvent;
use Symfony\Component\Console\Event\ConsoleErrorEvent;
use Symfony\Component\Console\Event\ConsoleSignalEvent;
use Symfony\Component\Console\Event\ConsoleTerminateEvent;
use Symfony\Component\Console\Exception\CommandNotFoundException;
use Symfony\Component\Console\Exception\NamespaceNotFoundException;
Expand All @@ -42,6 +43,8 @@
use Symfony\Component\Console\Tester\ApplicationTester;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Process\Process;

class ApplicationTest extends TestCase
Expand Down Expand Up @@ -1843,39 +1846,107 @@ public function testCommandNameMismatchWithCommandLoaderKeyThrows()
/**
* @requires extension pcntl
*/
public function testSignal()
public function testSignalListenerNotCalledByDefault()
{
$command = new SignableCommand();
$command = new SignableCommand(false);

$dispatcherCalled = false;
$dispatcher = new EventDispatcher();
$dispatcher->addListener('console.signal', function () use (&$dispatcherCalled) {
$dispatcherCalled = true;
});

$application = new Application();
$application->setAutoExit(false);
$application->setDispatcher($dispatcher);
$application->setSignalsToDispatchEvent(\SIGALRM);
$application->add(new LazyCommand('signal', [], '', false, function () use ($command) { return $command; }, true));

$this->assertFalse($command->signaled);
$this->assertFalse($dispatcherCalled);
$application = $this->createSignalableApplication($command, $dispatcher);

$this->assertSame(0, $application->run(new ArrayInput(['signal'])));
$this->assertFalse($command->signaled);
$this->assertFalse($dispatcherCalled);
}

/**
* @requires extension pcntl
*/
public function testSignalListener()
{
$command = new SignableCommand();

$dispatcherCalled = false;
$dispatcher = new EventDispatcher();
$dispatcher->addListener('console.signal', function () use (&$dispatcherCalled) {
$dispatcherCalled = true;
});

$application = $this->createSignalableApplication($command, $dispatcher);

$command->loop = 100000;
pcntl_alarm(1);
$this->assertSame(1, $application->run(new ArrayInput(['signal'])));
$this->assertTrue($command->signaled);
$this->assertTrue($dispatcherCalled);
$this->assertTrue($command->signaled);
}

/**
* @requires extension pcntl
*/
public function testSignalSubscriberNotCalledByDefault()
{
$command = new BaseSignableCommand(false);

$subscriber = new SignalEventSubscriber();
$dispatcher = new EventDispatcher();
$dispatcher->addSubscriber($subscriber);

$application = $this->createSignalableApplication($command, $dispatcher);

$this->assertSame(0, $application->run(new ArrayInput(['signal'])));
$this->assertFalse($subscriber->signaled);
}

/**
* @requires extension pcntl
*/
public function testSignalSubscriber()
{
$command = new BaseSignableCommand();

$subscriber1 = new SignalEventSubscriber();
$subscriber2 = new SignalEventSubscriber();

$dispatcher = new EventDispatcher();
$dispatcher->addSubscriber($subscriber1);
$dispatcher->addSubscriber($subscriber2);

$application = $this->createSignalableApplication($command, $dispatcher);

$this->assertSame(1, $application->run(new ArrayInput(['signal'])));
$this->assertTrue($subscriber1->signaled);
$this->assertTrue($subscriber2->signaled);
}

/**
* @requires extension pcntl
*/
public function testSetSignalsToDispatchEvent()
{
$command = new BaseSignableCommand();

$subscriber = new SignalEventSubscriber();

$dispatcher = new EventDispatcher();
$dispatcher->addSubscriber($subscriber);

$application = $this->createSignalableApplication($command, $dispatcher);
$application->setSignalsToDispatchEvent(\SIGUSR2);
$this->assertSame(0, $application->run(new ArrayInput(['signal'])));
$this->assertFalse($subscriber->signaled);

$application = $this->createSignalableApplication($command, $dispatcher);
$application->setSignalsToDispatchEvent(\SIGUSR1);
$this->assertSame(1, $application->run(new ArrayInput(['signal'])));
$this->assertTrue($subscriber->signaled);
}

public function testSignalableCommandInterfaceWithoutSignals()
{
$command = new SignableCommand();
$command = new SignableCommand(false);

$dispatcher = new EventDispatcher();
$application = new Application();
Expand Down Expand Up @@ -1917,6 +1988,18 @@ public function testSignalableRestoresStty()

$this->assertSame($previousSttyMode, $sttyMode);
}

private function createSignalableApplication(Command $command, ?EventDispatcherInterface $dispatcher): Application
{
$application = new Application();
$application->setAutoExit(false);
if ($dispatcher) {
$application->setDispatcher($dispatcher);
}
$application->add(new LazyCommand('signal', [], '', false, function () use ($command) { return $command; }, true));

return $application;
}
}

class CustomApplication extends Application
Expand Down Expand Up @@ -1971,25 +2054,26 @@ public function isEnabled(): bool
}
}

class SignableCommand extends Command implements SignalableCommandInterface
class BaseSignableCommand extends Command
{
public $signaled = false;
public $loop = 100;
public $loop = 1000;
private $emitsSignal;

protected static $defaultName = 'signal';

public function getSubscribedSignals(): array
public function __construct(bool $emitsSignal = true)
{
return SignalRegistry::isSupported() ? [\SIGALRM] : [];
}

public function handleSignal(int $signal): void
{
$this->signaled = true;
parent::__construct();
$this->emitsSignal = $emitsSignal;
}

protected function execute(InputInterface $input, OutputInterface $output): int
{
if ($this->emitsSignal) {
posix_kill(posix_getpid(), SIGUSR1);
}

for ($i = 0; $i < $this->loop; ++$i) {
usleep(100);
if ($this->signaled) {
Expand All @@ -2000,3 +2084,34 @@ protected function execute(InputInterface $input, OutputInterface $output): int
return 0;
}
}

class SignableCommand extends BaseSignableCommand implements SignalableCommandInterface
{
protected static $defaultName = 'signal';

public function getSubscribedSignals(): array
{
return SignalRegistry::isSupported() ? [\SIGUSR1] : [];
}

public function handleSignal(int $signal): void
{
$this->signaled = true;
}
}

class SignalEventSubscriber implements EventSubscriberInterface
{
public $signaled = false;

public function onSignal(ConsoleSignalEvent $event): void
{
$this->signaled = true;
$event->getCommand()->signaled = true;
}

public static function getSubscribedEvents(): array
{
return ['console.signal' => 'onSignal'];
}
}