From 1bdb01ebff7a806e02e29c4e73adaec2536110e3 Mon Sep 17 00:00:00 2001 From: Christoph Reiter <reiter.christoph@gmail.com> Date: Tue, 14 Feb 2023 11:51:29 +0100 Subject: [PATCH] cron: Move the cron job handling into its own service splitting it out of the cron command. Makes it a bit clearer --- src/Cron/CronCommand.php | 97 +-------------- src/Cron/CronCompilerPass.php | 4 +- src/Cron/CronManager.php | 112 ++++++++++++++++++ .../DbpRelayCoreExtension.php | 3 +- src/Resources/config/services.yaml | 4 + tests/Cron/CronTest.php | 10 +- 6 files changed, 130 insertions(+), 100 deletions(-) create mode 100644 src/Cron/CronManager.php diff --git a/src/Cron/CronCommand.php b/src/Cron/CronCommand.php index 593ee4c..cf5b996 100644 --- a/src/Cron/CronCommand.php +++ b/src/Cron/CronCommand.php @@ -4,9 +4,7 @@ declare(strict_types=1); namespace Dbp\Relay\CoreBundle\Cron; -use Cron\CronExpression; use Dbp\Relay\CoreBundle\Cron\CronJobs\CachePrune; -use Psr\Cache\CacheItemPoolInterface; use Psr\Log\LoggerAwareInterface; use Psr\Log\LoggerAwareTrait; use Psr\Log\NullLogger; @@ -21,29 +19,16 @@ final class CronCommand extends Command implements LoggerAwareInterface // dbp:cron only for backwards compat protected static $defaultName = 'dbp:relay:core:cron|dbp:cron'; - /** @var CacheItemPoolInterface */ - private $cachePool; - /** - * @var CronJobInterface[] + * @var CronManager */ - private $jobs; + private $manager; - public function __construct() + public function __construct(CronManager $manager) { parent::__construct(); - $this->jobs = []; $this->logger = new NullLogger(); - } - - public function setCache(CacheItemPoolInterface $cachePool) - { - $this->cachePool = $cachePool; - } - - public function addJob(CronJobInterface $job) - { - $this->jobs[] = $job; + $this->manager = $manager; } protected function configure() @@ -51,78 +36,6 @@ final class CronCommand extends Command implements LoggerAwareInterface $this->setDescription('Runs various tasks which need to be executed periodically'); } - /** - * Returns if a job should run or not. Note that there is no feedback channel, so if you skip - * this run you will only be notified the next time the cron job should run. - * - * @param string $cronExpression A cron expression - * - * @return bool If the job should run - */ - public static function isDue(?\DateTimeInterface $previousRun, \DateTimeInterface $currentRun, string $cronExpression): bool - { - $cron = new CronExpression($cronExpression); - $previousExpectedRun = $cron->getPreviousRunDate($currentRun, 0, true); - $previousExpectedRun->setTimezone(new \DateTimeZone('UTC')); - - $shouldRun = false; - // If we were scheduled to run between now and the previous run (or just before of no previous run exists) - // then we should run - if ($previousExpectedRun <= $currentRun && ($previousRun === null || $previousExpectedRun > $previousRun)) { - $shouldRun = true; - } - - return $shouldRun; - } - - public function getPreviousRun(\DateTimeInterface $currentTime): ?\DateTimeInterface - { - $cachePool = $this->cachePool; - // Store the previous run time in the cache and fetch from there - assert($cachePool instanceof CacheItemPoolInterface); - $item = $cachePool->getItem('cron-previous-run'); - $value = $item->get(); - $previousRun = null; - if ($value !== null) { - $previousRun = (new \DateTimeImmutable())->setTimezone(new \DateTimeZone('UTC'))->setTimestamp($value); - if ($previousRun > $currentTime) { - // Something is wrong, cap at the current time - $previousRun = $currentTime; - } - } - $item->set($currentTime->getTimestamp()); - if ($cachePool->save($item) === false) { - throw new \RuntimeException('Saving cron timestamp failed'); - } - - return $previousRun; - } - - /** - * @return CronJobInterface[] - */ - protected function getDueJobs(): array - { - // Get all jobs that should have been run between the last time we were called and now - $currentTime = new \DateTimeImmutable('now', new \DateTimeZone('UTC')); - // round to full seconds, so we have the same resolution for both date times - $currentTime = $currentTime->setTimestamp($currentTime->getTimestamp()); - $previousRunTime = $this->getPreviousRun($currentTime); - - $toRun = []; - foreach ($this->jobs as $job) { - $interval = $job->getInterval(); - $name = $job->getName(); - $this->logger->info("cron: Checking '$name' ($interval)"); - $isDue = self::isDue($previousRunTime, $currentTime, $interval); - if ($isDue) { - $toRun[] = $job; - } - } - - return $toRun; - } - protected function execute(InputInterface $input, OutputInterface $output): int { // We need to pass the prune command to CachePrune since I didn't find an alternative @@ -132,7 +45,7 @@ final class CronCommand extends Command implements LoggerAwareInterface CachePrune::setPruneCommand($command); // Now run all jobs - $dueJobs = $this->getDueJobs(); + $dueJobs = $this->manager->getDueJobs(); foreach ($dueJobs as $job) { $name = $job->getName(); $this->logger->info("cron: Running '$name'"); diff --git a/src/Cron/CronCompilerPass.php b/src/Cron/CronCompilerPass.php index 9dc60c1..59f36d8 100644 --- a/src/Cron/CronCompilerPass.php +++ b/src/Cron/CronCompilerPass.php @@ -20,10 +20,10 @@ class CronCompilerPass implements CompilerPassInterface public function process(ContainerBuilder $container) { - if (!$container->has(CronCommand::class)) { + if (!$container->has(CronManager::class)) { return; } - $definition = $container->findDefinition(CronCommand::class); + $definition = $container->findDefinition(CronManager::class); $taggedServices = $container->findTaggedServiceIds(self::TAG); foreach ($taggedServices as $id => $tags) { $definition->addMethodCall('addJob', [new Reference($id)]); diff --git a/src/Cron/CronManager.php b/src/Cron/CronManager.php new file mode 100644 index 0000000..40ef666 --- /dev/null +++ b/src/Cron/CronManager.php @@ -0,0 +1,112 @@ +<?php + +declare(strict_types=1); + +namespace Dbp\Relay\CoreBundle\Cron; + +use Cron\CronExpression; +use Psr\Cache\CacheItemPoolInterface; +use Psr\Log\LoggerAwareInterface; +use Psr\Log\LoggerAwareTrait; +use Psr\Log\NullLogger; + +final class CronManager implements LoggerAwareInterface +{ + use LoggerAwareTrait; + + /** @var CacheItemPoolInterface */ + private $cachePool; + + /** + * @var CronJobInterface[] + */ + private $jobs; + + public function __construct() + { + $this->jobs = []; + $this->logger = new NullLogger(); + } + + public function setCache(CacheItemPoolInterface $cachePool) + { + $this->cachePool = $cachePool; + } + + public function addJob(CronJobInterface $job) + { + $this->jobs[] = $job; + } + + /** + * Returns if a job should run or not. Note that there is no feedback channel, so if you skip + * this run you will only be notified the next time the cron job should run. + * + * @param string $cronExpression A cron expression + * + * @return bool If the job should run + */ + public static function isDue(?\DateTimeInterface $previousRun, \DateTimeInterface $currentRun, string $cronExpression): bool + { + $cron = new CronExpression($cronExpression); + $previousExpectedRun = $cron->getPreviousRunDate($currentRun, 0, true); + $previousExpectedRun->setTimezone(new \DateTimeZone('UTC')); + + $shouldRun = false; + // If we were scheduled to run between now and the previous run (or just before of no previous run exists) + // then we should run + if ($previousExpectedRun <= $currentRun && ($previousRun === null || $previousExpectedRun > $previousRun)) { + $shouldRun = true; + } + + return $shouldRun; + } + + public function getPreviousRun(\DateTimeInterface $currentTime): ?\DateTimeInterface + { + $cachePool = $this->cachePool; + // Store the previous run time in the cache and fetch from there + assert($cachePool instanceof CacheItemPoolInterface); + $item = $cachePool->getItem('cron-previous-run'); + $value = $item->get(); + $previousRun = null; + if ($value !== null) { + $previousRun = (new \DateTimeImmutable())->setTimezone(new \DateTimeZone('UTC'))->setTimestamp($value); + if ($previousRun > $currentTime) { + // Something is wrong, cap at the current time + $previousRun = $currentTime; + } + } + $item->set($currentTime->getTimestamp()); + if ($cachePool->save($item) === false) { + throw new \RuntimeException('Saving cron timestamp failed'); + } + + return $previousRun; + } + + /** + * @return CronJobInterface[] + */ + public function getDueJobs(): array + { + // Get all jobs that should have been run between the last time we were called and now + $currentTime = new \DateTimeImmutable('now', new \DateTimeZone('UTC')); + // round to full seconds, so we have the same resolution for both date times + $currentTime = $currentTime->setTimestamp($currentTime->getTimestamp()); + $previousRunTime = $this->getPreviousRun($currentTime); + + $toRun = []; + foreach ($this->jobs as $job) { + $interval = $job->getInterval(); + $name = $job->getName(); + $this->logger->info("cron: Checking '$name' ($interval)"); + $isDue = self::isDue($previousRunTime, $currentTime, $interval); + if ($isDue) { + $toRun[] = $job; + } + } + + return $toRun; + } +} diff --git a/src/DependencyInjection/DbpRelayCoreExtension.php b/src/DependencyInjection/DbpRelayCoreExtension.php index cd51b7f..10ad237 100644 --- a/src/DependencyInjection/DbpRelayCoreExtension.php +++ b/src/DependencyInjection/DbpRelayCoreExtension.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Dbp\Relay\CoreBundle\DependencyInjection; use Dbp\Relay\CoreBundle\Auth\ProxyAuthenticator; +use Dbp\Relay\CoreBundle\Cron\CronManager; use Dbp\Relay\CoreBundle\DB\MigrateCommand; use Dbp\Relay\CoreBundle\Queue\TestMessage; use Dbp\Relay\CoreBundle\Queue\Utils as QueueUtils; @@ -36,7 +37,7 @@ class DbpRelayCoreExtension extends ConfigurableExtension implements PrependExte $cronCacheDef->setArguments(['core-cron', 0, '%kernel.cache_dir%/dbp/relay/core-cron']); $cronCacheDef->addTag('cache.pool'); - $definition = $container->getDefinition('Dbp\Relay\CoreBundle\Cron\CronCommand'); + $definition = $container->getDefinition(CronManager::class); $definition->addMethodCall('setCache', [$cronCacheDef]); $definition = $container->getDefinition(MigrateCommand::class); diff --git a/src/Resources/config/services.yaml b/src/Resources/config/services.yaml index 7f13c44..ac806d6 100644 --- a/src/Resources/config/services.yaml +++ b/src/Resources/config/services.yaml @@ -14,6 +14,10 @@ services: autowire: true autoconfigure: true + Dbp\Relay\CoreBundle\Cron\CronManager: + autowire: true + autoconfigure: true + Dbp\Relay\CoreBundle\Cron\CronJobs\: resource: '../../Cron/CronJobs' autowire: true diff --git a/tests/Cron/CronTest.php b/tests/Cron/CronTest.php index c11af7a..10fd497 100644 --- a/tests/Cron/CronTest.php +++ b/tests/Cron/CronTest.php @@ -4,20 +4,20 @@ declare(strict_types=1); namespace Dbp\Relay\CoreBundle\Tests\Cron; -use Dbp\Relay\CoreBundle\Cron\CronCommand; +use Dbp\Relay\CoreBundle\Cron\CronManager; use PHPUnit\Framework\TestCase; class CronTest extends TestCase { public function testCronisDue() { - $isDue = CronCommand::isDue(new \DateTimeImmutable('2021-09-07T09:36:26Z'), new \DateTimeImmutable('2021-09-07T09:36:26Z'), '* * * * *'); + $isDue = CronManager::isDue(new \DateTimeImmutable('2021-09-07T09:36:26Z'), new \DateTimeImmutable('2021-09-07T09:36:26Z'), '* * * * *'); $this->assertFalse($isDue); - $isDue = CronCommand::isDue(new \DateTimeImmutable('2021-09-07T09:35:59Z'), new \DateTimeImmutable('2021-09-07T09:36:00Z'), '* * * * *'); + $isDue = CronManager::isDue(new \DateTimeImmutable('2021-09-07T09:35:59Z'), new \DateTimeImmutable('2021-09-07T09:36:00Z'), '* * * * *'); $this->assertTrue($isDue); - $isDue = CronCommand::isDue(null, new \DateTimeImmutable('2021-09-07T09:36:00Z'), '0 0 1 1 *'); + $isDue = CronManager::isDue(null, new \DateTimeImmutable('2021-09-07T09:36:00Z'), '0 0 1 1 *'); $this->assertTrue($isDue); - $isDue = CronCommand::isDue(null, new \DateTimeImmutable('2021-09-07T09:36:00Z'), '* * * * *'); + $isDue = CronManager::isDue(null, new \DateTimeImmutable('2021-09-07T09:36:00Z'), '* * * * *'); $this->assertTrue($isDue); } } -- GitLab