Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed
nikophil wants to merge1 commit intosymfony:4.4fromnikophil:messenger/catch-all-exceptions-in-deserialization
Closed

[Messenger] catch all exceptions while deserializing a message#46996

nikophil wants to merge1 commit intosymfony:4.4fromnikophil:messenger/catch-all-exceptions-in-deserialization

Conversation

@nikophil
Copy link
Contributor

@nikophilnikophil commentedJul 20, 2022
edited
Loading

QA
Branch?4.4
Bug fix?yes/no
New feature?no
Deprecations?no
LicenseMIT

Hello,

In Messenger, while deserializing a message, if an error is encountered in a custom denormalizer, the error is not caught, and thenMessageDecodingFailedException is 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\ExceptionInterface in order to cover all cases related to type error an so on...

@carsonbotcarsonbot added this to the6.2 milestoneJul 20, 2022
@nikophilnikophil changed the title[Messenger] catch all exceptions while deserializing message[Messenger] catch all exceptions while deserializing a messageJul 20, 2022
@nikophilnikophil changed the base branch from6.2 to4.4July 20, 2022 13:48
@lyrixx
Copy link
Member

lyrixx commentedJul 20, 2022
edited
Loading

Did you see#39622 ?

But indeed we can merge this one too

@nikophil
Copy link
ContributorAuthor

Hi@lyrixx

I wasn't aware of this other PR - but I think they don't conflict with each other

lyrixx reacted with thumbs up emoji

@dunglas
Copy link
Member

dunglas commentedJul 20, 2022
edited
Loading

Shouldn't exceptions thrown by your custom normalizers implementSymfony\Component\Serializer\Exception\ExceptionInterface?

@nikophil
Copy link
ContributorAuthor

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
Copy link
Member

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.
Is there a use case where such uncaught exception cannot be quickly spotted in dev (and the normalizer be fixed accordingly)?

@nikophil
Copy link
ContributorAuthor

nikophil commentedJul 20, 2022
edited
Loading

typically some broken json/xml where some fields are missing that are passed to rabbit, that will cause aTypeError inside the normalizer
I mean, sometimes you cannot prevent some type error to occur, moreover if you deserialize some input from another app, and presently if this occurs in a custom normalizer the workers will be down until the message is manually removed from the queue

@lyrixx
Copy link
Member

lyrixx commentedJul 20, 2022
edited
Loading

Something "classic":

  1. You have a class "Foo"
  2. There are messages in the queue
  3. You rename it to "Bar"
  4. You deploy
  5. The consumer try to deserialize "Foo"
  6. 💥

You have to do some weird things to deal with that! (I really need to finish my PR BTW)

nikophil and meyerbaptiste reacted with thumbs up emoji

@dunglas
Copy link
Member

Can't the failing queue be used for that?https://symfonycasts.com/screencast/messenger/show-retry-failures

@lyrixx
Copy link
Member

lyrixx commentedJul 20, 2022
edited
Loading

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
Copy link
Contributor

You fixed it@lyrixx right? Only in 6.2 though?

@nicolas-grekasnicolas-grekas modified the milestones:6.2,6.3Nov 5, 2022
@lyrixx
Copy link
Member

I'm not sure I covered this case.

@nikophil Can you revise this PR and check if you still need it?

nikophil reacted with thumbs up emoji

@nikophilnikophil closed this by deleting the head repositoryDec 29, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

7 participants

@nikophil@lyrixx@dunglas@chalasr@B-Galati@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp