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.

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

Interpret x-death header from AMQP 0.9.1 client #11339

Merged
merged 1 commit into from
May 28, 2024
Merged

Interpret x-death header from AMQP 0.9.1 client #11339

merged 1 commit into from
May 28, 2024

Conversation

ansd
Copy link
Member

@ansd ansd commented May 28, 2024 •
edited
Loading

This PR supersedes #11043 and fixes #10709, #11331.

Interpret x-death header from AMQP 0.9.1 client

Fixes #10709
Fixes #11331

This commit fixes the following regression which worked in 3.12.x, but
stopped working in 3.13.0 - 3.13.2:

```
AMQP 0.9.1 client    --publish-->
Q                    --dead-letter-->
DLQ                  --consume-->
AMQP 0.9.1 client (death count is now 1) --republish-same-message-with-headers-as-just-received-->
Q                    --dead-letter-->
DLQ                  --consume -->
AMQP 0.9.1 (death count is now 1, but should be 2)
```

The reason this behaviour stopped to work in 3.13.0 is that the broker
won't specially interpret x-headers in general, and the x-death header
specifically in this case anymore.

In other words, the new desired 3.13 behaviour with message containers
is that "x-headers belong to the broker".

While this is correct, it does break client applications which depended
on the previous use case.
One simple fix is that the client application does not re-publish with
the x-death header, but instead sets its own custom count header to
determine the number of times it retries.

This commit will only be packported to v3.13.x branch.
In other words, 4.0 won't interpret x-headers as done in 3.13.0 - 3.13.2.

The reason we backport this commit to v3.13.x is that the Spring
documentation expliclity recommends re-publishing the message with
x-death header being set:
https://cloud.spring.io/spring-cloud-static/spring-cloud-stream-binder-rabbit/3.0.6.RELEASE/reference/html/spring-cloud-stream-binder-rabbit.html#_retry_with_the_rabbitmq_binder

@mergify mergify bot added the bazel label May 28, 2024
@ansd ansd added this to the 3.13.4 milestone May 28, 2024
@ansd ansd marked this pull request as ready for review May 28, 2024 14:01
@ansd ansd modified the milestones: 3.13.4, 3.13.3 May 28, 2024
@ansd
Interpret x-death header from AMQP 0.9.1 client
ea94a77 
Fixes #10709
Fixes #11331

This commit fixes the following regression which worked in 3.12.x, but
stopped working in 3.13.0 - 3.13.2:

```
AMQP 0.9.1 client    --publish-->
Q                    --dead-letter-->
DLQ                  --consume-->
AMQP 0.9.1 client (death count is now 1) --republish-same-message-with-headers-as-just-received-->
Q                    --dead-letter-->
DLQ                  --consume -->
AMQP 0.9.1 (death count is now 1, but should be 2)
```

The reason this behaviour stopped to work in 3.13.0 is that the broker
won't specially interpret x-headers in general, and the x-death header
specifically in this case anymore.

In other words, the new desired 3.13 behaviour with message containers
is that "x-headers belong to the broker".

While this is correct, it does break client applications which depended
on the previous use case.
One simple fix is that the client application does not re-publish with
the x-death header, but instead sets its own custom count header to
determine the number of times it retries.

This commit will only be packported to v3.13.x branch.
In other words, 4.0 won't interpret x-headers as done in 3.13.0 - 3.13.2.

The reason we backport this commit to v3.13.x is that the Spring
documentation expliclity recommends re-publishing the message with
x-death header being set:
https://cloud.spring.io/spring-cloud-static/spring-cloud-stream-binder-rabbit/3.0.6.RELEASE/reference/html/spring-cloud-stream-binder-rabbit.html#_retry_with_the_rabbitmq_binder
kjnilsson
kjnilsson approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@kjnilsson kjnilsson left a comment

Choose a reason for hiding this comment

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

Looks good, thank you.

@michaelklishin michaelklishin mentioned this pull request May 28, 2024
Closed
@michaelklishin michaelklishin merged commit 2bd6921 into v3.13.x May 28, 2024
18 checks passed
@michaelklishin michaelklishin deleted the v3.13.x-amqpl-x-death branch May 28, 2024 22:18
@ansd ansd mentioned this pull request May 29, 2024
Closed
ansd added a commit that referenced this pull request Jul 3, 2024
@ansd
Fix timestamp when recovering x-death header
18e3f1f 
This commit fixes a bug present in 3.13.3 which was introduced by
#11339

When an AMQP 0.9.1 client re-publishes a message with the x-death header
set, the `time` field was not converted from the AMQP 0.9.1 seconds
resolution to the internal millisecond resolution of `mc` fields
`first_time` and `last_time`.

Additionally, this commit will ignore invalid `time` fields sent in the
`x-death` header from AMQP 0.9.1 clients as reported in
https://rabbitmq.slack.com/archives/C1EDN83PA/p1719986557420719 which
results in the following crash later on when the message gets consumed:
```
 supervisor: {<0.476725.0>,rabbit_channel_sup}
    errorContext: child_terminated
    reason: {badarith,
                [{erlang,'div',
                     [<<"2024-07-02T02:58:50.000-07:00">>,1000],
                     [{error_info,#{module => erl_erts_errors}}]},
                 {mc_amqpl,death_table,7,[{file,"mc_amqpl.erl"},{line,806}]},
                 {lists,map,2,[{file,"lists.erl"},{line,1559}]},
                 {mc_amqpl,deaths_to_headers,2,
                     [{file,"mc_amqpl.erl"},{line,764}]},
                 {mc_amqpl,protocol_state,2,
                     [{file,"mc_amqpl.erl"},{line,406}]},
                 {rabbit_channel,handle_deliver0,4,
                     [{file,"rabbit_channel.erl"},{line,2685}]},
                 {lists,foldl,3,[{file,"lists.erl"},{line,1594}]},
                 {rabbit_channel,handle_cast,2,
                     [{file,"rabbit_channel.erl"},{line,732}]}]}
```
@ansd ansd mentioned this pull request Jul 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelklishin michaelklishin michaelklishin approved these changes

@kjnilsson kjnilsson kjnilsson approved these changes

Assignees
No one assigned
Labels
bazel
Projects
None yet
Milestone
3.13.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ansd @michaelklishin @kjnilsson