Add locks when creating short URL dependencies, to avoid race condition

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],

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<string, Tag> */
     private array $memoizedNewTags = [];
+    /** @var array<string, Lock> */
+    private array $tagLocks = [];
+    /** @var array<string, Lock> */
+    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<string, Lock> $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<string, Lock> $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 = [];
     }
 }

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());
     }