8000 Fixes #43866 configurable stop signals for messenger by Warxcell · Pull Request #44305 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Fixes #43866 configurable stop signals for messenger #44305

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

Closed
wants to merge 1 commit into from
Closed

Fixes #43866 configurable stop signals for messenger #44305

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1308,6 +1308,20 @@ private function addMessengerSection(ArrayNodeDefinition $rootNode, callable $en
->then(static function (array $v): void { throw new InvalidConfigurationException(sprintf('The specified default bus "%s" is not configured. Available buses are "%s".', $v['default_bus'], implode('", "', array_keys($v['buses'])))); })
->end()
->children()
->arrayNode('stop_signals')
Copy link
Member

Choose a reason for hiding this comment

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

For lists, you should add fixXmlConfig

->fixXmlConfig('stop_signal')
->beforeNormalization()
->ifString()
->then(static fn (string $value): int => constant($value))
->castToArray()
->end()
->validate()
->ifTrue(static fn ($v): bool => !function_exists('pcntl_signal'))
->then(static function (): void { throw new InvalidConfigurationException('You must install pcntl extension in order to use stop_signals.'); })
->ifNull()
->then(static function (): void { throw new InvalidConfigurationException('Invalid signal passed to stop_signals'); })
->end()
->end()
->arrayNode('routing')
->normalizeKeys(false)
->useAttributeAsKey('message_class')
Expand Down
8 changes: 5 additions & 3 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,9 +706,9 @@ private function registerHttpCacheConfiguration(array $config, ContainerBuilder

if ($httpMethodOverride) {
$container->getDefinition('http_cache')
->addArgument((new Definition('void'))
->setFactory([Request::class, 'enableHttpMethodParameterOverride'])
);
->addArgument((new Definition('void'))
->setFactory([Request::class, 'enableHttpMethodParameterOverride'])
);
}
}

Expand Down Expand Up @@ -1914,6 +1914,8 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
$config['default_bus'] = key($config['buses']);
}

$container->getDefinition('stop_worker_on_sigterm_signal_listener')->setArgument('$signals', $config['stop_signals']);

$defaultMiddleware = [
'before' => [
['id' => 'add_bus_name_stamp_middleware'],
Expand Down
4 changes: 3 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use Symfony\Component\Messenger\EventListener\SendFailedMessageToFailureTransportListener;
use Symfony\Component\Messenger\EventListener\StopWorkerOnCustomStopExceptionListener;
use Symfony\Component\Messenger\EventListener\StopWorkerOnRestartSignalListener;
use Symfony\Component\Messenger\EventListener\StopWorkerOnSignalListener;
use Symfony\Component\Messenger\EventListener\StopWorkerOnSigtermSignalListener;
use Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware;
use Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware;
Expand Down Expand Up @@ -192,8 +193,9 @@
->tag('kernel.event_subscriber')
->tag('monolog.logger', ['channel' => 'messenger'])

->set('messenger.listener.stop_worker_on_sigterm_signal_listener', StopWorkerOnSigtermSignalListener::class)
Copy link
Member

Choose a reason for hiding this comment

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

is it a BC break? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't that internal service used by framework itself? how can it be BC-break?

->set('messenger.listener.stop_worker_on_signal_listener', StopWorkerOnSignalListener::class)
->args([
abstract_arg('signals'),
service('logger')->ignoreOnInvalid(),
])
->tag('kernel.event_subscriber')
Expand Down
100 changes: 100 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php
View file
Open in desktop 8000
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,106 @@ public function testLockMergeConfigs()
);
}

public function testMessengerWithoutPcntl()
{
$processor = new Processor();
$configuration = new Configuration(true);

$config = $processor->processConfiguration($configuration, [
'framework' => [
'messenger' => [
],
],
]);

self::assertEquals([\SIGTERM], $config['framework']['messenger']['stop_signals']);
}

public function testMessengerWithPcntlWithoutExplicitlyConfigured()
{
$this->expectException(InvalidConfigurationException::class);
$this->expectExceptionMessage('You must install pcntl extension in order to use stop_signals.');
$processor = new Processor();
$configuration = new Configuration(true);

$processor->processConfiguration($configuration, [
'framework' => [
'messenger' => [
'stop_signals' => \SIGTERM
],
],
]);
}

public function testMessengerWithoutPcntlWithoutStopSignals()
{
$processor = new Processor();
$configuration = new Configuration(true);

$processor->processConfiguration($configuration, [
'framework' => [
'messenger' => [
],
],
]);

$this->expectNotToPerformAssertions();
}

public function testMessengerWithInvalidSignal()
{
$this->expectException(InvalidConfigurationException::class);
$this->expectExceptionMessage('Invalid signal passed to stop_signals');
$processor = new Processor();
$configuration = new Configuration(true);

$processor->processConfiguration($configuration, [
'framework' => [
'messenger' => [
'stop_signals' => \SIG_IGN
],
],
]);
}

public function testMessengerWithMultipleStopSignals()
{
$processor = new Processor();
$configuration = new Configuration(true);

$config = $processor->processConfiguration($configuration, [
'framework' => [
'messenger' => [
'stop_signals' => [\SIGTERM, \SIGQUIT]
],
],
]);

static::assertEquals(
[\SIGTERM, \SIGQUIT],
$config['messenger']['stop_signals']
);
}

public function testMessengerWithStringStopSignal()
{
$processor = new Processor();
$configuration = new Configuration(true);

$config = $processor->processConfiguration($configuration, [
'framework' => [
'messenger' => [
'stop_signals' => ['SIGTERM', 'SIGQUIT']
],
],
]);
< A3E2 /td>
static::assertEquals(
[\SIGTERM, \SIGQUIT],
$config['messenger']['stop_signals']
);
}

public function testItShowANiceMessageIfTwoMessengerBusesAreConfiguredButNoDefaultBus()
{
$expectedMessage = 'You must specify the "default_bus" if you define more than one bus.';
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/messenger.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Messenger\FooMessage;

$container->loadFromExtension('framework', [
'stop_signals'=> \SIGTERM,
'messenger' => [
'routing' => [
FooMessage::class => ['sender.bar', 'sender.biz'],
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/messenger.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
framework:
messenger:
stop_signals: SIGTERM
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this have to be plural (a list of signals) instead of just 1 signal?

Suggested change
stop_signals: SIGTERM
stop_signal: SIGTERM

Copy link
Contributor Author
@Warxcell Warxcell Nov 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

in case project is used in several envs (docker FPM alpine environment, and classical VMs/bare-metal with supervisor) where signals could be different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then configure the correct signal for that env. Doesn't make sense (to me) to just fire all the signals.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with @Warxcell. we should allow multiple signals.

routing:
'Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Messenger\FooMessage': ['sender.bar', 'sender.biz']
'Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Messenger\BarMessage': 'sender.foo'
Expand Down
29 changes: 3 additions & 26 deletions src/Symfony/Component/Messenger/EventListener/StopWorkerOnSigtermSignalListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,38 +12,15 @@
namespace Symfony\Component\Messenger\EventListener;

use Psr\Log\LoggerInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Messenger\Event\WorkerStartedEvent;

/**
* @author Tobias Schultze <http://tobion.de>
* @deprecated Use StopWorkerOnSignalListener instead.
*/
class StopWorkerOnSigtermSignalListener implements EventSubscriberInterface
class StopWorkerOnSigtermSignalListener extends StopWorkerOnSignalListener
{
private ?LoggerInterface $logger;

public function __construct(LoggerInterface $logger = null)
{
$this->logger = $logger;
}

public function onWorkerStarted(WorkerStartedEvent $event): void
{
pcntl_signal(\SIGTERM, function () use ($event) {
$this->logger?->info('Received SIGTERM signal.', ['transport_names' => $event->getWorker()->getMetadata()->getTransportNames()]);

$event->getWorker()->stop();
});
}

public static function getSubscribedEvents(): array
{
if (!\function_exists('pcntl_signal')) {
return [];
}

return [
WorkerStartedEvent::class => ['onWorkerStarted', 100],
];
parent::__construct(\SIGTERM, $logger);
}
}
0