Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Merged

Conversation

@keulinho
Copy link
Contributor

@keulinhokeulinho commentedJan 29, 2019•
edited
Loading

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

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#27215
LicenseMIT
Doc PRTodo

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 theChainedHandlerFailedException and does some custom retry logic. If you ensure that theHandledStamps on theEnvelope are preserved the message will be handled just by the failed handlers

@keulinhokeulinhoforce-pushed thehandle-messages-once-per-handler branch frome77c757 to1826b88CompareJanuary 30, 2019 06:45
@nicolas-grekasnicolas-grekas added this to thenext milestoneJan 30, 2019
@weaverryanweaverryan mentioned this pull requestMar 12, 2019
36 tasks
@sroze
Copy link
Contributor

We need to wait for#30557 to be merged to go forward with this one because we need to make sure theHandledStamps 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
Copy link
Member

#30557 is merged now :)

@weaverryan
Copy link
Member

@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 involvingWorker with a fake "receiver" and a bus with all the default middleware. That would allow us to test this complex behavior: e.g. useWorker 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 involvesWorker 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.

@keulinhokeulinhoforce-pushed thehandle-messages-once-per-handler branch from1826b88 tof18bd98CompareMarch 26, 2019 09:19
@keulinhokeulinho requested a review fromsroze as acode ownerMarch 26, 2019 09:19
@keulinhokeulinhoforce-pushed thehandle-messages-once-per-handler branch 2 times, most recently from92065aa todca8b9eCompareMarch 26, 2019 09:22
@keulinhokeulinhoforce-pushed thehandle-messages-once-per-handler branch 3 times, most recently from117ec51 to7c6fb32CompareMarch 29, 2019 08:25
@weaverryan
Copy link
Member

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

sroze reacted with thumbs up emoji

@keulinhokeulinhoforce-pushed thehandle-messages-once-per-handler branch 3 times, most recently from713158c to1cdf048CompareApril 2, 2019 13:59
@srozesrozeforce-pushed thehandle-messages-once-per-handler branch from1cdf048 toa0c04e4CompareApril 6, 2019 10:40
@sroze
Copy link
Contributor

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

weaverryan
weaverryan previously requested changesApr 6, 2019
$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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 hasnot already been handled always.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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


if (\count($exceptions)) {
thrownewHandlerFailedException($envelope, ...$exceptions);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 plusHandlerFailedException. So better always throwing it.

weaverryan reacted with thumbs up emoji
@srozesrozeforce-pushed thehandle-messages-once-per-handler branch 3 times, most recently from4fc0e80 to2bd4d29CompareApril 6, 2019 12:33
@srozesroze dismissedweaverryan’sstale reviewApril 6, 2019 12:40

Discussed on Slack 👍

keulinhoand others added2 commitsApril 6, 2019 14:40
Add check to ensure that a message is only handled once per handlerAdd try...catch to run all handlers before throwing exception
@srozesrozeforce-pushed thehandle-messages-once-per-handler branch from2bd4d29 to2e5e910CompareApril 6, 2019 12:41
@fabpot
Copy link
Member

Thank you@keulinho.

@fabpotfabpot merged commit2e5e910 intosymfony:masterApr 6, 2019
fabpot added a commit that referenced this pull requestApr 6, 2019
…ndler (keulinho, sroze)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] Ensure message is handled only once per handlerAdd check to ensure that a message is only handled once per handlerAdd 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        | TodoThis 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 handlersCommits-------2e5e910 Rename exception, add change log and a few other thingse6e4cde Ensure message is handled only once per handler
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@weaverryanweaverryanweaverryan left review comments

@OskarStarkOskarStarkOskarStark left review comments

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@srozesrozesroze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

7 participants

@keulinho@sroze@fabpot@weaverryan@OskarStark@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp