[HttpClient] simplify retry mechanism around RetryStrategyInterface

Author: nicolas-grekas

Response/AmpResponse.php

@@ -28,7 +28,6 @@
 use Symfony\Component\HttpClient\Exception\TransportException;
 use Symfony\Component\HttpClient\HttpClientTrait;
 use Symfony\Component\HttpClient\Internal\AmpBody;
-use Symfony\Component\HttpClient\Internal\AmpCanary;
 use Symfony\Component\HttpClient\Internal\AmpClientState;
 use Symfony\Component\HttpClient\Internal\Canary;
 use Symfony\Component\HttpClient\Internal\ClientState;

Retry/ExponentialBackOff.php

@@ -0,0 +1,84 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpClient\Retry;
+
+use Symfony\Component\HttpClient\Response\AsyncContext;
+use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
+
+/**
+ * Decides to retry the request when HTTP status codes belong to the given list of codes.
+ *
+ * @author Jérémy Derussé <[email protected]>
+ */
+class GenericRetryStrategy implements RetryStrategyInterface
+{
+    public const DEFAULT_RETRY_STATUS_CODES = [423, 425, 429, 500, 502, 503, 504, 507, 510];
+
+    private $statusCodes;
+    private $delayMs;
+    private $multiplier;
+    private $maxDelayMs;
+    private $jitter;
+
+    /**
+     * @param array $statusCodes List of HTTP status codes that trigger a retry
+     * @param int   $delayMs     Amount of time to delay (or the initial value when multiplier is used)
+     * @param float $multiplier  Multiplier to apply to the delay each time a retry occurs
+     * @param int   $maxDelayMs  Maximum delay to allow (0 means no maximum)
+     * @param float $jitter      Probability of randomness int delay (0 = none, 1 = 100% random)
+     */
+    public function __construct(array $statusCodes = self::DEFAULT_RETRY_STATUS_CODES, int $delayMs = 1000, float $multiplier = 2.0, int $maxDelayMs = 0, float $jitter = 0.1)
+    {
+        $this->statusCodes = $statusCodes;
+
+        if ($delayMs < 0) {
+            throw new InvalidArgumentException(sprintf('Delay must be greater than or equal to zero: "%s" given.', $delayMs));
+        }
+        $this->delayMs = $delayMs;
+
+        if ($multiplier < 1) {
+            throw new InvalidArgumentException(sprintf('Multiplier must be greater than or equal to one: "%s" given.', $multiplier));
+        }
+        $this->multiplier = $multiplier;
+
+        if ($maxDelayMs < 0) {
+            throw new InvalidArgumentException(sprintf('Max delay must be greater than or equal to zero: "%s" given.', $maxDelayMs));
+        }
+        $this->maxDelayMs = $maxDelayMs;
+
+        if ($jitter < 0 || $jitter > 1) {
+            throw new InvalidArgumentException(sprintf('Jitter must be between 0 and 1: "%s" given.', $jitter));
+        }
+        $this->jitter = $jitter;
+    }
+
+    public function shouldRetry(AsyncContext $context, ?string $responseContent, ?TransportExceptionInterface $exception): ?bool
+    {
+        return \in_array($context->getStatusCode(), $this->statusCodes, true);
+    }
+
+    public function getDelay(AsyncContext $context, ?string $responseContent, ?TransportExceptionInterface $exception): int
+    {
+        $delay = $this->delayMs * $this->multiplier ** $context->getInfo('retry_count');
+
+        if ($this->jitter > 0) {
+            $randomness = $delay * $this->jitter;
+            $delay = $delay + random_int(-$randomness, +$randomness);
+        }
+
+        if ($delay > $this->maxDelayMs && 0 !== $this->maxDelayMs) {
+            return $this->maxDelayMs;
+        }
+
+        return (int) $delay;
+    }
+}

Retry/GenericRetryStrategy.php

@@ -11,10 +11,14 @@
 
 namespace Symfony\Component\HttpClient\Retry;
 
+use Symfony\Component\HttpClient\Response\AsyncContext;
+use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
+
 /**
  * @author Jérémy Derussé <[email protected]>
+ * @author Nicolas Grekas <[email protected]>
  */
-interface RetryDeciderInterface
+interface RetryStrategyInterface
 {
     /**
      * Returns whether the request should be retried.
@@ -23,5 +27,10 @@ interface RetryDeciderInterface
      *
      * @return ?bool Returns null to signal that the body is required to take a decision
      */
-    public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseStatusCode, array $responseHeaders, ?string $responseContent): ?bool;
+    public function shouldRetry(AsyncContext $context, ?string $responseContent, ?TransportExceptionInterface $exception): ?bool;
+
+    /**
+     * Returns the time to wait in milliseconds.
+     */
+    public function getDelay(AsyncContext $context, ?string $responseContent, ?TransportExceptionInterface $exception): int;
 }

Retry/HttpStatusCodeDecider.php

@@ -15,10 +15,8 @@
 use Psr\Log\NullLogger;
 use Symfony\Component\HttpClient\Response\AsyncContext;
 use Symfony\Component\HttpClient\Response\AsyncResponse;
-use Symfony\Component\HttpClient\Retry\ExponentialBackOff;
-use Symfony\Component\HttpClient\Retry\HttpStatusCodeDecider;
-use Symfony\Component\HttpClient\Retry\RetryBackOffInterface;
-use Symfony\Component\HttpClient\Retry\RetryDeciderInterface;
+use Symfony\Component\HttpClient\Retry\GenericRetryStrategy;
+use Symfony\Component\HttpClient\Retry\RetryStrategyInterface;
 use Symfony\Contracts\HttpClient\ChunkInterface;
 use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
@@ -33,19 +31,17 @@ class RetryableHttpClient implements HttpClientInterface
 {
     use AsyncDecoratorTrait;
 
-    private $decider;
     private $strategy;
     private $maxRetries;
     private $logger;
 
     /**
      * @param int $maxRetries The maximum number of times to retry
      */
-    public function __construct(HttpClientInterface $client, RetryDeciderInterface $decider = null, RetryBackOffInterface $strategy = null, int $maxRetries = 3, LoggerInterface $logger = null)
+    public function __construct(HttpClientInterface $client, RetryStrategyInterface $strategy = null, int $maxRetries = 3, LoggerInterface $logger = null)
     {
         $this->client = $client;
-        $this->decider = $decider ?? new HttpStatusCodeDecider();
-        $this->strategy = $strategy ?? new ExponentialBackOff();
+        $this->strategy = $strategy ?? new GenericRetryStrategy();
         $this->maxRetries = $maxRetries;
         $this->logger = $logger ?: new NullLogger();
     }
@@ -69,23 +65,22 @@ public function request(string $method, string $url, array $options = []): Respo
                     return;
                 }
             } catch (TransportExceptionInterface $exception) {
-                // catch TransportExceptionInterface to send it to strategy.
+                // catch TransportExceptionInterface to send it to the strategy
+                $context->setInfo('retry_count', $retryCount);
             }
 
-            $statusCode = $context->getStatusCode();
-            $headers = $context->getHeaders();
             if (null === $exception) {
                 if ($chunk->isFirst()) {
-                    $shouldRetry = $this->decider->shouldRetry($method, $url, $options, $statusCode, $headers, null);
+                    $context->setInfo('retry_count', $retryCount);
 
-                    if (false === $shouldRetry) {
+                    if (false === $shouldRetry = $this->strategy->shouldRetry($context, null, null)) {
                         $context->passthru();
                         yield $chunk;
 
                         return;
                     }
 
-                    // Decider need body to decide
+                    // Body is needed to decide
                     if (null === $shouldRetry) {
                         $firstChunk = $chunk;
                         $content = '';
@@ -94,12 +89,13 @@ public function request(string $method, string $url, array $options = []): Respo
                     }
                 } else {
                     $content .= $chunk->getContent();
+
                     if (!$chunk->isLast()) {
                         return;
                     }
-                    $shouldRetry = $this->decider->shouldRetry($method, $url, $options, $statusCode, $headers, $content);
-                    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 (null === $shouldRetry = $this->strategy->shouldRetry($context, $content, null)) {
+                        throw new \LogicException(sprintf('The "%s::shouldRetry()" method must not return null when called with a body.', \get_class($this->strategy)));
                     }
 
                     if (false === $shouldRetry) {
@@ -113,14 +109,13 @@ public function request(string $method, string $url, array $options = []): Respo
                 }
             }
 
-            $context->setInfo('retry_count', $retryCount);
             $context->getResponse()->cancel();
 
-            $delay = $this->getDelayFromHeader($headers) ?? $this->strategy->getDelay($retryCount, $method, $url, $options, $statusCode, $headers, $chunk instanceof LastChunk ? $content : null, $exception);
+            $delay = $this->getDelayFromHeader($context->getHeaders()) ?? $this->strategy->getDelay($context, $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), [
-                'retryCount' => $retryCount,
+            $this->logger->info('Try #{count} after {delay}ms'.($exception ? ': '.$exception->getMessage() : ', status code: '.$context->getStatusCode()), [
+                'count' => $retryCount,
                 'delay' => $delay,
             ]);
 
@@ -139,6 +134,7 @@ private function getDelayFromHeader(array $headers): ?int
             if (is_numeric($after)) {
                 return (int) $after * 1000;
             }
+
             if (false !== $time = strtotime($after)) {
                 return max(0, $time - time()) * 1000;
             }