Skip to content

[RFC][Messenger] General message retry mechanism #27215

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

Closed
kbond opened this issue May 9, 2018 · 4 comments
Closed

[RFC][Messenger] General message retry mechanism #27215

kbond opened this issue May 9, 2018 · 4 comments
Labels
Messenger RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@kbond
Copy link
Member

kbond commented May 9, 2018

This was initially discussed in #27008 (comment) and #27008 (comment). That PR is for an AMQP-specific retry mechanism but it got me thinking of a more general retry system.

I am opening this issue to discuss further.

Here are some thoughts I have on this:

  1. A blanket retry could cause some issues:
    • If a message is handled by multiple handlers and only one fails it might not be desirable for it to be retried on all handlers.
    • The handler may fail at a point where retrying would not be required or desired.
  2. One solution to above could be for handlers to notify the bus that a retry is required for this specific handler:
    // in your handler
    try {
        // ... some logic that throws an exception
    } catch (\Throwable $e) {
        throw new RetryException(__CLASS__, $e); // or instead of __CLASS__ we can get the class from the exception's trace? Not sure about this...
    }
    The message should then only be retried on that handler.
  3. It would obviously be beneficial to provide a $maxRetry (and probably a $delay). Maybe at the handler level, maybe not...
    throw new RetryException(__CLASS__, $e, $maxRetries, $delayInSeconds);
  4. The ability to record when a message fails too many times. Laravel has a feature that stores these in the database for later review and re-run.
@xabbuh xabbuh added RFC RFC = Request For Comments (proposals about features that you want to be discussed) Messenger labels May 10, 2018
@sroze sroze added this to the next milestone May 10, 2018
@nicolas-grekas nicolas-grekas removed this from the next milestone May 13, 2018
@kbond
Copy link
Member Author

kbond commented May 15, 2018

I have created a POC retry system with just a few extension points added to the framework. I understand that how retries work can be very opinionated. My sample flex app is here with the specific changes from an out of the box flex website-skeleton here.

Some notes on how the app's bus works:

  • If any exception is thrown in a handler, the exception is wrapped in a FailedMessageException which contains the context of the exception: message, wrapped exception, single handler id. (see WrapExceptionHandleMessageMiddleware and CatchExceptionChainHandler)
  • These failed messages are caught and saved. They are just logged for now but could be saved to a database for later rerun. (see SaveFailedMessageMiddleware)
  • These failed messages can be rerun on just the handler they failed using a SingleHandlerConfiguration which takes the saved "single handler id" from the FailedMessageException above. (see SingleHandlerMiddleware)
  • Added a RetryMessageException which can be thrown from your handlers with a max attempt and delay. These failed messages are retried x times before failing fully and saved with SaveFailedMessageMiddleware above. (see RetryMessageMiddleware for the retry mechanism and FileTransport for the delay)

In order to make this work, I had to make 3 changes to Symfony:

  1. kbond@be52d56: This changes the custom handler method service IDs to be class::method. This allows (2) below to work. As an added benefit, the debug:messenger command is improved. See the output of a custom handler method without this change:
     ------------------------------------------------------------ 
      App\Event\BarEvent                                          
          handled by App\Event\BarEventSubscriber                 
      App\Event\FooEvent                                          
          handled by App\Event\FooEventSubscriber1                
          handled by .messenger.method_on_object_wrapper.Bb2WIx8  
          handled by App\Event\FooEventSubscriber3                
     ------------------------------------------------------------
    
    And after:
     ----------------------------------------------------- 
      App\Event\BarEvent                                   
          handled by App\Event\BarEventSubscriber          
      App\Event\FooEvent                                   
          handled by App\Event\FooEventSubscriber1         
          handled by App\Event\FooEventSubscriber2::doFoo  
          handled by App\Event\FooEventSubscriber3         
     -----------------------------------------------------
    
  2. kbond@7af9dc0: This adds an optional ContainerSingleHandlerLocator service that allows you to resolve a single handler. This is needed for the SingleHandlerMiddleware in my POC above.
  3. kbond@3a46d1a: This allows you to configure your own ChainHandler. Currently, if the chain handler is handling multiple messages and one fails, all subsequent messages are not run. I wanted to ensure this wasn't the case. I also use it to aggregate FailedMessageException's into a ChainFailedMessageException

Anyway, this scope of this issue has grown pretty large but I just wanted to share a possible use-case I see for the component and where the the blockers are in Symfony.

@kbond
Copy link
Member Author

kbond commented May 15, 2018

Ideally, I'd like to see FailedMessageException, ChainFailedMessageException, SingleHandlerMiddleware and CatchExceptionChainHandler (perhaps as the default thus not needing configuring) in core.

Of course, for synchronous message buses you would want the actual exception thrown - this could be done with a default middleware that unwraps the FailedMessageException.

@keulinho
Copy link
Contributor

keulinho commented Jan 29, 2019

This is still a thing as it is currently not possible to built a retry mechanism that takes into account that just one of many handlers had failed and on redelivery just the failed handlers should be called again.
One way to dramatically improve this would be to change the HandleMessageMiddleware in two ways

  1. try all registered handlers for each message and catch Exceptions from handlers. Throw a Exception which has a collection of all occured Exceptions like the ChainFailedMessageException described here. This has the advantage that one failing handler won't stop the whole execution chain.

  2. don't run handlers multiple times on same message. Just check the handled stamp on the envelope if the message was already handled by the handler, then skip it.

Combined this would lead to the possibility of just redelivering the message by dispatching the modified envelope, what can be easily achieved with a custom middleware. IMHO this redelivering mechanism is not necessarily part of the Framework, because it will depend on custom circumstances, like if the used transport already brings an redelivering mechanism, or if the redelivering should be delayed, etc. But in general it should be possible to build a solid retry mechanism.

Let me know what you think, i'll happilly prepare a PR for this one.

@weaverryan
Copy link
Member

Closing in favor of #30557 and also #30020

fabpot added a commit that referenced this issue Mar 23, 2019
… (weaverryan)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger] Worker events + global retry functionality

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes, on Messenger only
| Deprecations? | no
| Tests pass?   | NEEDED
| Fixed tickets | #29132, #27008, #27215 and part of #30540
| License       | MIT
| Doc PR        | TODO

This is an alternative to #29132 and #27008. There are several big things:

1) The `messenger:consume` does not die if a handler has an error
2) Events are dispatched before, after and on error a message being handled
3) Logic is moved out of Amqp and into the Worker so that we can have some consistent features, like error handling.
4) A generic retry system was added, which works with Amqp and future transports should support.
 It will work out of the box for users. Retrying works by putting the received `Envelope` back into the bus, but with the `ReceivedStamp` removed. The retry functionality has an integration test for AMQP.
5) Added a new `MessageDecodingFailedException` that transport Serializers should throw if `decode()` fails. It allows us to reject a message in this situation, as allowing it to fail but remain on the queue causes it to be retried forever.
6) A new `DelayStamp` was added, which is the first of (later) more stamps for configuring the transport layer (see #30558).

BC breaks are documented in the CHANGELOG.

Thanks!

Commits
-------

a989384 Adding global retry support, events & more to messenger transport
fabpot added a commit that referenced this issue Apr 6, 2019
…ndler (keulinho, sroze)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger] Ensure message is handled only once per handler

Add check to ensure that a message is only handled once per handler
Add try...catch to run all handlers before throwing exception

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? |no
| Tests pass?   | yes
| Fixed tickets | #27215
| License       | MIT
| Doc PR        | Todo

This would make error handling and retrying of messages much more easier. As statet  here #27008 (comment) there is currently no way to retry a for all failed handlers if there are mutliple handlers and just some throw an exception.
Also if an Exception in an handler occurs the execution chain is disrupted and the other handlers are never invoked.
With this change it is easily possible to create an userland middleware that catches the `ChainedHandlerFailedException` and does some custom retry logic. If you ensure that the `HandledStamps` on the `Envelope` are preserved the message will be handled just by the failed handlers

Commits
-------

2e5e910 Rename exception, add change log and a few other things
e6e4cde Ensure message is handled only once per handler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Messenger RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests

6 participants