-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Comments
From what I can see, this happens because the message is sent to the failure bus with 0 retries listed in the We could, in 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. |
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
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
Symfony version(s) affected: 4.3.*
How to reproduce
messenger:failed:retry
to retry the message. But, make sure it fails again.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:

Message on failure transport after retrying:

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.
The text was updated successfully, but these errors were encountered: