diff --git a/CHANGELOG.md b/CHANGELOG.md index d90d1aab..a51cfd00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ Latest ------ +## Changes +* [#339](https://github.com/cleverage/process-bundle/issues/339) A shared service referenced by several tasks of the same process is rejected when the container is compiled (`InvalidArgumentException` naming the service, the tasks and the process): all these tasks ran with the same instance, initialized with the options of each task in turn, without error. Declare the task services with `shared: false` (applied by the autoconfiguration of `TaskInterface` since 5.4). Update documentation, add tests. + ## Fixes * [#343](https://github.com/cleverage/process-bundle/issues/343) The temporary files of `FileSplitterTask` (`file_splitter`), `CsvSplitterTask` (`csv_splitter`) and of the JSON buffer of `ProcessLauncherTask` (`json_buffering`) are created with a random name, exclusively (an existing path, symbolic links included, is never reused) and readable by their owner only, instead of a name derived from the time (`uniqid()`), opened in `w` mode with the permissions of the umask. Update documentation, add tests. * [#331](https://github.com/cleverage/process-bundle/issues/331) `ProcessManager`: the iterable tasks that produced items and the blocking tasks waiting for `proceed()` are tracked by task configuration instead of task code, and forgotten when their process ends. A task of a sub-process (`process_executor`) with the same code as a task of the parent no longer makes the parent lose the output of its blocking task (without error) or skip the flush at the end of its iteration (merged batches), and a failed sub-process no longer leaves its tasks to its next execution. Add tests. diff --git a/docs/03-custom_tasks.md b/docs/03-custom_tasks.md index 386cd043..72bca29a 100644 --- a/docs/03-custom_tasks.md +++ b/docs/03-custom_tasks.md @@ -20,7 +20,8 @@ design them like any other service, with only small specificities: Most of the time you don't want a task declared in a process to share some data with another declaration using the same service, or even with another instance of your process. Since a task is a service, by default, Symfony will only create one instance (keeping values in every attribute of your class). With `shared: false`, a new instance is created -each time a task configuration is initialized. +each time a task configuration is initialized. A shared service referenced by several tasks of the same process is +rejected when the container is compiled. With autoconfiguration (`autoconfigure: true`, set in the `_defaults` of the Symfony skeleton), the bundle applies these settings to every service implementing `TaskInterface`: `shared: false` and a `monolog.logger` tag on the diff --git a/docs/05-good_practices.md b/docs/05-good_practices.md index 9ff7c731..ebe20410 100644 --- a/docs/05-good_practices.md +++ b/docs/05-good_practices.md @@ -19,8 +19,8 @@ in several places, declare it once as a [generic transformer](reference/03-gener Task services should be `shared: false`: a task often keeps state in its properties (an open file, a buffer, a counter...). A shared service would share this state between two tasks using the same service -in a process, or between two executions of a process in the same PHP process (sub-processes, workers, tests). See -[service declaration](03-custom_tasks.md#service-declaration). +in a process (this is rejected when the container is compiled), or between two executions of a process in the same PHP +process (sub-processes, workers, tests). See [service declaration](03-custom_tasks.md#service-declaration). ## Split big workflows into small processes diff --git a/src/DependencyInjection/Compiler/TaskLocatorCompilerPass.php b/src/DependencyInjection/Compiler/TaskLocatorCompilerPass.php index aa8fa2ab..3d6ba77a 100644 --- a/src/DependencyInjection/Compiler/TaskLocatorCompilerPass.php +++ b/src/DependencyInjection/Compiler/TaskLocatorCompilerPass.php @@ -42,20 +42,38 @@ public function process(ContainerBuilder $container): void $configurations = $container->getParameterBag()->resolveValue($processConfigurationRegistry->getArgument(0)); $services = []; - foreach (\is_array($configurations) ? $configurations : [] as $configuration) { - foreach ($configuration['tasks'] ?? [] as $task) { + foreach (\is_array($configurations) ? $configurations : [] as $processCode => $configuration) { + $taskCodesByService = []; + foreach ($configuration['tasks'] ?? [] as $taskCode => $task) { $id = ltrim((string) ($task['service'] ?? ''), '@'); // Unknown services keep failing at runtime, with the same message if ('' !== $id && $container->has($id)) { $services[$id] = new Reference($id); + $taskCodesByService[$id][] = (string) $taskCode; } } + $this->checkSharedServices($container, (string) $processCode, $taskCodesByService); } $container->getDefinition('cleverage_process.registry.task') ->replaceArgument(0, ServiceLocatorTagPass::register($container, $services)); } + /** + * A shared service referenced by several tasks of a process would run all of them with the same instance: the + * options of the last initialization would apply to every task, and their states would be mixed. + * + * @param array> $taskCodesByService + */ + private function checkSharedServices(ContainerBuilder $container, string $processCode, array $taskCodesByService): void + { + foreach ($taskCodesByService as $id => $taskCodes) { + if (\count($taskCodes) > 1 && $container->findDefinition($id)->isShared()) { + throw new InvalidArgumentException(\sprintf('The service "%s" is used by the tasks "%s" of the process "%s", but it is shared: all these tasks would run with the same instance (and the options of one of them). Declare the service with "shared: false".', $id, implode('", "', $taskCodes), $processCode)); + } + } + } + /** * Sets the "service" of the tasks configured with a "task" code. The configurations are not resolved (parameters), * only the "service" key is set. diff --git a/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php b/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php index 75825e9c..a5a10342 100644 --- a/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php +++ b/tests/DependencyInjection/Compiler/TaskLocatorCompilerPassTest.php @@ -134,6 +134,37 @@ public function testInvalidTaskCodeAttributeThrows(): void (new TaskLocatorCompilerPass())->process($container); } + public function testSharedServiceUsedBySeveralTasksOfAProcessThrows(): void + { + $container = $this->createContainerWithTasks(['test.process' => ['first' => 'app_task', 'second' => 'app_task']]); + $container->setDefinition('app.task', (new Definition(DummyTask::class))->addTag('cleverage.task', ['code' => 'app_task'])); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('The service "app.task" is used by the tasks "first", "second" of the process "test.process", but it is shared: all these tasks would run with the same instance (and the options of one of them). Declare the service with "shared: false".'); + + (new TaskLocatorCompilerPass())->process($container); + } + + public function testNonSharedServiceCanBeUsedBySeveralTasksOfAProcess(): void + { + $container = $this->createContainerWithTasks(['test.process' => ['first' => 'app_task', 'second' => 'app_task']]); + $container->setDefinition('app.task', (new Definition(DummyTask::class))->setShared(false)->addTag('cleverage.task', ['code' => 'app_task'])); + + (new TaskLocatorCompilerPass())->process($container); + + self::assertInstanceOf(Reference::class, $container->getDefinition('cleverage_process.registry.task')->getArgument(0)); + } + + public function testSharedServiceCanBeUsedOnceByEachProcess(): void + { + $container = $this->createContainerWithTasks(['test.first' => ['task' => 'app_task'], 'test.second' => ['task' => 'app_task']]); + $container->setDefinition('app.task', (new Definition(DummyTask::class))->addTag('cleverage.task', ['code' => 'app_task'])); + + (new TaskLocatorCompilerPass())->process($container); + + self::assertInstanceOf(Reference::class, $container->getDefinition('cleverage_process.registry.task')->getArgument(0)); + } + public function testNothingIsDoneWithoutTheRegistries(): void { $container = new ContainerBuilder(); @@ -150,6 +181,27 @@ private function createContainerWithTask(array $task): ContainerBuilder { $container = new ContainerBuilder(); (new CleverAgeProcessExtension())->load([['configurations' => ['test.process' => ['tasks' => ['my_task' => $task]]]]], $container); + + return $this->clearTaskTags($container); + } + + /** + * @param array> $taskCodesByProcess task code by task, for each process + */ + private function createContainerWithTasks(array $taskCodesByProcess): ContainerBuilder + { + $configurations = []; + foreach ($taskCodesByProcess as $processCode => $taskCodes) { + $configurations[$processCode]['tasks'] = array_map(static fn (string $code): array => ['task' => $code], $taskCodes); + } + $container = new ContainerBuilder(); + (new CleverAgeProcessExtension())->load([['configurations' => $configurations]], $container); + + return $this->clearTaskTags($container); + } + + private function clearTaskTags(ContainerBuilder $container): ContainerBuilder + { // Only the tasks of the test foreach (array_keys($container->findTaggedServiceIds('cleverage.task')) as $id) { $container->getDefinition($id)->clearTag('cleverage.task');