Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Add "--force-consumption" option to force the consumption of messages#29132
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
| newInputOption('memory-limit','m', InputOption::VALUE_REQUIRED,'The memory limit the worker can consume'), | ||
| newInputOption('time-limit','t', InputOption::VALUE_REQUIRED,'The time limit in seconds the worker can run'), | ||
| newInputOption('bus','b', InputOption::VALUE_REQUIRED,'Name of the bus to which received messages should be dispatched',$defaultBusName), | ||
| newInputOption('silent','s', InputOption::VALUE_REQUIRED,'Does not stop when an exception is thrown',false), |
webignitionNov 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 change introduces a 'do not die if a message handler throws an exception' option.
Asilent option suggests that the command is otherwise non-silent and that the addition of this option addresses existing non-silent behaviour. From what I've seen in my usage of the messenger component,messenger:consume-messages is currently pretty good at being silent.
It doesn't seem that this option relates to the silencing of existing non-silent behaviour.
Naming the optiondo-not-die-if-a-message-handler-throws-an-exception is closer to describing what the option does. Obviously this is not the best choice as it is a little on the verbose side.
Can this option be re-named to better convey what is being addressed?
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.
So--force? Silent to me sounds confusing with existing--quiet|-q
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'm not sure about--force. It's certainly not as confusing assilent could be with respect to the existing--quiet but it still lacks a little descriptiveness.
I'm considering this from the perspective of seeing the command in use and being able to understand what will happen based on the options being provided.
messenger:consume-messages --force doesn't tell me much about what is being forced, particularly given how--force is often used to say 'do this anyway despite the potential for destructive irreversible changes`.
messenger:consume-messages --swallow-exceptions comes closer to conveying what is actually happening. I don't like this as it is too implementation-specific (I'm assuming the possibility of implementing the desired behaviour without having to swallow exceptions).
messenger:consume-messages --naming-is-hard ...
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.
messenger:consume-messages --force doesn't tell me much about what is being forced
It does to me: "force the consumption of messages".
Running it with -vvv you'd see the logs.
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.
Cool, thanks for the clarification.
"force the consumption of messages" in this context expands to "force the consumption of messages even if an exception is thrown by a message handler".
This could be expressed in a more generic form of "force the consumption of messages in the case of X".
If X is only ever "if an exception is thrown by a message handler" then--force seems fine to me now that I understand where you're coming from.
On the other hand, if we would ever need to force the consumption of messages for a reason other than due to an exception being thrown by a message handler,--force is probably too vague.
roukmouteNov 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
So I try withforce option?
Or maybe--force-consumption to be sure.
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.
@roukmoute I'm not certain thatforce is definitely what is needed. It's better thansilent.
My feeling is that it would be better asforce-<something descriptive> but, due to naming being hard, I haven't any useful suggestions.
roukmouteNov 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Ok I like--force-consumption, I try and we will see
| function ($message)use ($handler) { | ||
| try { | ||
| $handler($message); | ||
| }catch (\Throwable$exception) { |
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.
An exception thrown by a message handler is a good indicator of there being something that needs addressing. Silently swallowing such exceptions hides potential faults in the relevant message handler.
Can we/should we introduce an option to log the exception?
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.
Or, to put it another way, I'd love to be able to runmessenger:consume-messages knowing that it won't stop if an exception occurs within a message handler. But if an exception does occur, I would really need to know about it as it probably relates to a bug that I need to fix.
| } | ||
| if ($input->getOption('silent')) { | ||
| $receiver =newFailSilentlyReceiver($receiver); |
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.
What about using DI for this instead of hardcoding ? It will allow to add parameters to your decorator (seehttps://github.com/symfony/symfony/pull/29132/files#r232573819) or to customize totally your option behavior with another decorator.
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.
Sorry I don't understand how you see your suggestion, could you give me an example?
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.
Hum... I didn't see it was done in this way for the other options. Sorry for the noise :)
webignition commentedNov 12, 2018
@roukmoute Thanks for the changes, it's all looking much more clear to me now! |
| try { | ||
| $handler($message); | ||
| }catch (\Throwable$exception) { | ||
| if (!$this->isVerbose) { |
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 flag is not useful. If I don't care about the log messages, I can just inject a NullLogger myself or set no logger at all.
| /** @var LoggerInterface */ | ||
| private$logger; | ||
| publicfunction__construct(ReceiverInterface$decoratedReceiver,bool$isVerbose,LoggerInterface$logger) |
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.
The logger needs to be nullable as the logger is also nullable in ConsumeMessagesCommand which is being passed here.
roukmoute commentedNov 19, 2018
@Tobion is it ok for you? |
| /** @var ReceiverInterface */ | ||
| private$decoratedReceiver; | ||
| /** @var LoggerInterface */ |
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.
symfony does not add phpdocs for private properties as it can be inferred from the constructor. also i would need to beLoggerInterface|null
| publicfunctionreceive(callable$handler):void | ||
| { | ||
| $this->decoratedReceiver->receive( | ||
| function ($message)use ($handler) { |
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 now accepting an evelope. it's missing typehint and $message should be renamed, see other receivers
Tobion commentedNov 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The implementation here might depend on how we add retry support, ref.#27008 |
l-vo commentedNov 21, 2018
@Tobion reversing the logic may break the applications which are already using the worker. What is usually done on Symfony in these cases ? Wait and Introduce the change at the next major ? |
ro0NL commentedNov 22, 2018
@l-vo all the Messenger code is still experimental. If we have good reasons to change it we should:https://symfony.com/doc/current/contributing/code/experimental.html |
l-vo commentedNov 22, 2018
@ro0NL Great ! I missed this annotation in Symfony. Thank you :) |
roukmoute commentedNov 29, 2018
Any news? |
weaverryan commentedMar 12, 2019
Why not make this the default behavior? Shouldn't a worker continue staying "alive" as it handles messages? And then shouldn't there be a way to "hook in" to the failure process to do something? Maybe even be able to decide whether or not you want to re-queue to retry? |
fabpot commentedMar 17, 2019
Closing in favor of#30557 |
… (weaverryan)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] Worker events + global retry functionality| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | yes, on Messenger only| Deprecations? | no| Tests pass? | NEEDED| Fixed tickets |#29132,#27008,#27215 and part of#30540| License | MIT| Doc PR | TODOThis is an alternative to#29132 and#27008. There are several big things:1) The `messenger:consume` does not die if a handler has an error2) Events are dispatched before, after and on error a message being handled3) Logic is moved out of Amqp and into the Worker so that we can have some consistent features, like error handling.4) A generic retry system was added, which works with Amqp and future transports should support. It will work out of the box for users. Retrying works by putting the received `Envelope` back into the bus, but with the `ReceivedStamp` removed. The retry functionality has an integration test for AMQP.5) Added a new `MessageDecodingFailedException` that transport Serializers should throw if `decode()` fails. It allows us to reject a message in this situation, as allowing it to fail but remain on the queue causes it to be retried forever.6) A new `DelayStamp` was added, which is the first of (later) more stamps for configuring the transport layer (see#30558).BC breaks are documented in the CHANGELOG.Thanks!Commits-------a989384 Adding global retry support, events & more to messenger transport
This P.R. adds a new option in
messenger:consume-messagesto do not stop the consumer when it receives a thrown exception.