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.

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

[Console] Rework the signal integration #37827

Merged
merged 1 commit into from Aug 13, 2020
Merged

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Aug 13, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets refs #33729
License MIT
Doc PR

I updated the code to have a better design and DX:

  • SignalRegistry::$handlingSignals is gone. This was a temporary data store to
    bind signal to symfony event. This does not belong to the SignalRegistry. It
    has been replaced by Application::$signalsToDispatchEvent. A method has been
    added to edit this list Application::setSignalsToDispatchEvent()

  • The default value for Application::$signalsToDispatchEvent is SIGINT, SIGTERM, SIGUSR1, SIGUSR2. Theses defaults seems good enough for most of
    application. for recall:

    • SIGINT: CTRL+C
    • SIGTERM: kill PID, this is what occurs when stopping a process with SystemD, upstart & co
    • SIGUSR1 and SIGUSR2: Signal for user space
  • Application::setSignalRegistry() is gone. Now the Application always owns a
    signal registry. Since it's a CLI, it's legit to always have support for
    signals. A method has been added for convenience, and to register signal
    handler manually from the command:

        $application->getSignalRegistry()->register(SIGINT, function ($signal) {
            dump("Signal ($signal) caught");
        });
  • The interface SignalableCommandInterface has been added. When a command
    implements this interface, the command is automatically called when a
    registered signal has been caught

A note about the BC:

  • If one register an handler before the Symfony ones, the Signal Registry keeps
    existing registered handlers. The BC is kept
  • If one register an handler after the Symfony ones, it overrides the Symfony
    behavior. Since the feature is new. the BC is kept

So now, If one want to listen a signal, they have few options depending on the context:

  • A global action is common to all commands (such as logging, enabling a
    profiler (👋 blackfire.io)): Use a listener. With autoconfigure, the following
    code is enough:

    class SignalSubscriber implements EventSubscriberInterface
    {
        private $logger;
    
        public function __construct(LoggerInterface $logger = null)
        {
            $this->logger = $logger ?: new NullLogger();
        }
    
        public function handleSignal(ConsoleSignalEvent $event)
        {
            $signal = $event->getHandlingSignal();
    
            $this->logger->debug('The application has been signaled', [
                'signal' => $signal,
            ]);
        }
    
        public static function getSubscribedEvents()
        {
            return [
                ConsoleEvents::SIGNAL => 'handleSignal',
            ];
        }
    }
  • The command should react to a signal: Implements the interface:

    class SignalCommand extends Command implements SignalableCommandInterface
    {
        protected static $defaultName = 'signal';
    
        private $shouldStop = false;
    
        protected function execute(InputInterface $input, OutputInterface $output): int
        {
            $output->writeln('hello, I am '. getmypid());
    
            for ($i=0; $i < 60; $i++) {
                if ($this->shouldStop) {
                    break;
                }
    
                $output->write('.');
    
                sleep(1);
            }
            $output->writeln('');
    
            $output->writeln('bye');
    
            return 0;
        }
    
        public function handleSignal(int $signal)
        {
            dump([__METHOD__, $signal]);
    
            $this->shouldStop = true;
        }
    
        public function getSubscribedSignals(): array
        {
            return [SIGINT];
        }
    }
  • The command should react differently to many event and/or one wants a full control on name of the method called

    class SignalCommand extends Command
    {
        protected static $defaultName = 'signal';
    
        private $shouldStop = false;
    
        protected function execute(InputInterface $input, OutputInterface $output): int
        {
            $this->getApplication()->getSignalRegistry()->register(SIGINT, [$this, 'stop']);
            $this->getApplication()->getSignalRegistry()->register(SIGUSR1, [$this, 'enableBlackfire']);
    
            $output->writeln('hello, I am '. getmypid());
    
            for ($i=0; $i < 60; $i++) {
                if ($this->shouldStop) {
                    break;
                }
    
                $output->write('.');
    
                sleep(1);
            }
            $output->writeln('');
    
            $output->writeln('bye');
    
            return 0;
        }
    
        public function stop()
        {
            $this->shouldStop = true;
        }
    
        public function enableBlackfire()
        {
            // ...
        }
    }

ping @marie

Copy link
Member

@fabpot fabpot left a comment

LGTM, minor comments

@lyrixx
Copy link
Member Author

lyrixx commented Aug 13, 2020

@fabpot I addressed your comments. Thanks

fabpot
fabpot approved these changes Aug 13, 2020
@lyrixx lyrixx force-pushed the console-signal branch 4 times, most recently from 2993709 to 93c7e1f Compare Aug 13, 2020
@lyrixx
Copy link
Member Author

lyrixx commented Aug 13, 2020

Oh, don't merge this PR, there are something wrong: Since we listen some signal, the app never stop :)

@lyrixx
Copy link
Member Author

lyrixx commented Aug 13, 2020

Fixed :)

fabpot
fabpot approved these changes Aug 13, 2020
@fabpot
Copy link
Member

fabpot commented Aug 13, 2020

Thank you @lyrixx.

@fabpot fabpot merged commit d6ccc4f into symfony:master Aug 13, 2020
1 of 3 checks passed
@lyrixx lyrixx deleted the console-signal branch Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants