Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] catch all exceptions while deserializing a message#46996
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Messenger] catch all exceptions while deserializing a message#46996
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lyrixx commentedJul 20, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Did you see#39622 ? But indeed we can merge this one too |
nikophil commentedJul 20, 2022
Hi@lyrixx I wasn't aware of this other PR - but I think they don't conflict with each other |
dunglas commentedJul 20, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Shouldn't exceptions thrown by your custom normalizers implement |
nikophil commentedJul 20, 2022
yes@dunglas but if any error that the developer didn't think about occurs in this normalizer, it will result in the message indefinitely requeued, and the worker will be in error until the message has not been manually removed from the queue. |
chalasr commentedJul 20, 2022
I think that ensuring that the proper exception is thrown is the responsibility of the normalizer implementation and I fear that doing this in core would lead to confusing situations more than it would help. |
nikophil commentedJul 20, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
typically some broken json/xml where some fields are missing that are passed to rabbit, that will cause a |
lyrixx commentedJul 20, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Something "classic":
You have to do some weird things to deal with that! (I really need to finish my PR BTW) |
dunglas commentedJul 20, 2022
Can't the failing queue be used for that?https://symfonycasts.com/screencast/messenger/show-retry-failures |
lyrixx commentedJul 20, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
No because the exception breaks everything and is out of the scope. But yes! it should be. I asked to my employer some time to work on this topic ASAP. |
B-Galati commentedOct 22, 2022
You fixed it@lyrixx right? Only in 6.2 though? |
lyrixx commentedNov 9, 2022
I'm not sure I covered this case. @nikophil Can you revise this PR and check if you still need it? |
Uh oh!
There was an error while loading.Please reload this page.
Hello,
In Messenger, while deserializing a message, if an error is encountered in a custom denormalizer, the error is not caught, and then
MessageDecodingFailedExceptionis not thrown. Then, if RabbitMQ is used, it results in a cyclic error where the message is always requeued without any way to kick it out from the queue.My guess would be to catch any error that would happen while deserializing, even if the general policy is to never catch
\Throwable. There is high amount of risk that if a given message have created an error once, it will create an error on every attempt to deserialize it.Or maybe we can at least catch
\Error|Symfony\Component\Serializer\Exception\ExceptionInterfacein order to cover all cases related to type error an so on...