8000 [Messenger] DispatchAfterCurrentBusMiddleware does not cancel messages from delayed handlers · Issue #32370 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] DispatchAfterCurrentBusMiddleware does not cancel messages from delayed handlers #32370

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.

Sign up for GitHub

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

Jump to bottom
Closed
BastienClement opened this issue Jul 4, 2019 · 3 comments
Closed

[Messenger] DispatchAfterCurrentBusMiddleware does not cancel messages from delayed handlers #32370

BastienClement opened this issue Jul 4, 2019 · 3 comments
Labels
Bug Messenger Status: Needs Review

Comments

@BastienClement
Copy link
Contributor
BastienClement commented Jul 4, 2019 •
edited
Loading

Symfony version(s) affected: 4.3.1

Description
When an exception is thrown during the handling of the first message, the queue is flushed and delayed messages which might already have been dispatched are "canceled".

On the other hand, if a delayed message handler throws an exception, it is simply collected without any queue cleanup being done.

Therefore, the DispatchAfterCurrentBusStamp semantic is not the same whether we are in the first message handler or a delayed one. In the first case an exception "cancels" previously sent messages, in the second case it does not. I believe this is not intended since the messenger documentation clearly states (as an unintended consequences):

If the message is dispatched to a different bus, then the dispatched message will be handled even if some code later in the current handler throws an exception.

How to reproduce

  • Message A dispatches (after current bus) Message B and returns,
  • Message B dispatches (after current bus) Message C but then throws an exception,
  • Message C is handled despite its dispatcher having thrown an exception

Possible Solution
Either:

  • Implement two queues instead of one: delayed messages are stored in the second queue until the handler successfully completes, at that point the second queue is simply appended to the first one (the one being consumed by the while loop). That way we know which messages to throw away if an exception is thrown.
  • Add some bookkeeping to save the length of the queue before executing a handler and discard any messages that have been added to it (by comparing length afterward) if the handler throws an exception. This solution is probably simpler since it does not affect the handling of the first message.

Additional context
n/a
@weaverryan
Copy link
Member

Ping @Nyholm - need your expert eyes on this - it's a tricky feature :)

@Nyholm
Copy link
Member
Nyholm commented Jul 9, 2019 •
edited
Loading

Thank you for the ping.

This is an excellent issue description. And yes, it is a tricky bug.

Hm. You are correct. You can do an infinite chain of “dispatch message after current buss”.
We should probably do something similar to your second suggestion. Could we be smart about how we store the delayed messages inside the middleware? Maybe we should index that array depending on what “level” (Nummer of “dispatch after current”) in the recursion we are.

Bastien: do you want to give a try to implement a solution?

Nyholm added a commit to Nyholm/symfony that referenced this issue Jul 9, 2019
@Nyholm
Added tests for symfony#32370
35c76a3
@Nyholm
Copy link
Member
Nyholm commented Jul 9, 2019

I added some tests bug (I think). See Nyholm@35c76a3

@BastienClement BastienClement mentioned this issue Jul 11, 2019
Merged
@fabpot fabpot closed this as completed Aug 18, 2019
fabpot added a commit that referenced this issue Aug 18, 2019
@fabpot
bug #32497 [Messenger] DispatchAfterCurrentBusMiddleware does not can…
d1d0943 
…cel messages from delayed handlers (Nyholm, BastienClement)

This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger] DispatchAfterCurrentBusMiddleware does not cancel messages from delayed handlers

| Q             | A
| ------------- | ---
| Branch?       | 4.3 for bug fixes
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? |no
| Tests pass?   | yes  (thanks @Nyholm)
| Fixed tickets | #32370
| License       | MIT
| Doc PR        | -

This is a fix for #32370. There is no need for anything sophisticated. There is no recursion or fancy stuff going on, just a queue of message handled sequentially. A simple variable is enough to keep track of the queue state.

Thanks @Nyholm for the test, it would clearly have been the hardest part of the job.

Commits
-------

1f5c8a6 Cancel delayed message if handler fails
35c76a3 Added tests for #32370
nicolas-grekas added a commit that referenced this issue Aug 19, 2019
@nicolas-grekas
Merge branch '4.3' into 4.4
3cd20c9 
* 4.3:
  [Routing] Add a param annotation for $annot.
  [DI] fix docblock
  [Console] fix docblock
  Add missing translations for Armenian locale
  [Process] Added missing return type.
  [Process] Doc block backport.
  Added doc block for Registry::supports().
  [Cache] Fix predis test
  Don't duplicate addresses in Sendgrid Transport
  Remove unnecessary statement
  Fix some docblocks.
  [Messenger] make delay exchange and queues durable like the normal ones by default
  Cancel delayed message if handler fails
  Added tests for #32370
nicolas-grekas added a commit that referenced this issue Aug 19, 2019
@nicolas-grekas
Merge branch '4.4'
b8c646f 
* 4.4:
  [Routing] Add a param annotation for $annot.
  [DI] fix docblock
  [Console] fix docblock
  Add missing translations for Armenian locale
  [Process] Added missing return type.
  [Process] Doc block backport.
  Added doc block for Registry::supports().
  [Cache] Fix predis test
  Don't duplicate addresses in Sendgrid Transport
  Remove unnecessary statement
  Fix some docblocks.
  [Messenger] make delay exchange and queues durable like the normal ones by default
  Cancel delayed message if handler fails
  Added tests for #32370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Messenger Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @javiereguiluz @weaverryan @BastienClement @Nyholm @carsonbot
0