Change logic in determineDelayTimeout and shouldRetryHttpResponse to fix issue #36

Author: caseyamcl

src/GuzzleRetryMiddleware.php

@@ -280,6 +280,16 @@ protected function shouldRetryHttpResponse(
         $statuses = array_map('\intval', (array) $options['retry_on_status']);
         $hasRetryAfterHeader = $response && $response->hasHeader('Retry-After');
 
+        // Abort if next request time would be greater than the time
+        if ((int) $options['give_up_after_secs'] > 0) {
+            $delayTimeout = ($this->determineDelayTimeout($options, $response));
+            $nextRequestTime = $options['first_request_timestamp'] + $delayTimeout;
+            $giveUpAfterTime = $options['first_request_timestamp'] + $options['give_up_after_secs'];
+            if ($nextRequestTime > $giveUpAfterTime) {
+                return false;
+            }
+        }
+
         switch (true) {
             case $options['retry_enabled'] === false:
             case $this->hasTimeAvailable($options) === false:
@@ -385,6 +395,8 @@ protected function returnResponse(array $options, ResponseInterface $response):
      */
     protected function determineDelayTimeout(array $options, ?ResponseInterface $response = null): float
     {
+        $timeoutDerivedFromServer = false;
+
         // If 'default_retry_multiplier' option is a callable, call it to determine the default timeout...
         if (is_callable($options['default_retry_multiplier'])) {
             $defaultDelayTimeout = (float) call_user_func(
@@ -402,7 +414,12 @@ protected function determineDelayTimeout(array $options, ?ResponseInterface $res
             $timeout = $this->deriveTimeoutFromHeader(
                 $response->getHeader($options['retry_after_header'])[0],
                 $options['retry_after_date_format']
-            ) ?? $defaultDelayTimeout;
+            );
+            if ($timeout !== null) {
+                $timeoutDerivedFromServer = true;
+            } else {
+                $timeout = $defaultDelayTimeout;
+            }
         } else {
             $timeout = abs($defaultDelayTimeout);
         }
@@ -415,7 +432,7 @@ protected function determineDelayTimeout(array $options, ?ResponseInterface $res
         }
 
         // If 'give_up_after_secs' is set, account for it in determining the timeout
-        if ($options['give_up_after_secs']) {
+        if ($options['give_up_after_secs'] && ! $timeoutDerivedFromServer) {
             $giveUpAfterSecs = abs((float) $options['give_up_after_secs']);
             $timeSinceFirstReq =  $options['request_timestamp'] - $options['first_request_timestamp'];
             $timeout = min($timeout, ($giveUpAfterSecs - $timeSinceFirstReq));
@@ -450,8 +467,7 @@ protected function hasTimeAvailable(array $options): bool
      */
     protected function deriveTimeoutFromHeader(string $headerValue, string $dateFormat = self::DATE_FORMAT): ?float
     {
-        // The timeout will either be a number or a HTTP-formatted date,
-        // or seconds (integer)
+        // The timeout will either be a number of seconds (int) or an HTTP-formatted date
         if (is_numeric($headerValue)) {
             return (float) trim($headerValue);
         } elseif ($date = DateTime::createFromFormat($dateFormat ?: self::DATE_FORMAT, trim($headerValue))) {

tests/GuzzleRetryMiddlewareTest.php

@@ -25,6 +25,7 @@
 use GuzzleHttp\Exception\BadResponseException;
 use GuzzleHttp\Exception\ClientException;
 use GuzzleHttp\Exception\ConnectException;
+use GuzzleHttp\Exception\GuzzleException;
 use GuzzleHttp\Exception\ServerException;
 use GuzzleHttp\Exception\TransferException;
 use GuzzleHttp\Handler\MockHandler;
@@ -58,6 +59,7 @@ public function testInstantiation(): void
      * @dataProvider providerForRetryOccursWhenStatusCodeMatches
      * @param Response $response
      * @param bool $retryShouldOccur
+     * @throws GuzzleException
      */
     public function testRetryOccursWhenStatusCodeMatches(Response $response, bool $retryShouldOccur): void
     {
@@ -101,6 +103,7 @@ public function providerForRetryOccursWhenStatusCodeMatches(): array
      *
      * @dataProvider retriesFailAfterSpecifiedLimitProvider
      * @param array<int,Response> $responses
+     * @throws GuzzleException
      */
     public function testRetriesFailAfterSpecifiedLimit(array $responses): void
     {
@@ -529,7 +532,7 @@ public function testDelayTakesIntoAccountGiveUpAfterTime(): void
             $client->request('GET', '/');
             $this->fail('Should have timed out');
         } catch (ClientException $e) {
-            $this->assertEquals([3.0, 7.0], $delayTimes);
+            $this->assertEquals([3.0, 10.0], $delayTimes);
         }
     }
 
@@ -762,24 +765,39 @@ public function testGiveUpAfterSecsFailsWhenTimeLimitExceeded(): void
         $client->request('GET', '/');
     }
 
-    public function testGiveUpAfterSecsWithLongerTimes(): void
+    public function testRespectGiveUpAfterSecs(): void
     {
         $responses = [
-            new Response(429, [], 'Wait 1'),
-            new Response(429, [], 'Wait 2'),
-            new Response(503, [], 'Wait 3'),
-            new Response(429, [], 'Wait 4'),
+            new Response(503, ['Retry-After' => '1'], 'Resp 1'),
+            new Response(503, ['Retry-After' => '1'], 'Resp 2'),
+            new Response(503, ['Retry-After' => '1'], 'Resp 3'),
+            new Response(503, ['Retry-After' => '360'], 'Resp 4'),
+            new Response(503, ['Retry-After' => '10'], 'Resp 5'),
             new Response(200, [], 'Good')
         ];
 
+        $numRetries = 0;
+
         $stack = HandlerStack::create(new MockHandler($responses));
         $stack->push(GuzzleRetryMiddleware::factory([
-            'default_retry_multiplier' => 1.5,
-            'give_up_after_secs' => 1
+            'default_retry_multiplier' => 2,
+            'give_up_after_secs' => 5,
+            'on_retry_callback' => function ($retryCount) use (&$numRetries) {
+                $numRetries = $retryCount;
+            }
         ]));
 
-        $client = new Client(['handler' => $stack]);
-        $client->request('GET', '/');
+        $respBody = null;
+
+        try {
+            $client = new Client(['handler' => $stack]);
+            $client->request('GET', '/');
+        } catch (ServerException $e) {
+            $respBody = $e->getResponse()->getBody()->getContents();
+        }
+
+        $this->assertEquals(3, $numRetries);
+        $this->assertEquals('Resp 4', $respBody);
     }
 
     public function testGiveUpAfterSecsSucceedsWhenTimeLimitNotExceeded(): void
@@ -896,7 +914,7 @@ public function testRetryCallbackReferenceModification(): void
         $client->request('GET', '/');
 
         $this->assertEquals('GoodHeader', $testRequest->getHeaderLine('TestHeader'));
-        $this->assertArrayHasKey('TestOption', $testOptions);
+        $this->assertArrayHasKey('TestOption', $testOptions ?? []);
         $this->assertEquals('GoodOption', $testOptions['TestOption']);
     }