diff --git a/CHANGELOG.md b/CHANGELOG.md index a70df1e1..8c9fb870 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/composer.json b/composer.json index 596d7d4c..1dfb891f 100644 --- a/composer.json +++ b/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" diff --git a/src/AutoMapper.php b/src/AutoMapper.php index 9373abd3..45f9c008 100644 --- a/src/AutoMapper.php +++ b/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); diff --git a/src/Loader/FileLoader.php b/src/Loader/FileLoader.php index 1bacc21d..b4986c9d 100644 --- a/src/Loader/FileLoader.php +++ b/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 * - * @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 diff --git a/src/Symfony/Bundle/DependencyInjection/AutoMapperExtension.php b/src/Symfony/Bundle/DependencyInjection/AutoMapperExtension.php index 63f9f25d..2df4cd41 100644 --- a/src/Symfony/Bundle/DependencyInjection/AutoMapperExtension.php +++ b/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) diff --git a/src/Symfony/Bundle/Resources/config/automapper.php b/src/Symfony/Bundle/Resources/config/automapper.php index 5a7c0501..c9b95220 100644 --- a/src/Symfony/Bundle/Resources/config/automapper.php +++ b/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) diff --git a/tests/Bundle/DependencyInjection/AutoMapperExtensionTest.php b/tests/Bundle/DependencyInjection/AutoMapperExtensionTest.php index 4fccf4eb..c286976b 100644 --- a/tests/Bundle/DependencyInjection/AutoMapperExtensionTest.php +++ b/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