Merge pull request #1789 from acelaya-forks/feature/improved-dependency-locks

Feature/improved dependency locks

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.
 
 

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