Skip to content

[Messenger] Messages retried on failure transport lose exception metadata #32311

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

Closed
weaverryan opened this issue Jul 1, 2019 · 1 comment
Closed

Comments

@weaverryan
Copy link
Member

Symfony version(s) affected: 4.3.*

How to reproduce

  • Try to handle a messenger message that fails... and allow it to fail until it goes to the failure transport
  • Once on the failure transport, use messenger:failed:retry to retry the message. But, make sure it fails again.
  • Try to view the specific information about the message - e.g. messenger:failed:show <id>. It will lose details about its exception. I think they are probably still there... but not being read correctly.

Message on failure transport before retrying:
before

Message on failure transport after retrying:
after

Possible Solution
We need to figure out if the exception information is being lost on redelivery (and if that's the case, make it not lost) or if it is present, but not being read correctly.

@TimoBakx
Copy link
Member

TimoBakx commented Jul 3, 2019

From what I can see, this happens because the message is sent to the failure bus with 0 retries listed in the SendFailedMessageToFailureTransportListener. Then, when running bin/console messenger:failed:retry [id], the message is only retried once. Because the retry count is reset, the SendFailedMessageToFailureTransportListener is not triggered again, so only the RedeliveryStamp of the first try is added (in Worker, line 152) without any exceptions.

We could, in Worker, check previous RedeliveryStamps to check if there are any exceptions in those, and then add them in the next one.

I've done this bit in #32341. Please let me know if I'm on the right track with this and I'll try to create unit tests for it.

@Tobion Tobion closed this as completed Sep 17, 2019
Tobion added a commit that referenced this issue Oct 23, 2019
This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger] Show exceptions after multiple retries

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32311
| License       | MIT
| Doc PR        | n/a

After retrying a failed message, the `RedeliveryStamp` looses it's exception information. This PR will remedy that.

Commits
-------

598bd92 [Messenger] Show exceptions on after empty fails
fabpot added a commit that referenced this issue Oct 2, 2020
This PR was merged into the 5.2-dev branch.

Discussion
----------

[Messenger] Added ErrorDetailsStamp

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32311
| License       | MIT
| Doc PR        | No doc changes are needed

#SymfonyHackday

This PR is part of the work started in #32341. That PR has a workaround for showing exceptions added to a previous retry. This PR stores error messages in a separate stamp, so they're more easily accessed.

I also added the exceptionClass as a separate string (independant of FlattenException), so that information is always available, even if the trace is not (due to FlattenException not being available).

Duplicated exceptions (compared to the last one) are not stored separately.

**Questions:**
- Should we limit the total amount of exceptions (remove the oldest when adding a new one)?
  - Yes, but in a new PR
- The current implementation adds this stamp in the Worker instead of the listeners to prevent duplicate code (due to the immutability of the envelope in the event). Is this the proper way to do this?
  - No, create a new listener and a way to add stamps to the envelope inside the event.
- When adding details of a `HandlerFailedException`, should we add a stamp per wrapped `Throwable`? There can be multiple errors wrapped by a single `HandlerFailedException`.
  - Yes, but in a later PR

**Todo:**
- [x] only add new information if it differs from the previous exception
- [x] add deprecations
- [x] fall back to old stamp data if no new stamp is available
- [x] rebase and retarget to master
- [x] update CHANGELOG.md
- [x] check for docs PR

**BC Breaks:**
When this PR is merged, RedeliveryStamps will no longer be populated with exception data. Any implementations that use `RedeliveryStamp::getExceptionMessage()` or `RedeliveryStamp::getFlattenedException()` will receive an empty string or `null` respectively for stamps added after this update. They should rely on `ErrorDetailsStamp` instead.

**New stuff:**
- New stamp `ErrorDetailsStamp`.
- New event subscriber `AddErrorDetailsStampListener`.
- New method `AbstractWorkerMessageEvent::addStamps`.

Commits
-------

cd27b86 [Messenger] Added FailedMessageErrorDetailsStamp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants