From 321be5884d41c27483b35b01f3973eb651b71a46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Deruss=C3=A9?= Date: Wed, 23 Sep 2020 23:25:08 +0200 Subject: [PATCH] [HttpClient] provide response body to the RetryDecider --- .../HttpClient/Response/AsyncResponse.php | 5 ++ .../HttpClient/Retry/ExponentialBackOff.php | 6 +- .../Retry/HttpStatusCodeDecider.php | 11 +--- .../Retry/RetryBackOffInterface.php | 4 +- .../Retry/RetryDeciderInterface.php | 8 ++- .../HttpClient/RetryableHttpClient.php | 65 ++++++++++++++----- .../Tests/Retry/ExponentialBackOffTest.php | 3 +- .../Tests/Retry/HttpStatusCodeDeciderTest.php | 13 +--- .../Tests/RetryableHttpClientTest.php | 50 +++++++++++++- 9 files changed, 117 insertions(+), 48 deletions(-) diff --git a/src/Symfony/Component/HttpClient/Response/AsyncResponse.php b/src/Symfony/Component/HttpClient/Response/AsyncResponse.php index 3650706fd997a..a3375e3dc8206 100644 --- a/src/Symfony/Component/HttpClient/Response/AsyncResponse.php +++ b/src/Symfony/Component/HttpClient/Response/AsyncResponse.php @@ -219,6 +219,11 @@ public static function stream(iterable $responses, float $timeout = null, string continue; } + if (null === $chunk->getError() && $chunk->isFirst()) { + // Ensure no exception is thrown on destruct for the wrapped response + $r->response->getStatusCode(); + } + foreach (self::passthru($r->client, $r, $chunk, $asyncMap) as $chunk) { yield $r => $chunk; } diff --git a/src/Symfony/Component/HttpClient/Retry/ExponentialBackOff.php b/src/Symfony/Component/HttpClient/Retry/ExponentialBackOff.php index e8ff6dc5e59e5..022e0b247430d 100644 --- a/src/Symfony/Component/HttpClient/Retry/ExponentialBackOff.php +++ b/src/Symfony/Component/HttpClient/Retry/ExponentialBackOff.php @@ -11,8 +11,8 @@ namespace Symfony\Component\HttpClient\Retry; -use Symfony\Component\Messenger\Exception\InvalidArgumentException; -use Symfony\Contracts\HttpClient\ResponseInterface; +use Symfony\Component\HttpClient\Exception\InvalidArgumentException; +use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface; /** * A retry backOff with a constant or exponential retry delay. @@ -57,7 +57,7 @@ public function __construct(int $delayMilliseconds = 1000, float $multiplier = 2 $this->maxDelayMilliseconds = $maxDelayMilliseconds; } - public function getDelay(int $retryCount, string $requestMethod, string $requestUrl, array $requestOptions, ResponseInterface $partialResponse, \Throwable $throwable = null): int + public function getDelay(int $retryCount, string $requestMethod, string $requestUrl, array $requestOptions, int $responseStatusCode, array $responseHeaders, ?string $responseContent, ?TransportExceptionInterface $exception): int { $delay = $this->delayMilliseconds * $this->multiplier ** $retryCount; diff --git a/src/Symfony/Component/HttpClient/Retry/HttpStatusCodeDecider.php b/src/Symfony/Component/HttpClient/Retry/HttpStatusCodeDecider.php index 9e2b7a68b66d8..9415cd33dfeb8 100644 --- a/src/Symfony/Component/HttpClient/Retry/HttpStatusCodeDecider.php +++ b/src/Symfony/Component/HttpClient/Retry/HttpStatusCodeDecider.php @@ -11,9 +11,6 @@ namespace Symfony\Component\HttpClient\Retry; -use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface; -use Symfony\Contracts\HttpClient\ResponseInterface; - /** * Decides to retry the request when HTTP status codes belong to the given list of codes. * @@ -31,12 +28,8 @@ public function __construct(array $statusCodes = [423, 425, 429, 500, 502, 503, $this->statusCodes = $statusCodes; } - public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, ResponseInterface $partialResponse, \Throwable $throwable = null): bool + public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseStatusCode, array $responseHeaders, ?string $responseContent): ?bool { - if ($throwable instanceof TransportExceptionInterface) { - return true; - } - - return \in_array($partialResponse->getStatusCode(), $this->statusCodes, true); + return \in_array($responseStatusCode, $this->statusCodes, true); } } diff --git a/src/Symfony/Component/HttpClient/Retry/RetryBackOffInterface.php b/src/Symfony/Component/HttpClient/Retry/RetryBackOffInterface.php index 86f2503523820..2935119d21632 100644 --- a/src/Symfony/Component/HttpClient/Retry/RetryBackOffInterface.php +++ b/src/Symfony/Component/HttpClient/Retry/RetryBackOffInterface.php @@ -11,7 +11,7 @@ namespace Symfony\Component\HttpClient\Retry; -use Symfony\Contracts\HttpClient\ResponseInterface; +use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface; /** * @author Jérémy Derussé @@ -21,5 +21,5 @@ interface RetryBackOffInterface /** * Returns the time to wait in milliseconds. */ - public function getDelay(int $retryCount, string $requestMethod, string $requestUrl, array $requestOptions, ResponseInterface $partialResponse, \Throwable $throwable = null): int; + public function getDelay(int $retryCount, string $requestMethod, string $requestUrl, array $requestOptions, int $responseStatusCode, array $responseHeaders, ?string $responseContent, ?TransportExceptionInterface $exception): int; } diff --git a/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php b/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php index d7f9f12a878f8..fef606e697c87 100644 --- a/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php +++ b/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php @@ -11,8 +11,6 @@ namespace Symfony\Component\HttpClient\Retry; -use Symfony\Contracts\HttpClient\ResponseInterface; - /** * @author Jérémy Derussé */ @@ -20,6 +18,10 @@ interface RetryDeciderInterface { /** * Returns whether the request should be retried. + * + * @param ?string $responseContent Null is passed when the body did not arrive yet + * + * @return ?bool Returns null to signal that the body is required to take a decision */ - public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, ResponseInterface $partialResponse, \Throwable $throwable = null): bool; + public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseStatusCode, array $responseHeaders, ?string $responseContent): ?bool; } diff --git a/src/Symfony/Component/HttpClient/RetryableHttpClient.php b/src/Symfony/Component/HttpClient/RetryableHttpClient.php index 0a86383246aaa..cef53e426fc3f 100644 --- a/src/Symfony/Component/HttpClient/RetryableHttpClient.php +++ b/src/Symfony/Component/HttpClient/RetryableHttpClient.php @@ -15,7 +15,6 @@ use Psr\Log\NullLogger; use Symfony\Component\HttpClient\Response\AsyncContext; use Symfony\Component\HttpClient\Response\AsyncResponse; -use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Component\HttpClient\Retry\ExponentialBackOff; use Symfony\Component\HttpClient\Retry\HttpStatusCodeDecider; use Symfony\Component\HttpClient\Retry\RetryBackOffInterface; @@ -53,9 +52,15 @@ public function __construct(HttpClientInterface $client, RetryDeciderInterface $ public function request(string $method, string $url, array $options = []): ResponseInterface { + if ($this->maxRetries <= 0) { + return $this->client->request($method, $url, $options); + } + $retryCount = 0; + $content = ''; + $firstChunk = null; - return new AsyncResponse($this->client, $method, $url, $options, function (ChunkInterface $chunk, AsyncContext $context) use ($method, $url, $options, &$retryCount) { + return new AsyncResponse($this->client, $method, $url, $options, function (ChunkInterface $chunk, AsyncContext $context) use ($method, $url, $options, &$retryCount, &$content, &$firstChunk) { $exception = null; try { if ($chunk->isTimeout() || null !== $chunk->getInformationalStatus()) { @@ -63,31 +68,55 @@ public function request(string $method, string $url, array $options = []): Respo return; } - - // only retry first chunk - if (!$chunk->isFirst()) { - $context->passthru(); - yield $chunk; - - return; - } } catch (TransportExceptionInterface $exception) { // catch TransportExceptionInterface to send it to strategy. } $statusCode = $context->getStatusCode(); $headers = $context->getHeaders(); - if ($retryCount >= $this->maxRetries || !$this->decider->shouldRetry($method, $url, $options, $partialResponse = new MockResponse($context->getContent(), ['http_code' => $statusCode, 'headers' => $headers]), $exception)) { - $context->passthru(); - yield $chunk; - - return; + if (null === $exception) { + if ($chunk->isFirst()) { + $shouldRetry = $this->decider->shouldRetry($method, $url, $options, $statusCode, $headers, null); + + if (false === $shouldRetry) { + $context->passthru(); + yield $chunk; + + return; + } + + // Decider need body to decide + if (null === $shouldRetry) { + $firstChunk = $chunk; + $content = ''; + + return; + } + } else { + $content .= $chunk->getContent(); + if (!$chunk->isLast()) { + return; + } + $shouldRetry = $this->decider->shouldRetry($method, $url, $options, $statusCode, $headers, $content, null); + if (null === $shouldRetry) { + throw new \LogicException(sprintf('The "%s::shouldRetry" method must not return null when called with a body.', \get_class($this->decider))); + } + + if (false === $shouldRetry) { + $context->passthru(); + yield $firstChunk; + yield $context->createChunk($content); + $content = ''; + + return; + } + } } $context->setInfo('retry_count', $retryCount); $context->getResponse()->cancel(); - $delay = $this->getDelayFromHeader($headers) ?? $this->strategy->getDelay($retryCount, $method, $url, $options, $partialResponse, $exception); + $delay = $this->getDelayFromHeader($headers) ?? $this->strategy->getDelay($retryCount, $method, $url, $options, $statusCode, $headers, $chunk instanceof LastChunk ? $content : null, $exception); ++$retryCount; $this->logger->info('Error returned by the server. Retrying #{retryCount} using {delay} ms delay: '.($exception ? $exception->getMessage() : 'StatusCode: '.$statusCode), [ @@ -97,6 +126,10 @@ public function request(string $method, string $url, array $options = []): Respo $context->replaceRequest($method, $url, $options); $context->pause($delay / 1000); + + if ($retryCount >= $this->maxRetries) { + $context->passthru(); + } }); } diff --git a/src/Symfony/Component/HttpClient/Tests/Retry/ExponentialBackOffTest.php b/src/Symfony/Component/HttpClient/Tests/Retry/ExponentialBackOffTest.php index f97572ecc42fc..91ba50e59da0c 100644 --- a/src/Symfony/Component/HttpClient/Tests/Retry/ExponentialBackOffTest.php +++ b/src/Symfony/Component/HttpClient/Tests/Retry/ExponentialBackOffTest.php @@ -12,7 +12,6 @@ namespace Symfony\Component\HttpClient\Tests\Retry; use PHPUnit\Framework\TestCase; -use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Component\HttpClient\Retry\ExponentialBackOff; class ExponentialBackOffTest extends TestCase @@ -24,7 +23,7 @@ public function testGetDelay(int $delay, int $multiplier, int $maxDelay, int $pr { $backOff = new ExponentialBackOff($delay, $multiplier, $maxDelay); - self::assertSame($expectedDelay, $backOff->getDelay($previousRetries, 'GET', 'http://example.com/', [], new MockResponse(), null)); + self::assertSame($expectedDelay, $backOff->getDelay($previousRetries, 'GET', 'http://example.com/', [], 200, [], null, null)); } public function provideDelay(): iterable diff --git a/src/Symfony/Component/HttpClient/Tests/Retry/HttpStatusCodeDeciderTest.php b/src/Symfony/Component/HttpClient/Tests/Retry/HttpStatusCodeDeciderTest.php index 3c9a882b02e82..d634c15ae6559 100644 --- a/src/Symfony/Component/HttpClient/Tests/Retry/HttpStatusCodeDeciderTest.php +++ b/src/Symfony/Component/HttpClient/Tests/Retry/HttpStatusCodeDeciderTest.php @@ -12,30 +12,21 @@ namespace Symfony\Component\HttpClient\Tests\Retry; use PHPUnit\Framework\TestCase; -use Symfony\Component\HttpClient\Exception\TransportException; -use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Component\HttpClient\Retry\HttpStatusCodeDecider; class HttpStatusCodeDeciderTest extends TestCase { - public function testShouldRetryException() - { - $decider = new HttpStatusCodeDecider([500]); - - self::assertTrue($decider->shouldRetry('GET', 'http://example.com/', [], new MockResponse(), new TransportException())); - } - public function testShouldRetryStatusCode() { $decider = new HttpStatusCodeDecider([500]); - self::assertTrue($decider->shouldRetry('GET', 'http://example.com/', [], new MockResponse('', ['http_code' => 500]), null)); + self::assertTrue($decider->shouldRetry('GET', 'http://example.com/', [], 500, [], null)); } public function testIsNotRetryableOk() { $decider = new HttpStatusCodeDecider([500]); - self::assertFalse($decider->shouldRetry('GET', 'http://example.com/', [], new MockResponse(''), null)); + self::assertFalse($decider->shouldRetry('GET', 'http://example.com/', [], 200, [], null)); } } diff --git a/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php b/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php index c7b67117288cd..83a3179154fd2 100644 --- a/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php +++ b/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php @@ -8,11 +8,12 @@ use Symfony\Component\HttpClient\Response\MockResponse; use Symfony\Component\HttpClient\Retry\ExponentialBackOff; use Symfony\Component\HttpClient\Retry\HttpStatusCodeDecider; +use Symfony\Component\HttpClient\Retry\RetryDeciderInterface; use Symfony\Component\HttpClient\RetryableHttpClient; class RetryableHttpClientTest extends TestCase { - public function testRetryOnError(): void + public function testRetryOnError() { $client = new RetryableHttpClient( new MockHttpClient([ @@ -29,7 +30,7 @@ public function testRetryOnError(): void self::assertSame(200, $response->getStatusCode()); } - public function testRetryRespectStrategy(): void + public function testRetryRespectStrategy() { $client = new RetryableHttpClient( new MockHttpClient([ @@ -47,4 +48,49 @@ public function testRetryRespectStrategy(): void $this->expectException(ServerException::class); $response->getHeaders(); } + + public function testRetryWithBody() + { + $client = new RetryableHttpClient( + new MockHttpClient([ + new MockResponse('', ['http_code' => 500]), + new MockResponse('', ['http_code' => 200]), + ]), + new class() implements RetryDeciderInterface { + public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseCode, array $responseHeaders, ?string $responseContent): ?bool + { + return null === $responseContent ? null : 200 !== $responseCode; + } + }, + new ExponentialBackOff(0), + 1 + ); + + $response = $client->request('GET', 'http://example.com/foo-bar'); + + self::assertSame(200, $response->getStatusCode()); + } + + public function testRetryWithBodyInvalid() + { + $client = new RetryableHttpClient( + new MockHttpClient([ + new MockResponse('', ['http_code' => 500]), + new MockResponse('', ['http_code' => 200]), + ]), + new class() implements RetryDeciderInterface { + public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseCode, array $responseHeaders, ?string $responseContent, \Throwable $throwable = null): ?bool + { + return null; + } + }, + new ExponentialBackOff(0), + 1 + ); + + $response = $client->request('GET', 'http://example.com/foo-bar'); + + $this->expectExceptionMessageMatches('/must not return null when called with a body/'); + $response->getHeaders(); + } }