symfony: [Console] Commands aren't able to fully gracefully exit by themselves
Symfony version(s) affected
5.2.0+
Description
Since https://github.com/symfony/symfony/commit/df57119884f62c2c29e8a9232b56a287196dad32 symfony/console is reacting to signals, dispatching events, and exiting right away.
My main issue here is the exit()
inside of the signal handler, as it imposes a specific application design for graceful shutdowns and prevents the dispatch of console.terminate
event.
How to reproduce
A command like this should suffice:
<?php
declare(strict_types=1);
namespace MyApp;
use Symfony\Component\Console\Attribute\AsCommand;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Command\SignalableCommandInterface;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use function sleep;
use const SIGINT;
use const SIGTERM;
#[AsCommand('testing')]
final class Testing extends Command implements SignalableCommandInterface
{
private bool $shouldContinue = true;
protected function execute(InputInterface $input, OutputInterface $output): int
{
while ($this->shouldContinue) {
$output->writeln('Still processing...');
sleep(1);
}
$output->writeln('Wrapping up, wait a sec...'); // this message is never shown (unless we remove the `exit()` call)
return self::SUCCESS;
}
public function getSubscribedSignals(): array
{
return [SIGINT, SIGTERM];
}
public function handleSignal(int $signal): void
{
$this->shouldContinue = false;
}
}
Possible Solution
I believe Symfony should not auto exit on that situation or at least provide a way to skip it.
Additional Context
Execution of the sample command with the exit()
:
$ bin/console testing
{"message":"Notified event \"console.command\" to listener \"Symfony\\Component\\HttpKernel\\EventListener\\DebugHandlersListener::configure\".","context":{"event":"console.command","listener":"Symfony\\Component\\HttpKernel\\EventListener\\DebugHandlersListener::configure"},"channel":"event","severity":"DEBUG","timestamp":"2022-10-07T10:12:24.056+00:00"}
{"message":"Notified event \"console.command\" to listener \"Symfony\\Bridge\\Monolog\\Handler\\ConsoleHandler::onCommand\".","context":{"event":"console.command","listener":"Symfony\\Bridge\\Monolog\\Handler\\ConsoleHandler::onCommand"},"channel":"event","severity":"DEBUG","timestamp":"2022-10-07T10:12:24.056+00:00"}
Still processing...
Still processing...
Still processing...
Still processing...
^C $
Execution of the sample command without the exit()
:
$ bin/console testing
{"message":"Notified event \"console.command\" to listener \"Symfony\\Component\\HttpKernel\\EventListener\\DebugHandlersListener::configure\".","context":{"event":"console.command","listener":"Symfony\\Component\\HttpKernel\\EventListener\\DebugHandlersListener::configure"},"channel":"event","severity":"DEBUG","timestamp":"2022-10-07T10:13:39.000+00:00"}
{"message":"Notified event \"console.command\" to listener \"Symfony\\Bridge\\Monolog\\Handler\\ConsoleHandler::onCommand\".","context":{"event":"console.command","listener":"Symfony\\Bridge\\Monolog\\Handler\\ConsoleHandler::onCommand"},"channel":"event","severity":"DEBUG","timestamp":"2022-10-07T10:13:39.000+00:00"}
Still processing...
Still processing...
Still processing...
^CWrapping up, wait a sec...
{"message":"Notified event \"console.terminate\" to listener \"Symfony\\Component\\Console\\EventListener\\ErrorListener::onConsoleTerminate\".","context":{"event":"console.terminate","listener":"Symfony\\Component\\Console\\EventListener\\ErrorListener::onConsoleTerminate"},"channel":"event","severity":"DEBUG","timestamp":"2022-10-07T10:13:41.554+00:00"}
{"message":"Notified event \"console.terminate\" to listener \"Symfony\\Bridge\\Monolog\\Handler\\ConsoleHandler::onTerminate\".","context":{"event":"console.terminate","listener":"Symfony\\Bridge\\Monolog\\Handler\\ConsoleHandler::onTerminate"},"channel":"event","severity":"DEBUG","timestamp":"2022-10-07T10:13:41.554+00:00"}
$
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Comments: 25 (25 by maintainers)
Commits related to this issue
- [Console] #47809 remove exit() call in last SignalHandler — committed to akuzia/symfony by akuzia 2 years ago
- [Console] #47809 remove exit() call in last SignalHandler — committed to timgchile/symfony by akuzia 2 years ago
- feature #49529 [Console] Add support for managing exit code while handling signals (lyrixx) This PR was merged into the 6.3 branch. Discussion ---------- [Console] Add support for managing exit cod... — committed to symfony/symfony by nicolas-grekas a year ago
Stumbled across this too (https://github.com/symfony/symfony/discussions/48024) How about not exit at all? Many of signals can be ignored, this is intended behavior. Just call all of the handlers with
handleSignals
as last one and leave it to the developer to decide what to do next. For outside integrations there is SIGKILL that most of process managers sends after SIGTERM anyway, as it cannot be ignored and always result in process termination. (Yes, i know there is TASK_UNINTERRUPTIBLE kernel state in linux. But it is outside of the application scope)One more example why SIGTERM shouldn’t immediately exit: when working with Messenger on k8s, your worker could be working for a while, say 10min.
If an rolling update comes in, the pod with the worker will receive a SIGTERM to “please shutdown”. It’s not expected to immediately exit:
K8s will wait for a
terminationGracePeriodSeconds
before killing the pod. What the Messenger consume command should do is accept the signal and mark itself as “not accepting any new messages”, but try to complete the current one, if any.I reverted to the expected behaviour in this MR: https://github.com/symfony/symfony/pull/48210, but this is still inconsistent whenever event listeners or signal handlers are used.
I think we need a
preventDefault()
method on the SignalEvent to skip the exit and handle shutdown gracefully.@lyrixx I’d expect that Symfony would only propagate the signals down to my application. No exiting whatsoever, regardless of having registered listeners.
Specially because there might be other components/libraries that are handling the signals and allowing the application to gracefully shutdown.
as a user what would you except for each use cases?
No signal handler has been registered for SIGINT => the program exit as soon as possible
A signal handler has been been registered for SIGINT => Symfony execute your handler, and then do nothing, it means it’ll resume your code “normally”
I agree this should be the default behavior. I added the exit because I wanted the mimic the default PHP behavior, but now I think I was wrong. Your example is really clear, and it should work as in the second case.
About
I don’t know! cc @nicolas-grekas
Just to make sure we’re aligned, the goal of the PR is to remove the exit call, right?
Although I completely agree with that decision, it’s a behavioural BC-break. Is Symfony alright with it?
100% that!
The application should be the one in control here, not Symfony.