feature #38426 [HttpClient] Parameterize list of retryable methods (j…

…derusse)

This PR was merged into the 5.x branch.

Discussion
----------

[HttpClient] Parameterize list of retryable methods

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | /
| License       | MIT
| Doc PR        | TODO

Retrying non-idempotent methods is not always acceptable for user. This PR adds an easy way to configure this behavior.

The `RetryDeciderInterface::shouldRetry()` now take the exception in parameter, in order to let decider not retrying the request when the methods should never by retried.

With #38420, this code would belongs to the RetryStrategy implementation, and would return an `NeverRetryDecider` when method is not allowed.

Commits
-------

56809d1 Parameterize list of retryed Http methods

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -1653,13 +1653,43 @@ private function addHttpClientRetrySection()
                         ->performNoDeepMerging()
                         ->beforeNormalization()
                             ->ifArray()
-                            ->then(function ($v) {
-                                return array_filter(array_values($v));
+                            ->then(static function ($v) {
+                                $list = [];
+                                foreach ($v as $key => $val) {
+                                    if (is_numeric($val)) {
+                                        $list[] = ['code' => $val];
+                                    } elseif (\is_array($val)) {
+                                        if (isset($val['code']) || isset($val['methods'])) {
+                                            $list[] = $val;
+                                        } else {
+                                            $list[] = ['code' => $key, 'methods' => $val];
+                                        }
+                                    } elseif (true === $val || null === $val) {
+                                        $list[] = ['code' => $key];
+                                    }
+                                }
+
+                                return $list;
                             })
                         ->end()
-                        ->prototype('integer')->end()
+                        ->useAttributeAsKey('code')
+                        ->arrayPrototype()
+                            ->fixXmlConfig('method')
+                            ->children()
+                                ->integerNode('code')->end()
+                                ->arrayNode('methods')
+                                    ->beforeNormalization()
+                                    ->ifArray()
+                                        ->then(function ($v) {
+                                            return array_map('strtoupper', $v);
+                                        })
+                                    ->end()
+                                    ->prototype('scalar')->end()
+                                    ->info('A list of HTTP methods that triggers a retry for this status code. When empty, all methods are retried')
+                                ->end()
+                            ->end()
+                        ->end()
                         ->info('A list of HTTP status code that triggers a retry')
-                        ->defaultValue([423, 425, 429, 500, 502, 503, 504, 507, 510])
                     ->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()

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -64,6 +64,7 @@
 use Symfony\Component\Form\FormTypeGuesserInterface;
 use Symfony\Component\Form\FormTypeInterface;
 use Symfony\Component\HttpClient\MockHttpClient;
+use Symfony\Component\HttpClient\Retry\GenericRetryStrategy;
 use Symfony\Component\HttpClient\RetryableHttpClient;
 use Symfony\Component\HttpClient\ScopingHttpClient;
 use Symfony\Component\HttpFoundation\Request;
@@ -2074,8 +2075,17 @@ private function registerRetryableHttpClient(array $options, string $name, Conta
             $retryStrategy = new Reference($options['retry_strategy']);
         } else {
             $retryStrategy = new ChildDefinition('http_client.abstract_retry_strategy');
+            $codes = [];
+            foreach ($options['http_codes'] as $code => $codeOptions) {
+                if ($codeOptions['methods']) {
+                    $codes[$code] = $codeOptions['methods'];
+                } else {
+                    $codes[] = $code;
+                }
+            }
+
             $retryStrategy
-                ->replaceArgument(0, $options['http_codes'])
+                ->replaceArgument(0, $codes ?: GenericRetryStrategy::DEFAULT_RETRY_STATUS_CODES)
                 ->replaceArgument(1, $options['delay'])
                 ->replaceArgument(2, $options['multiplier'])
                 ->replaceArgument(3, $options['max_delay'])

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

@@ -578,7 +578,7 @@
 
     <xsd:complexType name="http_client_retry_failed">
         <xsd:sequence>
-            <xsd:element name="http-code" type="xsd:integer" minOccurs="0" maxOccurs="unbounded" />
+            <xsd:element name="http-code" type="http_client_retry_code" minOccurs="0" maxOccurs="unbounded" />
         </xsd:sequence>
         <xsd:attribute name="enabled" type="xsd:boolean" />
         <xsd:attribute name="retry-strategy" type="xsd:string" />
@@ -590,6 +590,13 @@
         <xsd:attribute name="response_header" type="xsd:boolean" />
     </xsd:complexType>
 
+    <xsd:complexType name="http_client_retry_code">
+        <xsd:sequence>
+            <xsd:element name="method" type="xsd:string" minOccurs="0" maxOccurs="unbounded" />
+        </xsd:sequence>
+        <xsd:attribute name="code" type="xsd:integer" />
+    </xsd:complexType>
+
     <xsd:complexType name="http_query" mixed="true">
         <xsd:attribute name="key" type="xsd:string" />
     </xsd:complexType>

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/http_client_retry.php

@@ -5,7 +5,7 @@
         'default_options' => [
             'retry_failed' => [
                 'retry_strategy' => null,
-                'http_codes' => [429, 500],
+                'http_codes' => [429, 500 => ['GET', 'HEAD']],
                 'max_retries' => 2,
                 'delay' => 100,
                 'multiplier' => 2,

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/http_client_retry.xml

@@ -14,8 +14,11 @@
                         max-retries="2"
                         multiplier="2"
                         jitter="0.3">
-                    <framework:http-code>429</framework:http-code>
-                    <framework:http-code>500</framework:http-code>
+                    <framework:http-code code="429"/>
+                    <framework:http-code code="500">
+                        <framework:method>GET</framework:method>
+                        <framework:method>HEAD</framework:method>
+                    </framework:http-code>
                 </framework:retry-failed>
             </framework:default-options>
             <framework:scoped-client name="foo" base-uri="http://example.com">

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/http_client_retry.yml

@@ -3,7 +3,9 @@ framework:
         default_options:
             retry_failed:
                 retry_strategy: null
-                http_codes: [429, 500]
+                http_codes:
+                    429: true
+                    500: ['GET', 'HEAD']
                 max_retries: 2
                 delay: 100
                 multiplier: 2

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

@@ -1500,7 +1500,7 @@ public function testHttpClientRetry()
         }
         $container = $this->createContainerFromFile('http_client_retry');
 
-        $this->assertSame([429, 500], $container->getDefinition('http_client.retry_strategy')->getArgument(0));
+        $this->assertSame([429, 500 => ['GET', 'HEAD']], $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));

src/Symfony/Component/HttpClient/Retry/GenericRetryStrategy.php

@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\HttpClient\Retry;
 
+use Symfony\Component\HttpClient\Exception\InvalidArgumentException;
 use Symfony\Component\HttpClient\Response\AsyncContext;
 use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
 
@@ -21,7 +22,19 @@
  */
 class GenericRetryStrategy implements RetryStrategyInterface
 {
-    public const DEFAULT_RETRY_STATUS_CODES = [423, 425, 429, 500, 502, 503, 504, 507, 510];
+    public const IDEMPOTENT_METHODS = ['GET', 'HEAD', 'PUT', 'DELETE', 'OPTIONS', 'TRACE'];
+    public const DEFAULT_RETRY_STATUS_CODES = [
+        0 => self::IDEMPOTENT_METHODS, // for transport exceptions
+        423,
+        425,
+        429,
+        500 => self::IDEMPOTENT_METHODS,
+        502,
+        503,
+        504 => self::IDEMPOTENT_METHODS,
+        507 => self::IDEMPOTENT_METHODS,
+        510 => self::IDEMPOTENT_METHODS,
+    ];
 
     private $statusCodes;
     private $delayMs;
@@ -63,7 +76,25 @@ public function __construct(array $statusCodes = self::DEFAULT_RETRY_STATUS_CODE
 
     public function shouldRetry(AsyncContext $context, ?string $responseContent, ?TransportExceptionInterface $exception): ?bool
     {
-        return \in_array($context->getStatusCode(), $this->statusCodes, true);
+        $statusCode = $context->getStatusCode();
+        if (\in_array($statusCode, $this->statusCodes, true)) {
+            return true;
+        }
+        if (isset($this->statusCodes[$statusCode]) && \is_array($this->statusCodes[$statusCode])) {
+            return \in_array($context->getInfo('http_method'), $this->statusCodes[$statusCode], true);
+        }
+        if (null === $exception) {
+            return false;
+        }
+
+        if (\in_array(0, $this->statusCodes, true)) {
+            return true;
+        }
+        if (isset($this->statusCodes[0]) && \is_array($this->statusCodes[0])) {
+            return \in_array($context->getInfo('http_method'), $this->statusCodes[0], true);
+        }
+
+        return false;
     }
 
     public function getDelay(AsyncContext $context, ?string $responseContent, ?TransportExceptionInterface $exception): int

src/Symfony/Component/HttpClient/RetryableHttpClient.php

@@ -68,44 +68,63 @@ public function request(string $method, string $url, array $options = []): Respo
                 // catch TransportExceptionInterface to send it to the strategy
                 $context->setInfo('retry_count', $retryCount);
             }
+            if (null !== $exception) {
+                // always retry request that fail to resolve DNS
+                if ('' !== $context->getInfo('primary_ip')) {
+                    $shouldRetry = $this->strategy->shouldRetry($context, null, $exception);
+                    if (null === $shouldRetry) {
+                        throw new \LogicException(sprintf('The "%s::shouldRetry()" method must not return null when called with an exception.', \get_class($this->decider)));
+                    }
 
-            if (null === $exception) {
-                if ($chunk->isFirst()) {
-                    $context->setInfo('retry_count', $retryCount);
-
-                    if (false === $shouldRetry = $this->strategy->shouldRetry($context, null, null)) {
+                    if (false === $shouldRetry) {
                         $context->passthru();
-                        yield $chunk;
+                        if (null !== $firstChunk) {
+                            yield $firstChunk;
+                            yield $context->createChunk($content);
+                            yield $chunk;
+                        } else {
+                            yield $chunk;
+                        }
+                        $content = '';
 
                         return;
                     }
+                }
+            } elseif ($chunk->isFirst()) {
+                $context->setInfo('retry_count', $retryCount);
 
-                    // Body is needed to decide
-                    if (null === $shouldRetry) {
-                        $firstChunk = $chunk;
-                        $content = '';
+                if (false === $shouldRetry = $this->strategy->shouldRetry($context, null, null)) {
+                    $context->passthru();
+                    yield $chunk;
 
-                        return;
-                    }
-                } else {
-                    $content .= $chunk->getContent();
+                    return;
+                }
 
-                    if (!$chunk->isLast()) {
-                        return;
-                    }
+                // Body is needed to decide
+                if (null === $shouldRetry) {
+                    $firstChunk = $chunk;
+                    $content = '';
 
-                    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)));
-                    }
+                    return;
+                }
+            } else {
+                $content .= $chunk->getContent();
 
-                    if (false === $shouldRetry) {
-                        $context->passthru();
-                        yield $firstChunk;
-                        yield $context->createChunk($content);
-                        $content = '';
+                if (!$chunk->isLast()) {
+                    return;
+                }
 
-                        return;
-                    }
+                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) {
+                    $context->passthru();
+                    yield $firstChunk;
+                    yield $context->createChunk($content);
+                    $content = '';
+
+                    return;
                 }
             }
 

src/Symfony/Component/HttpClient/Tests/Retry/GenericRetryStrategyTest.php

@@ -12,25 +12,47 @@
 namespace Symfony\Component\HttpClient\Tests\Retry;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\HttpClient\Exception\TransportException;
 use Symfony\Component\HttpClient\MockHttpClient;
 use Symfony\Component\HttpClient\Response\AsyncContext;
 use Symfony\Component\HttpClient\Response\MockResponse;
 use Symfony\Component\HttpClient\Retry\GenericRetryStrategy;
+use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
 
 class GenericRetryStrategyTest extends TestCase
 {
-    public function testShouldRetryStatusCode()
+    /**
+     * @dataProvider provideRetryable
+     */
+    public function testShouldRetry(string $method, int $code, ?TransportExceptionInterface $exception)
+    {
+        $strategy = new GenericRetryStrategy();
+
+        self::assertTrue($strategy->shouldRetry($this->getContext(0, $method, 'http://example.com/', $code), null, $exception));
+    }
+
+    /**
+     * @dataProvider provideNotRetryable
+     */
+    public function testShouldNotRetry(string $method, int $code, ?TransportExceptionInterface $exception)
     {
-        $strategy = new GenericRetryStrategy([500]);
+        $strategy = new GenericRetryStrategy();
 
-        self::assertTrue($strategy->shouldRetry($this->getContext(0, 'GET', 'http://example.com/', 500), null, null));
+        self::assertFalse($strategy->shouldRetry($this->getContext(0, $method, 'http://example.com/', $code), null, $exception));
     }
 
-    public function testIsNotRetryableOk()
+    public function provideRetryable(): iterable
     {
-        $strategy = new GenericRetryStrategy([500]);
+        yield ['GET', 200, new TransportException()];
+        yield ['GET', 500, null];
+        yield ['POST', 429, null];
+    }
 
-        self::assertFalse($strategy->shouldRetry($this->getContext(0, 'GET', 'http://example.com/', 200), null, null));
+    public function provideNotRetryable(): iterable
+    {
+        yield ['POST', 200, null];
+        yield ['POST', 200, new TransportException()];
+        yield ['POST', 500, null];
     }
 
     /**