From d932f0a2047892bad561ceabb2646ec093b2deee Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 12 Feb 2021 22:59:40 +0100 Subject: [PATCH 1/2] Increased timeout on db commands to 10 minutes --- module/CLI/config/dependencies.config.php | 7 ++- .../Command/Db/AbstractDatabaseCommand.php | 17 +++--- .../src/Command/Db/CreateDatabaseCommand.php | 6 +-- .../src/Command/Util/LockedCommandConfig.php | 14 ++++- .../src/Command/Visit/LocateVisitsCommand.php | 2 +- module/CLI/src/Util/ProcessRunner.php | 54 +++++++++++++++++++ .../CLI/src/Util/ProcessRunnerInterface.php | 12 +++++ .../Command/Db/CreateDatabaseCommandTest.php | 9 ++-- .../Command/Db/MigrateDatabaseCommandTest.php | 9 ++-- .../Command/Visit/LocateVisitsCommandTest.php | 2 +- phpstan.neon | 1 - 11 files changed, 106 insertions(+), 27 deletions(-) create mode 100644 module/CLI/src/Util/ProcessRunner.php create mode 100644 module/CLI/src/Util/ProcessRunnerInterface.php diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 7e224c33..80b26b8d 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -34,6 +34,8 @@ return [ PhpExecutableFinder::class => InvokableFactory::class, Util\GeolocationDbUpdater::class => ConfigAbstractFactory::class, + Util\ProcessRunner::class => ConfigAbstractFactory::class, + ApiKey\RoleResolver::class => ConfigAbstractFactory::class, Command\ShortUrl\GenerateShortUrlCommand::class => ConfigAbstractFactory::class, @@ -62,6 +64,7 @@ return [ ConfigAbstractFactory::class => [ Util\GeolocationDbUpdater::class => [DbUpdater::class, Reader::class, LOCAL_LOCK_FACTORY], + Util\ProcessRunner::class => [SymfonyCli\Helper\ProcessHelper::class], ApiKey\RoleResolver::class => [DomainService::class], Command\ShortUrl\GenerateShortUrlCommand::class => [ @@ -97,14 +100,14 @@ return [ Command\Db\CreateDatabaseCommand::class => [ LockFactory::class, - SymfonyCli\Helper\ProcessHelper::class, + Util\ProcessRunner::class, PhpExecutableFinder::class, Connection::class, NoDbNameConnectionFactory::SERVICE_NAME, ], Command\Db\MigrateDatabaseCommand::class => [ LockFactory::class, - SymfonyCli\Helper\ProcessHelper::class, + Util\ProcessRunner::class, PhpExecutableFinder::class, ], ], diff --git a/module/CLI/src/Command/Db/AbstractDatabaseCommand.php b/module/CLI/src/Command/Db/AbstractDatabaseCommand.php index 5e9374cf..e4515ab5 100644 --- a/module/CLI/src/Command/Db/AbstractDatabaseCommand.php +++ b/module/CLI/src/Command/Db/AbstractDatabaseCommand.php @@ -6,31 +6,34 @@ namespace Shlinkio\Shlink\CLI\Command\Db; use Shlinkio\Shlink\CLI\Command\Util\AbstractLockedCommand; use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig; -use Symfony\Component\Console\Helper\ProcessHelper; +use Shlinkio\Shlink\CLI\Util\ProcessRunnerInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Lock\LockFactory; use Symfony\Component\Process\PhpExecutableFinder; abstract class AbstractDatabaseCommand extends AbstractLockedCommand { - private ProcessHelper $processHelper; + private ProcessRunnerInterface $processRunner; private string $phpBinary; - public function __construct(LockFactory $locker, ProcessHelper $processHelper, PhpExecutableFinder $phpFinder) - { + public function __construct( + LockFactory $locker, + ProcessRunnerInterface $processRunner, + PhpExecutableFinder $phpFinder + ) { parent::__construct($locker); - $this->processHelper = $processHelper; + $this->processRunner = $processRunner; $this->phpBinary = $phpFinder->find(false) ?: 'php'; } protected function runPhpCommand(OutputInterface $output, array $command): void { $command = [$this->phpBinary, ...$command, '--no-interaction']; - $this->processHelper->mustRun($output, $command); + $this->processRunner->run($output, $command); } protected function getLockConfig(): LockedCommandConfig { - return new LockedCommandConfig($this->getName(), true); + return LockedCommandConfig::blocking($this->getName()); } } diff --git a/module/CLI/src/Command/Db/CreateDatabaseCommand.php b/module/CLI/src/Command/Db/CreateDatabaseCommand.php index b8e88688..ca68f818 100644 --- a/module/CLI/src/Command/Db/CreateDatabaseCommand.php +++ b/module/CLI/src/Command/Db/CreateDatabaseCommand.php @@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\CLI\Command\Db; use Doctrine\DBAL\Connection; use Shlinkio\Shlink\CLI\Util\ExitCodes; -use Symfony\Component\Console\Helper\ProcessHelper; +use Shlinkio\Shlink\CLI\Util\ProcessRunnerInterface; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; @@ -26,12 +26,12 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand public function __construct( LockFactory $locker, - ProcessHelper $processHelper, + ProcessRunnerInterface $processRunner, PhpExecutableFinder $phpFinder, Connection $conn, Connection $noDbNameConn ) { - parent::__construct($locker, $processHelper, $phpFinder); + parent::__construct($locker, $processRunner, $phpFinder); $this->regularConn = $conn; $this->noDbNameConn = $noDbNameConn; } diff --git a/module/CLI/src/Command/Util/LockedCommandConfig.php b/module/CLI/src/Command/Util/LockedCommandConfig.php index 8a217f85..8de204c5 100644 --- a/module/CLI/src/Command/Util/LockedCommandConfig.php +++ b/module/CLI/src/Command/Util/LockedCommandConfig.php @@ -6,19 +6,29 @@ namespace Shlinkio\Shlink\CLI\Command\Util; final class LockedCommandConfig { - private const DEFAULT_TTL = 90.0; // 1.5 minutes + public const DEFAULT_TTL = 600.0; // 10 minutes private string $lockName; private bool $isBlocking; private float $ttl; - public function __construct(string $lockName, bool $isBlocking = false, float $ttl = self::DEFAULT_TTL) + private function __construct(string $lockName, bool $isBlocking, float $ttl = self::DEFAULT_TTL) { $this->lockName = $lockName; $this->isBlocking = $isBlocking; $this->ttl = $ttl; } + public static function blocking(string $lockName): self + { + return new self($lockName, true); + } + + public static function nonBlocking(string $lockName): self + { + return new self($lockName, false); + } + public function lockName(): string { return $this->lockName; diff --git a/module/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index bf1ac14b..67678d4d 100644 --- a/module/CLI/src/Command/Visit/LocateVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -208,6 +208,6 @@ class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocat protected function getLockConfig(): LockedCommandConfig { - return new LockedCommandConfig($this->getName()); + return LockedCommandConfig::nonBlocking($this->getName()); } } diff --git a/module/CLI/src/Util/ProcessRunner.php b/module/CLI/src/Util/ProcessRunner.php new file mode 100644 index 00000000..4da5cb9f --- /dev/null +++ b/module/CLI/src/Util/ProcessRunner.php @@ -0,0 +1,54 @@ +helper = $helper; + } + + public function run(OutputInterface $output, array $cmd): void + { + if ($output instanceof ConsoleOutputInterface) { + $output = $output->getErrorOutput(); + } + + /** @var DebugFormatterHelper $formatter */ + $formatter = $this->helper->getHelperSet()->get('debug_formatter'); + $process = new Process($cmd, null, null, null, LockedCommandConfig::DEFAULT_TTL); + + if ($output->isVeryVerbose()) { + $output->write( + $formatter->start(spl_object_hash($process), str_replace('<', '\\<', $process->getCommandLine())), + ); + } + + $callback = $output->isDebug() ? $this->helper->wrapCallback($output, $process) : null; + $process->mustRun($callback); + + if ($output->isVeryVerbose()) { + $message = $process->isSuccessful() ? 'Command ran successfully' : sprintf( + '%s Command did not run successfully', + $process->getExitCode(), + ); + $output->write($formatter->stop(spl_object_hash($process), $message, $process->isSuccessful())); + } + } +} diff --git a/module/CLI/src/Util/ProcessRunnerInterface.php b/module/CLI/src/Util/ProcessRunnerInterface.php new file mode 100644 index 00000000..c00a4691 --- /dev/null +++ b/module/CLI/src/Util/ProcessRunnerInterface.php @@ -0,0 +1,12 @@ +prophesize(PhpExecutableFinder::class); $phpExecutableFinder->find(false)->willReturn('/usr/local/bin/php'); - $this->processHelper = $this->prophesize(ProcessHelper::class); + $this->processHelper = $this->prophesize(ProcessRunnerInterface::class); $this->schemaManager = $this->prophesize(AbstractSchemaManager::class); $this->databasePlatform = $this->prophesize(AbstractPlatform::class); @@ -113,12 +112,12 @@ class CreateDatabaseCommandTest extends TestCase $createDatabase = $this->schemaManager->createDatabase($shlinkDatabase)->will(function (): void { }); $listTables = $this->schemaManager->listTableNames()->willReturn([]); - $runCommand = $this->processHelper->mustRun(Argument::type(OutputInterface::class), [ + $runCommand = $this->processHelper->run(Argument::type(OutputInterface::class), [ '/usr/local/bin/php', CreateDatabaseCommand::DOCTRINE_SCRIPT, CreateDatabaseCommand::DOCTRINE_CREATE_SCHEMA_COMMAND, '--no-interaction', - ], Argument::cetera())->willReturn(new Process([])); + ]); $this->commandTester->execute([]); $output = $this->commandTester->getDisplay(); diff --git a/module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php b/module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php index 9875c2f6..d25f44f2 100644 --- a/module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php +++ b/module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php @@ -9,14 +9,13 @@ use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\Db\MigrateDatabaseCommand; +use Shlinkio\Shlink\CLI\Util\ProcessRunnerInterface; use Symfony\Component\Console\Application; -use Symfony\Component\Console\Helper\ProcessHelper; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Tester\CommandTester; use Symfony\Component\Lock\LockFactory; use Symfony\Component\Lock\LockInterface; use Symfony\Component\Process\PhpExecutableFinder; -use Symfony\Component\Process\Process; class MigrateDatabaseCommandTest extends TestCase { @@ -37,7 +36,7 @@ class MigrateDatabaseCommandTest extends TestCase $phpExecutableFinder = $this->prophesize(PhpExecutableFinder::class); $phpExecutableFinder->find(false)->willReturn('/usr/local/bin/php'); - $this->processHelper = $this->prophesize(ProcessHelper::class); + $this->processHelper = $this->prophesize(ProcessRunnerInterface::class); $command = new MigrateDatabaseCommand( $locker->reveal(), @@ -53,12 +52,12 @@ class MigrateDatabaseCommandTest extends TestCase /** @test */ public function migrationsCommandIsRunWithProperVerbosity(): void { - $runCommand = $this->processHelper->mustRun(Argument::type(OutputInterface::class), [ + $runCommand = $this->processHelper->run(Argument::type(OutputInterface::class), [ '/usr/local/bin/php', MigrateDatabaseCommand::DOCTRINE_MIGRATIONS_SCRIPT, MigrateDatabaseCommand::DOCTRINE_MIGRATE_COMMAND, '--no-interaction', - ], Argument::cetera())->willReturn(new Process([])); + ]); $this->commandTester->execute([]); $output = $this->commandTester->getDisplay(); diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 236fac50..d5ee2982 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -52,7 +52,7 @@ class LocateVisitsCommandTest extends TestCase $this->lock->acquire(false)->willReturn(true); $this->lock->release()->will(function (): void { }); - $locker->createLock(Argument::type('string'), 90.0, false)->willReturn($this->lock->reveal()); + $locker->createLock(Argument::type('string'), 600.0, false)->willReturn($this->lock->reveal()); $command = new LocateVisitsCommand( $this->visitService->reveal(), diff --git a/phpstan.neon b/phpstan.neon index 969b00b4..80f1b083 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -2,5 +2,4 @@ parameters: checkMissingIterableValueType: false checkGenericClassInNonGenericObjectType: false ignoreErrors: - - '#mustRun\(\)#' - '#If condition is always false#' From 4e00c950cc2d1fa13eaf3d663f2e6d8cbb93cb81 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 12 Feb 2021 23:23:34 +0100 Subject: [PATCH 2/2] Created ProcessRunnerTest --- CHANGELOG.md | 1 + module/CLI/src/Util/ProcessRunner.php | 10 +- module/CLI/test/Util/ProcessRunnerTest.php | 106 +++++++++++++++++++++ 3 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 module/CLI/test/Util/ProcessRunnerTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f00b680..78fe5104 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Changed * [#977](https://github.com/shlinkio/shlink/issues/977) Migrated from `laminas/laminas-paginator` to `pagerfanta/core` to handle pagination. * [#986](https://github.com/shlinkio/shlink/issues/986) Updated official docker image to use PHP 8. +* [#1010](https://github.com/shlinkio/shlink/issues/1010) Increased timeout for database commands to 10 minutes. ### Deprecated * [#959](https://github.com/shlinkio/shlink/issues/959) Deprecated all command flags using camelCase format (like `--expirationDate`), adding kebab-case replacements for all of them (like `--expiration-date`). diff --git a/module/CLI/src/Util/ProcessRunner.php b/module/CLI/src/Util/ProcessRunner.php index 4da5cb9f..1a6b826e 100644 --- a/module/CLI/src/Util/ProcessRunner.php +++ b/module/CLI/src/Util/ProcessRunner.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Util; +use Closure; use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig; use Symfony\Component\Console\Helper\DebugFormatterHelper; use Symfony\Component\Console\Helper\ProcessHelper; @@ -18,10 +19,14 @@ use function str_replace; class ProcessRunner implements ProcessRunnerInterface { private ProcessHelper $helper; + private Closure $createProcess; - public function __construct(ProcessHelper $helper) + public function __construct(ProcessHelper $helper, ?callable $createProcess = null) { $this->helper = $helper; + $this->createProcess = $createProcess !== null + ? Closure::fromCallable($createProcess) + : static fn (array $cmd) => new Process($cmd, null, null, null, LockedCommandConfig::DEFAULT_TTL); } public function run(OutputInterface $output, array $cmd): void @@ -32,7 +37,8 @@ class ProcessRunner implements ProcessRunnerInterface /** @var DebugFormatterHelper $formatter */ $formatter = $this->helper->getHelperSet()->get('debug_formatter'); - $process = new Process($cmd, null, null, null, LockedCommandConfig::DEFAULT_TTL); + /** @var Process $process */ + $process = ($this->createProcess)($cmd); if ($output->isVeryVerbose()) { $output->write( diff --git a/module/CLI/test/Util/ProcessRunnerTest.php b/module/CLI/test/Util/ProcessRunnerTest.php new file mode 100644 index 00000000..05ac5dd7 --- /dev/null +++ b/module/CLI/test/Util/ProcessRunnerTest.php @@ -0,0 +1,106 @@ +helper = $this->prophesize(ProcessHelper::class); + $this->formatter = $this->prophesize(DebugFormatterHelper::class); + $helperSet = $this->prophesize(HelperSet::class); + $helperSet->get('debug_formatter')->willReturn($this->formatter->reveal()); + $this->helper->getHelperSet()->willReturn($helperSet->reveal()); + $this->process = $this->prophesize(Process::class); + + $this->runner = new ProcessRunner($this->helper->reveal(), fn () => $this->process->reveal()); + $this->output = $this->prophesize(OutputInterface::class); + } + + /** @test */ + public function noMessagesAreWrittenWhenOutputIsNotVerbose(): void + { + $isVeryVerbose = $this->output->isVeryVerbose()->willReturn(false); + $isDebug = $this->output->isDebug()->willReturn(false); + $mustRun = $this->process->mustRun(Argument::cetera())->willReturn($this->process->reveal()); + + $this->runner->run($this->output->reveal(), []); + + $isVeryVerbose->shouldHaveBeenCalledTimes(2); + $isDebug->shouldHaveBeenCalledOnce(); + $mustRun->shouldHaveBeenCalledOnce(); + $this->process->isSuccessful()->shouldNotHaveBeenCalled(); + $this->process->getCommandLine()->shouldNotHaveBeenCalled(); + $this->output->write(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->helper->wrapCallback(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->formatter->start(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->formatter->stop(Argument::cetera())->shouldNotHaveBeenCalled(); + } + + /** @test */ + public function someMessagesAreWrittenWhenOutputIsVerbose(): void + { + $isVeryVerbose = $this->output->isVeryVerbose()->willReturn(true); + $isDebug = $this->output->isDebug()->willReturn(false); + $mustRun = $this->process->mustRun(Argument::cetera())->willReturn($this->process->reveal()); + $isSuccessful = $this->process->isSuccessful()->willReturn(true); + $getCommandLine = $this->process->getCommandLine()->willReturn('true'); + $start = $this->formatter->start(Argument::cetera())->willReturn(''); + $stop = $this->formatter->stop(Argument::cetera())->willReturn(''); + + $this->runner->run($this->output->reveal(), []); + + $isVeryVerbose->shouldHaveBeenCalledTimes(2); + $isDebug->shouldHaveBeenCalledOnce(); + $mustRun->shouldHaveBeenCalledOnce(); + $this->output->write(Argument::cetera())->shouldHaveBeenCalledTimes(2); + $this->helper->wrapCallback(Argument::cetera())->shouldNotHaveBeenCalled(); + $isSuccessful->shouldHaveBeenCalledTimes(2); + $getCommandLine->shouldHaveBeenCalledOnce(); + $start->shouldHaveBeenCalledOnce(); + $stop->shouldHaveBeenCalledOnce(); + } + + /** @test */ + public function wrapsCallbackWhenOutputIsDebug(): void + { + $isVeryVerbose = $this->output->isVeryVerbose()->willReturn(false); + $isDebug = $this->output->isDebug()->willReturn(true); + $mustRun = $this->process->mustRun(Argument::cetera())->willReturn($this->process->reveal()); + $wrapCallback = $this->helper->wrapCallback(Argument::cetera())->willReturn(function (): void { + }); + + $this->runner->run($this->output->reveal(), []); + + $isVeryVerbose->shouldHaveBeenCalledTimes(2); + $isDebug->shouldHaveBeenCalledOnce(); + $mustRun->shouldHaveBeenCalledOnce(); + $wrapCallback->shouldHaveBeenCalledOnce(); + $this->process->isSuccessful()->shouldNotHaveBeenCalled(); + $this->process->getCommandLine()->shouldNotHaveBeenCalled(); + $this->output->write(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->formatter->start(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->formatter->stop(Argument::cetera())->shouldNotHaveBeenCalled(); + } +}