From b6d1e5d18882bd0eb5ce3c27649dbde9f62767bb Mon Sep 17 00:00:00 2001 From: Dmitriy Derepko Date: Fri, 6 Nov 2026 21:17:29 +0400 Subject: [PATCH] refactor(client): handle a failed status in the retry loop without try The only ServiceClientException in call() was thrown and caught in the same block. A custom stub that throws this exception is no longer retried. --- src/Client/GRPC/BaseClient.php | 109 +++++++++--------- tests/Unit/Client/GRPC/BaseClientTestCase.php | 30 ++--- tests/Unit/Client/GRPC/FailedCall.php | 19 +++ 3 files changed, 82 insertions(+), 76 deletions(-) create mode 100644 tests/Unit/Client/GRPC/FailedCall.php diff --git a/src/Client/GRPC/BaseClient.php b/src/Client/GRPC/BaseClient.php index b3941bea7..0fa7394aa 100644 --- a/src/Client/GRPC/BaseClient.php +++ b/src/Client/GRPC/BaseClient.php @@ -271,73 +271,72 @@ private function call(string $method, object $arg, ContextInterface $ctx): objec do { ++$attempt; - try { - $options = $ctx->getOptions(); - if ($deadline !== null) { - $diff = (new \DateTime())->diff($deadline); - $options['timeout'] = \max(0, (int) CarbonInterval::instance($diff)->totalMicroseconds); - } - /** @var UnaryCall $call */ - $call = $this->connection->getClient()->{$method}($arg, $ctx->getMetadata(), $options); - [$result, $status] = $call->wait(); + $options = $ctx->getOptions(); + if ($deadline !== null) { + $diff = (new \DateTime())->diff($deadline); + $options['timeout'] = \max(0, (int) CarbonInterval::instance($diff)->totalMicroseconds); + } - if ($status->code !== 0) { - throw new ServiceClientException($status); - } + /** @var UnaryCall $call */ + $call = $this->connection->getClient()->{$method}($arg, $ctx->getMetadata(), $options); + [$result, $status] = $call->wait(); + if ($status->code === 0) { \assert($result !== null); return $result; - } catch (ServiceClientException $e) { - if (!\in_array($e->getCode(), self::RETRYABLE_ERRORS, true)) { - if ($e->getCode() === StatusCode::DEADLINE_EXCEEDED) { - throw new TimeoutException($e->getMessage(), $e->getCode(), $e); - } - - if ($e->getCode() === StatusCode::CANCELLED) { - throw new CanceledException($e->getMessage(), $e->getCode(), $e); - } - - // non retryable - throw $e; - } + } + + $e = new ServiceClientException($status); - if ($retryOption->maximumAttempts !== 0 && $attempt >= $retryOption->maximumAttempts) { - // Reached maximum attempts - throw $e; + if (!\in_array($e->getCode(), self::RETRYABLE_ERRORS, true)) { + if ($e->getCode() === StatusCode::DEADLINE_EXCEEDED) { + throw new TimeoutException($e->getMessage(), $e->getCode(), $e); } - // Init interval values in milliseconds - $initialIntervalMs ??= $retryOption->initialInterval === null - ? (int) CarbonInterval::millisecond(50)->totalMilliseconds - : (int) (new CarbonInterval($retryOption->initialInterval))->totalMilliseconds; - $congestionInitialIntervalMs ??= $retryOption->congestionInitialInterval === null - ? (int) CarbonInterval::millisecond(1000)->totalMilliseconds - : (int) (new CarbonInterval($retryOption->congestionInitialInterval))->totalMilliseconds; - - $throttler ??= new BackoffThrottler( - maxInterval: $retryOption->maximumInterval !== null - ? (int) (new CarbonInterval($retryOption->maximumInterval))->totalMilliseconds - : $initialIntervalMs * 200, - maxJitterCoefficient: $retryOption->maximumJitterCoefficient, - backoffCoefficient: $retryOption->backoffCoefficient, - ); - - // Initial interval always depends on the *most recent* failure. - $baseInterval = $e->getCode() === StatusCode::RESOURCE_EXHAUSTED - ? $congestionInitialIntervalMs - : $initialIntervalMs; - - $wait = $throttler->calculateSleepTime(failureCount: $attempt, initialInterval: $baseInterval) * 1000; - - if ($deadline !== null && new \DateTimeImmutable("+{$wait} microseconds") > $deadline) { - throw new TimeoutException('Call timeout has been reached', $e->getCode(), $e); + if ($e->getCode() === StatusCode::CANCELLED) { + throw new CanceledException($e->getMessage(), $e->getCode(), $e); } - // wait till the next call - $this->usleep($wait); + // non retryable + throw $e; + } + + if ($retryOption->maximumAttempts !== 0 && $attempt >= $retryOption->maximumAttempts) { + // Reached maximum attempts + throw $e; } + + // Init interval values in milliseconds + $initialIntervalMs ??= $retryOption->initialInterval === null + ? (int) CarbonInterval::millisecond(50)->totalMilliseconds + : (int) (new CarbonInterval($retryOption->initialInterval))->totalMilliseconds; + $congestionInitialIntervalMs ??= $retryOption->congestionInitialInterval === null + ? (int) CarbonInterval::millisecond(1000)->totalMilliseconds + : (int) (new CarbonInterval($retryOption->congestionInitialInterval))->totalMilliseconds; + + $throttler ??= new BackoffThrottler( + maxInterval: $retryOption->maximumInterval !== null + ? (int) (new CarbonInterval($retryOption->maximumInterval))->totalMilliseconds + : $initialIntervalMs * 200, + maxJitterCoefficient: $retryOption->maximumJitterCoefficient, + backoffCoefficient: $retryOption->backoffCoefficient, + ); + + // Initial interval always depends on the *most recent* failure. + $baseInterval = $e->getCode() === StatusCode::RESOURCE_EXHAUSTED + ? $congestionInitialIntervalMs + : $initialIntervalMs; + + $wait = $throttler->calculateSleepTime(failureCount: $attempt, initialInterval: $baseInterval) * 1000; + + if ($deadline !== null && new \DateTimeImmutable("+{$wait} microseconds") > $deadline) { + throw new TimeoutException('Call timeout has been reached', $e->getCode(), $e); + } + + // wait till the next call + $this->usleep($wait); } while (true); } diff --git a/tests/Unit/Client/GRPC/BaseClientTestCase.php b/tests/Unit/Client/GRPC/BaseClientTestCase.php index c8300b7c5..e0f5271f6 100644 --- a/tests/Unit/Client/GRPC/BaseClientTestCase.php +++ b/tests/Unit/Client/GRPC/BaseClientTestCase.php @@ -15,7 +15,6 @@ use Temporal\Client\GRPC\Connection\ConnectionState; use Temporal\Client\GRPC\ContextInterface; use Temporal\Client\GRPC\ServiceClient; -use Temporal\Client\GRPC\StatusCode; use Temporal\Exception\Client\ServiceClientException; use Temporal\Exception\Client\TimeoutException; use Temporal\Internal\Interceptor\Pipeline; @@ -163,10 +162,10 @@ public function __construct( private readonly \ArrayObject $timeouts, ) {} - public function testCall(object $arg, array $metadata, array $options): void + public function testCall(object $arg, array $metadata, array $options): object { $this->timeouts[] = $options['timeout']; - throw new class((object) ['code' => StatusCode::UNKNOWN, 'metadata' => []]) extends ServiceClientException {}; + return new FailedCall(); } public function close(): void {} @@ -195,10 +194,10 @@ public function __construct( private readonly \ArrayObject $timeouts, ) {} - public function testCall(object $arg, array $metadata, array $options): void + public function testCall(object $arg, array $metadata, array $options): object { $this->timeouts[] = $options['timeout']; - throw new class((object) ['code' => StatusCode::UNKNOWN, 'metadata' => []]) extends ServiceClientException {}; + return new FailedCall(); } public function close(): void {} @@ -230,9 +229,9 @@ public function testServiceClientCallTimeoutDoesNotSleepPastTheDeadline(): void $client = $this->createClientMock(static fn() => new class extends WorkflowServiceClient { public function __construct() {} - public function testCall(): void + public function testCall(): object { - throw new class((object) ['code' => StatusCode::UNKNOWN, 'metadata' => []]) extends ServiceClientException {}; + return new FailedCall(); } public function close(): void {} @@ -292,20 +291,10 @@ public function testServiceClientCallMaximumAttemptsReached(): void $client = $this->createClientMock(fn() => new class extends WorkflowServiceClient { public function __construct() {} - public function testCall(): void + public function testCall(): object { static $counter = 0; - throw new class(++$counter) extends ServiceClientException { - public function __construct(public int $attempt) - { - parent::__construct((object) ['code' => StatusCode::UNKNOWN, 'metadata' => []]); - } - - public function isTestError(): bool - { - return true; - } - }; + return new FailedCall('attempt ' . ++$counter); } public function close(): void {} @@ -321,8 +310,7 @@ public function close(): void {} $client->testCall(); self::fail('Expected exception'); } catch (ServiceClientException $e) { - self::assertTrue($e->isTestError()); - self::assertSame(3, $e->attempt); + self::assertSame('attempt 3 (code: 2)', $e->getMessage()); } } diff --git a/tests/Unit/Client/GRPC/FailedCall.php b/tests/Unit/Client/GRPC/FailedCall.php new file mode 100644 index 000000000..3d69e3318 --- /dev/null +++ b/tests/Unit/Client/GRPC/FailedCall.php @@ -0,0 +1,19 @@ + StatusCode::UNKNOWN, 'details' => $this->details, 'metadata' => []]]; + } +}