[Messenger] fix retry of messages losing the routing key and properties #34134

Tobion · 2019-10-26T18:11:35Z

Q	A
Branch?	4.3
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #32994
License	MIT
Doc PR

Messages sent for retry in rabbitmq lost the routing key and properties like the priority. Now we read those original properties and sent the retry message with the same properties (unless those properties have already been set manually before).

weaverryan · 2019-10-31T14:24:25Z

src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php

@@ -225,7 +225,7 @@ private function publishWithDelay(string $body, array $headers, int $delay, Amqp
    private function publishOnExchange(\AMQPExchange $exchange, string $body, string $routingKey = null, array $headers = [], AmqpStamp $amqpStamp = null)
    {
        $attributes = $amqpStamp ? $amqpStamp->getAttributes() : [];
-        $attributes['headers'] = array_merge($headers, $attributes['headers'] ?? []);
+        $attributes['headers'] = array_merge($attributes['headers'] ?? [], $headers);


This looks subjective either way (or am I missing something)? The $headers comes ultimately from the serializer. So, should headers from the AmqpStamp win or headers from the serialization of the message. I dunno :). But I was curious what motivated the change.

This just gives headers from the serialization more prio than headers from the stamp. As the headers from the stamp are copied for retry, this just makes more sense now (in case re-serialization returns different headers).

This was huge BC break, it broke our app and make overriding type header impossible.

weaverryan · 2019-10-31T14:27:19Z

src/Symfony/Component/Messenger/Transport/AmqpExt/AmqpSender.php

-                $attributes['content_type'] = $contentType;
-
-                $amqpStamp = new AmqpStamp($amqpStamp ? $amqpStamp->getRoutingKey() : null, $amqpStamp ? $amqpStamp->getFlags() : AMQP_NOPARAM, $attributes);
+            if (!$amqpStamp || !isset($amqpStamp->getAttributes()['content_type'])) {


This feels like the opposite of the merging logic in Connection::publishOnExchnage(). In that method, headers from the serializer win over headers from the AmqpStamp. But here, we're saying: "only use the Content-Type from the serializer if it is NOT already set on AmqpStamp (i.e. AmqpStamp wins)

If people set the content-type manually on the envelope, we don't want to overwrite this. THis is how it has been before already.

weaverryan · 2019-10-31T14:42:06Z

src/Symfony/Component/Messenger/Transport/AmqpExt/AmqpSender.php

            }
        }

+        $amqpReceivedStamp = $envelope->last(AmqpReceivedStamp::class);
+        if ($amqpReceivedStamp instanceof AmqpReceivedStamp) {
+            $amqpStamp = AmqpStamp::createFromAmqpEnvelope($amqpReceivedStamp->getAmqpEnvelope(), $amqpStamp);


Will this work when it's being sent to the failure transport... and the failure transport is a direct AMQP exchange that relies on the default_publish_routing_key config to route to some "failure" queue?

I think flow would be:

A) Message is received for last retry
B) Message fails again and so is sent to the failure transport.
C) This code will see the routingKey from the original message (e.g. foo) and set it on the AmqpStamp
D) When this code is forwarded into Connection::publish() below, the routing key (foo) in AmqpStamp will be used instead of the default_publish_routing_key from the "failure transport".
E) Message will be sent once again with the foo routing key, instead of the one from the failure transport.

If I'm correct, on a high level, it seems like we want to do this trick unless we detect that the message is being sent through a different transport than it was originally sent through.... or maybe the "failure transport" is added as an edge-case.

Yes the flow would not work. But again, it would lose the original routing key in the failure transport which this PR tries to solve. So retrying from the failure transport might not work if the routing key is different suddenly.

What you described is also what is described in #32994 but the problem is that the failed transport should not need to be direct. I'll answer there.

Documenting that a failure transport should not be direct is a valid solution I think. But:

So retrying from the failure transport might not work if the routing key is different suddenly.

I don't think I understand what you're saying here. Assuming we successfully deliver a message to the routing transport (by setting the routing key correctly for the situation), what problem would the routing key being different cause? At that point, the routing key isn't used anymore: the messenger:failed:retry command consumes from the failure transport and dispatches into the bus. If it failed again, then re-using the "last routing key" (what this PR does) would correctly send it back to the failure transport, right?

People could use the routing key using the AmqpReceivedStamp in their handler to make decisions. But again all these things are edge cases I would not worry about unless you have a certain change request in mind?

Well, the options are:

document not to use a direct transport as your failure transport.

somehow add code here for the failure transport situation: like look for the RedeliveredToFailureTransportStamp... or something cleaner (not sure what). If there’s no reasonable way to do this, then option 1 becomes the only option.

Honestly there is nothing to document or fix here. Your case is just one of endless configuration possibilities. I can also configure the failure transport with the appropriate binding keys and voila it still works with the original routing keys. We shouldn't enforce or document a random edge case here that does not apply always.

What this PR is about is publishing retry/failure with the same routing key and priority etc as the original message. And this is absolutely the right and logical thing to do because retry message should be the same as the original ones.

The default is a fanout exchange. So we're covering 99% of all cases. If someone changes it to a direct exchange for failure, it's not the messenger job to interfere with this and make certain assumptions.

fabpot · 2019-11-04T06:59:31Z

Thank you @Tobion.

…nd properties (Tobion) This PR was merged into the 4.3 branch. Discussion ---------- [Messenger] fix retry of messages losing the routing key and properties | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no  | Deprecations? | no  | Tickets | Fix #32994  | License | MIT | Doc PR | Messages sent for retry in rabbitmq lost the routing key and properties like the priority. Now we read those original properties and sent the retry message with the same properties (unless those properties have already been set manually before). Commits ------- 75c674d [Messenger] fix retry of messages losing the routing key and properties

…n retrying a message (fancyweb) This PR was merged into the 4.4 branch. Discussion ---------- [Messenger] Retain correlation id from \AmqpEnvelope when retrying a message | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Ref #34134 The correlation id is lost when the message is retried. Commits ------- 28d23af [Messenger] Retain correlation id from \AmqpEnvelope when retrying a message

…n retrying a message (fancyweb) This PR was merged into the 4.4 branch. Discussion ---------- [Messenger] Retain correlation id from \AmqpEnvelope when retrying a message | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Ref symfony/symfony#34134 The correlation id is lost when the message is retried. Commits ------- 28d23af5d0 [Messenger] Retain correlation id from \AmqpEnvelope when retrying a message

carsonbot added Status: Needs Review Messenger Bug labels Oct 26, 2019

Tobion force-pushed the fix-amqp-retry-with-routing-key branch from 3fdb0a6 to db541da Compare October 26, 2019 18:17

Tobion mentioned this pull request Oct 26, 2019

[Messenger] Messages with AmqpStamp($routingKey) and direct RabbitMQ exchange are not retry-able #32994

Closed

[Messenger] fix retry of messages losing the routing key and properties

75c674d

Tobion force-pushed the fix-amqp-retry-with-routing-key branch from db541da to 75c674d Compare October 26, 2019 18:29

nicolas-grekas added this to the 4.3 milestone Oct 26, 2019

weaverryan reviewed Oct 31, 2019

View reviewed changes

weaverryan approved these changes Nov 4, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 4, 2019

fabpot approved these changes Nov 4, 2019

View reviewed changes

fabpot merged commit 75c674d into symfony:4.3 Nov 4, 2019

Tobion deleted the fix-amqp-retry-with-routing-key branch November 4, 2019 14:42

fabpot mentioned this pull request Nov 11, 2019

Release v4.3.7 #34323

Merged

mleczakm mentioned this pull request Jan 22, 2020

[Messenger] BC break when merging headers for amqp messages #35439

Closed

fancyweb mentioned this pull request Feb 4, 2022

[Messenger] Retain correlation id from \AmqpEnvelope when retrying a message #45317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

[Messenger] fix retry of messages losing the routing key and properties #34134

[Messenger] fix retry of messages losing the routing key and properties #34134

Tobion commented Oct 26, 2019 •

edited

Loading

weaverryan Oct 31, 2019

Tobion Oct 31, 2019

mleczakm Jan 22, 2020

weaverryan Oct 31, 2019

Tobion Oct 31, 2019

weaverryan Oct 31, 2019

Tobion Oct 31, 2019

Tobion Oct 31, 2019

weaverryan Oct 31, 2019

Tobion Oct 31, 2019

weaverryan Oct 31, 2019

Tobion Nov 1, 2019

Tobion Nov 1, 2019

Tobion Nov 1, 2019

fabpot commented Nov 4, 2019

[Messenger] fix retry of messages losing the routing key and properties #34134

[Messenger] fix retry of messages losing the routing key and properties #34134

Conversation

Tobion commented Oct 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Nov 4, 2019

Tobion commented Oct 26, 2019 •

edited

Loading