From 2f918a2285df52da1892a3aff4c34b6517e94f2e Mon Sep 17 00:00:00 2001 From: Guillaume Loulier Date: Mon, 4 Jul 2022 08:49:21 +0200 Subject: [PATCH 1/3] refactor(worker): middleware call moved --- src/Worker/Worker.php | 9 +++--- tests/Worker/WorkerTest.php | 56 +++++++++++++++---------------------- 2 files changed, 27 insertions(+), 38 deletions(-) diff --git a/src/Worker/Worker.php b/src/Worker/Worker.php index 5d5d3ee9..1b08b5c8 100644 --- a/src/Worker/Worker.php +++ b/src/Worker/Worker.php @@ -292,13 +292,8 @@ protected function handleTask(TaskInterface $task, TaskListInterface $taskList): $this->taskExecutionTracker->endTracking(task: $task); $task->setExecutionEndTime(dateTimeImmutable: new DateTimeImmutable()); - $task->setLastExecution(dateTimeImmutable: new DateTimeImmutable()); - $this->defineTaskExecutionState(task: $task, output: $output); - - $this->middlewareStack->runPostExecutionMiddleware(task: $task, worker: $this); $this->eventDispatcher->dispatch(new TaskExecutedEvent(task: $task, output: $output)); - $this->configuration->setLastExecutedTask(lastExecutedTask: $task); $executedTasksCount = $this->configuration->getExecutedTasksCount(); @@ -309,8 +304,12 @@ protected function handleTask(TaskInterface $task, TaskListInterface $taskList): $this->getFailedTasks()->add(task: $failedTask); $this->eventDispatcher->dispatch(event: new TaskFailedEvent(task: $failedTask)); } finally { + $task->setLastExecution(dateTimeImmutable: new DateTimeImmutable()); + $this->configuration->setCurrentlyExecutedTask(task: null); $this->configuration->run(isRunning: false); + + $this->middlewareStack->runPostExecutionMiddleware(task: $task, worker: $this); $this->eventDispatcher->dispatch(event: new WorkerRunningEvent(worker: $this, isIdle: true)); } } diff --git a/tests/Worker/WorkerTest.php b/tests/Worker/WorkerTest.php index 67f9b891..a27d6cb7 100644 --- a/tests/Worker/WorkerTest.php +++ b/tests/Worker/WorkerTest.php @@ -77,25 +77,23 @@ final class WorkerTest extends TestCase */ public function testTaskCannotBeExecutedWithoutRunner(): void { - $watcher = $this->createMock(TaskExecutionTrackerInterface::class); - $worker = new Worker( - new Scheduler('UTC', new InMemoryTransport(new InMemoryConfiguration(), new SchedulePolicyOrchestrator([ + scheduler: new Scheduler(timezone: 'UTC', transport: new InMemoryTransport(new InMemoryConfiguration(), schedulePolicyOrchestrator: new SchedulePolicyOrchestrator(policies: [ new FirstInFirstOutPolicy(), - ])), new SchedulerMiddlewareStack([]), new EventDispatcher()), - new RunnerRegistry([]), - new ExecutionPolicyRegistry([]), - $watcher, - new WorkerMiddlewareStack(), - new EventDispatcher(), - new LockFactory(new InMemoryStore()), - new NullLogger() + ])), middlewareStack: new SchedulerMiddlewareStack([]), eventDispatcher: new EventDispatcher()), + runnerRegistry: new RunnerRegistry(runners: []), + executionPolicyRegistry: new ExecutionPolicyRegistry(policies: []), + taskExecutionTracker: new TaskExecutionTracker(watch: new Stopwatch()), + middlewareStack: new WorkerMiddlewareStack(), + eventDispatcher: new EventDispatcher(), + lockFactory: new LockFactory(store: new InMemoryStore()), + logger: new NullLogger() ); - self::expectException(UndefinedRunnerException::class); - self::expectExceptionMessage('No runner found'); - self::expectExceptionCode(0); - $worker->execute(WorkerConfiguration::create()); + self::expectException(exception: UndefinedRunnerException::class); + self::expectExceptionMessage(message: 'No runner found'); + self::expectExceptionCode(code: 0); + $worker->execute(configuration: WorkerConfiguration::create()); } /** @@ -103,8 +101,6 @@ public function testTaskCannotBeExecutedWithoutRunner(): void */ public function testWorkerCanBeConfigured(): void { - $watcher = $this->createMock(TaskExecutionTrackerInterface::class); - $lockFactory = new LockFactory(new InMemoryStore()); $worker = new Worker(new Scheduler('UTC', new InMemoryTransport(new InMemoryConfiguration(), new SchedulePolicyOrchestrator([ @@ -113,7 +109,7 @@ public function testWorkerCanBeConfigured(): void new NullTaskRunner(), ]), new ExecutionPolicyRegistry([ new DefaultPolicy(), - ]), $watcher, new WorkerMiddlewareStack([ + ]), new TaskExecutionTracker(new Stopwatch()), new WorkerMiddlewareStack([ new TaskLockBagMiddleware($lockFactory), ]), new EventDispatcher(), $lockFactory, new NullLogger()); @@ -183,9 +179,6 @@ public function testWorkerCanBeForked(): void */ public function testTaskCannotBeExecutedWithoutSupportingRunner(): void { - $watcher = $this->createMock(TaskExecutionTrackerInterface::class); - $logger = $this->createMock(LoggerInterface::class); - $transport = new InMemoryTransport(new InMemoryConfiguration(), new SchedulePolicyOrchestrator([ new FirstInFirstOutPolicy(), ])); @@ -203,10 +196,10 @@ public function testTaskCannotBeExecutedWithoutSupportingRunner(): void new ShellTaskRunner(), ]), new ExecutionPolicyRegistry([ new DefaultPolicy(), - ]), $watcher, new WorkerMiddlewareStack([ + ]), new TaskExecutionTracker(new Stopwatch()), new WorkerMiddlewareStack([ new TaskUpdateMiddleware($transport), new TaskLockBagMiddleware($lockFactory), - ]), $eventDispatcher, $lockFactory, $logger); + ]), $eventDispatcher, $lockFactory, new NullLogger()); $worker->execute(WorkerConfiguration::create()); self::assertNull($worker->getLastExecutedTask()); @@ -218,7 +211,7 @@ public function testTaskCannotBeExecutedWithoutSupportingRunner(): void $task = $failedTask->getTask(); self::assertSame('foo', $task->getName()); self::assertNull($task->getExecutionState()); - self::assertNull($task->getLastExecution()); + self::assertInstanceOf(DateTimeImmutable::class, $task->getLastExecution()); } /** @@ -507,10 +500,6 @@ public function testTaskCanBeExecutedWithErroredAfterExecutionCallback(): void $logger = $this->createMock(LoggerInterface::class); $logger->expects(self::never())->method('info'); - $tracker = $this->createMock(TaskExecutionTrackerInterface::class); - $tracker->expects(self::exactly(2))->method('startTracking')->withConsecutive([$task], [$validTask]); - $tracker->expects(self::exactly(2))->method('endTracking')->withConsecutive([$task], [$validTask]); - $transport = new InMemoryTransport(new InMemoryConfiguration(), new SchedulePolicyOrchestrator([ new FirstInFirstOutPolicy(), ])); @@ -528,15 +517,16 @@ public function testTaskCanBeExecutedWithErroredAfterExecutionCallback(): void new NullTaskRunner(), ]), new ExecutionPolicyRegistry([ new DefaultPolicy(), - ]), $tracker, new WorkerMiddlewareStack([ + ]), new TaskExecutionTracker(new Stopwatch()), new WorkerMiddlewareStack([ new SingleRunTaskMiddleware($transport), new TaskCallbackMiddleware(), new TaskLockBagMiddleware($lockFactory), ]), $eventDispatcher, $lockFactory, $logger); $worker->execute(WorkerConfiguration::create()); - self::assertCount(1, $worker->getFailedTasks()); - self::assertInstanceOf(FailedTask::class, $worker->getFailedTasks()->get('foo.failed')); + $failedTasks = $worker->getFailedTasks(); + self::assertCount(1, $failedTasks); + self::assertInstanceOf(FailedTask::class, $failedTasks->get('foo.failed')); self::assertNotNull($worker->getLastExecutedTask()); self::assertSame($validTask, $worker->getLastExecutedTask()); } @@ -1300,8 +1290,8 @@ public function testWorkerCanExecuteChainedTasks(): void ])); $scheduler = new Scheduler('UTC', $transport, new SchedulerMiddlewareStack(), new EventDispatcher()); - $scheduler->schedule($chainedTask); - $scheduler->schedule($shellTask); + $scheduler->schedule(task: $chainedTask); + $scheduler->schedule(task: $shellTask); $eventDispatcher = new EventDispatcher(); $lockFactory = new LockFactory(new InMemoryStore()); From 83d63012de44be59de0ac3ee7260e829747fd436 Mon Sep 17 00:00:00 2001 From: Guillaume Loulier Date: Mon, 18 Jul 2022 19:05:04 +0200 Subject: [PATCH 2/3] refactor(middleware): improvements on error handling --- src/Middleware/AbstractMiddlewareStack.php | 6 +++--- src/Middleware/MiddlewareRegistry.php | 9 +++++++++ src/Middleware/MiddlewareRegistryInterface.php | 5 +++++ src/Worker/Worker.php | 6 +++--- tests/Middleware/WorkerMiddlewareStackTest.php | 7 ++++--- .../ExecutionPolicy/ExecutionPolicyRegistryTest.php | 10 +++++----- tests/Worker/WorkerTest.php | 2 +- 7 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/Middleware/AbstractMiddlewareStack.php b/src/Middleware/AbstractMiddlewareStack.php index d888b038..5745d914 100644 --- a/src/Middleware/AbstractMiddlewareStack.php +++ b/src/Middleware/AbstractMiddlewareStack.php @@ -62,7 +62,9 @@ protected function runMiddleware(MiddlewareRegistryInterface $middlewareList, Cl $this->executedMiddleware->attach(object: $middleware); }); - } catch (Throwable $throwable) { + } catch (Throwable) { + $middlewareList->next(); + } finally { foreach ($requiredMiddlewareList as $singleRequiredMiddlewareList) { if ($this->executedMiddleware->contains(object: $singleRequiredMiddlewareList)) { continue; @@ -70,8 +72,6 @@ protected function runMiddleware(MiddlewareRegistryInterface $middlewareList, Cl $func($singleRequiredMiddlewareList); } - - throw $throwable; } } diff --git a/src/Middleware/MiddlewareRegistry.php b/src/Middleware/MiddlewareRegistry.php index b33dcbdd..347f44aa 100644 --- a/src/Middleware/MiddlewareRegistry.php +++ b/src/Middleware/MiddlewareRegistry.php @@ -13,6 +13,7 @@ use function is_array; use function iterator_to_array; use function uasort; +use function next; use const ARRAY_FILTER_USE_BOTH; @@ -73,6 +74,14 @@ public function toArray(): array return $this->middlewareList; } + /** + * {@inheritdoc} + */ + public function next(): void + { + next(array: $this->middlewareList); + } + /** * {@inheritdoc} */ diff --git a/src/Middleware/MiddlewareRegistryInterface.php b/src/Middleware/MiddlewareRegistryInterface.php index f09890bc..d1c8049f 100644 --- a/src/Middleware/MiddlewareRegistryInterface.php +++ b/src/Middleware/MiddlewareRegistryInterface.php @@ -44,6 +44,11 @@ public function walk(Closure $func): MiddlewareRegistryInterface; */ public function uasort(Closure $func): MiddlewareRegistryInterface; + /** + * Change the internal cursor of the middleware list, mainly used by {@see AbstractMiddlewareStack::runMiddleware()} to iterate over the list when an exception is thrown. + */ + public function next(): void; + /** * @return array */ diff --git a/src/Worker/Worker.php b/src/Worker/Worker.php index 1b08b5c8..90505747 100644 --- a/src/Worker/Worker.php +++ b/src/Worker/Worker.php @@ -292,7 +292,10 @@ protected function handleTask(TaskInterface $task, TaskListInterface $taskList): $this->taskExecutionTracker->endTracking(task: $task); $task->setExecutionEndTime(dateTimeImmutable: new DateTimeImmutable()); + $task->setLastExecution(dateTimeImmutable: new DateTimeImmutable()); $this->defineTaskExecutionState(task: $task, output: $output); + + $this->middlewareStack->runPostExecutionMiddleware(task: $task, worker: $this); $this->eventDispatcher->dispatch(new TaskExecutedEvent(task: $task, output: $output)); $this->configuration->setLastExecutedTask(lastExecutedTask: $task); @@ -304,12 +307,9 @@ protected function handleTask(TaskInterface $task, TaskListInterface $taskList): $this->getFailedTasks()->add(task: $failedTask); $this->eventDispatcher->dispatch(event: new TaskFailedEvent(task: $failedTask)); } finally { - $task->setLastExecution(dateTimeImmutable: new DateTimeImmutable()); - $this->configuration->setCurrentlyExecutedTask(task: null); $this->configuration->run(isRunning: false); - $this->middlewareStack->runPostExecutionMiddleware(task: $task, worker: $this); $this->eventDispatcher->dispatch(event: new WorkerRunningEvent(worker: $this, isIdle: true)); } } diff --git a/tests/Middleware/WorkerMiddlewareStackTest.php b/tests/Middleware/WorkerMiddlewareStackTest.php index c6d9b130..89f99b52 100644 --- a/tests/Middleware/WorkerMiddlewareStackTest.php +++ b/tests/Middleware/WorkerMiddlewareStackTest.php @@ -9,6 +9,7 @@ use SchedulerBundle\Middleware\PostExecutionMiddlewareInterface; use SchedulerBundle\Middleware\PreExecutionMiddlewareInterface; use SchedulerBundle\Middleware\WorkerMiddlewareStack; +use SchedulerBundle\Task\NullTask; use SchedulerBundle\Task\TaskInterface; use SchedulerBundle\Worker\WorkerInterface; use Throwable; @@ -23,7 +24,7 @@ final class WorkerMiddlewareStackTest extends TestCase */ public function testStackCanRunEmptyPreMiddlewareList(): void { - $task = $this->createMock(TaskInterface::class); + $task = new NullTask(name: 'foo'); $middleware = $this->createMock(PostExecutionMiddlewareInterface::class); $middleware->expects(self::never())->method('postExecute')->with($task); @@ -40,7 +41,7 @@ public function testStackCanRunEmptyPreMiddlewareList(): void */ public function testStackCanRunPreMiddlewareList(): void { - $task = $this->createMock(TaskInterface::class); + $task = new NullTask(name: 'foo'); $middleware = $this->createMock(PreExecutionMiddlewareInterface::class); $middleware->expects(self::once())->method('preExecute')->with($task); @@ -62,7 +63,7 @@ public function testStackCanRunPreMiddlewareList(): void public function testStackCanRunEmptyPostMiddlewareList(): void { $worker = $this->createMock(WorkerInterface::class); - $task = $this->createMock(TaskInterface::class); + $task = new NullTask(name: 'foo'); $middleware = $this->createMock(PreExecutionMiddlewareInterface::class); $middleware->expects(self::never())->method('preExecute')->with($task); diff --git a/tests/Worker/ExecutionPolicy/ExecutionPolicyRegistryTest.php b/tests/Worker/ExecutionPolicy/ExecutionPolicyRegistryTest.php index d60ad777..a198cac4 100644 --- a/tests/Worker/ExecutionPolicy/ExecutionPolicyRegistryTest.php +++ b/tests/Worker/ExecutionPolicy/ExecutionPolicyRegistryTest.php @@ -16,14 +16,14 @@ final class ExecutionPolicyRegistryTest extends TestCase { public function testRegistryCanCount(): void { - $registry = new ExecutionPolicyRegistry([]); + $registry = new ExecutionPolicyRegistry(policies: []); self::assertCount(0, $registry); } public function testRegistryCannotReturnInvalidPolicy(): void { - $registry = new ExecutionPolicyRegistry([ + $registry = new ExecutionPolicyRegistry(policies: [ new DefaultPolicy(), ]); self::assertCount(1, $registry); @@ -36,7 +36,7 @@ public function testRegistryCannotReturnInvalidPolicy(): void public function testRegistryCannotReturnMultiplePolicies(): void { - $registry = new ExecutionPolicyRegistry([ + $registry = new ExecutionPolicyRegistry(policies: [ new DefaultPolicy(), new DefaultPolicy(), ]); @@ -50,12 +50,12 @@ public function testRegistryCannotReturnMultiplePolicies(): void public function testRegistryCanReturnPolicy(): void { - $registry = new ExecutionPolicyRegistry([ + $registry = new ExecutionPolicyRegistry(policies: [ new DefaultPolicy(), ]); self::assertCount(1, $registry); - $policy = $registry->find('default'); + $policy = $registry->find(policy: 'default'); self::assertInstanceOf(DefaultPolicy::class, $policy); } } diff --git a/tests/Worker/WorkerTest.php b/tests/Worker/WorkerTest.php index a27d6cb7..24c06e90 100644 --- a/tests/Worker/WorkerTest.php +++ b/tests/Worker/WorkerTest.php @@ -1367,7 +1367,7 @@ public function testWorkerCanRetrieveTasksLazily(): void ]), $eventDispatcher, $lockFactory, $logger); $configuration = WorkerConfiguration::create(); - $configuration->mustRetrieveTasksLazily(true); + $configuration->mustRetrieveTasksLazily(mustRetrieveTasksLazily: true); $worker->execute($configuration); From de4988fd24f68bf2d76993a8b8111f6e775af465 Mon Sep 17 00:00:00 2001 From: Guillaume Loulier Date: Mon, 5 Sep 2022 18:02:44 +0200 Subject: [PATCH 3/3] refactor(scheduler): improvements --- src/Middleware/SchedulerMiddlewareStack.php | 7 +++++-- tests/SchedulerTest.php | 8 ++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/Middleware/SchedulerMiddlewareStack.php b/src/Middleware/SchedulerMiddlewareStack.php index 21f3bc6d..eec97caf 100644 --- a/src/Middleware/SchedulerMiddlewareStack.php +++ b/src/Middleware/SchedulerMiddlewareStack.php @@ -41,9 +41,12 @@ public function runPostSchedulingMiddleware(TaskInterface $task, SchedulerInterf */ public function getMiddlewareList(): array { + $preSchedulingMiddlewareList = $this->getPreSchedulingMiddleware(); + $postSchedulingMiddlewareList = $this->getPostSchedulingMiddleware(); + return array_unique(array: [ - ...$this->getPreSchedulingMiddleware()->toArray(), - ...$this->getPostSchedulingMiddleware()->toArray(), + ...$preSchedulingMiddlewareList->toArray(), + ...$postSchedulingMiddlewareList->toArray(), ], flags: SORT_REGULAR); } } diff --git a/tests/SchedulerTest.php b/tests/SchedulerTest.php index 1cf67289..d5e7ece0 100644 --- a/tests/SchedulerTest.php +++ b/tests/SchedulerTest.php @@ -112,10 +112,10 @@ public function testSchedulerCannotScheduleTasksWithErroredBeforeCallback(): voi new TaskCallbackMiddleware(), ])), new EventDispatcher()); - self::expectException(RuntimeException::class); - self::expectExceptionMessage('The task cannot be scheduled'); - self::expectExceptionCode(0); - $scheduler->schedule(new NullTask('foo', [ + self::expectException(exception: RuntimeException::class); + self::expectExceptionMessage(message: 'The task cannot be scheduled'); + self::expectExceptionCode(code: 0); + $scheduler->schedule(task: new NullTask(name: 'foo', options: [ 'before_scheduling' => static fn (): bool => false, ])); }