From cb45a927cd9385dbc8de6077d5d448ba22f9a9ea Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Wed, 12 Jul 2023 23:41:11 +0400 Subject: [PATCH 1/4] Fix exceptions propagation from Signal methods --- src/Internal/Declaration/WorkflowInstance.php | 5 ++ .../WorkflowInstance/SignalQueue.php | 5 ++ .../Declaration/WorkflowInstanceInterface.php | 2 + src/Internal/Workflow/Process/Scope.php | 19 +++-- src/Internal/Workflow/WorkflowContext.php | 4 + .../src/Workflow/SignalExceptionsWorkflow.php | 75 +++++++++++++++++++ tests/Functional/Client/AwaitTestCase.php | 11 ++- tests/Functional/Client/FailureTestCase.php | 65 ++++++++++++++++ tests/docker-compose.yaml | 6 +- 9 files changed, 181 insertions(+), 11 deletions(-) create mode 100644 tests/Fixtures/src/Workflow/SignalExceptionsWorkflow.php diff --git a/src/Internal/Declaration/WorkflowInstance.php b/src/Internal/Declaration/WorkflowInstance.php index dfc7f800..0b0feb9a 100644 --- a/src/Internal/Declaration/WorkflowInstance.php +++ b/src/Internal/Declaration/WorkflowInstance.php @@ -121,4 +121,9 @@ public function addSignalHandler(string $name, callable $handler): void $this->signalHandlers[$name] = $this->createCallableHandler($handler); $this->signalQueue->attach($name, $this->signalHandlers[$name]); } + + public function clearSignalQueue(): void + { + $this->signalQueue->clear(); + } } diff --git a/src/Internal/Declaration/WorkflowInstance/SignalQueue.php b/src/Internal/Declaration/WorkflowInstance/SignalQueue.php index 5fb9a1f7..0b1bdba0 100644 --- a/src/Internal/Declaration/WorkflowInstance/SignalQueue.php +++ b/src/Internal/Declaration/WorkflowInstance/SignalQueue.php @@ -67,6 +67,11 @@ public function attach(string $signal, callable $consumer): void $this->flush($signal); } + public function clear(): void + { + $this->queue = []; + } + /** * @param string $signal * @psalm-suppress UnusedVariable diff --git a/src/Internal/Declaration/WorkflowInstanceInterface.php b/src/Internal/Declaration/WorkflowInstanceInterface.php index 3cefc378..ec953ed1 100644 --- a/src/Internal/Declaration/WorkflowInstanceInterface.php +++ b/src/Internal/Declaration/WorkflowInstanceInterface.php @@ -41,4 +41,6 @@ public function getSignalHandler(string $name): \Closure; * @param callable $handler */ public function addSignalHandler(string $name, callable $handler): void; + + public function clearSignalQueue(): void; } diff --git a/src/Internal/Workflow/Process/Scope.php b/src/Internal/Workflow/Process/Scope.php index cdcffdf4..7cd8779a 100644 --- a/src/Internal/Workflow/Process/Scope.php +++ b/src/Internal/Workflow/Process/Scope.php @@ -167,6 +167,7 @@ public function getContext(): WorkflowContext public function start(callable $handler, ValuesInterface $values = null): void { try { + // Create a coroutine generator $this->coroutine = $this->call($handler, $values ?? EncodedValues::empty()); $this->context->resolveConditions(); } catch (\Throwable $e) { @@ -322,16 +323,20 @@ function () use ($cancelID): void { */ protected function call(callable $handler, ValuesInterface $values): \Generator { - $this->makeCurrent(); - $result = $handler($values); + try { + $this->makeCurrent(); + $result = $handler($values); - if ($result instanceof \Generator) { - yield from $result; + if ($result instanceof \Generator) { + yield from $result; - return $result->getReturn(); - } + return $result->getReturn(); + } - return $result; + return $result; + } catch (\Throwable $e) { + $this->onException($e); + } } /** diff --git a/src/Internal/Workflow/WorkflowContext.php b/src/Internal/Workflow/WorkflowContext.php index 2cc3ab8c..212ad412 100644 --- a/src/Internal/Workflow/WorkflowContext.php +++ b/src/Internal/Workflow/WorkflowContext.php @@ -228,6 +228,10 @@ public function isReplaying(): bool */ public function complete(array $result = null, \Throwable $failure = null): PromiseInterface { + if ($failure !== null) { + $this->workflowInstance->clearSignalQueue(); + } + if ($result !== null) { $values = EncodedValues::fromValues($result); } else { diff --git a/tests/Fixtures/src/Workflow/SignalExceptionsWorkflow.php b/tests/Fixtures/src/Workflow/SignalExceptionsWorkflow.php new file mode 100644 index 00000000..5cb07add --- /dev/null +++ b/tests/Fixtures/src/Workflow/SignalExceptionsWorkflow.php @@ -0,0 +1,75 @@ + $this->greetings !== [] || $this->exit); + if ($this->greetings === [] && $this->exit) { + return $received; + } + + $message = array_shift($this->greetings); + $received[] = $message; + } + } + + #[SignalMethod] + public function failWithName(string $name): void + { + $this->greetings[] = $name; + throw new \RuntimeException("Signal exception $name"); + } + + #[SignalMethod] + public function failInvalidArgument($name = 'foo'): void + { + $this->greetings[] = "invalidArgument $name"; + throw new InvalidArgumentException("Invalid argument $name"); + } + + #[SignalMethod] + public function failActivity($name = 'foo') + { + yield Workflow::newUntypedActivityStub( + ActivityOptions::new() + ->withScheduleToStartTimeout(1) + ->withRetryOptions( + RetryOptions::new()->withMaximumAttempts(1) + ) + ->withStartToCloseTimeout(1), + )->execute('nonExistingActivityName', [$name]); + } + + #[SignalMethod] + public function exit(): void + { + $this->exit = true; + } +} diff --git a/tests/Functional/Client/AwaitTestCase.php b/tests/Functional/Client/AwaitTestCase.php index 2b4614a7..b142ed18 100644 --- a/tests/Functional/Client/AwaitTestCase.php +++ b/tests/Functional/Client/AwaitTestCase.php @@ -133,7 +133,16 @@ public function testFailSignalSerialization() $wait->addValue('test3'); // breaks the invocation - $wait->addValue(['hello'], 123); + // + // Throws: + // Temporal\Exception\Failure\ApplicationFailure + // Previous: + // Temporal\Exception\InvalidArgumentException: + // The passed value of type "array" can not be converted to required type "string" in + // src\Internal\Declaration\Dispatcher\AutowiredPayloads.php:34 + // + // todo should it be retried by default? + // $wait->addValue(['hello'], 123); $wait->addValue('test4'); diff --git a/tests/Functional/Client/FailureTestCase.php b/tests/Functional/Client/FailureTestCase.php index 88297e2d..c6f12328 100644 --- a/tests/Functional/Client/FailureTestCase.php +++ b/tests/Functional/Client/FailureTestCase.php @@ -11,10 +11,13 @@ namespace Temporal\Tests\Functional\Client; +use PHPUnit\Framework\AssertionFailedError; use Temporal\Exception\Client\WorkflowFailedException; +use Temporal\Exception\Client\WorkflowNotFoundException; use Temporal\Exception\Failure\ActivityFailure; use Temporal\Exception\Failure\ApplicationFailure; use Temporal\Exception\Failure\ChildWorkflowFailure; +use Temporal\Tests\Workflow\SignalExceptionsWorkflow; /** * @group client @@ -80,4 +83,66 @@ public function testChildWorkflowFailurePropagation() $this->assertStringContainsString('SimpleActivity->fail', $e->getPrevious()->getMessage()); } } + + public function testSignalThatThrowsCustomError() + { + $client = $this->createClient(); + $wf = $client->newWorkflowStub(SignalExceptionsWorkflow::class); + + $run = $client->start($wf); + + $wf->failWithName('test1'); + + try { + // The next + sleep(1); + $wf->exit(); + $this->fail('Signal must fail'); + } catch (AssertionFailedError $e) { + throw $e; + } catch (\Throwable $e) { + $this->assertInstanceOf(WorkflowNotFoundException::class, $e); + // \dump($e); + } + + $this->expectException(WorkflowFailedException::class); + $result = $run->getResult(); + $this->fail(sprintf("Workflow must fail. Got result %s", \print_r($result, true))); + } + + public function testSignalThatThrowsInvalidArgumentException() + { + $client = $this->createClient(); + $wf = $client->newWorkflowStub(SignalExceptionsWorkflow::class); + + $run = $client->start($wf); + + $wf->failInvalidArgument('test1'); + + $this->expectException(WorkflowFailedException::class); + $result = $run->getResult(); + $this->fail(sprintf("Workflow must fail. Got result %s", \print_r($result, true))); + } + + public function testSignalThatThrowsInternalException() + { + $client = $this->createClient(); + $wf = $client->newWorkflowStub(SignalExceptionsWorkflow::class); + + $run = $client->startWithSignal($wf, 'failActivity', ['foo']); + + try { + sleep(5); + $wf->failActivity('foo'); + $this->fail('Signal must fail'); + } catch (AssertionFailedError $e) { + throw $e; + } catch (\Throwable $e) { + $this->assertInstanceOf(WorkflowNotFoundException::class, $e); + } + + $this->expectException(WorkflowFailedException::class); + $result = $run->getResult(); + $this->fail(sprintf("Workflow must fail. Got result %s", \print_r($result, true))); + } } diff --git a/tests/docker-compose.yaml b/tests/docker-compose.yaml index dbd68e6e..fd936663 100644 --- a/tests/docker-compose.yaml +++ b/tests/docker-compose.yaml @@ -6,7 +6,7 @@ services: ports: - "9042:9042" temporal: - image: temporalio/auto-setup:1.16.2 + image: temporalio/auto-setup:1.21.1.0 ports: - "7233:7233" volumes: @@ -17,7 +17,7 @@ services: depends_on: - cassandra temporal-admin-tools: - image: temporalio/admin-tools:1.16.2 + image: temporalio/admin-tools:1.21.1.0 stdin_open: true tty: true environment: @@ -25,7 +25,7 @@ services: depends_on: - temporal temporal-ui: - image: temporalio/ui:2.5.0 + image: temporalio/ui:2.16.2 environment: - TEMPORAL_ADDRESS=temporal:7233 - TEMPORAL_CORS_ORIGINS=http://localhost:3000 From 5be4cd286aef51ae6dcc77ac99eac3dedb3cc181 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 17 Jul 2023 18:02:37 +0400 Subject: [PATCH 2/4] Make SDK's InvalidArgumentException retryable; fix a deprecated interpolation; --- src/Activity/ActivityCancellationType.php | 2 +- src/Exception/ExceptionInterceptor.php | 2 +- .../src/Workflow/SignalExceptionsWorkflow.php | 6 ++++ tests/Functional/Client/AwaitTestCase.php | 32 +++++-------------- tests/Functional/Client/FailureTestCase.php | 16 ++++++++++ 5 files changed, 32 insertions(+), 26 deletions(-) diff --git a/src/Activity/ActivityCancellationType.php b/src/Activity/ActivityCancellationType.php index 9c596756..ef459a76 100644 --- a/src/Activity/ActivityCancellationType.php +++ b/src/Activity/ActivityCancellationType.php @@ -63,7 +63,7 @@ public function serialize($value) return false; default: - $error = "Option #${value} is currently not supported"; + $error = "Option #{$value} is currently not supported"; throw new \InvalidArgumentException($error); } } diff --git a/src/Exception/ExceptionInterceptor.php b/src/Exception/ExceptionInterceptor.php index 4eb08aff..866775cd 100644 --- a/src/Exception/ExceptionInterceptor.php +++ b/src/Exception/ExceptionInterceptor.php @@ -50,6 +50,6 @@ public function isRetryable(\Throwable $e): bool */ public static function createDefault(): self { - return new self([\Error::class]); + return new self([\Error::class, InvalidArgumentException::class]); } } diff --git a/tests/Fixtures/src/Workflow/SignalExceptionsWorkflow.php b/tests/Fixtures/src/Workflow/SignalExceptionsWorkflow.php index 5cb07add..044b2537 100644 --- a/tests/Fixtures/src/Workflow/SignalExceptionsWorkflow.php +++ b/tests/Fixtures/src/Workflow/SignalExceptionsWorkflow.php @@ -67,6 +67,12 @@ public function failActivity($name = 'foo') )->execute('nonExistingActivityName', [$name]); } + #[SignalMethod] + public function failRetryable() + { + 10 / 0; + } + #[SignalMethod] public function exit(): void { diff --git a/tests/Functional/Client/AwaitTestCase.php b/tests/Functional/Client/AwaitTestCase.php index b142ed18..1b2c9ff0 100644 --- a/tests/Functional/Client/AwaitTestCase.php +++ b/tests/Functional/Client/AwaitTestCase.php @@ -12,6 +12,7 @@ namespace Temporal\Tests\Functional\Client; use Temporal\DataConverter\Type; +use Temporal\Exception\Client\TimeoutException; use Temporal\Exception\Client\WorkflowFailedException; use Temporal\Exception\Failure\ActivityFailure; use Temporal\Exception\Failure\ApplicationFailure; @@ -132,33 +133,16 @@ public function testFailSignalSerialization() $wait->addValue('test2'); $wait->addValue('test3'); - // breaks the invocation - // - // Throws: - // Temporal\Exception\Failure\ApplicationFailure - // Previous: - // Temporal\Exception\InvalidArgumentException: - // The passed value of type "array" can not be converted to required type "string" in - // src\Internal\Declaration\Dispatcher\AutowiredPayloads.php:34 - // - // todo should it be retried by default? - // $wait->addValue(['hello'], 123); + $wait->addValue(['hello'], 123); + \sleep(1); + // There is no any exception because the workflow has not failed after signal with invalid data $wait->addValue('test4'); - $result = $run->getResult(); - asort($result); - $result = array_values($result); - - $this->assertSame( - [ - 'IN SIGNAL 2 IN SIGNAL TEST1', - 'IN SIGNAL 2 IN SIGNAL TEST2', - 'IN SIGNAL 2 IN SIGNAL TEST3', - 'IN SIGNAL 2 IN SIGNAL TEST4' - ], - $result - ); + // The workflow will be in the `running` state. By the reason the TimeoutException will be thrown + // on getResult with timeout + $this->expectException(TimeoutException::class); + $run->getResult(timeout: 2); } public function testFailSignalErrored() diff --git a/tests/Functional/Client/FailureTestCase.php b/tests/Functional/Client/FailureTestCase.php index c6f12328..351ede99 100644 --- a/tests/Functional/Client/FailureTestCase.php +++ b/tests/Functional/Client/FailureTestCase.php @@ -84,6 +84,22 @@ public function testChildWorkflowFailurePropagation() } } + public function testSignalThatThrowsRetryableException() + { + $client = $this->createClient(); + $wf = $client->newWorkflowStub(SignalExceptionsWorkflow::class); + + $run = $client->start($wf); + + $wf->failRetryable(); + + sleep(1); + $wf->exit(); + + // There is no any exception because the workflow has not failed after the `failRetryable` signal. + $this->assertTrue(true); + } + public function testSignalThatThrowsCustomError() { $client = $this->createClient(); From 2d6352190a3d083d1104c4111a4b20a79300b2e0 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Thu, 20 Jul 2023 14:40:26 +0400 Subject: [PATCH 3/4] Skip deserialization errors when Signal method is called --- src/Exception/ExceptionInterceptor.php | 2 +- src/Internal/Workflow/Process/Scope.php | 8 +++++- .../LoopWithSignalCoroutinesWorkflow.php | 2 +- tests/Functional/Client/AwaitTestCase.php | 25 +++++++++++++------ 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/Exception/ExceptionInterceptor.php b/src/Exception/ExceptionInterceptor.php index 866775cd..4eb08aff 100644 --- a/src/Exception/ExceptionInterceptor.php +++ b/src/Exception/ExceptionInterceptor.php @@ -50,6 +50,6 @@ public function isRetryable(\Throwable $e): bool */ public static function createDefault(): self { - return new self([\Error::class, InvalidArgumentException::class]); + return new self([\Error::class]); } } diff --git a/src/Internal/Workflow/Process/Scope.php b/src/Internal/Workflow/Process/Scope.php index 7cd8779a..41d603b1 100644 --- a/src/Internal/Workflow/Process/Scope.php +++ b/src/Internal/Workflow/Process/Scope.php @@ -19,6 +19,7 @@ use Temporal\Exception\DestructMemorizedInstanceException; use Temporal\Exception\Failure\CanceledFailure; use Temporal\Exception\Failure\TemporalFailure; +use Temporal\Exception\InvalidArgumentException; use Temporal\Internal\ServiceContainer; use Temporal\Internal\Transport\Request\Cancel; use Temporal\Internal\Workflow\ScopeContext; @@ -325,7 +326,12 @@ protected function call(callable $handler, ValuesInterface $values): \Generator { try { $this->makeCurrent(); - $result = $handler($values); + try { + $result = $handler($values); + } catch (InvalidArgumentException) { + // Skip deserialization errors + return null; + } if ($result instanceof \Generator) { yield from $result; diff --git a/tests/Fixtures/src/Workflow/LoopWithSignalCoroutinesWorkflow.php b/tests/Fixtures/src/Workflow/LoopWithSignalCoroutinesWorkflow.php index da6c50ea..d304a8a5 100644 --- a/tests/Fixtures/src/Workflow/LoopWithSignalCoroutinesWorkflow.php +++ b/tests/Fixtures/src/Workflow/LoopWithSignalCoroutinesWorkflow.php @@ -30,7 +30,7 @@ public function __construct() $this->simple = Workflow::newActivityStub( SimpleActivity::class, ActivityOptions::new() - ->withStartToCloseTimeout(5) + ->withStartToCloseTimeout(10) ->withRetryOptions(RetryOptions::new()->withMaximumAttempts(1)) ); } diff --git a/tests/Functional/Client/AwaitTestCase.php b/tests/Functional/Client/AwaitTestCase.php index 1b2c9ff0..44dbfc5f 100644 --- a/tests/Functional/Client/AwaitTestCase.php +++ b/tests/Functional/Client/AwaitTestCase.php @@ -12,7 +12,6 @@ namespace Temporal\Tests\Functional\Client; use Temporal\DataConverter\Type; -use Temporal\Exception\Client\TimeoutException; use Temporal\Exception\Client\WorkflowFailedException; use Temporal\Exception\Failure\ActivityFailure; use Temporal\Exception\Failure\ApplicationFailure; @@ -133,16 +132,28 @@ public function testFailSignalSerialization() $wait->addValue('test2'); $wait->addValue('test3'); + /** + * Breaks the invocation + * Deserialization errors must be ignored. Called Signal method in this case will be skipped. + * @link https://github.com/temporalio/sdk-php/pull/331 + */ $wait->addValue(['hello'], 123); - \sleep(1); - // There is no any exception because the workflow has not failed after signal with invalid data $wait->addValue('test4'); - // The workflow will be in the `running` state. By the reason the TimeoutException will be thrown - // on getResult with timeout - $this->expectException(TimeoutException::class); - $run->getResult(timeout: 2); + $result = $run->getResult(); + \asort($result); + $result = \array_values($result); + + $this->assertSame( + [ + 'IN SIGNAL 2 IN SIGNAL TEST1', + 'IN SIGNAL 2 IN SIGNAL TEST2', + 'IN SIGNAL 2 IN SIGNAL TEST3', + 'IN SIGNAL 2 IN SIGNAL TEST4' + ], + $result + ); } public function testFailSignalErrored() From 9f962fd0d9bbf7fdf6b7138998fafd635468eb44 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Fri, 21 Jul 2023 20:52:58 +0400 Subject: [PATCH 4/4] Add test that start a workflow with bad arguments; improve behavior: decoder exceptions must be skipped in signal methods only --- src/Internal/Workflow/Process/Process.php | 7 +--- src/Internal/Workflow/Process/Scope.php | 37 ++++++++++++++++++- tests/Functional/Client/FailureTestCase.php | 4 +- .../Client/UntypedWorkflowStubTestCase.php | 10 +++++ 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/src/Internal/Workflow/Process/Process.php b/src/Internal/Workflow/Process/Process.php index 64ec6243..9f16fddf 100644 --- a/src/Internal/Workflow/Process/Process.php +++ b/src/Internal/Workflow/Process/Process.php @@ -14,7 +14,6 @@ use JetBrains\PhpStorm\Pure; use Temporal\DataConverter\ValuesInterface; use Temporal\Exception\DestructMemorizedInstanceException; -use Temporal\Exception\InvalidArgumentException; use Temporal\Internal\Declaration\WorkflowInstanceInterface; use Temporal\Internal\ServiceContainer; use Temporal\Internal\Workflow\WorkflowContext; @@ -44,11 +43,7 @@ function (?\Throwable $error): void { } ); - try { - $scope->start($handler); - } catch (InvalidArgumentException $e) { - // invalid signal invocation, destroy the scope with no traces - } + $scope->startSignal($handler); } ); diff --git a/src/Internal/Workflow/Process/Scope.php b/src/Internal/Workflow/Process/Scope.php index 41d603b1..545ba177 100644 --- a/src/Internal/Workflow/Process/Scope.php +++ b/src/Internal/Workflow/Process/Scope.php @@ -179,6 +179,17 @@ public function start(callable $handler, ValuesInterface $values = null): void $this->next(); } + /** + * @param callable $handler + */ + public function startSignal(callable $handler): void + { + // Create a coroutine generator + $this->coroutine = $this->callSignalHandler($handler); + $this->context->resolveConditions(); + $this->next(); + } + /** * @param \Generator $generator * @return self @@ -323,11 +334,35 @@ function () use ($cancelID): void { * @return \Generator */ protected function call(callable $handler, ValuesInterface $values): \Generator + { + try { + $this->makeCurrent(); + $result = $handler($values); + + if ($result instanceof \Generator) { + yield from $result; + + return $result->getReturn(); + } + + return $result; + } catch (\Throwable $e) { + $this->onException($e); + } + } + + /** + * Call a Signal method. In this case deserialization errors are skipped. + * + * @param callable $handler + * @return \Generator + */ + protected function callSignalHandler(callable $handler): \Generator { try { $this->makeCurrent(); try { - $result = $handler($values); + $result = $handler(); } catch (InvalidArgumentException) { // Skip deserialization errors return null; diff --git a/tests/Functional/Client/FailureTestCase.php b/tests/Functional/Client/FailureTestCase.php index 351ede99..275dc893 100644 --- a/tests/Functional/Client/FailureTestCase.php +++ b/tests/Functional/Client/FailureTestCase.php @@ -111,7 +111,7 @@ public function testSignalThatThrowsCustomError() try { // The next - sleep(1); + sleep(2); $wf->exit(); $this->fail('Signal must fail'); } catch (AssertionFailedError $e) { @@ -148,7 +148,7 @@ public function testSignalThatThrowsInternalException() $run = $client->startWithSignal($wf, 'failActivity', ['foo']); try { - sleep(5); + sleep(8); $wf->failActivity('foo'); $this->fail('Signal must fail'); } catch (AssertionFailedError $e) { diff --git a/tests/Functional/Client/UntypedWorkflowStubTestCase.php b/tests/Functional/Client/UntypedWorkflowStubTestCase.php index b2a90245..8822f1fa 100644 --- a/tests/Functional/Client/UntypedWorkflowStubTestCase.php +++ b/tests/Functional/Client/UntypedWorkflowStubTestCase.php @@ -38,6 +38,16 @@ public function testUntypedStartAndWaitResult() $this->assertSame('HELLO WORLD', $simple->getResult()); } + public function testUntypedStartWithWrongData() + { + $client = $this->createClient(); + $simple = $client->newUntypedWorkflowStub('SimpleWorkflow'); + $client->start($simple, ['hello world']); + + $this->expectException(WorkflowFailedException::class); + $simple->getResult(); + } + public function testUntypedStartViaClient() { $client = $this->createClient();