Skip to content
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

[Messenger] Message retried in failure transport dropped on exception #32719

Closed
amenophis opened this issue Jul 24, 2019 · 20 comments · Fixed by #37087
Closed

[Messenger] Message retried in failure transport dropped on exception #32719

amenophis opened this issue Jul 24, 2019 · 20 comments · Fixed by #37087

Comments

@amenophis
Copy link

amenophis commented Jul 24, 2019

Symfony version(s) affected: 4.3.2

Description
Message retried in failure transport dropped on exception

How to reproduce
I configured a failure transport on symfony messenger (like explain here: https://symfony.com/blog/new-in-symfony-4-3-messenger-failure-transport)
I throw an error on my handler, the message is retried 3 times, and then sent to the failure transport.

Then, I retry the failed message with bin/console messenger:consume failed or bin/console messenger:failed:retry {id}.
The message handling fail again and the message is dropped.

Is it the normal behaviour ? Or is it a bug ?

@weaverryan
Copy link
Member

Hmm, I’ve actually not seen this behavior - we even illustrate the correct behavior on our SymfonyCasts screencast.

There IS a bug (and PR) where the failed message IS sent back to the failure transport successsfully, but the error details are lost. Is it possible the message IS sent back to the failure transport (it will have a different id now) but that you don’t recognize it due to the missing/different error on that message?

@amenophis
Copy link
Author

This is a screenshot with the error thrown during retry.

error

@Tobion
Copy link
Member

Tobion commented Jul 25, 2019

This looks like a serialization error. In this case it's meant to be dropped. Please investigate why serialization fails after manual retry.

@ke20
Copy link

ke20 commented Jul 26, 2019

@Tobion Yes that's it.

With the default php serialization there is no problem, but the problem occurs when we add in the file messenger.yaml this configuration :

  messenger:
    serializer:
      default_serializer: messenger.transport.symfony_serializer
      symfony_serializer:
        format: json
        context: { }

We are trying to find a solution at this problem.

@skalpa
Copy link
Contributor

skalpa commented Jul 30, 2019

@ke20 Quickly looking at the Symfony\Component\Debug\Exception\FlattenException class, it seems that in some cases non-serializable instances could be created (and that seems to correspond to the error you get):

  • An instance is created with new FlattenException()
  • During serialization getHeaders() returns null as the $headers property is not initialized
  • Unserialization fails as setHeaders() only accepts an array argument and not null

Could you change the declaration of the $headers property of FlattenException to private $headers = [] and see if that fixes your problem ? (You'll have to re-test from the beginning as instances that were serialized before you apply the fix are invalid for good).

@ke20
Copy link

ke20 commented Aug 2, 2019

@skalpa I've made the modification on the class FlattenException like you have suggered me.

I confirm you that the problem has been fixed with this modification.

I've succeed to relaunch many time my message in error with the command bin/console messenger:failed:retry <id>.

@krewetka
Copy link

krewetka commented Aug 20, 2019

We experienced the same problem. Would be good to have fix for it introduced.

@weaverryan
Copy link
Member

Yea, that issue makes sense - if you ran new FlattenException()... then you would create an object that will not be serialized/deserialized correctly when using the Symfony serializer.

But, question - how/where are you calling new FlattenException()? The core code calls the correct FlattenException::createFromThrowable()

$flattenedException = class_exists(FlattenException::class) ? FlattenException::createFromThrowable($throwable) : null;

@tienvx
Copy link
Contributor

tienvx commented Sep 16, 2019

I had the same problem too. I created a project to show the problem https://github.com/tienvx/symfony-failed-message-reproduction.

Here is the PR to fix it #33600

@Tobion
Copy link
Member

Tobion commented Sep 16, 2019

$message = (new FlattenException())->setMessage($message)->getMessage();
seems to call new FlattenException() without initializing the headers. I don't know if that is triggering it. But getHeaders also does not have a return phpdoc. So it's not clear if it should be able to return null or not. I would assume headers should always be an array.
Unfortunately, the new ErrorHandler FlattenException (https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/ErrorRenderer/Exception/FlattenException.php) has the same problem which we could also fix (ref. #32605):

@tienvx
Copy link
Contributor

tienvx commented Sep 16, 2019

To make this issue clear, I will list 2 problems I can see in this issue:

  1. the failed message IS sent back to the failure transport successsfully, but the error details are lost
  2. The serialization with headers

My PR fix 1, not 2.

@skalpa
Copy link
Contributor

skalpa commented Sep 16, 2019

@Tobion I also think that getHeaders should never return null as setHeaders does have an array type-hint.

I'll work on fixes for both FlattenException versions tonight (the one from Debug and the one from ErrorRenderer).

fabpot added a commit that referenced this issue Sep 17, 2019
…pped on retry (tienvx)

This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger] Fix exception message of failed message is dropped on retry

| Q             | A
| ------------- | ---
| Branch?       | 4.3 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #32719
| License       | MIT
| Doc PR        | NA <!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->

Commits
-------

8f9f44e [Messenger] Fix exception message of failed message is dropped on retry
@fabpot fabpot closed this as completed Sep 17, 2019
@fabpot
Copy link
Member

fabpot commented Sep 17, 2019

@skalpa To really close this issue, I think we would need your PR on getHeaders, right?

@skalpa
Copy link
Contributor

skalpa commented Sep 18, 2019

@fabpot Yes, the PR you merged fixes a related issue but not the one described here afaik, so I guess it should be re-opened.
Mine is actually ready, I should be able to submit it soon.

@skalpa
Copy link
Contributor

skalpa commented Sep 18, 2019

I submitted #33630 to fix this, however I couldn't find a way to do it without breaking BC, as more methods had a similar problem (i.e: setPrevious and setTrace) that couldn't get solved by setting a default property value as discussed for setHeaders, so I don't know if my solution is valid.

As I don't really fancy the BC, I tried to find better solutions and the only alternate fixes that come to my mind would be to:

  • Modify messenger so it doesn't use the FlattenException from the Debug component at all
  • Or add a FlattenException denormalizer (my preferred one atm)

Strictly speaking, both would also break BC, but as messenger is still experimental it might be more acceptable.

@tienvx
Copy link
Contributor

tienvx commented Sep 18, 2019

Now I think I understand the confusion between this issue and #32311:

@skalpa
Copy link
Contributor

skalpa commented Sep 18, 2019

@tienvx Here the entire message is dropped, as the Serializer component is unable to deserialize the message.

Not everyone may be affected, as it only happens when using the component as the messenger serializer, which is not the default, and has to be configured as described in #32719 (comment).

@krewetka
Copy link

In our project we faced this issue exactly in case of using symfony serializer instead of default one.

@PaydoW
Copy link

PaydoW commented Mar 30, 2020

Hello guys.
What the status for this issue? Are you still planning fix this?

@weaverryan
Copy link
Member

I think #33650 addresses this, but it may need to be updated now that the FlattenException is in a different component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.