Skip to content

Commit 9f221c5

Browse files
fabpotfabpot
committed
feature #38426 [HttpClient] Parameterize list of retryable methods (jderusse)
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 ------- 56809d1b91 Parameterize list of retryed Http methods
2 parents fe4f436 + 270228d commit 9f221c5

File tree

3 files changed

+107
-35
lines changed

3 files changed

+107
-35
lines changed

‎Retry/GenericRetryStrategy.php

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\HttpClient\Retry;
1313

14+
use Symfony\Component\HttpClient\Exception\InvalidArgumentException;
1415
use Symfony\Component\HttpClient\Response\AsyncContext;
1516
use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
1617

@@ -21,7 +22,19 @@
2122
*/
2223
class GenericRetryStrategy implements RetryStrategyInterface
2324
{
24-
public const DEFAULT_RETRY_STATUS_CODES = [423, 425, 429, 500, 502, 503, 504, 507, 510];
25+
public const IDEMPOTENT_METHODS = ['GET', 'HEAD', 'PUT', 'DELETE', 'OPTIONS', 'TRACE'];
26+
public const DEFAULT_RETRY_STATUS_CODES = [
27+
0 => self::IDEMPOTENT_METHODS, // for transport exceptions
28+
423,
29+
425,
30+
429,
31+
500 => self::IDEMPOTENT_METHODS,
32+
502,
33+
503,
34+
504 => self::IDEMPOTENT_METHODS,
35+
507 => self::IDEMPOTENT_METHODS,
36+
510 => self::IDEMPOTENT_METHODS,
37+
];
2538

2639
private $statusCodes;
2740
private $delayMs;
@@ -63,7 +76,25 @@ public function __construct(array $statusCodes = self::DEFAULT_RETRY_STATUS_CODE
6376

6477
public function shouldRetry(AsyncContext $context, ?string $responseContent, ?TransportExceptionInterface $exception): ?bool
6578
{
66-
return \in_array($context->getStatusCode(), $this->statusCodes, true);
79+
$statusCode = $context->getStatusCode();
80+
if (\in_array($statusCode, $this->statusCodes, true)) {
81+
return true;
82+
}
83+
if (isset($this->statusCodes[$statusCode]) && \is_array($this->statusCodes[$statusCode])) {
84+
return \in_array($context->getInfo('http_method'), $this->statusCodes[$statusCode], true);
85+
}
86+
if (null === $exception) {
87+
return false;
88+
}
89+
90+
if (\in_array(0, $this->statusCodes, true)) {
91+
return true;
92+
}
93+
if (isset($this->statusCodes[0]) && \is_array($this->statusCodes[0])) {
94+
return \in_array($context->getInfo('http_method'), $this->statusCodes[0], true);
95+
}
96+
97+
return false;
6798
}
6899

69100
public function getDelay(AsyncContext $context, ?string $responseContent, ?TransportExceptionInterface $exception): int

‎RetryableHttpClient.php

Lines changed: 46 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -68,44 +68,63 @@ public function request(string $method, string $url, array $options = []): Respo
6868
// catch TransportExceptionInterface to send it to the strategy
6969
$context->setInfo('retry_count', $retryCount);
7070
}
71+
if (null !== $exception) {
72+
// always retry request that fail to resolve DNS
73+
if ('' !== $context->getInfo('primary_ip')) {
74+
$shouldRetry = $this->strategy->shouldRetry($context, null, $exception);
75+
if (null === $shouldRetry) {
76+
throw new \LogicException(sprintf('The "%s::shouldRetry()" method must not return null when called with an exception.', \get_class($this->decider)));
77+
}
7178

72-
if (null === $exception) {
73-
if ($chunk->isFirst()) {
74-
$context->setInfo('retry_count', $retryCount);
75-
76-
if (false === $shouldRetry = $this->strategy->shouldRetry($context, null, null)) {
79+
if (false === $shouldRetry) {
7780
$context->passthru();
78-
yield $chunk;
81+
if (null !== $firstChunk) {
82+
yield $firstChunk;
83+
yield $context->createChunk($content);
84+
yield $chunk;
85+
} else {
86+
yield $chunk;
87+
}
88+
$content = '';
7989

8090
return;
8191
}
92+
}
93+
} elseif ($chunk->isFirst()) {
94+
$context->setInfo('retry_count', $retryCount);
8295

83-
// Body is needed to decide
84-
if (null === $shouldRetry) {
85-
$firstChunk = $chunk;
86-
$content = '';
96+
if (false === $shouldRetry = $this->strategy->shouldRetry($context, null, null)) {
97+
$context->passthru();
98+
yield $chunk;
8799

88-
return;
89-
}
90-
} else {
91-
$content .= $chunk->getContent();
100+
return;
101+
}
92102

93-
if (!$chunk->isLast()) {
94-
return;
95-
}
103+
// Body is needed to decide
104+
if (null === $shouldRetry) {
105+
$firstChunk = $chunk;
106+
$content = '';
96107

97-
if (null === $shouldRetry = $this->strategy->shouldRetry($context, $content, null)) {
98-
throw new \LogicException(sprintf('The "%s::shouldRetry()" method must not return null when called with a body.', \get_class($this->strategy)));
99-
}
108+
return;
109+
}
110+
} else {
111+
$content .= $chunk->getContent();
100112

101-
if (false === $shouldRetry) {
102-
$context->passthru();
103-
yield $firstChunk;
104-
yield $context->createChunk($content);
105-
$content = '';
113+
if (!$chunk->isLast()) {
114+
return;
115+
}
106116

107-
return;
108-
}
117+
if (null === $shouldRetry = $this->strategy->shouldRetry($context, $content, null)) {
118+
throw new \LogicException(sprintf('The "%s::shouldRetry()" method must not return null when called with a body.', \get_class($this->strategy)));
119+
}
120+
121+
if (false === $shouldRetry) {
122+
$context->passthru();
123+
yield $firstChunk;
124+
yield $context->createChunk($content);
125+
$content = '';
126+
127+
return;
109128
}
110129
}
111130

‎Tests/Retry/GenericRetryStrategyTest.php

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,25 +12,47 @@
1212
namespace Symfony\Component\HttpClient\Tests\Retry;
1313

1414
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\HttpClient\Exception\TransportException;
1516
use Symfony\Component\HttpClient\MockHttpClient;
1617
use Symfony\Component\HttpClient\Response\AsyncContext;
1718
use Symfony\Component\HttpClient\Response\MockResponse;
1819
use Symfony\Component\HttpClient\Retry\GenericRetryStrategy;
20+
use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
1921

2022
class GenericRetryStrategyTest extends TestCase
2123
{
22-
public function testShouldRetryStatusCode()
24+
/**
25+
* @dataProvider provideRetryable
26+
*/
27+
public function testShouldRetry(string $method, int $code, ?TransportExceptionInterface $exception)
28+
{
29+
$strategy = new GenericRetryStrategy();
30+
31+
self::assertTrue($strategy->shouldRetry($this->getContext(0, $method, 'http://example.com/', $code), null, $exception));
32+
}
33+
34+
/**
35+
* @dataProvider provideNotRetryable
36+
*/
37+
public function testShouldNotRetry(string $method, int $code, ?TransportExceptionInterface $exception)
2338
{
24-
$strategy = new GenericRetryStrategy([500]);
39+
$strategy = new GenericRetryStrategy();
2540

26-
self::assertTrue($strategy->shouldRetry($this->getContext(0, 'GET', 'http://example.com/', 500), null, null));
41+
self::assertFalse($strategy->shouldRetry($this->getContext(0, $method, 'http://example.com/', $code), null, $exception));
2742
}
2843

29-
public function testIsNotRetryableOk()
44+
public function provideRetryable(): iterable
3045
{
31-
$strategy = new GenericRetryStrategy([500]);
46+
yield ['GET', 200, new TransportException()];
47+
yield ['GET', 500, null];
48+
yield ['POST', 429, null];
49+
}
3250

33-
self::assertFalse($strategy->shouldRetry($this->getContext(0, 'GET', 'http://example.com/', 200), null, null));
51+
public function provideNotRetryable(): iterable
52+
{
53+
yield ['POST', 200, null];
54+
yield ['POST', 200, new TransportException()];
55+
yield ['POST', 500, null];
3456
}
3557

3658
/**

0 commit comments

Comments
 (0)