Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fixes #43866 configurable stop signals for messenger#44305
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedNov 28, 2021
Hey! I think@ruudk has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
ruudk left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks great, added a few comments.
| @@ -1,5 +1,6 @@ | |||
| framework: | |||
| messenger: | |||
| stop_signals:SIGTERM | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why does this have to be plural (a list of signals) instead of just 1 signal?
| stop_signals:SIGTERM | |
| stop_signal:SIGTERM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
in case project is used in several envs (docker FPM alpine environment, and classical VMs/bare-metal with supervisor) where signals could be different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then configure the correct signal for that env. Doesn't make sense (to me) to just fire all the signals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed with@Warxcell. we should allow multiple signals.
| privatefunctionstrsignal(int$signal):string | ||
| { | ||
| foreach (get_defined_constants(true)['pcntl']as$name =>$num) { | ||
| // the _ is to ignore SIG_IGN and SIG_DFL and SIG_ERR and SIG_BLOCK and SIG_UNBLOCK and SIG_SETMARK, and maybe more, who knows | ||
| if ($num ===$signal &&'SIG' ===substr($name,0,3) &&'_' !==$name[3]) { | ||
| return$name; | ||
| } | ||
| } | ||
| thrownew \InvalidArgumentException(sprintf('The signal "%d" is not valid.',$signal)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would move this responsibility to the compilation/configuration step and just always have the signal int injected via the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is the case, but for logger, we need the name of the signal - so I need to reverse the logic - get the signal name by its id (int)
jderusse left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
for 6.1 as it's a new feature
| ->children() | ||
| ->arrayNode('stop_signals') | ||
| ->beforeNormalization() | ||
| ->ifString()->then(staticfn (string$value):int =>constant($value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't we check of the constant exists and is a supported signal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if constant doesn't exists - it will return null, and will be caught at validate. About supported signal, it's checked inside the Listeners itself.
| ->then(staticfunction (array$v):void {thrownewInvalidConfigurationException(sprintf('The specified default bus "%s" is not configured. Available buses are "%s".',$v['default_bus'],implode('", "',array_keys($v['buses'])))); }) | ||
| ->end() | ||
| ->children() | ||
| ->arrayNode('stop_signals') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For lists, you should addfixXmlConfig
| useSymfony\Component\Workflow\WorkflowEvents; | ||
| usefunctionarray_keys; | ||
| usefunctionconstant; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do not import functions please.
| ->tag('kernel.event_subscriber') | ||
| ->tag('monolog.logger', ['channel' =>'messenger']) | ||
| ->set('messenger.listener.stop_worker_on_sigterm_signal_listener',StopWorkerOnSigtermSignalListener::class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is it a BC break? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
isn't that internal service used by framework itself? how can it be BC-break?
| @@ -1,5 +1,6 @@ | |||
| framework: | |||
| messenger: | |||
| stop_signals:SIGTERM | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed with@Warxcell. we should allow multiple signals.
magnetik commentedFeb 22, 2022
@Warxcell hi, sorry for the direct ping :) |
Warxcell commentedFeb 28, 2022
Yes, sorry. Will try to finish till the end of the week. |
33341f9 to8aaa16dCompare58cd5d6 todac1bd8Comparedac1bd8 to2aef839CompareWarxcell commentedMar 15, 2022
Need a little help - one test needs PCNTL extension, and the other don't need it - is this achievable? |
fabpot commentedApr 8, 2024
Closing in favor of#54510 |
Uh oh!
There was an error while loading.Please reload this page.