[Messenger] Add "--force-consumption" option to force the consumption of messages #29132

roukmoute · 2018-11-08T08:56:13Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

This P.R. adds a new option in messenger:consume-messages to do not stop the consumer when it receives a thrown exception.

webignition · 2018-11-12T08:53:46Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

@@ -66,6 +67,7 @@ protected function configure(): void
                new InputOption('memory-limit', 'm', InputOption::VALUE_REQUIRED, 'The memory limit the worker can consume'),
                new InputOption('time-limit', 't', InputOption::VALUE_REQUIRED, 'The time limit in seconds the worker can run'),
                new InputOption('bus', 'b', InputOption::VALUE_REQUIRED, 'Name of the bus to which received messages should be dispatched', $defaultBusName),
+                new InputOption('silent', 's', InputOption::VALUE_REQUIRED, 'Does not stop when an exception is thrown', false),


This change introduces a 'do not die if a message handler throws an exception' option.

A silent 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 option do-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?

So --force? Silent to me sounds confusing with existing --quiet|-q

I'm not sure about --force. It's certainly not as confusing as silent 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 ...

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.

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.

So I try with force option?
Or maybe --force-consumption to be sure.

@roukmoute I'm not certain that force is definitely what is needed. It's better than silent.

My feeling is that it would be better as force-<something descriptive> but, due to naming being hard, I haven't any useful suggestions.

Ok I like --force-consumption, I try and we will see ☺️

webignition · 2018-11-12T08:56:20Z

src/Symfony/Component/Messenger/Transport/Receiver/FailSilentlyReceiver.php

+            function ($message) use ($handler) {
+                try {
+                    $handler($message);
+                } catch (\Throwable $exception) {


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?

Or, to put it another way, I'd love to be able to run messenger: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.

l-vo · 2018-11-12T09:40:53Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

@@ -155,6 +161,10 @@ protected function execute(InputInterface $input, OutputInterface $output): void
            $receiver = new StopWhenTimeLimitIsReachedReceiver($receiver, $timeLimit, $this->logger);
        }

+        if ($input->getOption('silent')) {
+            $receiver = new FailSilentlyReceiver($receiver);


What about using DI for this instead of hardcoding ? It will allow to add parameters to your decorator (see https://github.com/symfony/symfony/pull/29132/files#r232573819) or to customize totally your option behavior with another decorator.

Sorry I don't understand how you see your suggestion, could you give me an example?

Hum... I didn't see it was done in this way for the other options. Sorry for the noise :)

webignition · 2018-11-12T13:47:00Z

@roukmoute Thanks for the changes, it's all looking much more clear to me now!

Tobion · 2018-11-15T02:43:01Z

src/Symfony/Component/Messenger/Transport/Receiver/ForceConsumptionReceiver.php

+                try {
+                    $handler($message);
+                } catch (\Throwable $exception) {
+                    if (!$this->isVerbose) {


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.

Tobion · 2018-11-15T02:46:46Z

src/Symfony/Component/Messenger/Transport/Receiver/ForceConsumptionReceiver.php

+    /** @var LoggerInterface */
+    private $logger;
+
+    public function __construct(ReceiverInterface $decoratedReceiver, bool $isVerbose, LoggerInterface $logger)


The logger needs to be nullable as the logger is also nullable in ConsumeMessagesCommand which is being passed here.

roukmoute · 2018-11-19T09:57:17Z

@Tobion is it ok for you?

Tobion · 2018-11-20T20:39:54Z

src/Symfony/Component/Messenger/Transport/Receiver/ForceConsumptionReceiver.php

+    /** @var ReceiverInterface */
+    private $decoratedReceiver;
+
+    /** @var LoggerInterface */


symfony does not add phpdocs for private properties as it can be inferred from the constructor. also i would need to be LoggerInterface|null

Tobion · 2018-11-21T01:40:42Z

src/Symfony/Component/Messenger/Transport/Receiver/ForceConsumptionReceiver.php

+    public function receive(callable $handler): void
+    {
+        $this->decoratedReceiver->receive(
+            function ($message) use ($handler) {


this is now accepting an evelope. it's missing typehint and $message should be renamed, see other receivers

Tobion · 2018-11-21T01:43:50Z

The implementation here might depend on how we add retry support, ref. #27008
But in general I think the workers should catch errors by default. So the logic should IMO be reversed to default to this behavior and only if a flag is passed, then abort on error.
Also it might be more straight-forward to implement this behavior as an constructor flag to the Worker class instead of creating a new receiver.

l-vo · 2018-11-21T16:21:46Z

@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 · 2018-11-22T09:29:43Z

@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 · 2018-11-22T20:35:50Z

@ro0NL Great ! I missed this annotation in Symfony. Thank you :)

roukmoute · 2018-11-29T17:21:11Z

Any news?

weaverryan · 2019-03-12T17:23:48Z

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 · 2019-03-17T06:59:48Z

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 | TODO This is an alternative to #29132 and #27008. There are several big things: 1) The `messenger:consume` does not die if a handler has an error 2) Events are dispatched before, after and on error a message being handled 3) 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

carsonbot added Status: Needs Review Messenger Feature labels Nov 8, 2018

roukmoute requested a review from sroze as a code owner November 8, 2018 10:08

chalasr added this to the next milestone Nov 8, 2018

webignition reviewed Nov 12, 2018

View reviewed changes

l-vo reviewed Nov 12, 2018

View reviewed changes

roukmoute changed the title ~~[Messenger] Add "silent" option~~ [Messenger] Add "--force-consumption" option to force the consumption of messages Nov 12, 2018

Tobion reviewed Nov 15, 2018

View reviewed changes

Tobion reviewed Nov 20, 2018

View reviewed changes

Tobion reviewed Nov 21, 2018

View reviewed changes

Add "force-consumption" option

4f174a7

This was referenced Mar 12, 2019

[Messenger] Making it Shine #30540

Closed

[Messenger] Worker events + global retry functionality #30557

Merged

fabpot closed this Mar 17, 2019

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Add "--force-consumption" option to force the consumption of messages #29132

[Messenger] Add "--force-consumption" option to force the consumption of messages #29132

roukmoute commented Nov 8, 2018

webignition Nov 12, 2018 •

edited

Loading

ro0NL Nov 12, 2018

webignition Nov 12, 2018

ro0NL Nov 12, 2018

webignition Nov 12, 2018

roukmoute Nov 12, 2018 •

edited

Loading

webignition Nov 12, 2018

roukmoute Nov 12, 2018 •

edited

Loading

webignition Nov 12, 2018

webignition Nov 12, 2018

l-vo Nov 12, 2018

roukmoute Nov 12, 2018

l-vo Nov 12, 2018

webignition commented Nov 12, 2018

Tobion Nov 15, 2018 •

edited

Loading

Tobion Nov 15, 2018

roukmoute commented Nov 19, 2018

Tobion Nov 20, 2018

Tobion Nov 21, 2018

Tobion commented Nov 21, 2018 •

edited

Loading

l-vo commented Nov 21, 2018

ro0NL commented Nov 22, 2018

l-vo commented Nov 22, 2018

roukmoute commented Nov 29, 2018

weaverryan commented Mar 12, 2019

fabpot commented Mar 17, 2019

[Messenger] Add "--force-consumption" option to force the consumption of messages #29132

[Messenger] Add "--force-consumption" option to force the consumption of messages #29132

Conversation

roukmoute commented Nov 8, 2018

webignition Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roukmoute Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roukmoute Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webignition commented Nov 12, 2018

Tobion Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roukmoute commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Nov 21, 2018 • edited Loading

l-vo commented Nov 21, 2018

ro0NL commented Nov 22, 2018

l-vo commented Nov 22, 2018

roukmoute commented Nov 29, 2018

weaverryan commented Mar 12, 2019

fabpot commented Mar 17, 2019

webignition Nov 12, 2018 •

edited

Loading

roukmoute Nov 12, 2018 •

edited

Loading

roukmoute Nov 12, 2018 •

edited

Loading

Tobion Nov 15, 2018 •

edited

Loading

Tobion commented Nov 21, 2018 •

edited

Loading