[Messenger] prevent infinite redelivery loops and blocked queues #34107
Conversation
| $this->bus->dispatch($retryEnvelope); | ||
| // acknowledge the previous message has received | ||
| $receiver->ack($envelope); |
There was a problem hiding this comment.
I changed this to reject and moved it outside the if else. Reject is more appropriate. Technically ack and reject are the same. There is only a semantical difference. Here we are handling a failure case and whether we retry or not does not change this.
2269ead to
9ec36b4
Compare
9ec36b4 to
a00b354
Compare
by republishing the redelivered messages as retries with a retry limit and potential delay
a00b354 to
d211904
Compare
| $this->bus->dispatch($retryEnvelope); | ||
| // acknowledge the previous message has received | ||
| $receiver->ack($envelope); |
| @@ -135,6 +136,13 @@ private function handleMessage(Envelope $envelope, ReceiverInterface $receiver, | |||
| try { | |||
| $envelope = $this->bus->dispatch($envelope->with(new ReceivedStamp($transportName))); | |||
| } catch (\Throwable $throwable) { | |||
| $rejectFirst = $throwable instanceof RejectRedeliveredMessageException; | |||
There was a problem hiding this comment.
It seems to me that the BIG important thing here is: if a message is redelivered, don't actually try to handle it again (because it'll probably timeout again). Instead, skip handling and have it be redelivered.
Question then: to fix this bug, is it actually important to reject this message before sending the redelivery? If we manage to skip the AMQP-redelivered message from being handled, isn't it ok if we follow the normal messenger-redelivery logic (redeliver the message and then reject it)? Or am I missing something?
If rejecting it first is not actually important, we could simplify this patch considerably.
thx :)
There was a problem hiding this comment.
Rejecting redelivered messages first is important. As I described in #32055 (comment) and #34082, the repeating errors can happen inside the listeners or the retry publishing (anywhere inside the catch block). In this case, the message never gets rejected (as the exception happens before) and would be redelivered forever.
There was a problem hiding this comment.
See also comment two lines belo 10000 w
There was a problem hiding this comment.
Hmm, ok. To make it feel like less of a one-off solution, wdyt about a RejectMessageImmediatelyExceptionInterface that we actually look for (then the class implements this)?
There was a problem hiding this comment.
We only want to reject redelivered messages first and not others to prevent potential loss of messages. So the interface would suggest something we don't want to promote. Also the RejectRedeliveredMessageException is not specific to amqp (hence the generic name) and we could use it also for doctrine and other transports. But for doctrine it's not that important as it has a timer in
| } | ||
|
|
||
| return $stack->next()->handle($envelope, $stack); | ||
| } |
There was a problem hiding this comment.
I wish there were a way to only run this if at least one Amqp transport were configured, but I know that’s not possible (so not a blocker or real feedback). It makes me wish each transport was actually its own package...
There was a problem hiding this comment.
As I said in #34107 (comment), we might want to enhance the middleware later to handle other transports like doctrine.
…queues (Tobion) This PR was merged into the 4.3 branch. Discussion ---------- [Messenger] prevent infinite redelivery loops and blocked queues | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #32055 | License | MIT | Doc PR | This PR solves a very common fitfall of amqp redeliveries. It's for example explained in https://blog.forma-pro.com/rabbitmq-redelivery-pitfalls-440e0347f4e0 Newer RabbitMQ versions provide a solution for this by itself but only for quorum queues and not the classic ones, see rabbitmq/rabbitmq-server#1889 This PR adds a middleware that throws a RejectRedeliveredMessageException when a message is detected that has been redelivered by AMQP. The middleware runs before the HandleMessageMiddleware and prevents redelivered messages from being handled directly. The thrown exception is caught by the worker and will trigger the retry logic according to the retry strategy. AMQP redelivers messages when they do not get acknowledged or rejected. This can happen when the connection times out or an exception is thrown before acknowledging or rejecting. When such errors happen again while handling the redelivered message, the message would get redelivered again and again. The purpose of this middleware is to prevent infinite redelivery loops and to unblock the queue by republishing the redelivered messages as retries with a retry limit and potential delay. Commits ------- d211904 [Messenger] prevent infinite redelivery loops and blocked queues
|
Would it be possible to make this behaviour opt-in (or opt-out)? This is causing problems when you are relying on message order. I don't know about other queues, and there are some gotchas, but RabbitMQ states that messages are queued and consumed in the FIFO manner. When you have a process that consumes messages and expects them to be in order (as we do), this could mess that up because a message which was in the front of the queue is now placed at the back of the queue. [edit] I get that for most use cases this PR is the right way to go, and it prevents a real world problem. Which is why opting out would be a nice solution for the people who want to solve this problem in a different way. |
|
You can opt-out by not using the But relying on message order has quite a few drawbacks. E.g. you also need to disable retry because it the message back to the end of the queue (optionally with delay). |
This is what we did today to work around this. However when the original definition changes, we have to copy the configuration to keep up-to-date. It's not really a forwards compatible situation.
We also did this: we exit the consumer once this happens, fix the message handling, and resume consuming. Which is why we are very happy with the events PR, which makes this much easier to do. |
|
I would really not recommend to rely on message ordering. There are many things that can make the order non predictable. Like having more than one consumer when you scale. But there are many others. So, I think we should not do anything that makes developers think they can rely on any order. |
This PR solves a very common fitfall of amqp redeliveries. It's for example explained in https://blog.forma-pro.com/rabbitmq-redelivery-pitfalls-440e0347f4e0
Newer RabbitMQ versions provide a solution for this by itself but only for quorum queues and not the classic ones, see rabbitmq/rabbitmq-server#1889
This PR adds a middleware that throws a RejectRedeliveredMessageException when a message is detected that has been redelivered by AMQP.
The middleware runs before the HandleMessageMiddleware and prevents redelivered messages from being handled directly. The thrown exception is caught by the worker and will trigger the retry logic according to the retry strategy.
AMQP redelivers messages when they do not get acknowledged or rejected. This can happen when the connection times out or an exception is thrown before acknowledging or rejecting. When such errors happen again while handling the redelivered message, the message would get redelivered again and again. The purpose of this middleware is to prevent infinite redelivery loops and to unblock the queue by republishing the redelivered messages as retries with a retry limit and potential delay.