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 ability to schedule alarm signals and aConsoleAlarmEvent#53533

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:7.2fromHypeMC:add-alarmable-command
Oct 6, 2024

Conversation

HypeMC
Copy link
Member

@HypeMCHypeMC commentedJan 13, 2024•
edited
Loading

QA
Branch?7.2
Bug fix?no
New feature?yes
Deprecations?no
IssuesFix#47920
LicenseMIT

Part of#53508

This PR introduces the ability to schedule alarm signals and aconsole.alarm event. A command using this feature will automatically dispatch an alarm at the specified interval:

#[AsCommand('app:alarm')]class AlarmCommandextends Commandimplements SignalableCommandInterface{privatebool$run =true;privateint$count =0;privateOutputInterface$output;protectedfunctioninitialize(InputInterface$input,OutputInterface$output):void    {$this->getApplication()->setAlarmInterval(10);    }protectedfunctionexecute(InputInterface$input,OutputInterface$output):int    {$this->output =$output;while ($this->run) {$this->count++;$output->writeln('execute'.$this->count);sleep(1);        }$output->writeln('Done');return Command::SUCCESS;    }publicfunctiongetSubscribedSignals():array    {return [\SIGINT, \SIGALRM];    }publicfunctionhandleSignal(int$signal,false|int$previousExitCode =0):int|false    {if (\SIGINT ===$signal) {$this->run =false;$this->output->writeln('Bye');        }elseif (\SIGALRM ===$signal) {$this->count =0;$this->output->writeln('handleAlarm');        }returnfalse;    }}

Theconsole.alarm event is dispatched on everySIGALRM signal:

#[AsEventListener(event: ConsoleAlarmEvent::class)]finalclass ConsoleAlarmListener{publicfunction__invoke(ConsoleAlarmEvent$event):void    {$event->getOutput()->writeln('ConsoleAlarmListener');    }}

valtzu, dkarlovi, and OskarStark reacted with thumbs up emoji
@HypeMCHypeMCforce-pushed theadd-alarmable-command branch 3 times, most recently froma11d0da to93f7c17CompareJanuary 13, 2024 19:29
@HypeMCHypeMCforce-pushed theadd-alarmable-command branch 2 times, most recently from3496c6f to26d9c59CompareFebruary 25, 2024 13:45
@HypeMCHypeMCforce-pushed theadd-alarmable-command branch from26d9c59 to4ff3662CompareApril 9, 2024 18:51
@valtzu
Copy link
Contributor

valtzu commentedMay 7, 2024•
edited
Loading

Btw,SIGALRM has the same effect as any other signal – it interruptssleep &usleep. According to docs, one should check based on return code if thesleep was interrupted but I doubt many do that – andusleep returnsvoid so for that it's impossible.

So if a command logic f.e. includessleep(30); or$clock->sleep(30);, it will only sleep until next alarm, potentially causing unexpected behavior.

Another (old) thing I found that usingSIGALRM to provide timer functionalitymay result in deadlock – not sure if it's still relevant (or if it ever was, for php).

@xabbuhxabbuh modified the milestones:7.1,7.2May 15, 2024
@HypeMCHypeMCforce-pushed theadd-alarmable-command branch from4ff3662 to16b0fd9CompareAugust 11, 2024 14:18
Copy link
Contributor

@ro0NLro0NL left a comment•
edited
Loading

Choose a reason for hiding this comment

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

cc@chalasr

we're running a userland implementation in prod, it works fine

@HypeMCHypeMCforce-pushed theadd-alarmable-command branch 2 times, most recently fromd0507cd to5c32d9cCompareSeptember 17, 2024 23:14
@HypeMCHypeMC changed the title[Console] AddAlarmableCommandInterface andconsole.alarm event[Console] Add ability to schedule alarm signals and aconsole.alarm eventSep 17, 2024
Comment on lines -978 to +1002
if (!$this->signalRegistry) {
throw new RuntimeException('Unable to subscribe to signal events. Make sure that the "pcntl" extension is installed and that "pcntl_*" functions are not disabled by your php.ini\'s "disable_functions" directive.');
}
$signalRegistry = $this->getSignalRegistry();
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

getSignalRegistry throws a nearly identical exception, so it made sense to me to have it centralized.

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.

The implementation is now much simpler, looks good to me 👍

@HypeMCHypeMCforce-pushed theadd-alarmable-command branch 3 times, most recently from33fb7b7 to5541f00CompareSeptember 26, 2024 14:58
@HypeMCHypeMC changed the title[Console] Add ability to schedule alarm signals and aconsole.alarm event[Console] Add ability to schedule alarm signals and aConsoleAlarmEventSep 26, 2024
@HypeMCHypeMCforce-pushed theadd-alarmable-command branch from5541f00 to33f049cCompareOctober 1, 2024 10:12
@fabpotfabpotforce-pushed theadd-alarmable-command branch from33f049c to97e4391CompareOctober 6, 2024 09:25
@fabpot
Copy link
Member

Thank you@HypeMC.

@fabpotfabpot merged commit85380cf intosymfony:7.2Oct 6, 2024
8 of 10 checks passed
@HypeMCHypeMC deleted the add-alarmable-command branchOctober 6, 2024 10:12
chalasr added a commit that referenced this pull requestOct 7, 2024
…which messages are still being processed (HypeMC)This PR was merged into the 7.2 branch.Discussion----------[Console][Messenger] Asynchronously notify transports which messages are still being processed| Q             | A| ------------- | ---| Branch?       | 7.2| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Issues        | -| License       | MITCertain transports, such as Beanstalkd, have a timeout that determines how long a certain message is considered as "in progress". If the message is not acknowledged or rejected within that time period, it is returned to the "ready" queue.This could cause potential issues when it takes a handler longer to finish processing the message. In those instances, the message being processed could be returned to the ready queue and taken by another instance of the receiver. This would result in the message being processed multiple times, or even worse, in an infinite loop.Usually, transports that have a timeout also have the option to be notified that the message is still being processed, so the timeout can be postponed.The idea of this PR is to utilize the `SignalableCommandInterface` and `pcntl_alarm()` to notify transports which messages are still being processed. Since the `SignalRegistry` has [asynchronous signals enabled](https://github.com/symfony/symfony/blob/34915f6e16f04537eb18d9d2c303ec375e63cc4b/src/Symfony/Component/Console/SignalRegistry/SignalRegistry.php#L21) by default, the whole process would happen asynchronously. I've named this feature "keepalive" for lack of a better name.Currently, I've added this option only to the Beanstalkd transport since that's the one I'm familiar with and use, but as far as I was able to gather, at least one other transport supports this feature. Amazon SQS has a visibility timeout which can be [increased](https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-visibility-timeout.html#changing-message-visibility-timeout). This visibility timeout seems to be the same thing as Beanstalkd's TTR. (Disclaimer: I've never used Amazon SQS, so if I got something wrong, please let me know.)I've split the PR into two commits so it would (hopefully) be easier to review:1. I've extracted the first commit into a separate PR:#535332. The second commit adds the keepalive feature to the Messenger component.Commits-------57b556a [Messenger] Notify transports which messages are still being processed, using `pcntl_alarm()`
@dkarlovi
Copy link
Contributor

Thanks for working on this@HypeMC!

HypeMC reacted with thumbs up emoji

@fabpotfabpot mentioned this pull requestOct 27, 2024
@chalasrchalasr mentioned this pull requestDec 11, 2024
fabpot added a commit that referenced this pull requestDec 11, 2024
This PR was merged into the 7.2 branch.Discussion----------[Console] Fix tests| Q             | A| ------------- | ---| Branch?       | 7.2| Bug fix?      | no| New feature?  | no| Deprecations? | no| Issues        | -| License       | MITThe Console test suite was not running correctly since#53533.See unit tests on 7.2/7.3:<img width="863" alt="Screenshot 2024-12-11 at 04 53 56" src="https://github.com/user-attachments/assets/389281a3-9f62-4984-8d96-5704a123e476">/cc `@theofidry`Commits-------838eb95 [Console] Fix tests
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stofstofstof requested changes

@chalasrchalasrchalasr approved these changes

@fabpotfabpotfabpot approved these changes

@ro0NLro0NLro0NL approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.

[Console] add console.alarm event
9 participants
@HypeMC@valtzu@fabpot@dkarlovi@stof@ro0NL@chalasr@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp