symfony · fabpot · Oct 2, 2020 · Sep 23, 2020
diff --git 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
@@ -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
@@ -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
@@ -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;
 }
diff --git a/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php b/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php
@@ -11,15 +11,17 @@
 
 namespace Symfony\Component\HttpClient\Retry;
 
-use Symfony\Contracts\HttpClient\ResponseInterface;
-
 /**
  * @author Jérémy Derussé <jeremy@derusse.com>
  */
 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
@@ -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,41 +52,71 @@ 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()) {
                     yield $chunk;
 
                     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
@@ -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
@@ -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
@@ -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();
+    }
 }