Merge pull request #537: exception propagation from yielded generator

(cherry picked from commit 8059ef7)

src/Internal/Workflow/Process/DeferredGenerator.php

@@ -18,9 +18,6 @@ final class DeferredGenerator implements \Iterator
 {
     private bool $started = false;
     private bool $finished = false;
-    private mixed $key = null;
-    private mixed $value = null;
-    private mixed $result = null;
     private \Generator $generator;
 
     /** @var array<\Closure(\Throwable): mixed> */
@@ -50,7 +47,6 @@ public static function fromGenerator(\Generator $generator): self
         $self = new self();
         $self->generator = $generator;
         $self->started = true;
-        $self->fill();
         return $self;
     }
 
@@ -67,7 +63,6 @@ public function throw(\Throwable $exception): void
         );
         try {
             $this->generator->throw($exception);
-            $this->fill();
         } catch (\Throwable $e) {
             $this->handleException($e);
         }
@@ -80,15 +75,12 @@ public function throw(\Throwable $exception): void
      */
     public function send(mixed $value): mixed
     {
-        $this->started or throw new \LogicException('Cannot send value to a generator that was not started.');
+        $this->start();
         $this->finished and throw new \LogicException('Cannot send value to a generator that was already finished.');
         try {
-            $result = $this->generator->send($value);
-            $this->fill();
-            return $result;
+            return $this->generator->send($value);
         } catch (\Throwable $e) {
             $this->handleException($e);
-            return null;
         }
     }
 
@@ -97,8 +89,12 @@ public function send(mixed $value): mixed
      */
     public function getReturn(): mixed
     {
-        $this->finished or throw new \LogicException('Cannot get return value of a generator that was not finished.');
-        return $this->result;
+        // $this->start();
+        try {
+            return $this->generator->getReturn();
+        } catch (\Throwable $e) {
+            $this->handleException($e);
+        }
     }
 
     /**
@@ -107,7 +103,11 @@ public function getReturn(): mixed
     public function current(): mixed
     {
         $this->start();
-        return $this->value;
+        try {
+            return $this->generator->current();
+        } catch (\Throwable $e) {
+            $this->handleException($e);
+        }
     }
 
     /**
@@ -116,7 +116,11 @@ public function current(): mixed
     public function key(): mixed
     {
         $this->start();
-        return $this->key;
+        try {
+            return $this->generator->key();
+        } catch (\Throwable $e) {
+            $this->handleException($e);
+        }
     }
 
     /**
@@ -131,7 +135,6 @@ public function next(): void
 
         try {
             $this->generator->next();
-            $this->fill();
         } catch (\Throwable $e) {
             $this->handleException($e);
         }
@@ -145,12 +148,16 @@ public function next(): void
     public function valid(): bool
     {
         $this->start();
-        return !$this->finished;
+        try {
+            return $this->generator->valid();
+        } catch (\Throwable $e) {
+            $this->handleException($e);
+        }
     }
 
     public function rewind(): void
     {
-        $this->started and throw new \LogicException('Cannot rewind a generator that was already run.');
+        $this->generator->rewind();
     }
 
     /**
@@ -164,6 +171,20 @@ public function catch(callable $handler): self
         return $this;
     }
 
+    private static function getDummyGenerator(): \Generator
+    {
+        static $generator;
+
+        if ($generator === null) {
+            $generator = (static function (): \Generator {
+                yield;
+            })();
+            $generator->current();
+        }
+
+        return $generator;
+    }
+
     private function start(): void
     {
         if ($this->started) {
@@ -176,33 +197,36 @@ private function start(): void
 
             if ($result instanceof \Generator) {
                 $this->generator = $result;
-                $this->fill();
                 return;
             }
 
-            $this->result = $result;
+            /** @psalm-suppress all */
+            $this->generator = (static function (mixed $result): \Generator {
+                return $result;
+                yield;
+            })($result);
             $this->finished = true;
         } catch (\Throwable $e) {
+            $this->generator = self::getDummyGenerator();
             $this->handleException($e);
         } finally {
             unset($this->handler, $this->values);
         }
     }
 
-    private function fill(): void
+    private function handleException(\Throwable $e): never
     {
-        $this->key = $this->generator->key();
-        $this->value = $this->generator->current();
-        $this->finished = !$this->generator->valid() and $this->result = $this->generator->getReturn();
-    }
-
-    private function handleException(\Throwable $e): void
-    {
-        $this->key = null;
-        $this->value = null;
+        $this->finished and throw $e;
         $this->finished = true;
         foreach ($this->catchers as $catch) {
-            $catch($e);
+            try {
+                $catch($e);
+            } catch (\Throwable) {
+                // Do nothing.
+            }
         }
+
+        $this->catchers = [];
+        throw $e;
     }
 }

src/Internal/Workflow/Process/Scope.php

@@ -131,7 +131,8 @@ public function getContext(): WorkflowContext
     public function start(\Closure $handler, ?ValuesInterface $values, bool $deferred): void
     {
         // Create a coroutine generator
-        $this->coroutine = $this->call($handler, $values ?? EncodedValues::empty());
+        $this->coroutine = DeferredGenerator::fromHandler($handler, $values ?? EncodedValues::empty())
+            ->catch($this->onException(...));
 
         $deferred
             ? $this->services->loop->once($this->layer, $this->next(...))
@@ -330,14 +331,6 @@ function () use ($cancelID): void {
         return $scope;
     }
 
-    /**
-     * @param \Closure(ValuesInterface): mixed $handler
-     */
-    protected function call(\Closure $handler, ValuesInterface $values): DeferredGenerator
-    {
-        return DeferredGenerator::fromHandler($handler, $values)->catch($this->onException(...));
-    }
-
     /**
      * Call a Signal or Update method. In this case deserialization errors are skipped.
      *
@@ -397,7 +390,11 @@ protected function next(): void
         $this->context->resolveConditions();
 
         if (!$this->coroutine->valid()) {
-            $this->onResult($this->coroutine->getReturn());
+            try {
+                $this->onResult($this->coroutine->getReturn());
+            } catch (\Throwable) {
+                $this->onResult(null);
+            }
 
             return;
         }
@@ -428,7 +425,11 @@ protected function next(): void
                 break;
 
             default:
-                $this->coroutine->send($current);
+                try {
+                    $this->coroutine->send($current);
+                } catch (\Throwable) {
+                    // Ignore
+                }
                 goto begin;
         }
     }

tests/Acceptance/Harness/ChildWorkflow/ThrowOnExecuteTest.php

@@ -51,8 +51,6 @@ public function run()
     {
         return yield Workflow::newChildWorkflowStub(
             ChildWorkflow::class,
-            // TODO: remove after https://github.com/temporalio/sdk-php/issues/451 is fixed
-            Workflow\ChildWorkflowOptions::new()->withTaskQueue(Workflow::getInfo()->taskQueue),
         )->run();
     }
 }
@@ -63,6 +61,7 @@ class ChildWorkflow
     #[WorkflowMethod('Harness_ChildWorkflow_ThrowsOnExecute_Child')]
     public function run()
     {
+        yield 1;
         throw new ApplicationFailure('Test message', 'TestError', true, EncodedValues::fromValues([['foo' => 'bar']]));
     }
 }

tests/Fixtures/src/Workflow/GeneratorWorkflow.php

@@ -12,6 +12,7 @@
 namespace Temporal\Tests\Workflow;
 
 use Temporal\Activity\ActivityOptions;
+use Temporal\Common\RetryOptions;
 use Temporal\Internal\Workflow\ActivityProxy;
 use Temporal\Tests\Activity\SimpleActivity;
 use Temporal\Workflow;
@@ -27,7 +28,9 @@ public function handler(
         // typed stub
         $simple = Workflow::newActivityStub(
             SimpleActivity::class,
-            ActivityOptions::new()->withStartToCloseTimeout(5)
+            ActivityOptions::new()->withStartToCloseTimeout(5)->withRetryOptions(
+                RetryOptions::new()->withMaximumAttempts(1)
+            )
         );
 
         return [
@@ -38,11 +41,16 @@ public function handler(
 
     /**
      * @param ActivityProxy|SimpleActivity $simple
-     * @param string $input
-     * @return \Generator
      */
     private function doSomething(ActivityProxy $simple, string $input): \Generator
     {
+        $input === 'error' and throw new \Exception('error from generator');
+
+        if ($input === 'failure') {
+            yield $simple->fail();
+            throw new \Exception('Unreachable statement');
+        }
+
         $result = [];
         $result[] = yield $simple->echo($input);
         $result[] = yield $simple->echo($input);

tests/Functional/Client/TypedStubTestCase.php

@@ -11,7 +11,9 @@
 
 namespace Temporal\Tests\Functional\Client;
 
+use Temporal\Exception\Client\WorkflowFailedException;
 use Temporal\Exception\Client\WorkflowQueryException;
+use Temporal\Exception\Failure\ActivityFailure;
 use Temporal\Exception\InvalidArgumentException;
 use Temporal\Tests\DTO\Message;
 use Temporal\Tests\DTO\User;
@@ -151,6 +153,34 @@ public function testGeneratorCoroutines()
         );
     }
 
+    public function testGeneratorErrorCoroutines()
+    {
+        $client = $this->createClient();
+        $simple = $client->newWorkflowStub(GeneratorWorkflow::class);
+
+        try {
+            $simple->handler('error');
+            $this->fail('Expected exception to be thrown');
+        } catch (WorkflowFailedException $e) {
+            $this->assertStringContainsString('error from generator', $e->getPrevious()->getMessage());
+        }
+    }
+
+    public function testGeneratorErrorInNestedActionCoroutines()
+    {
+        $client = $this->createClient();
+        $simple = $client->newWorkflowStub(GeneratorWorkflow::class);
+
+        try {
+            $simple->handler('failure');
+            $this->fail('Expected exception to be thrown');
+        } catch (WorkflowFailedException $e) {
+            $this->assertStringNotContainsString('Unreachable statement', $e->getPrevious()->getMessage());
+            $this->assertInstanceOf(ActivityFailure::class, $e->getPrevious());
+            $this->assertStringContainsString('failed activity', $e->getPrevious()->getPrevious()->getMessage());
+        }
+    }
+
     /**
      * @group skip-on-test-server
      */