Skip to content

[HttpClient] Retry on timeout #38501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[HttpClient] Retry on timeout #38501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1667,6 +1667,7 @@ private function addHttpClientRetrySection()
->defaultValue([423, 425, 429, 500, 502, 503, 504, 507, 510])
->end()
->integerNode('max_retries')->defaultValue(3)->min(0)->end()
->floatNode('retry_timeout')->defaultNull()->min(0)->info('The idle timeout in seconds before retrying the request, defaults to the "default_socket_timeout" ini parameter.')->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()
->integerNode('max_delay')->defaultValue(0)->min(0)->info('Max time in ms that a retry should ever be delayed (0 = infinite)')->end()
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2096,7 +2096,7 @@ private function registerHttpClientRetry(array $retryOptions, string $name, Cont
$container
->register($name.'.retry', 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.'.retry.inner'), $deciderReference, $backoffReference, $retryOptions['max_retries'], $retryOptions['retry_timeout'], new Reference('logger')])
->addTag('monolog.logger', ['channel' => 'http_client']);
}

Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,7 @@
<xsd:attribute name="backoff-service" type="xsd:string" />
<xsd:attribute name="decider-service" type="xsd:string" />
<xsd:attribute name="max-retries" type="xsd:integer" />
<xsd:attribute name="retry-timeout" type="xsd:float" />
<xsd:attribute name="delay" type="xsd:integer" />
<xsd:attribute name="multiplier" type="xsd:float" />
<xsd:attribute name="max-delay" type="xsd:float" />
Expand Down
1 change: 1 addition & 0 deletions ...mfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/http_client_retry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
'decider_service' => null,
'http_codes' => [429, 500],
'max_retries' => 2,
'retry_timeout' => 10,
'delay' => 100,
'multiplier' => 2,
'max_delay' => 0,
Expand Down
1 change: 1 addition & 0 deletions ...mfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/http_client_retry.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
delay="100"
max-delay="0"
max-retries="2"
retry-timeout="10"
multiplier="2"
jitter="0.3">
<framework:http-code>429</framework:http-code>
Expand Down
1 change: 1 addition & 0 deletions ...mfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/http_client_retry.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ framework:
decider_service: null
http_codes: [429, 500]
max_retries: 2
retry_timeout: 10
delay: 100
multiplier: 2
max_delay: 0
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1506,6 +1506,7 @@ public function testHttpClientRetry()
$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(10, $container->getDefinition('http_client.retry')->getArgument(4));

$this->assertSame(RetryableHttpClient::class, $container->getDefinition('foo.retry')->getClass());
$this->assertSame(4, $container->getDefinition('foo.retry.exponential_backoff')->getArgument(1));
Expand Down
29 changes: 22 additions & 7 deletions src/Symfony/Component/HttpClient/RetryableHttpClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,21 @@ class RetryableHttpClient implements HttpClientInterface
private $decider;
private $strategy;
private $maxRetries;
private $retryTimeout;
private $logger;

/**
* @param int $maxRetries The maximum number of times to retry
* @param int $maxRetries The maximum number of times to retry
* @param int $retryTimeout the idle timeout in seconds before retrying the request, defaults to the "default_socket_timeout" ini parameter
*/
public function __construct(HttpClientInterface $client, RetryDeciderInterface $decider = null, RetryBackOffInterface $strategy = null, int $maxRetries = 3, LoggerInterface $logger = null)
public function __construct(HttpClientInterface $client, RetryDeciderInterface $decider = null, RetryBackOffInterface $strategy = null, int $maxRetries = 3, float $retryTimeout = null, LoggerInterface $logger = null)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's about array $options ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not convinced...

{
$this->client = $client;
$this->decider = $decider ?? new HttpStatusCodeDecider();
$this->strategy = $strategy ?? new ExponentialBackOff();
$this->maxRetries = $maxRetries;
$this->logger = $logger ?: new NullLogger();
$this->retryTimeout = $retryTimeout ?? (float) ini_get('default_socket_timeout');
}

public function request(string $method, string $url, array $options = []): ResponseInterface
Expand All @@ -59,11 +62,18 @@ public function request(string $method, string $url, array $options = []): Respo
$retryCount = 0;
$content = '';
$firstChunk = null;
$lastEventTime = microtime(true);

return new AsyncResponse($this->client, $method, $url, $options, function (ChunkInterface $chunk, AsyncContext $context) use ($method, $url, $options, &$retryCount, &$content, &$firstChunk) {
return new AsyncResponse($this->client, $method, $url, $options + ['timeout' => $this->retryTimeout], function (ChunkInterface $chunk, AsyncContext $context) use (&$lastEventTime, $method, $url, $options, &$retryCount, &$content, &$firstChunk) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user call the request method with an timeout option lower that retryTimeout, should'nt we use that timeout for the sub-request?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you want, we need a better solution. If you look at CommonResponseTrait::initialize(), you'll see that timeouts thrown during initialization are turned into errors. This is something that passthru filters don't have access to. This is what we should enhance instead. The initializer closure in AsyncResponse could ensure that an ErroChunk is passed to the passthru filter, so that the filter could act on it (dunno how exactly right now).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that's all we need in this PR actually: be able to detect when a timeout is an error, and retry these. That would allow removing the retryTimeout argument.

$exception = null;
try {
if ($chunk->isTimeout() || null !== $chunk->getInformationalStatus()) {
if ($chunk->isTimeout() && microtime(true) - $lastEventTime <= $this->retryTimeout) {
yield $chunk;

return;
}
$lastEventTime = microtime(true);
if (null !== $chunk->getInformationalStatus()) {
yield $chunk;

return;
Expand Down Expand Up @@ -124,11 +134,16 @@ public function request(string $method, string $url, array $options = []): Respo
'delay' => $delay,
]);

$context->replaceRequest($method, $url, $options);
$context->pause($delay / 1000);

// it's expected to no having chunk in the next $delay seconds
$lastEventTime = microtime(true) + $delay / 1000;
if ($retryCount >= $this->maxRetries) {
$context->replaceRequest($method, $url, $options);
$context->pause($delay / 1000);

$context->passthru();
} else {
$context->replaceRequest($method, $url, $options + ['timeout' => $this->retryTimeout + $delay / 1000]);
$context->pause($delay / 1000);
}
});
}
Expand Down