8000 [MESSENGER] Worker fail in loop on handling empty message · Issue #30649 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[MESSENGER] Worker fail in loop on handling empty message #30649

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.

Sign up for GitHub

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

Jump to bottom
Closed
SukachVitally opened this issue Mar 22, 2019 · 3 comments
Closed

[MESSENGER] Worker fail in loop on handling empty message #30649

SukachVitally opened this issue Mar 22, 2019 · 3 comments
Labels
Bug Messenger Status: Needs Review

Comments

@SukachVitally
Copy link

Symfony version(s) affected: 4.2

Description
Messenger worker fail after handling message with empty body (or headers) and restart this process in loop. Other messages in queue become blocked.
No way to catch this exception in middleware or handler.

How to reproduce

  1. Start worker php bin/console messenger:consume-messages some_transport.
  2. Add message with empty headers to queue.
  3. Worker fail and restart handling invalid message in loop.

Possible Solution

  1. Add additional catch block for Symfony\Component\Messenger\Exception\InvalidArgumentException to reject message.
  2. Add error handler for serializer in Symfony\Component\Messenger\Transport\AmqpExt\AmqpReceiver

Additional context
@weaverryan
Copy link
Member

I'm not sure if we should fix this or not in 4.2, but this is fixed in master (so, 4.3). In master, if a message fails to be decoded, we catch that and reject the message.

If we want to fix this temporarily in 4.2, in AmqpSender, we could catch UnexpectedValueException (which is what Symfony's serializer will throw if there is a problem decoding) and then reject.

But, as a mentioned, this is fixed in master :).

@SukachVitally
Copy link
Author

Its looks broken even on master branch.

InvalidArgumentException exception will be thrown before Symfony serializer handling.
https://github.com/symfony/messenger/blob/master/Transport/Serialization/Serializer.php#L66

And this exception in not catchable in AmqpReceiver.
https://github.com/symfony/messenger/blob/master/Transport/AmqpExt/AmqpReceiver.php#L66

@weaverryan
Copy link
Member

@SukachVitally You're right! We've covered the situation where the format of the message is invalid, but not the situation where the encoded data is empty. Those InvalidArgumentException should probably be MessageDecodingFailedException.

@weaverryan weaverryan mentioned this issue Mar 28, 2019
Merged
@fabpot fabpot closed this as completed Mar 31, 2019
fabpot added a commit that referenced this issue Mar 31, 2019
@fabpot
bug #30756 Changing to MessageDecodingFailedException so that invalid…
8253353 
… messages are rejected (weaverryan)

This PR was merged into the 4.3-dev branch.

Discussion
----------

Changing to MessageDecodingFailedException so that invalid messages are rejected

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  |  no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #30649
| License       | MIT
| Doc PR        | not needed for bug fix

Bug fix if a message body is completely blank. I'm fixing this on master only, because in 4.2 and earlier, there is actually no system in place to fail serialization and cause the messages to be rejected. In 4.3, we just need to throw this exception.

Cheers!

Commits
-------

4be827d Changing to MessageDecodingFailedException so that invalid messages are rejected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Messenger Status: Needs Review
Projects
None yet
Milestone
No milestone
Development
33A7

No branches or pull requests
5 participants
@fabpot @javiereguiluz @weaverryan @SukachVitally @carsonbot
0