From ef3f586ef8f04e8397e7c8df5150ae038633271b Mon Sep 17 00:00:00 2001 From: Aleksei Gagarin Date: Tue, 17 Dec 2024 22:30:41 +0400 Subject: [PATCH] Merge pull request #537: exception propagation from yielded generator (cherry picked from commit 8059ef74a045be5bc88b67000fc2eddfb07fe624) --- .../Workflow/Process/DeferredGenerator.php | 84 ++++-- src/Internal/Workflow/Process/Scope.php | 23 +- .../ChildWorkflow/ThrowOnExecuteTest.php | 3 +- .../src/Workflow/GeneratorWorkflow.php | 14 +- tests/Functional/Client/TypedStubTestCase.php | 30 ++ .../Workflow/DeferredGeneratorTestCase.php | 271 ++++++++++++++++++ 6 files changed, 379 insertions(+), 46 deletions(-) create mode 100644 tests/Unit/Workflow/DeferredGeneratorTestCase.php diff --git a/src/Internal/Workflow/Process/DeferredGenerator.php b/src/Internal/Workflow/Process/DeferredGenerator.php index 880732341..17d4a40cc 100644 --- a/src/Internal/Workflow/Process/DeferredGenerator.php +++ b/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; } } diff --git a/src/Internal/Workflow/Process/Scope.php b/src/Internal/Workflow/Process/Scope.php index 9a7c6fafd..26682112b 100644 --- a/src/Internal/Workflow/Process/Scope.php +++ b/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; } } diff --git a/tests/Acceptance/Harness/ChildWorkflow/ThrowOnExecuteTest.php b/tests/Acceptance/Harness/ChildWorkflow/ThrowOnExecuteTest.php index 3a8b77743..6c26c5528 100644 --- a/tests/Acceptance/Harness/ChildWorkflow/ThrowOnExecuteTest.php +++ b/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']])); } } diff --git a/tests/Fixtures/src/Workflow/GeneratorWorkflow.php b/tests/Fixtures/src/Workflow/GeneratorWorkflow.php index 7fbede754..7d228b829 100644 --- a/tests/Fixtures/src/Workflow/GeneratorWorkflow.php +++ b/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); diff --git a/tests/Functional/Client/TypedStubTestCase.php b/tests/Functional/Client/TypedStubTestCase.php index 3ae0b0625..13e3052c5 100644 --- a/tests/Functional/Client/TypedStubTestCase.php +++ b/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 */ diff --git a/tests/Unit/Workflow/DeferredGeneratorTestCase.php b/tests/Unit/Workflow/DeferredGeneratorTestCase.php new file mode 100644 index 000000000..691c9b375 --- /dev/null +++ b/tests/Unit/Workflow/DeferredGeneratorTestCase.php @@ -0,0 +1,271 @@ +compare( + fn() => (function () { + yield 1; + yield 42 => 2; + yield 3; + })(), + [ + 'current', 'key', 'current', 'key', + 'next', + 'current', 'key', 'current', 'key', 'valid', + 'next', + ['send', 'foo'], + 'current', 'key', 'current', 'key', 'valid', + ], + ); + } + + public function testCompareSendingValues(): void + { + $this->compare( + fn() => (function () { + $a = yield; + $b = yield $a; + $c = yield $b; + return [$a, $b, $c]; + })(), + [ + ['send', 'foo'], + ['send', 'bar'], + ['send', 'baz'], + 'current', 'key', 'current', 'key', 'valid', + ], + ); + } + + public function testCompareThrowingExceptions(): void + { + $this->compare( + fn() => (function () { + try { + yield; + throw new \Exception('foo'); + } catch (\Exception $e) { + yield $e->getMessage(); + } + })(), + [ + 'current', 'key', 'current', 'key', 'valid', + 'next', + 'current', 'key', 'current', 'key', 'valid', + 'next', + 'rewind', + ], + ); + } + + public function testCompareReturn(): void + { + $this->compare( + fn() => (function () { + yield 1; + return 2; + })(), + [ + 'current', 'key', 'current', 'key', 'valid', + 'next', + 'getReturn', + ], + ); + } + + public function testCompareEmpty(): void + { + $this->compare( + fn() => (function () { + yield from []; + })(), + [ + 'current', 'key', 'current', 'key', 'valid', + 'next', + 'rewind', + ], + ); + } + + public function testCompareEmptyReturn(): void + { + $this->compare( + fn() => (function () { + return; + yield; + })(), + [ + 'current', 'key', 'current', 'key', 'valid', + 'next', + 'getReturn', + ], + ); + } + + public function testCompareEmptyThrow(): void + { + $this->compare( + fn() => (function () { + throw new \Exception('foo'); + yield; + })(), + ['current', 'key', 'current', 'key', 'valid', 'getReturn', 'next', 'rewind'], + ); + } + + public function testCompareEmptyThrowValid(): void + { + $this->compare( + fn() => (function () { + throw new \Exception('foo'); + yield; + })(), + ['valid', 'valid'], + ); + } + + public function testCompareEmptyThrowGetReturn(): void + { + $this->compare( + fn() => (function () { + throw new \Exception('foo'); + yield; + })(), + ['getReturn', 'getReturn'], + ); + } + + public function testCompareEmptyThrowGetKey(): void + { + $this->compare( + fn() => (function () { + throw new \Exception('foo'); + yield; + })(), + ['key', 'key'], + ); + } + + public function testLazyNotGeneratorValidGetReturn(): void + { + $lazy = DeferredGenerator::fromHandler(fn() => 42, EncodedValues::empty()); + + $this->assertFalse($lazy->valid()); + $this->assertSame(42, $lazy->getReturn()); + } + + public function testLazyNotGeneratorCurrent(): void + { + $lazy = DeferredGenerator::fromHandler(fn() => 42, EncodedValues::empty()); + + $this->assertNull($lazy->current()); + } + + public function testLazyNotGeneratorWithException(): void + { + $lazy = DeferredGenerator::fromHandler(fn() => throw new \Exception('foo'), EncodedValues::empty()); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage('foo'); + + $lazy->current(); + } + + + public function testLazyNotGeneratorWithException2(): void + { + $lazy = DeferredGenerator::fromHandler(fn() => throw new \Exception('foo'), EncodedValues::empty()); + + try { + $lazy->current(); + } catch (\Exception) { + // ignore + } + + $this->assertNull($lazy->current()); + } + + /** + * @param callable(): \Generator $generatorFactory + * @param iterable $actions + * @return void + */ + private function compare( + callable $generatorFactory, + iterable $actions, + ): void { + $c1 = $c2 = null; + $caught = false; + $gen = $generatorFactory(); + $def = DeferredGenerator::fromGenerator($generatorFactory()); + $def->catch(function (\Throwable $e) use (&$c1) { + $c1 = $e; + }); + $lazy = DeferredGenerator::fromHandler($generatorFactory, EncodedValues::empty()); + $lazy->catch(function (\Throwable $e) use (&$c2) { + $c2 = $e; + }); + + + $i = 0; + foreach ($actions as $tuple) { + ++$i; + $argLess = \is_string($tuple); + $method = $argLess ? $tuple : $tuple[0]; + $arg = $argLess ? null : $tuple[1]; + $c1 = $c2 = $e = $e2 = $e3 = $result = $result2 = $result3 = null; + + try { + $result = $argLess ? $gen->$method() : $gen->$method($arg); + } catch (\Throwable $e) { + # ignore + } + + try { + $result2 = $argLess ? $def->$method() : $def->$method($arg); + } catch (\Throwable $e2) { + # ignore + } + + try { + $result3 = $argLess ? $lazy->$method() : $lazy->$method($arg); + } catch (\Throwable $e3) { + # ignore + } + + $this->assertSame($result, $result2, "Generator and DeferredGenerator results differ [$i] `$method`"); + $this->assertSame($result, $result3, "Generator and DeferredGenerator results differ [$i] `$method`"); + if ($caught) { + $this->assertNull($c1, "Error was caught twice [$i] `$method`"); + $this->assertNull($c2, "Error was caught twice [$i] `$method`"); + } + if ($e !== null) { + $this->assertNotNull($e2, "Generator and DeferredGenerator exceptions differ [$i] `$method`"); + $this->assertNotNull($e3, "Generator and DeferredGenerator exceptions differ [$i] `$method`"); + if (!$caught && !\in_array($method, ['rewind'], true)) { + $this->assertNotNull($c1, "Error was not caught [$i] `$method`"); + $this->assertNotNull($c2, "Error was not caught [$i] `$method`"); + $caught = true; + } + } else { + $this->assertNull($e2, "Generator and DeferredGenerator exceptions differ [$i] `$method`"); + $this->assertNull($e3, "Generator and DeferredGenerator exceptions differ [$i] `$method`"); + $this->assertNull($c1, "There must be no error caught [$i] `$method`"); + $this->assertNull($c2, "There must be no error caught [$i] `$method`"); + } + } + } +}