[Messenger] Dispatch events before & after each handler #52425

BenMorel · 2023-11-02T12:19:05Z

Q	A
Branch?	7.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	-
License	MIT

Context

We audit every message that goes through Messenger, together with each run of every handler, and its outcome (success/failure). At the moment, we have to do a lot of gymnastics in our subscriber to properly audit a message:

when we receive a WorkerMessageHandledEvent, we know that all handlers have succeeded, but in case the message was redelivered, we may have already audited some of the handler successes. So:
- for each HandledStamp:
  - check if we have a matching custom AuditedHandledStamp:
    - if we don't:
      - log the succcess for this handler
      - add the AuditedHandledStamp
when we receive a WorkerMessageFailedEvent:
- if the exception is a HandlerFailedException:
  - for each nested exception:
    - trace back the exception up to HandleMessageMiddleware to find the handler name
    - log the failure for this handler
- because some handlers may still have succeeded, run the same logic as for WorkerMessageHandledEvent

There is an additional issue with this: while this works fine for async messages consumed by workers, this logic isn't triggered for sync messages. Because we also use the worker events to keep a stack of message ids in our subscriber, and stamp each message with its parent id, our current auditing solution cannot properly infer the parent-child relationship of say, an async Event triggered by a sync Command.

Proposed solution

This PR aims to solve all the issues above, and simplify auditing a lot, by introducing 3 events triggered in the handler loop of HandleMessageMiddleware:

HandlerStartingEvent (dispatched right before calling the handler)
HandlerSuccessEvent
HandlerFailureEvent

Pi-Bouf

YES !

OskarStark · 2023-11-03T07:27:19Z

This must target 7.1, due to feature freeze period.

OskarStark · 2023-11-03T07:27:50Z

src/Symfony/Component/Messenger/Event/AbstractHandlerEvent.php

+    private Envelope $envelope;
+    private string $handlerName;
+
+    public function __construct(Envelope $envelope, string $handlerName)
+    {
+        $this->envelope = $envelope;
+        $this->handlerName = $handlerName;
+    }


Please use constructor property promotion

Yes! A couple questions:

as we target PHP 8.1, do we want to keep private + getters, or can we use public readonly instead? (and if targeting Symfony 7 and PHP 8.2, straight readonly class with public promoted properties)

does Symfony allow a mix of promoted and non-promoted, like this:
final class HandlerFailureEvent extends AbstractHandlerEvent { public function __construct( Envelope $envelope, string $handlerName, public \Throwable $exception, ) { parent::__construct($envelope, $handlerName); } }

I've settled on public readonly as I could find other occurrences in the 7.0 branch. I also used a mix of promoted and non-promoted in HandlerFailureEvent, as in the code sample above.

OskarStark · 2023-11-03T07:28:55Z

src/Symfony/Component/Messenger/Event/HandlerStartingEvent.php

+ * Event dispatched before a handler is called.
+ */
+final class HandlerStartingEvent extends AbstractHandlerEvent


The name of the class does not make it clear, that it is dispatched before.

Maybe BeforeHandlerStartEvent?

I actually thought about this, but wanted to keep things organized by prefix, like Worker* events. I don't mind though, what about just BeforeHandlerEvent?

OskarStark · 2023-11-03T07:30:26Z

src/Symfony/Component/Messenger/Middleware/HandleMessageMiddleware.php

@@ -35,6 +39,7 @@ class HandleMessageMiddleware implements MiddlewareInterface
    public function __construct(
        private HandlersLocatorInterface $handlersLocator,
        private bool $allowNoHandlers = false,
+        private ?EventDispatcherInterface $eventDispatcher = null,


Suggested change

private ?EventDispatcherInterface $eventDispatcher = null,

private ?EventDispatcherInterface $dispatcher = null,

I like it more, but lets see what the others say

Actually SendMessageMiddleware uses $eventDispatcher, so I guess it's better to keep it consistent here?

OskarStark · 2023-11-03T07:32:03Z

src/Symfony/Component/Messenger/Middleware/HandleMessageMiddleware.php

@@ -97,6 +106,14 @@ public function handle(Envelope $envelope, StackInterface $stack): Envelope
            } catch (\Throwable $e) {
                $exceptions[$handlerDescriptor->getName()] = $e;


Suggested change

$exceptions[$handlerDescriptor->getName()] = $e;

$exceptions[$handlerDescriptor->getName()] = $e;

$this->eventDispatcher?->dispatch(new HandlerFailureEvent($envelope, $handlerDescriptor->getName(), $e));

Same for the success case and remove the $e variable?

I don't mind dispatching the failure event in the catch block, however I'm a bit reluctant about dispatching the success event in the try block, as a failure in any listener/subscriber of this event would mark the handler as failed.

That's why I kept things outside the try/catch.

ruudk · 2023-11-03T10:19:04Z

src/Symfony/Component/Messenger/Middleware/HandleMessageMiddleware.php

@@ -58,6 +63,10 @@ public function handle(Envelope $envelope, StackInterface $stack): Envelope
                continue;
            }

+            $this->eventDispatcher?->dispatch(new HandlerStartingEvent($envelope, $handlerDescriptor->getName()));


It would be great if the $handlerDescriptor is fully passed to this event, and that a subscriber can mutate it.

This would solve my use case that I tried to implement in:

[Messenger] Make HandleMessageMiddleware::callHandler protected #50980

It would allow event subscribers to:

read everything from the HandlerDescriptor, so also the options

set a new HandlerDescriptor that has a custom callable that does specific logic.

I'm all for adding the whole HandlerDescriptor in the events, you're right that it opens up a whole lot of possibilities.

However, I have no idea of the potential consequences of live-replacing a handler, so I'll leave it to others to decide if making the HandlerDescriptor replaceable is the way to go!

@ro0NL @OskarStark What do you think?

I have to admit that I also don't know what consequences this can have

@ruudk Do you actually have a need to replace the HandlerDescriptor at the moment, or can we leave it for a later PR to maybe make it replaceable in the future?

Having it would be great, but maybe I can use the event system already to do what I want. I agree that we can later always decide to open this up.

Could we at least add the full HandlerDescriptor instead of only the name? That would make it a bit better.

Could we at least add the full HandlerDescriptor instead of only the name? That would make it a bit better.

Yes, definitely. I'll change this.

@ruudk events now have the full HandlerDescriptor!

ruudk · 2023-11-03T10:22:19Z

src/Symfony/Component/Messenger/Middleware/HandleMessageMiddleware.php

@@ -97,6 +106,14 @@ public function handle(Envelope $envelope, StackInterface $stack): Envelope
            } catch (\Throwable $e) {
                $exceptions[$handlerDescriptor->getName()] = $e;


It would be great if a HandlerFailureEvent subscriber could silence an exception.

Right now, it's only possible to log them. But by always passing the exception to the HandlerFailureEvent and dispatching that, a subscriber could say "ignore this".

A use case could be to annotate #[IgnoreException(SomeTimeoutException::class)] on the handler. In case of an exception, a HandlerFailureEvent subscriber would see that the handler wants to ignore this exception, and it will silence it.

From Messenger perspective all will be fine again.

Related to

[Messenger] Make HandleMessageMiddleware::callHandler protected #50980

We could add a HandlerFailureEvent::ignoreException(bool $ignore) method indeed. I don't mind adding this feature, and again I'll leave it to others to decide if adding it is a good idea.

@ro0NL @OskarStark What do you think?

Also fine, but then we should let a subscriber mutate the handler descriptor. See my previous comment.

BenMorel · 2023-11-03T11:03:35Z

This must target 7.1, due to feature freeze period.

Thank you, where is this documented please? I can't find any information on the Release Process page nor on the Releases page. Googling only gives me blog entries for older Symfony releases.

BenMorel · 2023-11-05T12:11:00Z

src/Symfony/Component/Messenger/Event/AbstractHandlerEvent.php

+        public readonly Envelope $envelope,
+        public readonly HandlerDescriptor $handlerDescriptor,


Do you mean no abstract? What about AbstractWorkerMessageEvent?

This PR was squashed before being merged into the 6.3 branch. Discussion ---------- Add info about feature freeze period Follows * symfony/symfony#52425 (comment) Commits ------- 809f169 Add info about feature freeze period

BenMorel · 2023-11-29T21:36:58Z

I've just rebased this PR against the new 7.1 branch. Is there something I can do to get it merged?

As it stands, there are a few points in the comments that would need to be ruled on. Ideally I'd like to keep this PR simple for now, but keep things extendable if someone else wants to build extra features on top of it.

Jeroeny · 2023-12-09T17:37:35Z

What if the callHandler method was moved to an interface (HandlerExecutor) ? Then there can also be a Traceable version of it to measure and debug handling per handler.

ruudk · 2023-12-11T15:23:49Z

What if the callHandler method was moved to an interface (HandlerExecutor) ? Then there can also be a Traceable version of it to measure and debug handling per handler.

I like this a lot 🙌

It makes a lot of sense to be able to probe execution of the handler(s). This makes #50980 and #50998 absolute. As with this interface, I can now create my own HandlerExecutor middleware that to do transaction wrapping or exception ignoring.

jerowork · 2023-12-15T15:13:09Z

What if the callHandler method was moved to an interface (HandlerExecutor) ? Then there can also be a Traceable version of it to measure and debug handling per handler.

Exactly what I was thinking (its in the name I guess 😉 ).
Making callHandler protected would solve it, but extracting the calling handler to a separate class makes it more flexible and open for extension for everybody who wants to change the calling handler for whatever reason (for me I would like to have a upgraded way of injecting other (optional) parameters in a handler)

carsonbot added Status: Needs Review Feature Messenger labels Nov 2, 2023

carsonbot added this to the 6.4 milestone Nov 2, 2023

BenMorel force-pushed the messenger_handler_events branch 3 times, most recently from 93f7c9d to 2406c88 Compare November 2, 2023 13:49

Pi-Bouf suggested changes Nov 2, 2023

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 2, 2023

BenMorel force-pushed the messenger_handler_events branch from 2406c88 to 21c29d9 Compare November 2, 2023 14:11

BenMorel requested a review from Pi-Bouf November 2, 2023 14:15

carsonbot added Status: Needs Review and removed Status: Needs Work labels Nov 2, 2023

Pi-Bouf approved these changes Nov 2, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 2, 2023

ro0NL mentioned this pull request Nov 2, 2023

[Messenger] Make HandleMessageMiddleware::callHandler protected #50980

Open

OskarStark modified the milestones: 6.4, 7.1 Nov 3, 2023

OskarStark reviewed Nov 3, 2023

View reviewed changes

ruudk reviewed Nov 3, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

OskarStark mentioned this pull request Nov 3, 2023

Add info about feature freeze period symfony/symfony-docs#19110

Merged

This comment was marked as outdated.

Sign in to view

BenMorel force-pushed the messenger_handler_events branch from 21c29d9 to a404917 Compare November 4, 2023 23:31

BenMorel requested a review from xabbuh as a code owner November 4, 2023 23:31

BenMorel requested review from lyrixx, dunglas, yceruto, welcoMattic, kbond, chalasr and jderusse as code owners November 4, 2023 23:31

carsonbot added Status: Needs Review and removed Status: Reviewed labels Nov 4, 2023

BenMorel changed the base branch from 6.4 to 7.0 November 4, 2023 23:32

BenMorel force-pushed the messenger_handler_events branch from a404917 to 2479453 Compare November 4, 2023 23:42

BenMorel commented Nov 5, 2023

View reviewed changes

welcoMattic removed their request for review November 8, 2023 22:27

[Messenger] Dispatch events before & after each handler

77b4b53

BenMorel force-pushed the messenger_handler_events branch from 2479453 to 77b4b53 Compare November 29, 2023 21:24

BenMorel changed the base branch from 7.0 to 7.1 November 29, 2023 21:24

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

	private ?EventDispatcherInterface $eventDispatcher = null,
	private ?EventDispatcherInterface $dispatcher = null,

		@@ -97,6 +106,14 @@ public function handle(Envelope $envelope, StackInterface $stack): Envelope
		} catch (\Throwable $e) {
		$exceptions[$handlerDescriptor->getName()] = $e;

		public readonly Envelope $envelope,
		public readonly HandlerDescriptor $handlerDescriptor,

Uh oh!

[Messenger] Dispatch events before & after each handler #52425

Are you sure you want to change the base?

[Messenger] Dispatch events before & after each handler #52425

Conversation

BenMorel commented Nov 2, 2023 • edited by OskarStark Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Proposed solution

Uh oh!

Pi-Bouf left a comment

Choose a reason for hiding this comment

Uh oh!

OskarStark commented Nov 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruudk Nov 3, 2023 • edited by OskarStark Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenMorel Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenMorel commented Nov 3, 2023

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

BenMorel commented Nov 29, 2023

Uh oh!

Jeroeny commented Dec 9, 2023

Uh oh!

ruudk commented Dec 11, 2023

Uh oh!

jerowork commented Dec 15, 2023

Uh oh!

Uh oh!

BenMorel commented Nov 2, 2023 •

edited by OskarStark

Loading

ruudk Nov 3, 2023 •

edited by OskarStark

Loading

BenMorel Nov 3, 2023 •

edited

Loading