Moved locking logic for CLI commands to a common abstract class

composer.json

@@ -31,41 +31,41 @@
         "monolog/monolog": "^1.21",
         "ocramius/proxy-manager": "^2.0",
         "phly/phly-event-dispatcher": "^1.0",
-        "shlinkio/shlink-installer": "^1.1",
+        "shlinkio/shlink-installer": "^1.2",
-        "symfony/console": "^4.2",
+        "symfony/console": "^4.3",
-        "symfony/filesystem": "^4.2",
+        "symfony/filesystem": "^4.3",
-        "symfony/lock": "^4.2",
+        "symfony/lock": "^4.3",
-        "symfony/process": "^4.2",
+        "symfony/process": "^4.3",
         "theorchard/monolog-cascade": "^0.4",
-        "zendframework/zend-config": "^3.0",
+        "zendframework/zend-config": "^3.3",
-        "zendframework/zend-config-aggregator": "^1.0",
+        "zendframework/zend-config-aggregator": "^1.1",
-        "zendframework/zend-diactoros": "^2.1.1",
+        "zendframework/zend-diactoros": "^2.1.3",
-        "zendframework/zend-expressive": "^3.0",
+        "zendframework/zend-expressive": "^3.2",
         "zendframework/zend-expressive-fastroute": "^3.0",
-        "zendframework/zend-expressive-helpers": "^5.0",
+        "zendframework/zend-expressive-helpers": "^5.3",
-        "zendframework/zend-expressive-platesrenderer": "^2.0",
+        "zendframework/zend-expressive-platesrenderer": "^2.1",
         "zendframework/zend-expressive-swoole": "^2.4",
-        "zendframework/zend-i18n": "^2.7",
+        "zendframework/zend-i18n": "^2.9",
-        "zendframework/zend-inputfilter": "^2.8",
+        "zendframework/zend-inputfilter": "^2.10",
-        "zendframework/zend-paginator": "^2.6",
+        "zendframework/zend-paginator": "^2.8",
-        "zendframework/zend-servicemanager": "^3.2",
+        "zendframework/zend-servicemanager": "^3.4",
-        "zendframework/zend-stdlib": "^3.0"
+        "zendframework/zend-stdlib": "^3.2"
     },
     "require-dev": {
         "devster/ubench": "^2.0",
         "doctrine/data-fixtures": "^1.3",
         "eaglewu/swoole-ide-helper": "dev-master",
-        "filp/whoops": "^2.0",
+        "filp/whoops": "^2.4",
         "infection/infection": "^0.12.2",
         "phpstan/phpstan": "^0.11.2",
         "phpunit/phpcov": "^6.0",
-        "phpunit/phpunit": "^8.0",
+        "phpunit/phpunit": "^8.3",
         "roave/security-advisories": "dev-master",
         "shlinkio/php-coding-standard": "~1.2.2",
-        "symfony/dotenv": "^4.2",
+        "symfony/dotenv": "^4.3",
-        "symfony/var-dumper": "^4.2",
+        "symfony/var-dumper": "^4.3",
         "zendframework/zend-component-installer": "^2.1",
-        "zendframework/zend-expressive-tooling": "^1.0"
+        "zendframework/zend-expressive-tooling": "^1.2"
     },
     "autoload": {
         "psr-4": {

module/CLI/src/Command/Util/AbstractLockedCommand.php

@@ -0,0 +1,47 @@
+<?php
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\CLI\Command\Util;
+
+use Shlinkio\Shlink\CLI\Util\ExitCodes;
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Lock\Factory as Locker;
+
+use function sprintf;
+
+abstract class AbstractLockedCommand extends Command
+{
+    /** @var Locker */
+    private $locker;
+
+    public function __construct(Locker $locker)
+    {
+        parent::__construct();
+        $this->locker = $locker;
+    }
+
+    final protected function execute(InputInterface $input, OutputInterface $output): ?int
+    {
+        $lockConfig = $this->getLockConfig();
+        $lock = $this->locker->createLock($lockConfig->lockName(), $lockConfig->ttl(), $lockConfig->isBlocking());
+
+        if (! $lock->acquire($lockConfig->isBlocking())) {
+            $output->writeln(
+                sprintf('<comment>Command "%s" is already in progress. Skipping.</comment>', $lockConfig->lockName())
+            );
+            return ExitCodes::EXIT_WARNING;
+        }
+
+        try {
+            return $this->lockedExecute($input, $output);
+        } finally {
+            $lock->release();
+        }
+    }
+
+    abstract protected function lockedExecute(InputInterface $input, OutputInterface $output): int;
+
+    abstract protected function getLockConfig(): LockedCommandConfig;
+}

module/CLI/src/Command/Util/LockedCommandConfig.php

@@ -0,0 +1,38 @@
+<?php
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\CLI\Command\Util;
+
+final class LockedCommandConfig
+{
+    private const DEFAULT_TTL = 90.0; // 1.5 minutes
+
+    /** @var string */
+    private $lockName;
+    /** @var bool */
+    private $isBlocking;
+    /** @var float */
+    private $ttl;
+
+    public function __construct(string $lockName, bool $isBlocking = false, float $ttl = self::DEFAULT_TTL)
+    {
+        $this->lockName = $lockName;
+        $this->isBlocking = $isBlocking;
+        $this->ttl = $ttl;
+    }
+
+    public function lockName(): string
+    {
+        return $this->lockName;
+    }
+
+    public function isBlocking(): bool
+    {
+        return $this->isBlocking;
+    }
+
+    public function ttl(): float
+    {
+        return $this->ttl;
+    }
+}

module/CLI/src/Command/Visit/LocateVisitsCommand.php

@@ -4,6 +4,8 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\CLI\Command\Visit;
 
 use Exception;
+use Shlinkio\Shlink\CLI\Command\Util\AbstractLockedCommand;
+use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig;
 use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException;
 use Shlinkio\Shlink\CLI\Util\ExitCodes;
 use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdaterInterface;
@@ -15,16 +17,16 @@ use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Entity\VisitLocation;
 use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException;
 use Shlinkio\Shlink\Core\Service\VisitServiceInterface;
-use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Helper\ProgressBar;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Style\SymfonyStyle;
 use Symfony\Component\Lock\Factory as Locker;
+use Throwable;
 
 use function sprintf;
 
-class LocateVisitsCommand extends Command
+class LocateVisitsCommand extends AbstractLockedCommand
 {
     public const NAME = 'visit:locate';
     public const ALIASES = ['visit:process'];
@@ -33,8 +35,6 @@ class LocateVisitsCommand extends Command
     private $visitService;
     /** @var IpLocationResolverInterface */
     private $ipLocationResolver;
-    /** @var Locker */
-    private $locker;
     /** @var GeolocationDbUpdaterInterface */
     private $dbUpdater;
 
@@ -49,10 +49,9 @@ class LocateVisitsCommand extends Command
         Locker $locker,
         GeolocationDbUpdaterInterface $dbUpdater
     ) {
-        parent::__construct();
+        parent::__construct($locker);
         $this->visitService = $visitService;
         $this->ipLocationResolver = $ipLocationResolver;
-        $this->locker = $locker;
         $this->dbUpdater = $dbUpdater;
     }
 
@@ -64,16 +63,10 @@ class LocateVisitsCommand extends Command
             ->setDescription('Resolves visits origin locations.');
     }
 
-    protected function execute(InputInterface $input, OutputInterface $output): ?int
+    protected function lockedExecute(InputInterface $input, OutputInterface $output): int
     {
         $this->io = new SymfonyStyle($input, $output);
 
-        $lock = $this->locker->createLock(self::NAME);
-        if (! $lock->acquire()) {
-            $this->io->warning(sprintf('There is already an instance of the "%s" command in execution', self::NAME));
-            return ExitCodes::EXIT_WARNING;
-        }
-
         try {
             $this->checkDbUpdate();
 
@@ -90,15 +83,13 @@ class LocateVisitsCommand extends Command
 
             $this->io->success('Finished processing all IPs');
             return ExitCodes::EXIT_SUCCESS;
-        } catch (Exception $e) {
+        } catch (Throwable $e) {
             $this->io->error($e->getMessage());
-            if ($this->io->isVerbose()) {
+            if ($e instanceof Exception && $this->io->isVerbose()) {
                 $this->getApplication()->renderException($e, $this->io);
             }
 
             return ExitCodes::EXIT_FAILURE;
-        } finally {
-            $lock->release();
         }
     }
 
@@ -160,4 +151,9 @@ class LocateVisitsCommand extends Command
             );
         }
     }
+
+    protected function getLockConfig(): LockedCommandConfig
+    {
+        return new LockedCommandConfig($this->getName());
+    }
 }

module/CLI/src/Factory/ApplicationFactory.php

@@ -4,32 +4,13 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\CLI\Factory;
 
 use Interop\Container\ContainerInterface;
-use Interop\Container\Exception\ContainerException;
-use Psr\Container\ContainerExceptionInterface;
-use Psr\Container\NotFoundExceptionInterface;
 use Shlinkio\Shlink\Core\Options\AppOptions;
 use Symfony\Component\Console\Application as CliApp;
 use Symfony\Component\Console\CommandLoader\ContainerCommandLoader;
-use Zend\ServiceManager\Exception\ServiceNotCreatedException;
-use Zend\ServiceManager\Exception\ServiceNotFoundException;
-use Zend\ServiceManager\Factory\FactoryInterface;
 
-class ApplicationFactory implements FactoryInterface
+class ApplicationFactory
 {
-    /**
+    public function __invoke(ContainerInterface $container): CliApp
-     * Create an object
-     *
-     * @param  ContainerInterface $container
-     * @param  string $requestedName
-     * @param  null|array $options
-     * @return CliApp
-     * @throws NotFoundExceptionInterface
-     * @throws ContainerExceptionInterface
-     * @throws ServiceNotFoundException if unable to resolve the service.
-     * @throws ServiceNotCreatedException if an exception is raised when creating a service.
-     * @throws ContainerException if any other error occurs
-     */
-    public function __invoke(ContainerInterface $container, $requestedName, ?array $options = null): CliApp
     {
         $config = $container->get('config')['cli'];
         $appOptions = $container->get(AppOptions::class);

module/CLI/test/Command/Visit/LocateVisitsCommandTest.php

@@ -49,10 +49,10 @@ class LocateVisitsCommandTest extends TestCase
 
         $this->locker = $this->prophesize(Lock\Factory::class);
         $this->lock = $this->prophesize(Lock\LockInterface::class);
-        $this->lock->acquire()->willReturn(true);
+        $this->lock->acquire(false)->willReturn(true);
         $this->lock->release()->will(function () {
         });
-        $this->locker->createLock(Argument::type('string'))->willReturn($this->lock->reveal());
+        $this->locker->createLock(Argument::type('string'), 90.0, false)->willReturn($this->lock->reveal());
 
         $command = new LocateVisitsCommand(
             $this->visitService->reveal(),
@@ -162,9 +162,9 @@ class LocateVisitsCommandTest extends TestCase
     }
 
     /** @test */
-    public function noActionIsPerformedIfLockIsAcquired()
+    public function noActionIsPerformedIfLockIsAcquired(): void
     {
-        $this->lock->acquire()->willReturn(false);
+        $this->lock->acquire(false)->willReturn(false);
 
         $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will(function () {
         });
@@ -174,7 +174,7 @@ class LocateVisitsCommandTest extends TestCase
         $output = $this->commandTester->getDisplay();
 
         $this->assertStringContainsString(
-            sprintf('There is already an instance of the "%s" command', LocateVisitsCommand::NAME),
+            sprintf('Command "%s" is already in progress. Skipping.', LocateVisitsCommand::NAME),
             $output
         );
         $locateVisits->shouldNotHaveBeenCalled();

module/CLI/test/Factory/ApplicationFactoryTest.php

@@ -25,14 +25,7 @@ class ApplicationFactoryTest extends TestCase
     }
 
     /** @test */
-    public function serviceIsCreated()
+    public function allCommandsWhichAreServicesAreAdded(): void
-    {
-        $instance = ($this->factory)($this->createServiceManager(), '');
-        $this->assertInstanceOf(Application::class, $instance);
-    }
-
-    /** @test */
-    public function allCommandsWhichAreServicesAreAdded()
     {
         $sm = $this->createServiceManager([
             'commands' => [
@@ -45,8 +38,7 @@ class ApplicationFactoryTest extends TestCase
         $sm->setService('bar', $this->createCommandMock('bar')->reveal());
 
         /** @var Application $instance */
-        $instance = ($this->factory)($sm, '');
+        $instance = ($this->factory)($sm);
-        $this->assertInstanceOf(Application::class, $instance);
 
         $this->assertTrue($instance->has('foo'));
         $this->assertTrue($instance->has('bar'));