Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Console] Add signal event#33729

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfrommarie:33411-console-stop-event
Jul 31, 2020

Conversation

@marie
Copy link
Contributor

@mariemarie commentedSep 27, 2019
edited by lyrixx
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#33411
LicenseMIT
Doc PRsymfony/symfony-docs#...

This new feature allows to set a listener for performing some actions after the console command get a signal.

Usage:

<?phprequire__DIR__.'/vendor/autoload.php';useSymfony\Component\Console\Application;useSymfony\Component\Console\Command\Command;useSymfony\Component\Console\ConsoleEvents;useSymfony\Component\Console\Event\ConsoleSignalEvent;useSymfony\Component\Console\Input\InputInterface;useSymfony\Component\Console\Output\OutputInterface;useSymfony\Component\Console\SignalRegistry\SignalRegistry;useSymfony\Component\EventDispatcher\EventDispatcher;class HelloWorldCommandextends Command {protectedstatic$defaultName ='app:hello-world';protectedfunctionexecute(InputInterface$input,OutputInterface$output):int {$output->writeln('hello');for ($i=0;$i <10;$i++) {$output->write('.');sleep(1);        }$output->writeln('');$output->writeln('bye');return0;    }}$signalRegistry =newSignalRegistry();// List of POSIX signals for handling$signalRegistry->addHandlingSignals(SIGINT,SIGUSR1);$dispatcher =newEventDispatcher();// Function that will handle signals$dispatcher->addListener(ConsoleEvents::SIGNAL,function (ConsoleSignalEvent$event) {echo'Handled signal #' .$event->getHandlingSignal();});$application =newApplication();$application->add(newHelloWorldCommand());$application->setDispatcher($dispatcher);$application->setSignalRegistry($signalRegistry);$application->run();

dillingham, bigfoot90, noniagriconomie, cfoehrdes, apfelbox, and maxhelias reacted with thumbs up emoji
@lyrixx
Copy link
Member

lyrixx commentedSep 28, 2019
edited
Loading

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 likehttps://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 itYou 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 usehttps://www.php.net/pcntl_async_signals
  3. You hardcode a list a signal, this should be configurable.
marie reacted with thumbs up emoji

@marie
Copy link
ContributorAuthor

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

@mariemarieforce-pushed the33411-console-stop-event branch from0118e9c to1797ac7CompareOctober 1, 2019 12:05
@mariemarie marked this pull request as ready for reviewOctober 23, 2019 17:15
@mariemarieforce-pushed the33411-console-stop-event branch 2 times, most recently from3b2c4a8 tof5ab3f6CompareOctober 26, 2019 18:28
@mariemarie requested a review fromlyrixxOctober 26, 2019 18:47
@dillingham
Copy link

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

@marie
Copy link
ContributorAuthor

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

@mariemarie closed thisNov 12, 2019
@dillingham
Copy link

dillingham commentedNov 12, 2019
edited
Loading

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
Copy link
Contributor

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

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 12, 2019
edited
Loading

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
Copy link
Member

lyrixx commentedNov 12, 2019
edited
Loading

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 :)

@chalasrchalasrforce-pushed the33411-console-stop-event branch 3 times, most recently from063f49d to1574956CompareJune 17, 2020 15:30
@marie
Copy link
ContributorAuthor

Hello! The merge request is in status "needs work", but there are no discussions. What do I need to do to make it accepted? :)

@lyrixx
Copy link
Member

lyrixx commentedJun 30, 2020
edited
Loading

Hello, Sorry for the delay. I did not test it, but If the code in the PR description works as expected, it seems good to me 👍

But (for another PR) :

  • And easier way to leverage this system in Symfony (framework) should be added (I could take care of this one)
  • The integration with messenger component should be fixed (maybe this one too)

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

We've got time to improve it. 👍

noniagriconomie reacted with thumbs up emoji
@fabpotfabpotforce-pushed the33411-console-stop-event branch from1574956 to859b692CompareJuly 31, 2020 07:50
@fabpot
Copy link
Member

Thank you@marie.

marie and dillingham reacted with hooray emoji

@fabpot
Copy link
Member

@lyrixx it's now merged, do you have time to implement the 2 items you mentioned above?

@fabpotfabpot merged commit2d5e7b0 intosymfony:masterJul 31, 2020
@lyrixx
Copy link
Member

@fabpot Yes :) I'll work on this feature on 13th august

xabbuh reacted with thumbs up emoji

@dillingham
Copy link

@marie I offered a bounty earlier, feel free to email me to collect. Thanks everyone for the effort on this!

damienalexandre reacted with hooray emoji

fabpot added a commit that referenced this pull requestAug 13, 2020
This PR was merged into the 5.2-dev branch.Discussion----------[Console] Rework the signal integration| 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:    ```php        $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 caughtA 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:    ```php    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:    ```php    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    ```php    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@marieCommits-------df57119 [Console] Rework the signal integration
@nicolas-grekasnicolas-grekas modified the milestones:next,5.2Oct 5, 2020
@fabpotfabpot mentioned this pull requestOct 5, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@lyrixxlyrixxlyrixx left review comments

@derrabusderrabusderrabus left review comments

@fabpotfabpotfabpot approved these changes

@chalasrchalasrchalasr approved these changes

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

+3 more reviewers

@noniagriconomienoniagriconomienoniagriconomie left review comments

@darielopperdarielopperdarielopper approved these changes

@dillinghamdillinghamdillingham approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

Feature Request / Console / ConsoleEvents::STOP

10 participants

@marie@lyrixx@dillingham@noniagriconomie@nicolas-grekas@fabpot@derrabus@chalasr@darielopper@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp