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] fix retry of messages losing the routing key and properties #34134

Merged
merged 1 commit into from Nov 4, 2019

Conversation

@Tobion
Copy link
Member

Tobion commented Oct 26, 2019

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

@Tobion Tobion force-pushed the Tobion:fix-amqp-retry-with-routing-key branch from db541da to 75c674d Oct 26, 2019
@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Oct 26, 2019
@@ -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 comment has been minimized.

Copy link
@weaverryan

weaverryan Oct 31, 2019

Member

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 comment has been minimized.

Copy link
@Tobion

Tobion Oct 31, 2019

Author Member

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 comment has been minimized.

Copy link
@mleczakm

mleczakm Jan 22, 2020

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

$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 comment has been minimized.

Copy link
@weaverryan

weaverryan Oct 31, 2019

Member

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)

This comment has been minimized.

Copy link
@Tobion

Tobion Oct 31, 2019

Author Member

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.

}
}

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Oct 31, 2019

Member

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.

This comment has been minimized.

Copy link
@Tobion

Tobion Oct 31, 2019

Author Member

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.

This comment has been minimized.

Copy link
@Tobion

Tobion Oct 31, 2019

Author Member

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.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Oct 31, 2019

Member

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?

This comment has been minimized.

Copy link
@Tobion

Tobion Oct 31, 2019

Author Member

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?

This comment has been minimized.

Copy link
@weaverryan

weaverryan Oct 31, 2019

Member

Well, the options are:

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

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

This comment has been minimized.

Copy link
@Tobion

Tobion Nov 1, 2019

Author Member

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.

This comment has been minimized.

Copy link
@Tobion

Tobion Nov 1, 2019

Author Member

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.

This comment has been minimized.

Copy link
@Tobion

Tobion Nov 1, 2019

Author Member

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
fabpot approved these changes Nov 4, 2019
@fabpot
Copy link
Member

fabpot commented Nov 4, 2019

Thank you @Tobion.

fabpot added a commit that referenced this pull request Nov 4, 2019
…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 <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #32994 <!-- prefix each issue number with "Fix #", if any -->
| 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
@fabpot fabpot merged commit 75c674d into symfony:4.3 Nov 4, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@Tobion Tobion deleted the Tobion:fix-amqp-retry-with-routing-key branch Nov 4, 2019
@fabpot fabpot mentioned this pull request Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.