feat(loader): lock load class on file loader to avoid race condition (#…

…174)

There can be race condition when multiple php process would generate the
class at the same time, so we add a lock here to avoid this problem

[Better diff with
`?w=1`](https://github.com/jolicode/automapper/pull/174/files?w=1)

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 ### Fixed
+- [GH#174](https://github.com/jolicode/automapper/pull/174) Fix race condition when writing generated mappers
 - [GH#167](https://github.com/jolicode/automapper/pull/167) Fix property metadata attribute name in docs
 - [GH#166](https://github.com/jolicode/automapper/pull/166) Remove cache for property info, use specific services instead
 

composer.json

@@ -21,6 +21,7 @@
         "symfony/deprecation-contracts": "^3.0",
         "symfony/event-dispatcher": "^6.4 || ^7.0",
         "symfony/expression-language": "^6.4 || ^7.0",
+        "symfony/lock": "^6.4 || ^7.0",
         "symfony/property-info": "^6.4 || ^7.0",
         "symfony/property-access": "^6.4 || ^7.0",
         "phpdocumentor/type-resolver": "^1.7"

src/AutoMapper.php

@@ -22,6 +22,8 @@
 use Symfony\Component\EventDispatcher\EventDispatcher;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
+use Symfony\Component\Lock\LockFactory;
+use Symfony\Component\Lock\Store\FlockStore;
 use Symfony\Component\Serializer\Mapping\ClassDiscriminatorFromClassMetadata;
 use Symfony\Component\Serializer\Mapping\Factory\ClassMetadataFactory;
 use Symfony\Component\Serializer\Mapping\Loader\AnnotationLoader;
@@ -181,10 +183,12 @@ public static function create(
             $expressionLanguage,
         );
 
+        $lockFactory = new LockFactory(new FlockStore());
+
         if (null === $cacheDirectory) {
             $loader = new EvalLoader($mapperGenerator, $metadataFactory);
         } else {
-            $loader = new FileLoader($mapperGenerator, $metadataFactory, $cacheDirectory);
+            $loader = new FileLoader($mapperGenerator, $metadataFactory, $cacheDirectory, $lockFactory);
         }
 
         return new self($loader, $customTransformerRegistry, $metadataRegistry, $providerRegistry, $expressionLanguageProvider);

src/Loader/FileLoader.php

@@ -11,13 +11,14 @@
 use AutoMapper\Metadata\MetadataRegistry;
 use PhpParser\PrettyPrinter\Standard;
 use PhpParser\PrettyPrinterAbstract;
+use Symfony\Component\Lock\LockFactory;
 
 /**
  * Use file system to load mapper, and persist them using a registry.
  *
  * @author Joel Wurtz <[email protected]>
  *
- *  @internal
+ * @internal
  */
 final class FileLoader implements ClassLoaderInterface
 {
@@ -30,6 +31,7 @@ public function __construct(
         private readonly MapperGenerator $generator,
         private readonly MetadataFactory $metadataFactory,
         private readonly string $directory,
+        private readonly LockFactory $lockFactory,
         private readonly FileReloadStrategy $reloadStrategy = FileReloadStrategy::ON_CHANGE,
     ) {
         $this->printer = new Standard();
@@ -40,25 +42,33 @@ public function loadClass(MapperMetadata $mapperMetadata): void
         $className = $mapperMetadata->className;
         $classPath = $this->directory . \DIRECTORY_SEPARATOR . $className . '.php';
 
-        if ($this->reloadStrategy === FileReloadStrategy::NEVER && file_exists($classPath)) {
-            require $classPath;
+        // We lock the file here, because another process could be writing the file at the same time
+        $lock = $this->lockFactory->createLock($className);
+        $lock->acquire(true);
 
-            return;
-        }
+        try {
+            if ($this->reloadStrategy === FileReloadStrategy::NEVER && file_exists($classPath)) {
+                require $classPath;
 
-        $shouldBuildMapper = true;
+                return;
+            }
 
-        if ($this->reloadStrategy === FileReloadStrategy::ON_CHANGE) {
-            $registry = $this->getRegistry();
-            $hash = $mapperMetadata->getHash();
-            $shouldBuildMapper = !isset($registry[$className]) || $registry[$className] !== $hash || !file_exists($classPath);
-        }
+            $shouldBuildMapper = true;
 
-        if ($shouldBuildMapper) {
-            $this->createGeneratedMapper($mapperMetadata);
-        }
+            if ($this->reloadStrategy === FileReloadStrategy::ON_CHANGE) {
+                $registry = $this->getRegistry();
+                $hash = $mapperMetadata->getHash();
+                $shouldBuildMapper = !isset($registry[$className]) || $registry[$className] !== $hash || !file_exists($classPath);
+            }
+
+            if ($shouldBuildMapper) {
+                $this->createGeneratedMapper($mapperMetadata);
+            }
 
-        require $classPath;
+            require $classPath;
+        } finally {
+            $lock->release();
+        }
     }
 
     public function buildMappers(MetadataRegistry $registry): bool

src/Symfony/Bundle/DependencyInjection/AutoMapperExtension.php

@@ -89,7 +89,7 @@ public function load(array $configs, ContainerBuilder $container): void
 
             $container
                 ->getDefinition(FileLoader::class)
-                ->replaceArgument(3, $generateStrategy);
+                ->replaceArgument(4, $generateStrategy);
 
             $container
                 ->setAlias(ClassLoaderInterface::class, FileLoader::class)

src/Symfony/Bundle/Resources/config/automapper.php

@@ -12,10 +12,13 @@
 use AutoMapper\Loader\ClassLoaderInterface;
 use AutoMapper\Loader\EvalLoader;
 use AutoMapper\Loader\FileLoader;
+use AutoMapper\Loader\FileReloadStrategy;
 use AutoMapper\Metadata\MetadataFactory;
 use AutoMapper\Provider\ProviderRegistry;
 use AutoMapper\Symfony\ExpressionLanguageProvider;
 use AutoMapper\Transformer\PropertyTransformer\PropertyTransformerRegistry;
+use Symfony\Component\Lock\LockFactory;
+use Symfony\Component\Lock\Store\FlockStore;
 
 return static function (ContainerConfigurator $container) {
     $container->services()
@@ -30,6 +33,13 @@
             ->alias(AutoMapperInterface::class, AutoMapper::class)->public()
             ->alias(AutoMapperRegistryInterface::class, AutoMapper::class)->public()
 
+        ->set('automapper.file_loader_lock_factory_store')
+            ->class(FlockStore::class)
+        ->set('automapper.file_loader_lock_factory')
+            ->class(LockFactory::class)
+            ->args([
+                service('automapper.file_loader_lock_factory_store'),
+            ])
         ->set(EvalLoader::class)
             ->args([
                 service(MapperGenerator::class),
@@ -41,7 +51,8 @@
                 service(MapperGenerator::class),
                 service(MetadataFactory::class),
                 '%kernel.cache_dir%/automapper',
-                true,
+                service('automapper.file_loader_lock_factory'),
+                FileReloadStrategy::ALWAYS,
             ])
 
         ->set(Configuration::class)

tests/Bundle/DependencyInjection/AutoMapperExtensionTest.php

@@ -29,7 +29,7 @@ public function testItCorrectlyConfiguresReloadStrategy(array $config, bool $deb
         $this->container->setParameter('kernel.debug', $debug);
         $this->load(['loader' => $config]);
 
-        $this->assertContainerBuilderHasServiceDefinitionWithArgument(FileLoader::class, 3, $expectedValue);
+        $this->assertContainerBuilderHasServiceDefinitionWithArgument(FileLoader::class, 4, $expectedValue);
     }
 
     public static function provideReloadStrategyConfiguration(): iterable