diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d0017fd..4d41319e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#1656](https://github.com/shlinkio/shlink/issues/1656) Add support for openswoole 22 * [#1784](https://github.com/shlinkio/shlink/issues/1784) Add new docker tag where the container runs as a non-root user. +* [#953](https://github.com/shlinkio/shlink/issues/953) Add locks that prevent errors on duplicated keys when creating short URLs in parallel that depend on the same new tag or domain. ### Changed * [#1755](https://github.com/shlinkio/shlink/issues/1755) Update to roadrunner 2023 @@ -45,6 +46,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#1760](https://github.com/shlinkio/shlink/issues/1760) Fix domain not being set to null when importing short URLs with default domain. +* [#953](https://github.com/shlinkio/shlink/issues/953) Fix duplicated key errors and short URL creation failing when creating short URLs in parallel that depend on the same new tag or domain. * Fix Shlink trying to connect to RabbitMQ even if configuration set to not connect. diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 8b6acc3e..da653406 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -13,6 +13,7 @@ use Shlinkio\Shlink\Core\ErrorHandler; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; +use Symfony\Component\Lock; return [ @@ -172,7 +173,11 @@ return [ ], Action\RobotsAction::class => [Crawling\CrawlingHelper::class], - ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ['em', Options\UrlShortenerOptions::class], + ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => [ + 'em', + Options\UrlShortenerOptions::class, + Lock\LockFactory::class, + ], ShortUrl\Helper\ShortUrlStringifier::class => ['config.url_shortener.domain', 'config.router.base_path'], ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => [Util\UrlValidator::class], ShortUrl\Helper\ShortUrlRedirectionBuilder::class => [Options\TrackingOptions::class], diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php index 48ec634e..17669f32 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -11,7 +11,11 @@ use Doctrine\ORM\Events; use Shlinkio\Shlink\Core\Domain\Entity\Domain; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Tag\Entity\Tag; +use Symfony\Component\Lock\Lock; +use Symfony\Component\Lock\LockFactory; +use Symfony\Component\Lock\Store\InMemoryStore; +use function Functional\invoke; use function Functional\map; use function Functional\unique; @@ -21,10 +25,15 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt private array $memoizedNewDomains = []; /** @var array */ private array $memoizedNewTags = []; + /** @var array */ + private array $tagLocks = []; + /** @var array */ + private array $domainLocks = []; public function __construct( private readonly EntityManagerInterface $em, private readonly UrlShortenerOptions $options = new UrlShortenerOptions(), + private readonly LockFactory $locker = new LockFactory(new InMemoryStore()), ) { // Registering this as an event listener will make the postFlush method to be called automatically $this->em->getEventManager()->addEventListener(Events::postFlush, $this); @@ -36,11 +45,18 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt return null; } + $this->lock($this->domainLocks, 'domain_' . $domain); + /** @var Domain|null $existingDomain */ $existingDomain = $this->em->getRepository(Domain::class)->findOneBy(['authority' => $domain]); + if ($existingDomain) { + // The lock can be released immediately of the domain is not new + $this->releaseLock($this->domainLocks, 'domain_' . $domain); + return $existingDomain; + } // Memoize only new domains, and let doctrine handle objects hydrated from persistence - return $existingDomain ?? $this->memoizeNewDomain($domain); + return $this->memoizeNewDomain($domain); } private function memoizeNewDomain(string $domain): Domain @@ -62,8 +78,16 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt $repo = $this->em->getRepository(Tag::class); return new Collections\ArrayCollection(map($tags, function (string $tagName) use ($repo): Tag { + $this->lock($this->tagLocks, 'tag_' . $tagName); + + $existingTag = $repo->findOneBy(['name' => $tagName]); + if ($existingTag) { + $this->releaseLock($this->tagLocks, 'tag_' . $tagName); + return $existingTag; + } + // Memoize only new tags, and let doctrine handle objects hydrated from persistence - $tag = $repo->findOneBy(['name' => $tagName]) ?? $this->memoizeNewTag($tagName); + $tag = $this->memoizeNewTag($tagName); $this->em->persist($tag); return $tag; @@ -75,9 +99,36 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt return $this->memoizedNewTags[$tagName] ??= new Tag($tagName); } + /** + * @param array $locks + */ + private function lock(array &$locks, string $name): void + { + // Lock dependency creation for up to 5 seconds. This will prevent errors when trying to create the same one + // more than once in parallel. + $locks[$name] = $lock = $this->locker->createLock($name, 5); + $lock->acquire(true); + } + + /** + * @param array $locks + */ + private function releaseLock(array &$locks, string $name): void + { + $locks[$name]->release(); + unset($locks[$name]); + } + public function postFlush(): void { + // Reset memoized domains and tags $this->memoizedNewDomains = []; $this->memoizedNewTags = []; + + // Release all locks + invoke($this->tagLocks, 'release'); + invoke($this->domainLocks, 'release'); + $this->tagLocks = []; + $this->domainLocks = []; } } diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index d31f3d9b..d7af118d 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -74,10 +74,12 @@ class PersistenceShortUrlRelationResolverTest extends TestCase #[Test, DataProvider('provideTags')] public function findsAndPersistsTagsWrappedIntoCollection(array $tags, array $expectedTags): void { - $expectedPersistedTags = count($expectedTags); + $expectedLookedOutTags = count($expectedTags); + // One of the tags will already exist. The rest will be new + $expectedPersistedTags = $expectedLookedOutTags - 1; $tagRepo = $this->createMock(TagRepositoryInterface::class); - $tagRepo->expects($this->exactly($expectedPersistedTags))->method('findOneBy')->with( + $tagRepo->expects($this->exactly($expectedLookedOutTags))->method('findOneBy')->with( $this->isType('array'), )->willReturnCallback(function (array $criteria): ?Tag { ['name' => $name] = $criteria; @@ -90,7 +92,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase $result = $this->resolver->resolveTags($tags); - self::assertCount($expectedPersistedTags, $result); + self::assertCount($expectedLookedOutTags, $result); self::assertEquals($expectedTags, $result->toArray()); }