[Scheduler] pre_run and post_run events #51805

alli83 · 2023-10-02T09:08:32Z

Q	A
Branch?	6.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	#49803 (comment)
License	MIT

Based on #49803 @kbond and taking into account #51553

The aim of this PR is to be able to act on the accumulated messages 'if something happens' and to be able to recalculate the heap via events (currently pre_run and post_run).
The aim is to have access to

the the schedule and therefore add/cancel a certain type of message.
MessageContexte (e.g. access the id)
The message itself

This PR would complement @Jeroeny #51553 PR by enabling action via events.

carsonbot · 2023-10-02T09:08:41Z

Hey!

Thanks for your PR. You are targeting branch "6.4" but it seems your PR description refers to branch "6.4 for features".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

kbond

Quick initial review

kbond · 2023-10-02T14:13:19Z

src/Symfony/Component/Scheduler/Event/SchedulerEvents.php

+
+namespace Symfony\Component\Scheduler\Event;
+
+final class SchedulerEvents


I'm pretty sure we don't want this class alias system anymore. We just the use event's FQCN.

kbond · 2023-10-02T14:18:50Z

src/Symfony/Component/Messenger/Worker.php

@@ -99,6 +102,10 @@ public function run(array $options = []): void
                if ($queueNames) {
                    $envelopes = $receiver->getFromQueues($queueNames);
                } else {
+                    if (method_exists($receiver, 'getMessageGenerator')) {


I don't think we'll want the messenger to be aware of the scheduler. Ideally, we want these events dispatched in the scheduler but can't currently think of how.

Indeed, as discussed, I will add listeners that listens to the WorkerMessageReceivedEvent and to the WorkerMessageHandledEvent and dispatch the corresponding scheduler event. Thank you for your feedback!

kbond · 2023-10-02T14:19:38Z

src/Symfony/Component/Scheduler/Event/PostRunEvent.php

+
+class PostRunEvent
+{
+    public function __construct(private readonly MessageGenerator $messageGenerator, private readonly Envelope $envelope)


Assuming we dispatch these events in the scheduler itself, we cannot use Envelope here as it could be any object.

Indeed the standalone usage

kbond · 2023-10-02T14:21:34Z

src/Symfony/Component/Scheduler/Schedule.php

 use Symfony\Component\Scheduler\Exception\LogicException;
 use Symfony\Contracts\Cache\CacheInterface;

 final class Schedule implements ScheduleProviderInterface
 {
+    public function __construct(private readonly EventDispatcherInterface $dispatcher)


This would need to be optional.

alli83 · 2023-10-02T21:37:30Z

src/Symfony/Component/Messenger/EventListener/DispatchSchedulerEventListener.php

+        $schedulerTransport =  $this->receiverLocator->get($event->getReceiverName());
+        $schedule = $schedulerTransport->getMessageGenerator()->schedule();
+
+        $this->eventDispatcher?->dispatch(new PostRunEvent($schedule, $messageContext, $envelope->getMessage()));


It seems that I would need the same info for pre_run and post_run event.
I will handle this code duplication.

fabpot

Just some CS stuff for now.

fabpot · 2023-10-03T08:46:38Z

src/Symfony/Component/Scheduler/Schedule.php

+    public function __construct(private readonly ?EventDispatcherInterface $dispatcher = null)
+    {


Suggested change

public function __construct(private readonly ?EventDispatcherInterface $dispatcher = null)

{

public function __construct(

private readonly ?EventDispatcherInterface $dispatcher = null,

) {

fabpot · 2023-10-03T08:47:57Z

src/Symfony/Component/Scheduler/Event/PreRunEvent.php

+    public function __construct(private readonly ScheduleProviderInterface $schedule, private readonly MessageContext $messageContext, private readonly object $message)
+    {


Suggested change

public function __construct(private readonly ScheduleProviderInterface $schedule, private readonly MessageContext $messageContext, private readonly object $message)

{

public function __construct(

private readonly ScheduleProviderInterface $schedule,

private readonly MessageContext $messageContext,

private readonly object $message,

) {

fabpot · 2023-10-03T08:48:39Z

src/Symfony/Component/Scheduler/Event/PostRunEvent.php

+    public function __construct(private readonly ScheduleProviderInterface $schedule, private readonly MessageContext $messageContext, private readonly object $message)
+    {


Suggested change

public function __construct(private readonly ScheduleProviderInterface $schedule, private readonly MessageContext $messageContext, private readonly object $message)

{

public function __construct(

private readonly ScheduleProviderInterface $schedule,

private readonly MessageContext $messageContext,

private readonly object $message,

) {

fabpot · 2023-10-03T08:48:53Z

src/Symfony/Component/Messenger/EventListener/DispatchSchedulerEventListener.php

+{
+    public function __construct(
+        private readonly ContainerInterface $receiverLocator,
+        private readonly ?EventDispatcherInterface $eventDispatcher = null


Suggested change

private readonly ?EventDispatcherInterface $eventDispatcher = null

private readonly ?EventDispatcherInterface $eventDispatcher = null,

fabpot · 2023-10-03T08:49:10Z

src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Messenger/DummySchedule.php

@@ -3,6 +3,7 @@
 namespace Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Messenger;

 use Symfony\Component\Cache\Adapter\ArrayAdapter;
+use Symfony\Component\EventDispatcher\EventDispatcher;


fabpot · 2023-10-03T08:49:26Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.php

+            ->args([
+                service('messenger.receiver_locator'),
+                service('event_dispatcher'),
+        ])


Suggested change

])

])

src/Symfony/Component/Scheduler/Event/PreRunEvent.php

fabpot · 2023-10-03T08:51:38Z

src/Symfony/Component/Scheduler/Event/PreRunEvent.php

+
+class PreRunEvent
+{
+    private bool $isCancel = false;


Suggested change

private bool $isCancel = false;

private bool $isCancelled = false;

alli83 · 2023-10-03T10:40:46Z

To access the Schedule, it has been decided to inject the transports via the locator service (messenger.receiver_locator). While it would have been possible to inject the schedules directly via the tagged locator scheduler.schedule_provider doing so would require memoizing the created schedule to avoid calling getSchedule() twice.

src/Symfony/Component/Scheduler/Scheduler.php

src/Symfony/Component/Messenger/EventListener/DispatchSchedulerEventListener.php

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.php

src/Symfony/Component/Scheduler/Event/PostRunEvent.php

src/Symfony/Component/Scheduler/Event/PreRunEvent.php

src/Symfony/Component/Scheduler/Generator/MessageGenerator.php

src/Symfony/Component/Scheduler/EventListener/DispatchSchedulerEventListener.php

src/Symfony/Component/Scheduler/Generator/MessageGeneratorInterface.php

src/Symfony/Component/Scheduler/Schedule.php

src/Symfony/Component/Scheduler/Scheduler.php

fabpot · 2023-10-11T08:51:11Z

src/Symfony/Component/Scheduler/Generator/MessageGeneratorInterface.php

 interface MessageGeneratorInterface
 {
    /**
     * @return iterable<MessageContext, object>
     */
    public function getMessages(): iterable;
+
+    public function getSchedule(): Schedule;


I'm uncomfortable adding this method to the interface as a MessageGenerator implementation might not use a Schedule.
In any case, I don't see a reason to interact with the schedule before or after handling a message; it doesn't seem right (especially as we now - in 6.4 - have other ways) and again a Schedule is not a requirement to send scheduled messages.
I would recommend removing the Schedule from the events.

This is the case when a message is scheduled to be sent every x hours until an event occurs.
It cannot be processed in the handler (messenger case), because it cannot be already processed at that time. The time at which its state (to be processed or not) can change lies within this x-hour interval. The aim, for example in pre_run, is to be able to check (before sending it to the handler) whether it is still relevant or whether, on the contrary, it should be removed from the stack of messages to be processed (since, for example, it will not have to be sent again if it has been processed in the meantime). The aim is to have access to the schedule and to be able to use the remove method, for example, which will recalculate the messages to be processed (using the setRestart introduced in 6.4).

Alternatively, I could inject the ScheduleProviderInterface directly into the event ?

what I don't understand is that on the Scheduler class side, generators will have a Schedule via ScheduleProviderInterface $scheduleProvider.

Sorry if I misunderstood your point/comment

fabpot

LGTM now (just a few minor comments).

fabpot · 2023-10-14T07:30:39Z

src/Symfony/Component/Messenger/Tests/EventListener/DispatchSchedulerEventListenerTest.php

+use Symfony\Component\Scheduler\Tests\Messenger\SomeScheduleProvider;
+use Symfony\Component\Scheduler\Trigger\TriggerInterface;
+
+class DispatchSchedulerEventListenerTest extends TestCase


Must be moved to the Scheduler component test suite.

fabpot · 2023-10-14T07:30:46Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/scheduler.php

+            ->args([
+                tagged_locator('scheduler.schedule_provider', 'name'),
+                service('event_dispatcher'),
+        ])


Suggested change

])

])

src/Symfony/Bundle/FrameworkBundle/Resources/config/scheduler.php

fabpot · 2023-10-14T07:31:55Z

src/Symfony/Component/Scheduler/Scheduler.php

@@ -62,8 +66,23 @@ public function run(array $options = []): void

            $ran = false;
            foreach ($this->generators as $generator) {
-                foreach ($generator->getMessages() as $message) {
+                foreach ($generator->getMessages() as $context => $message) {
+                    if ($this->dispatcher) {


What about having testing !$this->dispatcher to have less code in the condition?

fabpot · 2023-10-14T07:32:07Z

src/Symfony/Component/Scheduler/Schedule.php

 use Symfony\Component\Scheduler\Exception\LogicException;
 use Symfony\Contracts\Cache\CacheInterface;

 final class Schedule implements ScheduleProviderInterface
 {
+    public function __construct(
+        private readonly ?EventDispatcherInterface $dispatcher = null


Suggested change

private readonly ?EventDispatcherInterface $dispatcher = null

private readonly ?EventDispatcherInterface $dispatcher = null,

dunglas

(when last comments from Fabien will be addressed)

fabpot · 2023-10-16T09:28:09Z

Thank you @alli83.

This PR was merged into the 6.4 branch. Discussion ---------- [Scheduler] Fix dev requirements | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Follow-up of #51805 | License | MIT This should turn the CI green Commits ------- f241ed9 [Scheduler] Fix dev requirements

This PR was merged into the 6.4 branch. Discussion ---------- [Scheduler] Add `FailureEvent` | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | | License | MIT Following PR #51805, It would be interesting to add a `failureEvent` allowing you, for instance, to remove the recurring message, depending on the error caught. Commits ------- 891a404 [Scheduler] Add failureEvent

alli83 requested a review from kbond as a code owner October 2, 2023 09:08

carsonbot added Status: Needs Review Feature Scheduler labels Oct 2, 2023

carsonbot added this to the 6.4 milestone Oct 2, 2023

alli83 force-pushed the scheduler-events-pre-run-post-run branch 6 times, most recently from c523449 to 70218b2 Compare October 2, 2023 13:30

alli83 changed the title ~~[Scheduler] feat: pre_run and post_run events~~ [Scheduler] pre_run and post_run events Oct 2, 2023

alli83 force-pushed the scheduler-events-pre-run-post-run branch from 70218b2 to c90b629 Compare October 2, 2023 14:17

kbond reviewed Oct 2, 2023

View reviewed changes

alli83 force-pushed the scheduler-events-pre-run-post-run branch from c90b629 to 0a0fe36 Compare October 2, 2023 21:35

alli83 commented Oct 2, 2023

View reviewed changes

alli83 force-pushed the scheduler-events-pre-run-post-run branch 3 times, most recently from 99f8d96 to c30f5f2 Compare October 2, 2023 21:53

fabpot requested changes Oct 3, 2023

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 3, 2023

alli83 force-pushed the scheduler-events-pre-run-post-run branch 5 times, most recently from 4f8417b to 49d909f Compare October 3, 2023 10:22

dunglas reviewed Oct 5, 2023

View reviewed changes

src/Symfony/Component/Scheduler/Scheduler.php Outdated Show resolved Hide resolved

dunglas reviewed Oct 5, 2023

View reviewed changes

src/Symfony/Component/Messenger/EventListener/DispatchSchedulerEventListener.php Outdated Show resolved Hide resolved

fabpot requested changes Oct 6, 2023

View reviewed changes

src/Symfony/Component/Messenger/EventListener/DispatchSchedulerEventListener.php Outdated Show resolved Hide resolved

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.php Outdated Show resolved Hide resolved

fabpot reviewed Oct 6, 2023

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.php Outdated Show resolved Hide resolved

alli83 force-pushed the scheduler-events-pre-run-post-run branch from 49d909f to f2cfe76 Compare October 9, 2023 05:30

fabpot requested changes Oct 10, 2023

View reviewed changes

alli83 force-pushed the scheduler-events-pre-run-post-run branch 2 times, most recently from f462be9 to ffb8279 Compare October 10, 2023 06:58

fabpot requested changes Oct 11, 2023

View reviewed changes

alli83 force-pushed the scheduler-events-pre-run-post-run branch 3 times, most recently from ffd4881 to dc1d61e Compare October 12, 2023 03:03

fabpot reviewed Oct 14, 2023

View reviewed changes

dunglas approved these changes Oct 14, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 14, 2023

alli83 force-pushed the scheduler-events-pre-run-post-run branch 2 times, most recently from edecb50 to 49f05e3 Compare October 16, 2023 02:29

[Scheduler] add PRE_RUN and POST_RUN events

20fd21a

alli83 force-pushed the scheduler-events-pre-run-post-run branch from 49f05e3 to 20fd21a Compare October 16, 2023 03:45

fabpot approved these changes Oct 16, 2023

View reviewed changes

fabpot mentioned this pull request Oct 16, 2023

[Scheduler] pre_run and post_run events symfony/symfony-docs#19023

Closed

fabpot merged commit 50662d0 into symfony:6.4 Oct 16, 2023

alexandre-daubois mentioned this pull request Oct 16, 2023

[Scheduler] Fix dev requirements #52071

Merged

alli83 mentioned this pull request Oct 16, 2023

[Scheduler] Add FailureEvent #52087

Merged

This was referenced Oct 21, 2023

Release v6.4.0-BETA1 #52220

Merged

Release v7.0.0-BETA1 #52224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Scheduler] pre_run and post_run events #51805

[Scheduler] pre_run and post_run events #51805


		namespace Symfony\Component\Scheduler\Event;

		final class SchedulerEvents

		public function __construct(private readonly ?EventDispatcherInterface $dispatcher = null)
		{

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

	private bool $isCancel = false;
	private bool $isCancelled = false;

[Scheduler] pre_run and post_run events #51805

[Scheduler] pre_run and post_run events #51805

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment