#339 - Reject a shared task service used by several tasks of a process

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.
+
 v5.4.1
 -----
 

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

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
 

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<string, list<string>> $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.

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<string, array<string, string>> $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');