[HttpClient] simplify retry mechanism around RetryStrategyInterface

nicolas-grekas · nicolas-grekas · commit 8ef321b26e10 · 2020-10-13T09:28:31.000+02:00
diff --git a/DependencyInjection/Configuration.php b/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 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/DependencyInjection/FrameworkExtension.php b/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/Resources/config/http_client.php b/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/Resources/config/schema/symfony-1.0.xsd b/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/Tests/DependencyInjection/Fixtures/php/http_client_retry.php b/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/Tests/DependencyInjection/Fixtures/yml/http_client_retry.yml b/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/Tests/DependencyInjection/FrameworkExtensionTest.php b/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()
