Skip to content

[Messenger] Fix exception message of failed message is dropped on retry #33600

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

Merged
merged 1 commit into from
Sep 17, 2019
Merged

[Messenger] Fix exception message of failed message is dropped on retry #33600

merged 1 commit into from
Sep 17, 2019

Conversation

tienvx
Copy link
Contributor

@tienvx tienvx commented Sep 16, 2019

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

@tienvx
Copy link
Contributor Author

tienvx commented Sep 16, 2019

NOTE: The test case will failed on Symfony 4.4, because component Debug is removed from dependencies of the component Messenger

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Sep 16, 2019
@fabpot
Copy link
Member

fabpot commented Sep 17, 2019

Thank you @tienvx.

fabpot added a commit that referenced this pull request Sep 17, 2019
…pped on retry (tienvx)

This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger] Fix exception message of failed message is dropped on retry

| Q             | A
| ------------- | ---
| Branch?       | 4.3 <!-- see below -->
| 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 #32719
| License       | MIT
| Doc PR        | NA <!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->

Commits
-------

8f9f44e [Messenger] Fix exception message of failed message is dropped on retry
@fabpot fabpot merged commit 8f9f44e into symfony:4.3 Sep 17, 2019
@tienvx tienvx deleted the fix-exception-message-dropped branch September 17, 2019 13:34
@Tobion
Copy link
Contributor

Tobion commented Sep 17, 2019

This fixes #32311 and invalidates #32341 where @weaverryan wanted to go with a different solution.

@TimoBakx
Copy link
Member

This most likely also invalidates #32904, unless the separation is still something we want to do in the future.

@tienvx
Copy link
Contributor Author

tienvx commented Sep 17, 2019

Thanks @Tobion and @TimoBakx for adding references

@fabpot
Copy link
Member

fabpot commented Sep 17, 2019

I wasn't aware of the other issues/PRs; tell me how you want to proceed @weaverryan

@Tobion
Copy link
Contributor

Tobion commented Oct 23, 2019

We need to revert this in #34082 as it's breaking the retry on amqp when the message gets too big.

Tobion added a commit that referenced this pull request Oct 23, 2019
…e is dropped (Tobion)

This PR was merged into the 4.3 branch.

Discussion
----------

Revert "[Messenger] Fix exception message of failed message is dropped

| 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       |
| License       | MIT
| Doc PR        |

This reverts #33600 because it makes the message grow for each retry until AMQP cannot handle it anymore. On each retry, the full exception trace is added to the message. So in our case on the 5th retry, the message is too big for the AMQP library to encode it. AMQP extension then throws the exception

> Library error: table too large for buffer

(ref. alanxz/rabbitmq-c#224 and php-amqp/php-amqp#131) when trying to publish the message.

To solve this, I suggest to revert #33600 (this PR) and merge #32341 instead which does not re-add the exception on each failure.

Btw, the above problem causes other problematic side-effects of Symfony messenger. As the new retry message fails to be published with an exception, the old (currently processed message) also does not get removed (acknowledged) from the delay queue. So rabbitmq redelivers the message and the same thing happens forever. This can block the consumers and have a huge toll on your service. That's just another case for #32055 (comment). I'll try to fix this in another PR.

Commits
-------

3dbe924 Revert "[Messenger] Fix exception message of failed message is dropped on retry"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants