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

33411 [Console] Add stop event #33729

Open
wants to merge 9 commits into
base: 4.4
from
Open

Conversation

@marie
Copy link
Contributor

marie commented Sep 27, 2019

Q A
Branch? 4.4 for features
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #33411
License MIT
Doc PR symfony/symfony-docs#...

This new feature allows to set a listener for performing some actions after the console command get a signal to stop and before it was stopped.

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Sep 28, 2019

Thanks @marie for your first contribution.

This will be not so simple:

  1. You can register only one listener by signal. This is a limitation of PHP :/ I don't know what is the best solution to handle this. We can add an abstraction (Listener Registry, something like https://github.com/romainneutron/signal-handler). Or we can do nothing : Symfony will define a listener first. And if in my application I register another listener, then this new listener will be used. But there is a drawback: the original listener from SF will not be called. This could lead to some WTF.
  2. edit I missed the fact your already used it You need something to dispatch from PHP core (buffer) to PHP Userland the signal. Since Symfony requires PHP 7.1+ I think our best option is to use https://www.php.net/pcntl_async_signals
  3. You hardcode a list a signal, this should be configurable.
$onStopHandler = function () use ($event) {
$this->dispatcher->dispatch($event, ConsoleEvents::STOP);
/** @TODO what exit code to return? */
exit;

This comment has been minimized.

Copy link
@lyrixx

lyrixx Sep 28, 2019

Member

Instead of this, we can do:
Add a ConsoleStopEvent::shouldStop() merthod.

  • If it returns false: do nothing
  • If it returns true: trigger a ConsoleTerminateEvent and return $event->getExitCode(); (like below)
src/Symfony/Component/Console/ConsoleEvents.php Outdated Show resolved Hide resolved
@marie

This comment has been minimized.

Copy link
Contributor Author

marie commented Sep 29, 2019

Hello, @lyrixx. Thank you for your code review!
What do you think about the registry for signals? https://github.com/symfony/symfony/pull/33729/files#diff-a0717f72232896c363603bd89a126da5

/** @TODO what exit code to return? */
exit;
};
$this->signalRegistry->register(SIGINT, $onStopHandler);

This comment has been minimized.

Copy link
@marie

marie Sep 30, 2019

Author Contributor

@lyrixx, how it would be convenient to set the list of handling signals from the outside?

This comment has been minimized.

Copy link
@lyrixx

lyrixx Sep 30, 2019

Member

Use a static property + method on the registry ? or expose the registry on the command!

class MyCommand
{
    public __construct() { $this->getApplication()->getSignalResitry()->enable(SIGINT); }
}

This comment has been minimized.

Copy link
@marie

marie Sep 30, 2019

Author Contributor

Maybe it would be better to set signals for the application? Then we don't need to know about SignalRegistry in Commands. And through SignalResitry()->enable(SIGINT) I couldn't define a callback, thus this handler won't work until I assign callbacks. If we're going to use this Registry as a container for signals in whole project, there might be cases when one component wants to handle SIGINT with their own callback and another SIGUSR1 with another callback?

@marie marie force-pushed the marie:33411-console-stop-event branch from 0118e9c to 1797ac7 Oct 1, 2019
@marie marie marked this pull request as ready for review Oct 23, 2019
@marie marie force-pushed the marie:33411-console-stop-event branch from b029449 to 6279c3b Oct 26, 2019
@marie marie force-pushed the marie:33411-console-stop-event branch from 3b2c4a8 to f5ab3f6 Oct 26, 2019
@marie marie requested a review from lyrixx Oct 26, 2019
@@ -941,6 +959,17 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
// ignore invalid options/arguments for now, to allow the event listeners to customize the InputDefinition
}

if ($this->signalRegistry) {
$event = new ConsoleSignalEvent($command, $input, $output);

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 29, 2019

Member

Could you add the signal number (SIGINT, etc) to the event ?
This mean you will have to move this part of the code in the onStopHandler method

@@ -941,6 +959,17 @@ protected function doRunCommand(Command $command, InputInterface $input, OutputI
// ignore invalid options/arguments for now, to allow the event listeners to customize the InputDefinition
}

if ($this->signalRegistry) {
$event = new ConsoleSignalEvent($command, $input, $output);
$onStopHandler = function () use ($event) {

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 29, 2019

Member
  1. according to your code, you get the signumber here, let's use it (see my previous comment)
  2. onStopHandler is not representative. onSignalHandler seems better
@@ -186,6 +186,10 @@ protected function registerCommands()
$this->setCommandLoader($container->get('console.command_loader'));
}

if (\extension_loaded('pcntl') && $container->has('console.signal_registry')) {
$this->setSignalRegistry($container->get('console.signal_registry'));

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 29, 2019

Member

this service is not defined?

public function register(int $signal, callable $callback): void
{
if (!isset($this->signals[$signal])) {
$previousCallback = pcntl_signal_get_handler($signal);

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 29, 2019

Member

I'm not sure, but I think you should check if the previous callback is not this class.
Is it possible?

This comment has been minimized.

Copy link
@marie

marie Jan 13, 2020

Author Contributor

No, they are all Closures at this place.
When it may be helpful? I think it's fine to get all previous handlers.

}

$this->signals[$signal][] = $callback;
pcntl_signal($signal, [$this, 'handler']);

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 29, 2019

Member

s/handler/handle/


public function handler(int $signal): void
{
foreach ($this->signals[$signal] as $callback) {

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 29, 2019

Member

I don't really like callback name. handler or signalHandler seems better


namespace Symfony\Component\SignalRegistry;

interface SignalRegistryInterface

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 29, 2019

Member

Do we really need an interface ? I don't think so.

@dillingham

This comment has been minimized.

Copy link

dillingham commented Nov 11, 2019

Ps thanks @marie for putting time towards this. Much appreciated! 😄

@marie

This comment has been minimized.

Copy link
Contributor Author

marie commented Nov 12, 2019

@dillingham, I'm sorry I don't have time to finish the task now. I'll write in the related issue that it is unassigned.

@marie marie closed this Nov 12, 2019
@dillingham

This comment has been minimized.

Copy link

dillingham commented Nov 12, 2019

I understand. Thanks for trying

Seems like a waste to close this when the reviews only seem to be cosmetic

If someone wants to take this over, I'll happily tip via paypal

@noniagriconomie

This comment has been minimized.

Copy link
Contributor

noniagriconomie commented Nov 12, 2019

yes it's too bad, could be a great addition for 4.4
what is missing exaclty?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 12, 2019

If anyone is interested in pursuing the work, please check it out, squash all commits in one, add yours after, then submit a PR.

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Nov 12, 2019

Please, feel free to take it. if nobody wants to work on it, I may finish it (ping me in 1 month)

@noniagriconomie It will be for SF 5.1 or more :)

@noniagriconomie

This comment has been minimized.

Copy link
Contributor

noniagriconomie commented Nov 12, 2019

arf yes indeed :)

@dillingham

This comment has been minimized.

Copy link

dillingham commented Dec 19, 2019

@lyrixx ping :)

I would of myself but I have no idea how this works haha

@dillingham

This comment has been minimized.

Copy link

dillingham commented Jan 12, 2020

Putting $100 bounty on this if anyone can finish it

cc @lyrixx @nicolas-grekas @marie @noniagriconomie

@marie marie reopened this Jan 13, 2020
@marie marie requested a review from lyrixx Jan 15, 2020
@dillingham

This comment has been minimized.

Copy link

dillingham commented Jan 26, 2020

Hey @lyrixx 👋, has Marie satisfied your review / critiques?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.