From 1ceabf3bc3fee29cc568a21b350486ed3e26e326 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 17 Nov 2018 14:10:38 +0100 Subject: [PATCH] Added locking capabilities to process visits command --- composer.json | 1 + config/autoload/locks.global.php | 25 ++++ data/locks/.gitignore | 2 + module/CLI/config/dependencies.config.php | 2 + .../Command/Visit/ProcessVisitsCommand.php | 122 +++++++++++------- .../Visit/ProcessVisitsCommandTest.php | 42 ++++++ 6 files changed, 149 insertions(+), 45 deletions(-) create mode 100644 config/autoload/locks.global.php create mode 100644 data/locks/.gitignore diff --git a/composer.json b/composer.json index 87d7cabc..1f5db2ec 100644 --- a/composer.json +++ b/composer.json @@ -32,6 +32,7 @@ "roave/security-advisories": "dev-master", "symfony/console": "^4.1", "symfony/filesystem": "^4.1", + "symfony/lock": "^4.1", "symfony/process": "^4.1", "theorchard/monolog-cascade": "^0.4", "zendframework/zend-config": "^3.0", diff --git a/config/autoload/locks.global.php b/config/autoload/locks.global.php new file mode 100644 index 00000000..7deb06c7 --- /dev/null +++ b/config/autoload/locks.global.php @@ -0,0 +1,25 @@ + [ + 'locks_dir' => __DIR__ . '/../../data/locks', + ], + + 'dependencies' => [ + 'factories' => [ + Lock\Store\FlockStore::class => ConfigAbstractFactory::class, + Lock\Factory::class => ConfigAbstractFactory::class, + ], + ], + + ConfigAbstractFactory::class => [ + Lock\Store\FlockStore::class => ['config.locks.locks_dir'], + Lock\Factory::class => [Lock\Store\FlockStore::class], + ], + +]; diff --git a/data/locks/.gitignore b/data/locks/.gitignore new file mode 100644 index 00000000..d6b7ef32 --- /dev/null +++ b/data/locks/.gitignore @@ -0,0 +1,2 @@ +* +!.gitignore diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 8a31cb02..2781e115 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -9,6 +9,7 @@ use Shlinkio\Shlink\Common\Service\PreviewGenerator; use Shlinkio\Shlink\Core\Service; use Shlinkio\Shlink\Rest\Service\ApiKeyService; use Symfony\Component\Console\Application; +use Symfony\Component\Lock; use Zend\I18n\Translator\Translator; use Zend\ServiceManager\AbstractFactory\ConfigAbstractFactory; @@ -68,6 +69,7 @@ return [ Command\Visit\ProcessVisitsCommand::class => [ Service\VisitService::class, IpLocationResolverInterface::class, + Lock\Factory::class, 'translator', ], Command\Visit\UpdateDbCommand::class => [DbUpdater::class, 'translator'], diff --git a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php index 38dff527..f6a6290b 100644 --- a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php +++ b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php @@ -14,12 +14,14 @@ use Symfony\Component\Console\Command\Command; 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 Zend\I18n\Translator\TranslatorInterface; use function sprintf; class ProcessVisitsCommand extends Command { public const NAME = 'visit:process'; + private const LOCK_NAME = 'visit_process'; /** * @var VisitServiceInterface @@ -33,69 +35,99 @@ class ProcessVisitsCommand extends Command * @var TranslatorInterface */ private $translator; + /** + * @var Locker + */ + private $locker; + /** + * @var OutputInterface + */ + private $output; public function __construct( VisitServiceInterface $visitService, IpLocationResolverInterface $ipLocationResolver, + Locker $locker, TranslatorInterface $translator ) { $this->visitService = $visitService; $this->ipLocationResolver = $ipLocationResolver; $this->translator = $translator; + $this->locker = $locker; parent::__construct(); } protected function configure(): void { - $this->setName(self::NAME) - ->setDescription( - $this->translator->translate('Processes visits where location is not set yet') - ); + $this + ->setName(self::NAME) + ->setDescription($this->translator->translate('Processes visits where location is not set yet')); } protected function execute(InputInterface $input, OutputInterface $output): void { - $this->visitService->locateVisits(function (Visit $visit) use ($output) { - if (! $visit->hasRemoteAddr()) { - $output->writeln( - sprintf('%s', $this->translator->translate('Ignored visit with no IP address')), - OutputInterface::VERBOSITY_VERBOSE - ); - throw new IpCannotBeLocatedException('Ignored visit with no IP address'); - } - - $ipAddr = $visit->getRemoteAddr(); - $output->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); - if ($ipAddr === IpAddress::LOCALHOST) { - $output->writeln( - sprintf(' [%s]', $this->translator->translate('Ignored localhost address')) - ); - throw new IpCannotBeLocatedException('Ignored localhost address'); - } - - try { - return $this->ipLocationResolver->resolveIpLocation($ipAddr); - } catch (WrongIpException $e) { - $output->writeln( - sprintf( - ' [%s]', - $this->translator->translate('An error occurred while locating IP. Skipped') - ) - ); - if ($output->isVerbose()) { - $this->getApplication()->renderException($e, $output); - } - - throw new IpCannotBeLocatedException('An error occurred while locating IP', 0, $e); - } - }, function (VisitLocation $location) use ($output) { - $output->writeln(sprintf( - ' [' . $this->translator->translate('Address located at "%s"') . ']', - $location->getCountryName() - )); - }); - + $this->output = $output; $io = new SymfonyStyle($input, $output); - $io->success($this->translator->translate('Finished processing all IPs')); + + $lock = $this->locker->createLock(self::LOCK_NAME); + if (! $lock->acquire()) { + $io->warning(sprintf( + $this->translator->translate('There is already an instance of the "%s" command in execution'), + self::NAME + )); + return; + } + + try { + $this->visitService->locateVisits( + [$this, 'getGeolocationDataForVisit'], + function (VisitLocation $location) use ($output) { + $output->writeln(sprintf( + ' [' . $this->translator->translate('Address located at "%s"') . ']', + $location->getCountryName() + )); + } + ); + + $io->success($this->translator->translate('Finished processing all IPs')); + } finally { + $lock->release(); + } + } + + public function getGeolocationDataForVisit(Visit $visit): array + { + if (! $visit->hasRemoteAddr()) { + $this->output->writeln(sprintf( + '%s', + $this->translator->translate('Ignored visit with no IP address') + ), OutputInterface::VERBOSITY_VERBOSE); + throw new IpCannotBeLocatedException('Ignored visit with no IP address'); + } + + $ipAddr = $visit->getRemoteAddr(); + $this->output->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); + if ($ipAddr === IpAddress::LOCALHOST) { + $this->output->writeln( + sprintf(' [%s]', $this->translator->translate('Ignored localhost address')) + ); + throw new IpCannotBeLocatedException('Ignored localhost address'); + } + + try { + return $this->ipLocationResolver->resolveIpLocation($ipAddr); + } catch (WrongIpException $e) { + $this->output->writeln( + sprintf( + ' [%s]', + $this->translator->translate('An error occurred while locating IP. Skipped') + ) + ); + if ($this->output->isVerbose()) { + $this->getApplication()->renderException($e, $this->output); + } + + throw new IpCannotBeLocatedException('An error occurred while locating IP', 0, $e); + } } } diff --git a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php index efa77752..9c691492 100644 --- a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php @@ -19,9 +19,11 @@ use Shlinkio\Shlink\Core\Service\VisitService; use Symfony\Component\Console\Application; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Tester\CommandTester; +use Symfony\Component\Lock; use Throwable; use Zend\I18n\Translator\Translator; use function array_shift; +use function sprintf; class ProcessVisitsCommandTest extends TestCase { @@ -37,15 +39,31 @@ class ProcessVisitsCommandTest extends TestCase * @var ObjectProphecy */ private $ipResolver; + /** + * @var ObjectProphecy + */ + private $locker; + /** + * @var ObjectProphecy + */ + private $lock; public function setUp() { $this->visitService = $this->prophesize(VisitService::class); $this->ipResolver = $this->prophesize(IpApiLocationResolver::class); + $this->locker = $this->prophesize(Lock\Factory::class); + $this->lock = $this->prophesize(Lock\LockInterface::class); + $this->lock->acquire()->willReturn(true); + $this->lock->release()->will(function () { + }); + $this->locker->createLock(Argument::type('string'))->willReturn($this->lock->reveal()); + $command = new ProcessVisitsCommand( $this->visitService->reveal(), $this->ipResolver->reveal(), + $this->locker->reveal(), Translator::factory([]) ); $app = new Application(); @@ -160,4 +178,28 @@ class ProcessVisitsCommandTest extends TestCase $resolveIpLocation->shouldHaveBeenCalledOnce(); } } + + /** + * @test + */ + public function noActionIsPerformedIfLockIsAcquired() + { + $this->lock->acquire()->willReturn(false); + + $locateVisits = $this->visitService->locateVisits(Argument::cetera())->will(function () { + }); + $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]); + + $this->commandTester->execute([ + 'command' => 'visit:process', + ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); + $output = $this->commandTester->getDisplay(); + + $this->assertContains( + sprintf('There is already an instance of the "%s" command', ProcessVisitsCommand::NAME), + $output + ); + $locateVisits->shouldNotHaveBeenCalled(); + $resolveIpLocation->shouldNotHaveBeenCalled(); + } }