[Messenger] Ensure message is handled only once per handler #30020

keulinho · 2019-01-29T13:34:17Z

Add check to ensure that a message is only handled once per handler
Add try...catch to run all handlers before throwing exception

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#27215
License	MIT
Doc PR	Todo

This would make error handling and retrying of messages much more easier. As statet here #27008 (comment) there is currently no way to retry a for all failed handlers if there are mutliple handlers and just some throw an exception.
Also if an Exception in an handler occurs the execution chain is disrupted and the other handlers are never invoked.
With this change it is easily possible to create an userland middleware that catches the ChainedHandlerFailedException and does some custom retry logic. If you ensure that the HandledStamps on the Envelope are preserved the message will be handled just by the failed handlers

src/Symfony/Component/Messenger/Stamp/HandledStamp.php

src/Symfony/Component/Messenger/Tests/Middleware/HandleMessageMiddlewareTest.php

sroze · 2019-03-23T10:59:45Z

We need to wait for #30557 to be merged to go forward with this one because we need to make sure the HandledStamps are kept when retrying then.

But either way, wouldn't it make more sense to retry all the listeners by default? It might cause even more inconsistency if we only call the "remaining listeners" isn't it? (only if they are related to each other I guess)

fabpot · 2019-03-23T15:01:18Z

#30557 is merged now :)

weaverryan · 2019-03-24T00:46:24Z

@keulinho can you rebase now that #30557 is merged? This is a missing piece to handling failures correctly. It could be a bit more complex, but I'm wondering if we should add an integration test involving Worker with a fake "receiver" and a bus with all the default middleware. That would allow us to test this complex behavior: e.g. use Worker to "receive" a message that has 2 handlers where 1 fails the first time. Then see that there was a retry, but each handler was ultimately only called 1 time. The whole "flow" really involves Worker and several middleware working together properly.

But either way, wouldn't it make more sense to retry all the listeners by default? It might cause even more inconsistency if we only call the "remaining listeners" isn't it? (only if they are related to each other I guess)

Hmm, I don't think I agree with this. It seems like more risk to knowingly execute a handler two times.

src/Symfony/Component/Messenger/Exception/ChainedHandlerFailedException.php

src/Symfony/Component/Messenger/Middleware/HandleMessageMiddleware.php

src/Symfony/Component/Messenger/Tests/Fixtures/DummyMessageHandler.php

weaverryan · 2019-03-31T16:16:06Z

@keulinho could you please rebase again? And mark the previous comments as "Resolved" if you've handled them. A lot of things are being merged to messenger currently - so sorry for the repeated rebasing!

src/Symfony/Component/Messenger/Exception/ChainedHandlerFailedException.php

src/Symfony/Component/Messenger/Tests/RetryIntegrationTest.php

sroze · 2019-04-06T10:42:20Z

I've changed the exception name to HandlerFailedException and added a note in the CHANGELOG.

src/Symfony/Component/Messenger/Exception/HandlerFailedException.php

weaverryan · 2019-04-06T10:59:37Z

src/Symfony/Component/Messenger/Middleware/HandleMessageMiddleware.php

-            $handledStamp = HandledStamp::fromCallable($handler, $handler($message), \is_string($alias) ? $alias : null);
-            $envelope = $envelope->with($handledStamp);
-            $this->logger->info('Message "{class}" handled by "{handler}"', $context + ['handler' => $handledStamp->getCallableName()]);
+            $alias = \is_string($alias) ? $alias : null;


What if $alias is null and there are 2 handlers? Wouldn't that cause the second one to "appear" like it was handled? I think if the $alias is null, we have to assume that the message has not already been handled always.

They would not appear as handled because we track the handler name. Alias is just an optional thing we actually don't use in core. (I think we could even remove it, it's been introduced - #29166 - on the assumption that it might be useful later, while it complexifies reading this code).

weaverryan · 2019-04-06T11:01:44Z

src/Symfony/Component/Messenger/Middleware/HandleMessageMiddleware.php

@@ -66,6 +78,21 @@ public function handle(Envelope $envelope, StackInterface $stack): Envelope
            $this->logger->info('No handler for message "{class}"', $context);
        }

+        if (\count($exceptions)) {
+            throw new HandlerFailedException($envelope, ...$exceptions);
+        }


Just thinking out loud: one practical implication is that, if someone listens on the new WorkerMessageFailedEvent event, they will always (well, not technically "always", but pretty much always) receive this exception instead of the actual exception. Then they'll need to loop over getNestedExceptions() if they want to look for a specific exception. I think that's ok, just stating that.

Yes, exactly. I was thinking whether it would make sense or not to throw the original exception if there is only one but it would mean you need to catch your own exception plus HandlerFailedException. So better always throwing it.

src/Symfony/Component/Messenger/Tests/Fixtures/MessageHandlerFailingFirstTimes.php

Discussed on Slack 👍

Add check to ensure that a message is only handled once per handler Add try...catch to run all handlers before throwing exception

fabpot · 2019-04-06T12:59:20Z

Thank you @keulinho.

…ndler (keulinho, sroze) This PR was merged into the 4.3-dev branch. Discussion ---------- [Messenger] Ensure message is handled only once per handler Add check to ensure that a message is only handled once per handler Add try...catch to run all handlers before throwing exception | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? |no | Tests pass? | yes | Fixed tickets | #27215 | License | MIT | Doc PR | Todo This would make error handling and retrying of messages much more easier. As statet here #27008 (comment) there is currently no way to retry a for all failed handlers if there are mutliple handlers and just some throw an exception. Also if an Exception in an handler occurs the execution chain is disrupted and the other handlers are never invoked. With this change it is easily possible to create an userland middleware that catches the `ChainedHandlerFailedException` and does some custom retry logic. If you ensure that the `HandledStamps` on the `Envelope` are preserved the message will be handled just by the failed handlers Commits ------- 2e5e910 Rename exception, add change log and a few other things e6e4cde Ensure message is handled only once per handler

carsonbot added Status: Needs Review Messenger Feature labels Jan 29, 2019

keulinho force-pushed the handle-messages-once-per-handler branch from 5a1bcec to e77c757 Compare January 29, 2019 13:46

OskarStark reviewed Jan 29, 2019

View reviewed changes

src/Symfony/Component/Messenger/Stamp/HandledStamp.php Outdated Show resolved Hide resolved

OskarStark reviewed Jan 29, 2019

View reviewed changes

src/Symfony/Component/Messenger/Tests/Middleware/HandleMessageMiddlewareTest.php Outdated Show resolved Hide resolved

keulinho force-pushed the handle-messages-once-per-handler branch from e77c757 to 1826b88 Compare January 30, 2019 06:45

nicolas-grekas added this to the next milestone Jan 30, 2019

weaverryan mentioned this pull request Mar 12, 2019

[Messenger] Making it Shine #30540

Closed

36 tasks

weaverryan mentioned this pull request Mar 22, 2019

[RFC][Messenger] General message retry mechanism #27215

Closed

keulinho force-pushed the handle-messages-once-per-handler branch from 1826b88 to f18bd98 Compare March 26, 2019 09:19

keulinho requested a review from sroze as a code owner March 26, 2019 09:19

keulinho force-pushed the handle-messages-once-per-handler branch 2 times, most recently from 92065aa to dca8b9e Compare March 26, 2019 09:22

sroze suggested changes Mar 27, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 27, 2019

keulinho force-pushed the handle-messages-once-per-handler branch 3 times, most recently from 117ec51 to 7c6fb32 Compare March 29, 2019 08:25

keulinho force-pushed the handle-messages-once-per-handler branch 3 times, most recently from 713158c to 1cdf048 Compare April 2, 2019 13:59

sroze suggested changes Apr 6, 2019

View reviewed changes

sroze added the ⭐️ EUFOSSA Hackathon label Apr 6, 2019

sroze force-pushed the handle-messages-once-per-handler branch from 1cdf048 to a0c04e4 Compare April 6, 2019 10:40

carsonbot added Status: Reviewed and removed Status: Needs Work labels Apr 6, 2019

sroze approved these changes Apr 6, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Apr 6, 2019

weaverryan previously requested changes Apr 6, 2019

View reviewed changes

sroze force-pushed the handle-messages-once-per-handler branch 3 times, most recently from 4fc0e80 to 2bd4d29 Compare April 6, 2019 12:33

keulinho and others added 2 commits April 6, 2019 14:40

Ensure message is handled only once per handler

e6e4cde

Add check to ensure that a message is only handled once per handler Add try...catch to run all handlers before throwing exception

Rename exception, add change log and a few other things

2e5e910

sroze force-pushed the handle-messages-once-per-handler branch from 2bd4d29 to 2e5e910 Compare April 6, 2019 12:41

fabpot approved these changes Apr 6, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Apr 6, 2019

fabpot merged commit 2e5e910 into symfony:master Apr 6, 2019

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

fabpot mentioned this pull request May 9, 2019

Release v4.3.0-BETA1 #31435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Ensure message is handled only once per handler #30020

[Messenger] Ensure message is handled only once per handler #30020

keulinho commented Jan 29, 2019 •

edited

sroze commented Mar 23, 2019

fabpot commented Mar 23, 2019

weaverryan commented Mar 24, 2019

weaverryan commented Mar 31, 2019

sroze commented Apr 6, 2019

weaverryan Apr 6, 2019

sroze Apr 6, 2019

weaverryan Apr 6, 2019

sroze Apr 6, 2019

fabpot commented Apr 6, 2019

[Messenger] Ensure message is handled only once per handler #30020

[Messenger] Ensure message is handled only once per handler #30020

Conversation

keulinho commented Jan 29, 2019 • edited

sroze commented Mar 23, 2019

fabpot commented Mar 23, 2019

weaverryan commented Mar 24, 2019

weaverryan commented Mar 31, 2019

sroze commented Apr 6, 2019

weaverryan Apr 6, 2019

Choose a reason for hiding this comment

sroze Apr 6, 2019

Choose a reason for hiding this comment

weaverryan Apr 6, 2019

Choose a reason for hiding this comment

sroze Apr 6, 2019

Choose a reason for hiding this comment

fabpot commented Apr 6, 2019

keulinho commented Jan 29, 2019 •

edited