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

Most upvoted comments

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:

At this point, Kubernetes will send a SIGTERM signal to the containers in the pod. This signal lets the containers know that they are going to be shut down soon. Your code should listen for this event and start shutting down cleanly at this point. This may include stopping any long-lived connections (like a database connection or WebSocket stream), saving the current state, or anything like that.

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.

as a user what would you except for each use cases?

@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?

  1. No signal handler has been registered for SIGINT => the program exit as soon as possible

  2. 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

Although I completely agree with that decision, it’s a behavioural BC-break. Is Symfony alright with it?

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?

a sigterm does the same thing : it kills the application, isn’t?

No. It signals application to exit. In which case developer can decide to gracefully exit or ignore this signal.

100% that!

The application should be the one in control here, not Symfony.