diff --git a/CHANGELOG.md b/CHANGELOG.md index 37a749f4..355d89b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,29 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org). +## [Unreleased] + +#### Added + +* *Nothing* + +#### Changed + +* *Nothing* + +#### Deprecated + +* *Nothing* + +#### Removed + +* *Nothing* + +#### Fixed + +* [#271](https://github.com/shlinkio/shlink/issues/271) Fixed memory leak produced when processing high amounts of visits at the same time. + + ## 1.14.0 - 2018-11-16 #### Added diff --git a/README.md b/README.md index 46bbb6ca..b659c987 100644 --- a/README.md +++ b/README.md @@ -114,6 +114,8 @@ Those tasks can be performed using shlink's CLI, so it should be easy to schedul Running this will improve the performance of the `doma.in/abc123/preview` URLs, which return a preview of the site. +*Any of those commands accept the `-q` flag, which makes it not display any output. This is recommended when configuring the commands as cron jobs.* + ## Update to new version When a new Shlink version is available, you don't need to repeat the entire process yourself. Instead, follow these steps: diff --git a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php index a5a33fcb..38dff527 100644 --- a/module/CLI/src/Command/Visit/ProcessVisitsCommand.php +++ b/module/CLI/src/Command/Visit/ProcessVisitsCommand.php @@ -6,7 +6,9 @@ namespace Shlinkio\Shlink\CLI\Command\Visit; use Shlinkio\Shlink\Common\Exception\WrongIpException; use Shlinkio\Shlink\Common\IpGeolocation\IpLocationResolverInterface; use Shlinkio\Shlink\Common\Util\IpAddress; +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\Input\InputInterface; @@ -53,51 +55,47 @@ class ProcessVisitsCommand extends Command protected function execute(InputInterface $input, OutputInterface $output): void { - $io = new SymfonyStyle($input, $output); - $visits = $this->visitService->getUnlocatedVisits(); - - foreach ($visits as $visit) { + $this->visitService->locateVisits(function (Visit $visit) use ($output) { if (! $visit->hasRemoteAddr()) { - $io->writeln( + $output->writeln( sprintf('%s', $this->translator->translate('Ignored visit with no IP address')), OutputInterface::VERBOSITY_VERBOSE ); - continue; + throw new IpCannotBeLocatedException('Ignored visit with no IP address'); } $ipAddr = $visit->getRemoteAddr(); - $io->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); + $output->write(sprintf('%s %s', $this->translator->translate('Processing IP'), $ipAddr)); if ($ipAddr === IpAddress::LOCALHOST) { - $io->writeln( + $output->writeln( sprintf(' [%s]', $this->translator->translate('Ignored localhost address')) ); - continue; + throw new IpCannotBeLocatedException('Ignored localhost address'); } try { - $result = $this->ipLocationResolver->resolveIpLocation($ipAddr); - - $location = new VisitLocation($result); - $visit->setVisitLocation($location); - $this->visitService->saveVisit($visit); - - $io->writeln(sprintf( - ' [' . $this->translator->translate('Address located at "%s"') . ']', - $location->getCountryName() - )); + return $this->ipLocationResolver->resolveIpLocation($ipAddr); } catch (WrongIpException $e) { - $io->writeln( + $output->writeln( sprintf( ' [%s]', $this->translator->translate('An error occurred while locating IP. Skipped') ) ); - if ($io->isVerbose()) { + 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() + )); + }); + + $io = new SymfonyStyle($input, $output); $io->success($this->translator->translate('Finished processing all IPs')); } } diff --git a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php index f7523d78..d36c798a 100644 --- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php @@ -81,7 +81,7 @@ class GetVisitsCommandTest extends TestCase { $shortCode = 'abc123'; $this->visitsTracker->info($shortCode, Argument::any())->willReturn([ - (new Visit(new ShortUrl(''), new Visitor('bar', 'foo', '')))->setVisitLocation( + (new Visit(new ShortUrl(''), new Visitor('bar', 'foo', '')))->locate( new VisitLocation(['country_name' => 'Spain']) ), ])->shouldBeCalledOnce(); diff --git a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php index 5a28e5ab..efa77752 100644 --- a/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/ProcessVisitsCommandTest.php @@ -7,16 +7,21 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\Visit\ProcessVisitsCommand; +use Shlinkio\Shlink\Common\Exception\WrongIpException; use Shlinkio\Shlink\Common\IpGeolocation\IpApiLocationResolver; +use Shlinkio\Shlink\Common\Util\IpAddress; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Entity\VisitLocation; +use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Service\VisitService; use Symfony\Component\Console\Application; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Tester\CommandTester; +use Throwable; use Zend\I18n\Translator\Translator; -use function count; +use function array_shift; class ProcessVisitsCommandTest extends TestCase { @@ -52,59 +57,107 @@ class ProcessVisitsCommandTest extends TestCase /** * @test */ - public function allReturnedVisitsIpsAreProcessed() + public function allPendingVisitsAreProcessed() { - $shortUrl = new ShortUrl(''); + $visit = new Visit(new ShortUrl(''), new Visitor('', '', '1.2.3.4')); + $location = new VisitLocation([]); - $visits = [ - new Visit($shortUrl, new Visitor('', '', '1.2.3.4')), - new Visit($shortUrl, new Visitor('', '', '4.3.2.1')), - new Visit($shortUrl, new Visitor('', '', '12.34.56.78')), - ]; - $this->visitService->getUnlocatedVisits()->willReturn($visits) - ->shouldBeCalledOnce(); + $locateVisits = $this->visitService->locateVisits(Argument::cetera())->will( + function (array $args) use ($visit, $location) { + $firstCallback = array_shift($args); + $firstCallback($visit); - $this->visitService->saveVisit(Argument::any())->shouldBeCalledTimes(count($visits)); - $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]) - ->shouldBeCalledTimes(count($visits)); + $secondCallback = array_shift($args); + $secondCallback($location, $visit); + } + ); + $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]); $this->commandTester->execute([ 'command' => 'visit:process', ]); $output = $this->commandTester->getDisplay(); + $this->assertContains('Processing IP 1.2.3.0', $output); - $this->assertContains('Processing IP 4.3.2.0', $output); - $this->assertContains('Processing IP 12.34.56.0', $output); + $locateVisits->shouldHaveBeenCalledOnce(); + $resolveIpLocation->shouldHaveBeenCalledOnce(); + } + + /** + * @test + * @dataProvider provideIgnoredAddresses + */ + public function localhostAndEmptyAddressesAreIgnored(?string $address, string $message) + { + $visit = new Visit(new ShortUrl(''), new Visitor('', '', $address)); + $location = new VisitLocation([]); + + $locateVisits = $this->visitService->locateVisits(Argument::cetera())->will( + function (array $args) use ($visit, $location) { + $firstCallback = array_shift($args); + $firstCallback($visit); + + $secondCallback = array_shift($args); + $secondCallback($location, $visit); + } + ); + $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]); + + try { + $this->commandTester->execute([ + 'command' => 'visit:process', + ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); + } catch (Throwable $e) { + $output = $this->commandTester->getDisplay(); + + $this->assertInstanceOf(IpCannotBeLocatedException::class, $e); + + $this->assertContains($message, $output); + $locateVisits->shouldHaveBeenCalledOnce(); + $resolveIpLocation->shouldNotHaveBeenCalled(); + } + } + + public function provideIgnoredAddresses(): array + { + return [ + ['', 'Ignored visit with no IP address'], + [null, 'Ignored visit with no IP address'], + [IpAddress::LOCALHOST, 'Ignored localhost address'], + ]; } /** * @test */ - public function localhostAndEmptyAddressIsIgnored() + public function errorWhileLocatingIpIsDisplayed() { - $shortUrl = new ShortUrl(''); + $visit = new Visit(new ShortUrl(''), new Visitor('', '', '1.2.3.4')); + $location = new VisitLocation([]); - $visits = [ - new Visit($shortUrl, new Visitor('', '', '1.2.3.4')), - new Visit($shortUrl, new Visitor('', '', '4.3.2.1')), - new Visit($shortUrl, new Visitor('', '', '12.34.56.78')), - new Visit($shortUrl, new Visitor('', '', '127.0.0.1')), - new Visit($shortUrl, new Visitor('', '', '127.0.0.1')), - new Visit($shortUrl, new Visitor('', '', '')), - new Visit($shortUrl, new Visitor('', '', null)), - ]; - $this->visitService->getUnlocatedVisits()->willReturn($visits) - ->shouldBeCalledOnce(); + $locateVisits = $this->visitService->locateVisits(Argument::cetera())->will( + function (array $args) use ($visit, $location) { + $firstCallback = array_shift($args); + $firstCallback($visit); - $this->visitService->saveVisit(Argument::any())->shouldBeCalledTimes(count($visits) - 4); - $this->ipResolver->resolveIpLocation(Argument::any())->willReturn([]) - ->shouldBeCalledTimes(count($visits) - 4); + $secondCallback = array_shift($args); + $secondCallback($location, $visit); + } + ); + $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willThrow(WrongIpException::class); - $this->commandTester->execute([ - 'command' => 'visit:process', - ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); - $output = $this->commandTester->getDisplay(); - $this->assertContains('Ignored localhost address', $output); - $this->assertContains('Ignored visit with no IP address', $output); + try { + $this->commandTester->execute([ + 'command' => 'visit:process', + ], ['verbosity' => OutputInterface::VERBOSITY_VERBOSE]); + } catch (Throwable $e) { + $output = $this->commandTester->getDisplay(); + + $this->assertInstanceOf(IpCannotBeLocatedException::class, $e); + + $this->assertContains('An error occurred while locating IP. Skipped', $output); + $locateVisits->shouldHaveBeenCalledOnce(); + $resolveIpLocation->shouldHaveBeenCalledOnce(); + } } } diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index 54926e9e..3fd98db0 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -93,7 +93,7 @@ class Visit extends AbstractEntity implements JsonSerializable return $this->visitLocation; } - public function setVisitLocation(VisitLocation $visitLocation): self + public function locate(VisitLocation $visitLocation): self { $this->visitLocation = $visitLocation; return $this; diff --git a/module/Core/src/Exception/IpCannotBeLocatedException.php b/module/Core/src/Exception/IpCannotBeLocatedException.php new file mode 100644 index 00000000..e172e9c9 --- /dev/null +++ b/module/Core/src/Exception/IpCannotBeLocatedException.php @@ -0,0 +1,8 @@ +createQueryBuilder('v'); $qb->where($qb->expr()->isNull('v.visitLocation')); - return $qb->getQuery()->getResult(); + return $qb->getQuery()->iterate(); } /** diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index 4ee3ebde..72df2ed1 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -10,10 +10,7 @@ use Shlinkio\Shlink\Core\Entity\Visit; interface VisitRepositoryInterface extends ObjectRepository { - /** - * @return Visit[] - */ - public function findUnlocatedVisits(): array; + public function findUnlocatedVisits(): iterable; /** * @param ShortUrl|int $shortUrl diff --git a/module/Core/src/Service/VisitService.php b/module/Core/src/Service/VisitService.php index ab18ce1c..dea78f0f 100644 --- a/module/Core/src/Service/VisitService.php +++ b/module/Core/src/Service/VisitService.php @@ -5,6 +5,8 @@ namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Entity\VisitLocation; +use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; use Shlinkio\Shlink\Core\Repository\VisitRepository; class VisitService implements VisitServiceInterface @@ -19,22 +21,36 @@ class VisitService implements VisitServiceInterface $this->em = $em; } - /** - * @return Visit[] - */ - public function getUnlocatedVisits() + public function locateVisits(callable $getGeolocationData, ?callable $locatedVisit = null): void { /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); - return $repo->findUnlocatedVisits(); + $results = $repo->findUnlocatedVisits(); + + foreach ($results as [$visit]) { + try { + $locationData = $getGeolocationData($visit); + } catch (IpCannotBeLocatedException $e) { + // Skip if the visit's IP could not be located + continue; + } + + $location = new VisitLocation($locationData); + $this->locateVisit($visit, $location, $locatedVisit); + } } - /** - * @param Visit $visit - */ - public function saveVisit(Visit $visit) + private function locateVisit(Visit $visit, VisitLocation $location, ?callable $locatedVisit): void { + $visit->locate($location); + $this->em->persist($visit); $this->em->flush(); + + if ($locatedVisit !== null) { + $locatedVisit($location, $visit); + } + + $this->em->clear(); } } diff --git a/module/Core/src/Service/VisitServiceInterface.php b/module/Core/src/Service/VisitServiceInterface.php index 46f6ceb7..907729fa 100644 --- a/module/Core/src/Service/VisitServiceInterface.php +++ b/module/Core/src/Service/VisitServiceInterface.php @@ -3,17 +3,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; -use Shlinkio\Shlink\Core\Entity\Visit; - interface VisitServiceInterface { - /** - * @return Visit[] - */ - public function getUnlocatedVisits(); - - /** - * @param Visit $visit - */ - public function saveVisit(Visit $visit); + public function locateVisits(callable $getGeolocationData, ?callable $locatedVisit = null): void; } diff --git a/module/Core/test-func/Repository/VisitRepositoryTest.php b/module/Core/test-func/Repository/VisitRepositoryTest.php index 886cce70..1ef3e7d3 100644 --- a/module/Core/test-func/Repository/VisitRepositoryTest.php +++ b/module/Core/test-func/Repository/VisitRepositoryTest.php @@ -45,14 +45,20 @@ class VisitRepositoryTest extends DatabaseTestCase if ($i % 2 === 0) { $location = new VisitLocation([]); $this->getEntityManager()->persist($location); - $visit->setVisitLocation($location); + $visit->locate($location); } $this->getEntityManager()->persist($visit); } $this->getEntityManager()->flush(); - $this->assertCount(3, $this->repo->findUnlocatedVisits()); + $resultsCount = 0; + $results = $this->repo->findUnlocatedVisits(); + foreach ($results as $value) { + $resultsCount++; + } + + $this->assertEquals(3, $resultsCount); } /** diff --git a/module/Core/test/Service/VisitServiceTest.php b/module/Core/test/Service/VisitServiceTest.php index 728d18ee..04a90f84 100644 --- a/module/Core/test/Service/VisitServiceTest.php +++ b/module/Core/test/Service/VisitServiceTest.php @@ -5,12 +5,18 @@ namespace ShlinkioTest\Shlink\Core\Service; use Doctrine\ORM\EntityManager; use PHPUnit\Framework\TestCase; +use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Entity\VisitLocation; +use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Service\VisitService; +use function array_shift; +use function count; +use function func_get_args; class VisitServiceTest extends TestCase { @@ -32,22 +38,68 @@ class VisitServiceTest extends TestCase /** * @test */ - public function saveVisitsPersistsProvidedVisit() + public function locateVisitsIteratesAndLocatesUnlocatedVisits() { - $visit = new Visit(new ShortUrl(''), Visitor::emptyInstance()); - $this->em->persist($visit)->shouldBeCalledOnce(); - $this->em->flush()->shouldBeCalledOnce(); - $this->visitService->saveVisit($visit); + $unlocatedVisits = [ + [new Visit(new ShortUrl('foo'), Visitor::emptyInstance())], + [new Visit(new ShortUrl('bar'), Visitor::emptyInstance())], + ]; + + $repo = $this->prophesize(VisitRepository::class); + $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); + $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); + + $persist = $this->em->persist(Argument::type(Visit::class))->will(function () { + }); + $flush = $this->em->flush()->will(function () { + }); + $clear = $this->em->clear()->will(function () { + }); + + $this->visitService->locateVisits(function () { + return []; + }, function () { + $args = func_get_args(); + + $this->assertInstanceOf(VisitLocation::class, array_shift($args)); + $this->assertInstanceOf(Visit::class, array_shift($args)); + }); + + $findUnlocatedVisits->shouldHaveBeenCalledOnce(); + $getRepo->shouldHaveBeenCalledOnce(); + $persist->shouldHaveBeenCalledTimes(count($unlocatedVisits)); + $flush->shouldHaveBeenCalledTimes(count($unlocatedVisits)); + $clear->shouldHaveBeenCalledTimes(count($unlocatedVisits)); } /** * @test */ - public function getUnlocatedVisitsFallbacksToRepository() + public function visitsWhichCannotBeLocatedAreIgnored() { + $unlocatedVisits = [ + [new Visit(new ShortUrl('foo'), Visitor::emptyInstance())], + ]; + $repo = $this->prophesize(VisitRepository::class); - $repo->findUnlocatedVisits()->shouldBeCalledOnce(); - $this->em->getRepository(Visit::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); - $this->visitService->getUnlocatedVisits(); + $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); + $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); + + $persist = $this->em->persist(Argument::type(Visit::class))->will(function () { + }); + $flush = $this->em->flush()->will(function () { + }); + $clear = $this->em->clear()->will(function () { + }); + + $this->visitService->locateVisits(function () { + throw new IpCannotBeLocatedException('Cannot be located'); + }); + + $findUnlocatedVisits->shouldHaveBeenCalledOnce(); + $getRepo->shouldHaveBeenCalledOnce(); + $persist->shouldNotHaveBeenCalled(); + $flush->shouldNotHaveBeenCalled(); + $clear->shouldNotHaveBeenCalled(); } }