diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php index 0d4ee4f797528..c9287c4664255 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php @@ -1654,13 +1654,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() diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php index eb6aeb2c323b4..a05faf1200009 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php +++ b/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; @@ -2072,8 +2073,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']) 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 index 93058d190de22..61410d874cd05 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd @@ -578,7 +578,7 @@ - + @@ -590,6 +590,13 @@ + + + + + + + diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/http_client_retry.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/http_client_retry.php index 206db6b02af92..3260ac56a7510 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/http_client_retry.php +++ b/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, diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/http_client_retry.xml b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/http_client_retry.xml index 76004166a16b3..eb7798914488b 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/http_client_retry.xml +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/http_client_retry.xml @@ -14,8 +14,11 @@ max-retries="2" multiplier="2" jitter="0.3"> - 429 - 500 + + + GET + HEAD + diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/http_client_retry.yml b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/http_client_retry.yml index 0ef87bad38b49..eba686819c300 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/http_client_retry.yml +++ b/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 diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php index 22a6027b7c99e..fdb1d5a6014a5 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php +++ b/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)); diff --git a/src/Symfony/Component/HttpClient/Retry/GenericRetryStrategy.php b/src/Symfony/Component/HttpClient/Retry/GenericRetryStrategy.php index 14727f7584ad6..ebe10a2186962 100644 --- a/src/Symfony/Component/HttpClient/Retry/GenericRetryStrategy.php +++ b/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 diff --git a/src/Symfony/Component/HttpClient/RetryableHttpClient.php b/src/Symfony/Component/HttpClient/RetryableHttpClient.php index 6cf932e60a901..6bca91e4f1dd3 100644 --- a/src/Symfony/Component/HttpClient/RetryableHttpClient.php +++ b/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; } } diff --git a/src/Symfony/Component/HttpClient/Tests/Retry/GenericRetryStrategyTest.php b/src/Symfony/Component/HttpClient/Tests/Retry/GenericRetryStrategyTest.php index ecb84cb482ec7..e04cdb45b6811 100644 --- a/src/Symfony/Component/HttpClient/Tests/Retry/GenericRetryStrategyTest.php +++ b/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]; } /**