From 4c56aeb1ec933ae1a4181c1fa36787532f41d4fd Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 5 Oct 2020 19:21:43 +0200 Subject: [PATCH] [HttpClient] add RetryStrategyInterface --- .../Retry/HttpStatusCodeDecider.php | 2 +- .../Retry/RetryDeciderInterface.php | 2 +- .../Retry/RetryStrategyInterface.php | 20 ++++++++ .../Component/HttpClient/Retry/RetryToken.php | 47 +++++++++++++++++ .../HttpClient/Retry/StatelessStrategy.php | 40 +++++++++++++++ .../HttpClient/RetryableHttpClient.php | 35 +++++++------ .../Tests/Retry/HttpStatusCodeDeciderTest.php | 4 +- .../Tests/RetryableHttpClientTest.php | 51 +++++++++++-------- 8 files changed, 159 insertions(+), 42 deletions(-) create mode 100644 src/Symfony/Component/HttpClient/Retry/RetryStrategyInterface.php create mode 100644 src/Symfony/Component/HttpClient/Retry/RetryToken.php create mode 100644 src/Symfony/Component/HttpClient/Retry/StatelessStrategy.php diff --git a/src/Symfony/Component/HttpClient/Retry/HttpStatusCodeDecider.php b/src/Symfony/Component/HttpClient/Retry/HttpStatusCodeDecider.php index 9415cd33dfeb8..74611bd3dc06d 100644 --- a/src/Symfony/Component/HttpClient/Retry/HttpStatusCodeDecider.php +++ b/src/Symfony/Component/HttpClient/Retry/HttpStatusCodeDecider.php @@ -28,7 +28,7 @@ public function __construct(array $statusCodes = [423, 425, 429, 500, 502, 503, $this->statusCodes = $statusCodes; } - public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseStatusCode, array $responseHeaders, ?string $responseContent): ?bool + public function shouldRetry(int $retryCount, string $requestMethod, string $requestUrl, array $requestOptions, int $responseStatusCode, array $responseHeaders, ?string $responseContent): ?bool { return \in_array($responseStatusCode, $this->statusCodes, true); } diff --git a/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php b/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php index fef606e697c87..929b8660f4e1a 100644 --- a/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php +++ b/src/Symfony/Component/HttpClient/Retry/RetryDeciderInterface.php @@ -23,5 +23,5 @@ interface RetryDeciderInterface * * @return ?bool Returns null to signal that the body is required to take a decision */ - public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseStatusCode, array $responseHeaders, ?string $responseContent): ?bool; + public function shouldRetry(int $retryCount, string $requestMethod, string $requestUrl, array $requestOptions, int $responseStatusCode, array $responseHeaders, ?string $responseContent): ?bool; } diff --git a/src/Symfony/Component/HttpClient/Retry/RetryStrategyInterface.php b/src/Symfony/Component/HttpClient/Retry/RetryStrategyInterface.php new file mode 100644 index 0000000000000..8f1d40ab93be7 --- /dev/null +++ b/src/Symfony/Component/HttpClient/Retry/RetryStrategyInterface.php @@ -0,0 +1,20 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpClient\Retry; + +/** + * @author Nicolas Grekas + */ +interface RetryStrategyInterface +{ + public function getToken(string $requestMethod, string $requestUrl, array $requestOptions): ?RetryToken; +} diff --git a/src/Symfony/Component/HttpClient/Retry/RetryToken.php b/src/Symfony/Component/HttpClient/Retry/RetryToken.php new file mode 100644 index 0000000000000..3d75ae5867d76 --- /dev/null +++ b/src/Symfony/Component/HttpClient/Retry/RetryToken.php @@ -0,0 +1,47 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpClient\Retry; + +/** + * @author Nicolas Grekas + */ +class RetryToken +{ + private $shouldRetry; + private $getDelay; + + public function __construct(\Closure $shouldRetry, \Closure $getDelay) + { + $this->shouldRetry = $shouldRetry; + $this->getDelay = $getDelay; + } + + /** + * Returns whether the request should be retried. + * + * @param ?string $responseContent Null is passed when the body did not arrive yet + * + * @return ?bool Returns null to signal that the body is required to take a decision + */ + public function shouldRetry(int $retryCount, int $responseStatusCode, array $responseHeaders, ?string $responseContent): ?bool + { + return ($this->shouldRetry)($retryCount, $responseStatusCode, $responseHeaders, $responseContent); + } + + /** + * Returns the time to wait in milliseconds. + */ + public function getDelay(int $retryCount, int $responseStatusCode, array $responseHeaders, ?string $responseContent, ?TransportExceptionInterface $exception): int + { + return ($this->getDelay)($retryCount, $responseStatusCode, $responseHeaders, $responseContent, $exception); + } +} diff --git a/src/Symfony/Component/HttpClient/Retry/StatelessStrategy.php b/src/Symfony/Component/HttpClient/Retry/StatelessStrategy.php new file mode 100644 index 0000000000000..e64015d2fc401 --- /dev/null +++ b/src/Symfony/Component/HttpClient/Retry/StatelessStrategy.php @@ -0,0 +1,40 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpClient\Retry; + +/** + * @author Nicolas Grekas + */ +class StatelessStrategy implements RetryStrategyInterface +{ + private $decider; + private $backoff; + + public function __construct(RetryDeciderInterface $decider = null, RetryBackOffInterface $backoff = null) + { + $this->decider = $decider ?? new HttpStatusCodeDecider(); + $this->backoff = $backoff ?? new ExponentialBackOff(); + } + + public function getToken(string $requestMethod, string $requestUrl, array $requestOptions): ?RetryToken + { + $decider = function ($retryCount, $responseStatusCode, $responseHeaders, $responseContent) use ($requestMethod, $requestUrl, $requestOptions) { + return $this->decider->shouldRetry($retryCount, $requestMethod, $requestUrl, $requestOptions, $responseStatusCode, $responseHeaders, $responseContent); + }; + + $backoff = function ($retryCount, $responseStatusCode, $responseHeaders, $responseContent, $exception) use ($requestMethod, $requestUrl, $requestOptions) { + return $this->backoff->getDelay($retryCount, $requestMethod, $requestUrl, $requestOptions, $responseStatusCode, $responseHeaders, $responseContent, $exception); + }; + + return new RetryToken($decider, $backoff); + } +} diff --git a/src/Symfony/Component/HttpClient/RetryableHttpClient.php b/src/Symfony/Component/HttpClient/RetryableHttpClient.php index 1e78393d9d99f..1762ac17424ff 100644 --- a/src/Symfony/Component/HttpClient/RetryableHttpClient.php +++ b/src/Symfony/Component/HttpClient/RetryableHttpClient.php @@ -15,10 +15,8 @@ use Psr\Log\NullLogger; use Symfony\Component\HttpClient\Response\AsyncContext; use Symfony\Component\HttpClient\Response\AsyncResponse; -use Symfony\Component\HttpClient\Retry\ExponentialBackOff; -use Symfony\Component\HttpClient\Retry\HttpStatusCodeDecider; -use Symfony\Component\HttpClient\Retry\RetryBackOffInterface; -use Symfony\Component\HttpClient\Retry\RetryDeciderInterface; +use Symfony\Component\HttpClient\Retry\RetryStrategyInterface; +use Symfony\Component\HttpClient\Retry\StatelessStrategy; use Symfony\Contracts\HttpClient\ChunkInterface; use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; @@ -33,7 +31,6 @@ class RetryableHttpClient implements HttpClientInterface { use AsyncDecoratorTrait; - private $decider; private $strategy; private $maxRetries; private $logger; @@ -41,26 +38,25 @@ class RetryableHttpClient implements HttpClientInterface /** * @param int $maxRetries The maximum number of times to retry */ - public function __construct(HttpClientInterface $client, RetryDeciderInterface $decider = null, RetryBackOffInterface $strategy = null, int $maxRetries = 3, LoggerInterface $logger = null) + public function __construct(HttpClientInterface $client, RetryStrategyInterface $strategy = null, int $maxRetries = 3, LoggerInterface $logger = null) { $this->client = $client; - $this->decider = $decider ?? new HttpStatusCodeDecider(); - $this->strategy = $strategy ?? new ExponentialBackOff(); + $this->strategy = $strategy ?? new StatelessStrategy(); $this->maxRetries = $maxRetries; $this->logger = $logger ?: new NullLogger(); } public function request(string $method, string $url, array $options = []): ResponseInterface { - if ($this->maxRetries <= 0) { + $retryToken = $this->strategy->getToken($method, $url, $options); + + if (null === $retryToken || $this->maxRetries <= 0) { return new AsyncResponse($this->client, $method, $url, $options); } $retryCount = 0; - $content = ''; - $firstChunk = null; - 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, function (ChunkInterface $chunk, AsyncContext $context) use ($method, $url, $options, $retryToken, &$retryCount, &$content, &$firstChunk) { $exception = null; try { if ($chunk->isTimeout() || null !== $chunk->getInformationalStatus()) { @@ -69,14 +65,14 @@ public function request(string $method, string $url, array $options = []): Respo return; } } catch (TransportExceptionInterface $exception) { - // catch TransportExceptionInterface to send it to strategy. + // catch TransportExceptionInterface to send it to RetryToken::getDelay(). } $statusCode = $context->getStatusCode(); $headers = $context->getHeaders(); if (null === $exception) { if ($chunk->isFirst()) { - $shouldRetry = $this->decider->shouldRetry($method, $url, $options, $statusCode, $headers, null); + $shouldRetry = $retryToken->shouldRetry($retryCount, $statusCode, $headers, null); if (false === $shouldRetry) { $context->passthru(); @@ -85,7 +81,7 @@ public function request(string $method, string $url, array $options = []): Respo return; } - // Decider need body to decide + // Body is needed to decide if (null === $shouldRetry) { $firstChunk = $chunk; $content = ''; @@ -94,12 +90,15 @@ public function request(string $method, string $url, array $options = []): Respo } } else { $content .= $chunk->getContent(); + if (!$chunk->isLast()) { return; } - $shouldRetry = $this->decider->shouldRetry($method, $url, $options, $statusCode, $headers, $content); + + $shouldRetry = $retryToken->shouldRetry($retryCount, $statusCode, $headers, $content); + if (null === $shouldRetry) { - throw new \LogicException(sprintf('The "%s::shouldRetry" method must not return null when called with a body.', \get_class($this->decider))); + throw new \LogicException(sprintf('The "%s::shouldRetry()" method must not return null when called with a body.', get_debug_type($retryToken))); } if (false === $shouldRetry) { @@ -116,7 +115,7 @@ public function request(string $method, string $url, array $options = []): Respo $context->setInfo('retry_count', $retryCount); $context->getResponse()->cancel(); - $delay = $this->getDelayFromHeader($headers) ?? $this->strategy->getDelay($retryCount, $method, $url, $options, $statusCode, $headers, $chunk instanceof LastChunk ? $content : null, $exception); + $delay = $this->getDelayFromHeader($headers) ?? $retryToken->getDelay($retryCount, $statusCode, $headers, $chunk instanceof LastChunk ? $content : null, $exception); ++$retryCount; $this->logger->info('Error returned by the server. Retrying #{retryCount} using {delay} ms delay: '.($exception ? $exception->getMessage() : 'StatusCode: '.$statusCode), [ diff --git a/src/Symfony/Component/HttpClient/Tests/Retry/HttpStatusCodeDeciderTest.php b/src/Symfony/Component/HttpClient/Tests/Retry/HttpStatusCodeDeciderTest.php index d634c15ae6559..ebec5e9676475 100644 --- a/src/Symfony/Component/HttpClient/Tests/Retry/HttpStatusCodeDeciderTest.php +++ b/src/Symfony/Component/HttpClient/Tests/Retry/HttpStatusCodeDeciderTest.php @@ -20,13 +20,13 @@ public function testShouldRetryStatusCode() { $decider = new HttpStatusCodeDecider([500]); - self::assertTrue($decider->shouldRetry('GET', 'http://example.com/', [], 500, [], null)); + self::assertTrue($decider->shouldRetry(1, 'GET', 'http://example.com/', [], 500, [], null)); } public function testIsNotRetryableOk() { $decider = new HttpStatusCodeDecider([500]); - self::assertFalse($decider->shouldRetry('GET', 'http://example.com/', [], 200, [], null)); + self::assertFalse($decider->shouldRetry(1, 'GET', 'http://example.com/', [], 200, [], null)); } } diff --git a/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php b/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php index 9e502407e8c45..4400da039e5cc 100644 --- a/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php +++ b/src/Symfony/Component/HttpClient/Tests/RetryableHttpClientTest.php @@ -9,6 +9,7 @@ use Symfony\Component\HttpClient\Retry\ExponentialBackOff; use Symfony\Component\HttpClient\Retry\HttpStatusCodeDecider; use Symfony\Component\HttpClient\Retry\RetryDeciderInterface; +use Symfony\Component\HttpClient\Retry\StatelessStrategy; use Symfony\Component\HttpClient\RetryableHttpClient; class RetryableHttpClientTest extends TestCase @@ -20,8 +21,10 @@ public function testRetryOnError() new MockResponse('', ['http_code' => 500]), new MockResponse('', ['http_code' => 200]), ]), - new HttpStatusCodeDecider([500]), - new ExponentialBackOff(0), + new StatelessStrategy( + new HttpStatusCodeDecider([500]), + new ExponentialBackOff(0) + ), 1 ); @@ -38,8 +41,10 @@ public function testRetryRespectStrategy() new MockResponse('', ['http_code' => 500]), new MockResponse('', ['http_code' => 200]), ]), - new HttpStatusCodeDecider([500]), - new ExponentialBackOff(0), + new StatelessStrategy( + new HttpStatusCodeDecider([500]), + new ExponentialBackOff(0) + ), 1 ); @@ -56,13 +61,15 @@ public function testRetryWithBody() new MockResponse('', ['http_code' => 500]), new MockResponse('', ['http_code' => 200]), ]), - new class() implements RetryDeciderInterface { - public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseCode, array $responseHeaders, ?string $responseContent): ?bool - { - return null === $responseContent ? null : 200 !== $responseCode; - } - }, - new ExponentialBackOff(0), + new StatelessStrategy( + new class() implements RetryDeciderInterface { + public function shouldRetry(int $retryCount, string $requestMethod, string $requestUrl, array $requestOptions, int $responseCode, array $responseHeaders, ?string $responseContent): ?bool + { + return null === $responseContent ? null : 200 !== $responseCode; + } + }, + new ExponentialBackOff(0) + ), 1 ); @@ -78,13 +85,15 @@ public function testRetryWithBodyInvalid() new MockResponse('', ['http_code' => 500]), new MockResponse('', ['http_code' => 200]), ]), - new class() implements RetryDeciderInterface { - public function shouldRetry(string $requestMethod, string $requestUrl, array $requestOptions, int $responseCode, array $responseHeaders, ?string $responseContent, \Throwable $throwable = null): ?bool - { - return null; - } - }, - new ExponentialBackOff(0), + new StatelessStrategy( + new class() implements RetryDeciderInterface { + public function shouldRetry(int $retryCount, string $requestMethod, string $requestUrl, array $requestOptions, int $responseCode, array $responseHeaders, ?string $responseContent, \Throwable $throwable = null): ?bool + { + return null; + } + }, + new ExponentialBackOff(0) + ), 1 ); @@ -100,8 +109,10 @@ public function testStreamNoRetry() new MockHttpClient([ new MockResponse('', ['http_code' => 500]), ]), - new HttpStatusCodeDecider([500]), - new ExponentialBackOff(0), + new StatelessStrategy( + new HttpStatusCodeDecider([500]), + new ExponentialBackOff(0) + ), 0 );