feature #38289 [HttpClient] provide response body to the RetryDecider (jderusse)

This PR was merged into the 5.2-dev branch. Discussion ---------- [HttpClient] provide response body to the RetryDecider | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | yes but for not-yet released 5.2 feature | Tickets | / | License | MIT | Doc PR | TODO Some servers, like AWS, does not always return standard HTTP code. The strategy needs to parse the body to take a decision. example: ``` 400 x-amzn-requestid: XXXXX content-type: application/x-amz-json-1.1 content-length: 58 date: Thu, 24 Sep 2020 11:17:35 GMT connection: close {"__type":"ThrottlingException","message":"Rate exceeded"} ```` This PR update the `RetryDeciderInterface` interface to let the decider notifying the Client when it need the body to take a decision. in that case, the Client, fetch te client, and call again the decider with the full body. usage ```php class Decider implements RetryDeciderInterface { public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseCode, array $responseHeaders, ?string $responseContent, \Throwable $throwable = null): ?bool { if (null !== $throwable) { return true; } if (in_array($responseCode, [423, 425, 429, 500, 502, 503, 504, 507, 510])) { return true; } if ( $responseCode !== 400 || $headers['content-type'][0] ?? null !== 'application/x-amz-json-1.1' || (int) $headers['content-length'][0] ?? '0' > 1024 ) { return false; } if (null === $responseContent) { return null; // null mean no decision taken and need to be called again with the body } $data = json_decode($responseContent, true); return $data['__type'] ?? '' === 'ThrottlingException'; } } ``` Commits ------- 321be5884d [HttpClient] provide response body to the RetryDecider
2020-10-02 07:36:08 +02:00 · 2020-10-02 07:36:08 +02:00 · b35bbdbde5
commit b35bbdbde5
parent 127724d519 321be5884d
9 changed files with 117 additions and 48 deletions
--- a/src/Symfony/Component/HttpClient/Response/AsyncResponse.php
+++ b/src/Symfony/Component/HttpClient/Response/AsyncResponse.php
@ -219,6 +219,11 @@ final class AsyncResponse implements ResponseInterface, StreamableInterface
                    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;
                }
--- 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 @@ final class ExponentialBackOff implements RetryBackOffInterface
        $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;

--- 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 @@ final class HttpStatusCodeDecider implements RetryDeciderInterface
        $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);
    }
 }
--- 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é <jeremy@derusse.com>
@ -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;
 }
--- 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é <jeremy@derusse.com>
 */
@ -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;
 }
--- a/src/Symfony/Component/HttpClient/RetryableHttpClient.php
+++ b/src/Symfony/Component/HttpClient/RetryableHttpClient.php
@ -15,7 +15,6 @@ use Psr\Log\LoggerInterface;
 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 @@ class RetryableHttpClient implements HttpClientInterface

    public function request(string $method, string $url, array $options = []): ResponseInterface
    {
-        $retryCount = 0;
+        if ($this->maxRetries <= 0) {
+            return $this->client->request($method, $url, $options);
+        }

-        return new AsyncResponse($this->client, $method, $url, $options, function (ChunkInterface $chunk, AsyncContext $context) use ($method, $url, $options, &$retryCount) {
+        $retryCount = 0;
+        $content = '';
+        $firstChunk = null;
+
+        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 @@ class RetryableHttpClient implements HttpClientInterface

                    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;
+            if (null === $exception) {
+                if ($chunk->isFirst()) {
+                    $shouldRetry = $this->decider->shouldRetry($method, $url, $options, $statusCode, $headers, null);

-                return;
+                    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 @@ class RetryableHttpClient implements HttpClientInterface

            $context->replaceRequest($method, $url, $options);
            $context->pause($delay / 1000);
+
+            if ($retryCount >= $this->maxRetries) {
+                $context->passthru();
+            }
        });
    }

--- 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 @@ class ExponentialBackOffTest extends TestCase
    {
        $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
--- 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));
    }
 }
--- a/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php
+++ b/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php
@ -8,11 +8,12 @@ use Symfony\Component\HttpClient\MockHttpClient;
 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 @@ class RetryableHttpClientTest extends TestCase
        self::assertSame(200, $response->getStatusCode());
    }

-    public function testRetryRespectStrategy(): void
+    public function testRetryRespectStrategy()
    {
        $client = new RetryableHttpClient(
            new MockHttpClient([
@ -47,4 +48,49 @@ class RetryableHttpClientTest extends TestCase
        $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();
+    }
 }