Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Apr 6, 2019

Conversation

@keulinho
Copy link
Contributor

commented Jan 29, 2019

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

@keulinho keulinho force-pushed the keulinho:handle-messages-once-per-handler branch from 5a1bcec to e77c757 Jan 29, 2019

@keulinho keulinho force-pushed the keulinho:handle-messages-once-per-handler branch from e77c757 to 1826b88 Jan 30, 2019

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

@weaverryan weaverryan referenced this pull request Mar 12, 2019

Closed

[Messenger] Making it Shine #30540

30 of 36 tasks complete
@sroze

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

#30557 is merged now :)

@weaverryan

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

@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.

@keulinho keulinho force-pushed the keulinho:handle-messages-once-per-handler branch from 1826b88 to f18bd98 Mar 26, 2019

@keulinho keulinho requested a review from sroze as a code owner Mar 26, 2019

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

@keulinho keulinho force-pushed the keulinho:handle-messages-once-per-handler branch 3 times, most recently from 117ec51 to 7c6fb32 Mar 28, 2019

@weaverryan

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

@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!

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

@sroze sroze force-pushed the keulinho:handle-messages-once-per-handler branch from 1cdf048 to a0c04e4 Apr 6, 2019

@sroze

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

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

@sroze

sroze approved these changes Apr 6, 2019

Show resolved Hide resolved src/Symfony/Component/Messenger/Exception/HandlerFailedException.php Outdated
Show resolved Hide resolved src/Symfony/Component/Messenger/Exception/HandlerFailedException.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;

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 6, 2019

Member

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.

This comment has been minimized.

Copy link
@sroze

sroze Apr 6, 2019

Member

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).

@@ -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);
}

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 6, 2019

Member

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.

This comment has been minimized.

Copy link
@sroze

sroze Apr 6, 2019

Member

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.

Show resolved Hide resolved ...y/Component/Messenger/Tests/Fixtures/MessageHandlerFailingFirstTimes.php Outdated

@sroze sroze force-pushed the keulinho:handle-messages-once-per-handler branch 3 times, most recently from 4fc0e80 to 2bd4d29 Apr 6, 2019

Discussed on Slack 👍

keulinho and others added some commits Jan 29, 2019

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

@sroze sroze force-pushed the keulinho:handle-messages-once-per-handler branch from 2bd4d29 to 2e5e910 Apr 6, 2019

@fabpot

fabpot approved these changes Apr 6, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Thank you @keulinho.

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

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 6, 2019

feature #30020 [Messenger] Ensure message is handled only once per ha…
…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

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

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.