[HttpClient] simplify retry mechanism around RetryStrategyInterface

symfony · nicolas-grekas · Oct 13, 2020 · Oct 12, 2020 · Oct 13, 2020 · 544c3e867825dca2468673386245a0db2c065b2c
commit 544c3e867825dca2468673386245a0db2c065b2c
diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
@@ -1641,19 +1641,15 @@ private function addHttpClientRetrySection()
                 ->addDefaultsIfNotSet()
                 ->beforeNormalization()
                     ->always(function ($v) {
-                        if (isset($v['backoff_service']) && (isset($v['delay']) || isset($v['multiplier']) || isset($v['max_delay']) || isset($v['jitter']))) {
-                            throw new \InvalidArgumentException('The "backoff_service" option cannot be used along with the "delay", "multiplier", "max_delay" or "jitter" options.');
-                        }
-                        if (isset($v['decider_service']) && (isset($v['http_codes']))) {
-                            throw new \InvalidArgumentException('The "decider_service" option cannot be used along with the "http_codes" options.');
+                        if (isset($v['retry_strategy']) && (isset($v['http_codes']) || isset($v['delay']) || isset($v['multiplier']) || isset($v['max_delay']) || isset($v['jitter']))) {
+                            throw new \InvalidArgumentException('The "retry_strategy" option cannot be used along with the "http_codes", "delay", "multiplier", "max_delay" or "jitter" options.');
                         }
 
                         return $v;
                     })
                 ->end()
                 ->children()
-                    ->scalarNode('backoff_service')->defaultNull()->info('service id to override the retry backoff')->end()
-                    ->scalarNode('decider_service')->defaultNull()->info('service id to override the retry decider')->end()
+                    ->scalarNode('retry_strategy')->defaultNull()->info('service id to override the retry strategy')->end()
                     ->arrayNode('http_codes')
                         ->performNoDeepMerging()
                         ->beforeNormalization()
@@ -1668,9 +1664,9 @@ private function addHttpClientRetrySection()
                     ->end()
                     ->integerNode('max_retries')->defaultValue(3)->min(0)->end()
                     ->integerNode('delay')->defaultValue(1000)->min(0)->info('Time in
10000
 ms to delay (or the initial value when multiplier is used)')->end()
-                    ->floatNode('multiplier')->defaultValue(2)->min(1)->info('If greater than 1, delay will grow exponentially for each retry: (delay * (multiple ^ retries))')->end()
+                    ->floatNode('multiplier')->defaultValue(2)->min(1)->info('If greater than 1, delay will grow exponentially for each retry: delay * (multiple ^ retries)')->end()
                     ->integerNode('max_delay')->defaultValue(0)->min(0)->info('Max time in ms that a retry should ever be delayed (0 = infinite)')->end()
-                    ->floatNode('jitter')->defaultValue(0.1)->min(0)->max(1)->info('Randomness in percent (between 0 and 1)) to apply to the delay')->end()
+                    ->floatNode('jitter')->defaultValue(0.1)->min(0)->max(1)->info('Randomness in percent (between 0 and 1) to apply to the delay')->end()
                 ->end()
             ;
     }

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -2011,10 +2011,10 @@ private function registerHttpClientConfiguration(array $config, ContainerBuilder
         }
 
         if ($this->isConfigEnabled($container, $retryOptions)) {
-            $this->registerHttpClientRetry($retryOptions, 'http_client', $container);
+            $this->registerRetryableHttpClient($retryOptions, 'http_client', $container);
         }
 
-        $httpClientId = $retryOptions['enabled'] ?? false ? 'http_client.retry.inner' : ($this->isConfigEnabled($container, $profilerConfig) ? '.debug.http_client.inner' : 'http_client');
+        $httpClientId = ($retryOptions['enabled'] ?? false) ? 'http_client.retryable.inner' : ($this->isConfigEnabled($container, $profilerConfig) ? '.debug.http_client.inner' : 'http_client');
         foreach ($config['scoped_clients'] as $name => $scopeConfig) {
             if ('http_client' === $name) {
                 throw new InvalidArgumentException(sprintf('Invalid scope name: "%s" is reserved.', $name));
@@ -2042,7 +2042,7 @@ private function registerHttpClientConfiguration(array $config, ContainerBuilder
             }
 
             if ($this->isConfigEnabled($container, $retryOptions)) {
-                $this->registerHttpClientRetry($retryOptions, $name, $container);
+                $this->registerRetryableHttpClient($retryOptions, $name, $container);
             }
 
             $container->registerAliasForArgument($name, HttpClientInterface::class);
@@ -2062,42 +2062,31 @@ private function registerHttpClientConfiguration(array $config, ContainerBuilder
         }
     }
 
-    private function registerHttpClientRetry(array $retryOptions, string $name, ContainerBuilder $container)
+    private function registerRetryableHttpClient(array $options, string $name, ContainerBuilder $container)
     {
         if (!class_exists(RetryableHttpClient::class)) {
-            throw new LogicException('Retry failed request support cannot be enabled as version 5.2+ of the HTTP Client component is required.');
+            throw new LogicException('Support for retrying failed requests requires symfony/http-client 5.2 or higher, try upgrading.');
         }
 
-        if (null !== $retryOptions['backoff_service']) {
-            $backoffReference = new Reference($retryOptions['backoff_service']);
+        if (null !== $options['retry_strategy']) {
+            $retryStrategy = new Reference($options['retry_strategy']);
         } else {
-            $retryServiceId = $name.'.retry.exponential_backoff';
-            $retryDefinition = new ChildDefinition('http_client.retry.abstract_exponential_backoff');
-            $retryDefinition
-                ->replaceArgument(0, $retryOptions['delay'])
-                ->replaceArgument(1, $retryOptions['multiplier'])
-                ->replaceArgument(2, $retryOptions['max_delay'])
-                ->replaceArgument(3, $retryOptions['jitter']);
-            $container->setDefinition($retryServiceId, $retryDefinition);
-
-            $backoffReference = new Reference($retryServiceId);
-        }
-        if (null !== $retryOptions['decider_service']) {
-            $deciderReference = new Reference($retryOptions['decider_service']);
-        } else {
-            $retryServiceId = $name.'.retry.decider';
-            $retryDefinition = new ChildDefinition('http_client.retry.abstract_httpstatuscode_decider');
-            $retryDefinition
-                ->replaceArgument(0, $retryOptions['http_codes']);
-            $container->setDefinition($retryServiceId, $retryDefinition);
+            $retryStrategy = new ChildDefinition('http_client.abstract_retry_strategy');
+            $retryStrategy
+                ->replaceArgument(0, $options['http_codes'])
+                ->replaceArgument(1, $options['delay'])
+                ->replaceArgument(2, $options['multiplier'])
+                ->replaceArgument(3, $options['max_delay'])
+                ->replaceArgument(4, $options['jitter']);
+            $container->setDefinition($name.'.retry_strategy', $retryStrategy);
 
-            $deciderReference = new Reference($retryServiceId);
+            $retryStrategy = new Reference($name.'.retry_strategy');
         }
 
         $container
-            ->register($name.'.retry', RetryableHttpClient::class)
+            ->register($name.'.retryable', RetryableHttpClient::class)
             ->setDecoratedService($name, null, -10) // lower priority than TraceableHttpClient
-            ->setArguments([new Reference($name.'.retry.inner'), $deciderReference, $backoffReference, $retryOptions['max_retries'], new Reference('logger')])
+            ->setArguments([new Reference($name.'.retryable.inner'), $retryStrategy, $options['max_retries'], new Reference('logger')])
             ->addTag('monolog.logger', ['channel' => 'http_client']);
     }
 

diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/http_client.php b/src/Symfony/Bundle/FrameworkBundle/Resources/config/http_client.php
@@ -17,8 +17,7 @@
 use Symfony\Component\HttpClient\HttpClient;
 use Symfony\Component\HttpClient\HttplugClient;
 use Symfony\Component\HttpClient\Psr18Client;
-use Symfony\Component\HttpClient\Retry\ExponentialBackOff;
-use Symfony\Component\HttpClient\Retry\HttpStatusCodeDecider;
+use Symfony\Component\HttpClient\Retry\GenericRetryStrategy;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 
 return static function (ContainerConfigurator $container) {
@@ -51,19 +50,14 @@
                 service(StreamFactoryInterface::class)->ignoreOnInvalid(),
             ])
 
-        // retry
-        ->set('http_client.retry.abstract_exponential_backoff', ExponentialBackOff::class)
+        ->set('http_client.abstract_retry_strategy', GenericRetryStrategy::class)
             ->abstract()
             ->args([
+                abstract_arg('http codes'),
                 abstract_arg('delay ms'),
                 abstract_arg('multiplier'),
                 abstract_arg('max delay ms'),
                 abstract_arg('jitter'),
             ])
-        ->set('http_client.retry.abstract_httpstatuscode_decider', HttpStatusCodeDecider::class)
-            ->abstract()
-            ->args([
-                abstract_arg('http codes'),
-            ])
     ;
 };
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd b/src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
@@ -581,8 +581,7 @@
             <xsd:element name="http-code" type="xsd:integer" minOccurs="0" maxOccurs="unbounded" />
         </xsd:sequence>
         <xsd:attribute name="enabled" type="xsd:boolean" />
-        <xsd:attribute name="backoff-service" type="xsd:string" />
-        <xsd:attribute name="decider-service" type="xsd:string" />
+        <xsd:attribute name="retry-strategy" type="xsd:string" />
         <xsd:attribute name="max-retries" type="xsd:integer" />
         <xsd:attribute name="delay" type="xsd:integer" />
         <xsd:attribute name="multiplier" type="xsd:float" />

diff --git a/...mfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/http_client_retry.php b/...mfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/http_client_retry.php
@@ -4,8 +4,7 @@
     'http_client' => [
         'default_options' => [
             'retry_failed' => [
-                'backoff_service' => null,
-                'decider_service' => null,
+                'retry_strategy' => null,
                 'http_codes' => [429, 500],
                 'max_retries' => 2,
                 'delay' => 100,

diff --git a/...mfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/http_client_retry.yml b/...mfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/http_client_retry.yml
@@ -2,8 +2,7 @@ framework:
     http_client:
         default_options:
             retry_failed:
-                backoff_service: null
-                decider_service: null
+                retry_strategy: null
                 http_codes: [429, 500]
                 max_retries: 2
                 delay: 100

diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
@@ -1500,15 +1500,15 @@ public function testHttpClientRetry()
         }
         $container = $this->createContainerFromFile('http_client_retry');
 
-        $this->assertSame([429, 500], $container->getDefinition('http_client.retry.decider')->getArgument(0));
-        $this->assertSame(100, $container->getDefinition('http_client.retry.exponential_backoff')->getArgument(0));
-        $this->assertSame(2, $container->getDefinition('http_client.retry.exponential_backoff')->getArgument(1));
-        $this->assertSame(0, $container->getDefinition('http_client.retry.exponential_backoff')->getArgument(2));
-        $this->assertSame(0.3, $container->getDefinition('http_client.retry.exponential_backoff')->getArgument(3));
-        $this->assertSame(2, $container->getDefinition('http_client.retry')->getArgument(3));
-
-        $this->assertSame(RetryableHttpClient::class, $container->getDefinition('foo.retry')->getClass());
-        $this->assertSame(4, $container->getDefinition('foo.retry.exponential_backoff')->getArgument(1));
+        $this->assertSame([429, 500], $container->getDefinition('http_client.retry_strategy')->getArgument(0));
+        $this->assertSame(100, $container->getDefinition('http_client.retry_strategy')->getArgument(1));
+        $this->assertSame(2, $container->getDefinition('http_client.retry_strategy')->getArgument(2));
+        $this->assertSame(0, $container->getDefinition('http_client.retry_strategy')->getArgument(3));
+        $this->assertSame(0.3, $container->getDefinition('http_client.retry_strategy')->getArgument(4));
+        $this->assertSame(2, $container->getDefinition('http_client.retryable')->getArgument(2));
+
+        $this->assertSame(RetryableHttpClient::class, $container->getDefinition('foo.retryable')->getClass());
+        $this->assertSame(4, $container->getDefinition('foo.retry_strategy')->getArgument(2));
     }
 
     public function testHttpClientWithQueryParameterKey()

diff --git a/src/Symfony/Component/HttpClient/Response/AmpResponse.php b/src/Symfony/Component/HttpClient/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;

diff --git a/src/Symfony/Component/HttpClient/Retry/ExponentialBackOff.php b/src/Symfony/Component/HttpClient/Retry/ExponentialBackOff.php
diff --git a/src/Symfony/Component/HttpClient/Retry/GenericRetryStrategy.php b/src/Symfony/Component/HttpClient/Retry/GenericRetryStrategy.php
@@ -0,0 +1,84 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * 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é <jeremy@derusse.com>
+ */
+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;
+    }
+}