retry http 5xx status as well

CHANGELOG.md

@@ -4,7 +4,9 @@
 
 ### Changed
 - HttpClientRouter now throws a HttpClientNoMatchException instead of a RequestException if it can not find a client for the request.
-- RetryPlugin will no longer retry requests when the response failed with a HTTP code < 500.
+- RetryPlugin will only retry exceptions when there is no response, or a response in the 5xx HTTP code range.
+- RetryPlugin also retries when no exception is thrown if the responses has HTTP code in the 5xx range.
+  The callbacks for exception handling have been renamed and callbacks for response handling have been added.
 - Abstract method `HttpClientPool::chooseHttpClient()` has now an explicit return type (`Http\Client\Common\HttpClientPoolItem`)
 - Interface method `Plugin::handleRequest(...)` has now an explicit return type (`Http\Promise\Promise`)
 - Made  classes final that are not intended to be extended.
@@ -16,6 +18,7 @@
 
 ### Removed
 - Deprecated option `debug_plugins` has been removed from `PluginClient`
+- Deprecated options `decider` and `delay` have been removed from `RetryPlugin`, use `exception_decider` and `exception_delay` instead.
 
 ## 1.9.0 - 2019-01-03
 

spec/Plugin/RetryPluginSpec.php

@@ -150,11 +150,19 @@ public function it_does_not_keep_history_of_old_failure(RequestInterface $reques
         $this->handleRequest($request, $next, function () {})->shouldReturnAnInstanceOf(HttpFulfilledPromise::class);
     }
 
-    public function it_has_an_exponential_default_delay(RequestInterface $request, Exception\HttpException $exception)
+    public function it_has_an_exponential_default_error_response_delay(RequestInterface $request, ResponseInterface $response)
     {
-        $this->defaultDelay($request, $exception, 0)->shouldBe(500000);
-        $this->defaultDelay($request, $exception, 1)->shouldBe(1000000);
-        $this->defaultDelay($request, $exception, 2)->shouldBe(2000000);
-        $this->defaultDelay($request, $exception, 3)->shouldBe(4000000);
+        $this->defaultErrorResponseDelay($request, $response, 0)->shouldBe(500000);
+        $this->defaultErrorResponseDelay($request, $response, 1)->shouldBe(1000000);
+        $this->defaultErrorResponseDelay($request, $response, 2)->shouldBe(2000000);
+        $this->defaultErrorResponseDelay($request, $response, 3)->shouldBe(4000000);
+    }
+
+    public function it_has_an_exponential_default_exception_delay(RequestInterface $request, Exception\HttpException $exception)
+    {
+        $this->defaultExceptionDelay($request, $exception, 0)->shouldBe(500000);
+        $this->defaultExceptionDelay($request, $exception, 1)->shouldBe(1000000);
+        $this->defaultExceptionDelay($request, $exception, 2)->shouldBe(2000000);
+        $this->defaultExceptionDelay($request, $exception, 3)->shouldBe(4000000);
     }
 }

src/Plugin/RetryPlugin.php

@@ -31,13 +31,23 @@ final class RetryPlugin implements Plugin
     /**
      * @var callable
      */
-    private $exceptionDelay;
+    private $errorResponseDelay;
+
+    /**
+     * @var callable
+     */
+    private $errorResponseDecider;
 
     /**
      * @var callable
      */
     private $exceptionDecider;
 
+    /**
+     * @var callable
+     */
+    private $exceptionDelay;
+
     /**
      * Store the retry counter for each request.
      *
@@ -49,44 +59,39 @@ final class RetryPlugin implements Plugin
      * @param array $config {
      *
      *     @var int $retries Number of retries to attempt if an exception occurs before letting the exception bubble up
+     *     @var callable $error_response_decider A callback that gets a request and response to decide whether the request should be retried
      *     @var callable $exception_decider A callback that gets a request and an exception to decide after a failure whether the request should be retried
-     *     @var callable $exception_delay A callback that gets a request, an exception and the number of retries and returns how many microseconds we should wait before trying again
+     *     @var callable $error_response_delay A callback that gets a request and response and the current number of retries and returns how many microseconds we should wait before trying again
+     *     @var callable $exception_delay A callback that gets a request, an exception and the current number of retries and returns how many microseconds we should wait before trying again
      * }
      */
     public function __construct(array $config = [])
     {
-        if (array_key_exists('decider', $config)) {
-            if (array_key_exists('exception_decider', $config)) {
-                throw new \InvalidArgumentException('Do not set both the old "decider" and new "exception_decider" options');
-            }
-            trigger_error('The "decider" option has been deprecated in favour of "exception_decider"', E_USER_DEPRECATED);
-            $config['exception_decider'] = $config['decider'];
-            unset($config['decider']);
-        }
-        if (array_key_exists('delay', $config)) {
-            if (array_key_exists('exception_delay', $config)) {
-                throw new \InvalidArgumentException('Do not set both the old "delay" and new "exception_delay" options');
-            }
-            trigger_error('The "delay" option has been deprecated in favour of "exception_delay"', E_USER_DEPRECATED);
-            $config['exception_delay'] = $config['delay'];
-            unset($config['delay']);
-        }
-
         $resolver = new OptionsResolver();
         $resolver->setDefaults([
             'retries' => 1,
+            'error_response_decider' => function (RequestInterface $request, ResponseInterface $response) {
+                // do not retry client errors
+                return $response->getStatusCode() >= 500 && $response->getStatusCode() < 600;
+            },
             'exception_decider' => function (RequestInterface $request, Exception $e) {
                 // do not retry client errors
-                return !$e instanceof HttpException || $e->getCode() >= 500;
+                return !$e instanceof HttpException || $e->getCode() >= 500 && $e->getCode() < 600;
             },
-            'exception_delay' => __CLASS__.'::defaultDelay',
+            'error_response_delay' => __CLASS__.'::defaultErrorResponseDelay',
+            'exception_delay' => __CLASS__.'::defaultExceptionDelay',
         ]);
+
         $resolver->setAllowedTypes('retries', 'int');
+        $resolver->setAllowedTypes('error_response_decider', 'callable');
         $resolver->setAllowedTypes('exception_decider', 'callable');
+        $resolver->setAllowedTypes('error_response_delay', 'callable');
         $resolver->setAllowedTypes('exception_delay', 'callable');
         $options = $resolver->resolve($config);
 
         $this->retry = $options['retries'];
+        $this->errorResponseDecider = $options['error_response_decider'];
+        $this->errorResponseDelay = $options['error_response_delay'];
         $this->exceptionDecider = $options['exception_decider'];
         $this->exceptionDelay = $options['exception_delay'];
     }
@@ -98,7 +103,22 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
     {
         $chainIdentifier = spl_object_hash((object) $first);
 
-        return $next($request)->then(function (ResponseInterface $response) use ($request, $chainIdentifier) {
+        return $next($request)->then(function (ResponseInterface $response) use ($request, $next, $first, $chainIdentifier) {
+            if (!array_key_exists($chainIdentifier, $this->retryStorage)) {
+                $this->retryStorage[$chainIdentifier] = 0;
+            }
+
+            if ($this->retryStorage[$chainIdentifier] >= $this->retry) {
+                unset($this->retryStorage[$chainIdentifier]);
+
+                return $response;
+            }
+
+            if (call_user_func($this->errorResponseDecider, $request, $response)) {
+                $time = call_user_func($this->errorResponseDelay, $request, $response, $this->retryStorage[$chainIdentifier]);
+                $response = $this->retry($request, $next, $first, $chainIdentifier, $time);
+            }
+
             if (array_key_exists($chainIdentifier, $this->retryStorage)) {
                 unset($this->retryStorage[$chainIdentifier]);
             }
@@ -120,23 +140,38 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
             }
 
             $time = call_user_func($this->exceptionDelay, $request, $exception, $this->retryStorage[$chainIdentifier]);
-            usleep($time);
-
-            // Retry synchronously
-            ++$this->retryStorage[$chainIdentifier];
-            $promise = $this->handleRequest($request, $next, $first);
 
-            return $promise->wait();
+            return $this->retry($request, $next, $first, $chainIdentifier, $time);
         });
     }
 
     /**
      * @param int $retries The number of retries we made before. First time this get called it will be 0.
-     *
-     * @return int
      */
-    public static function defaultDelay(RequestInterface $request, Exception $e, $retries)
+    public static function defaultErrorResponseDelay(RequestInterface $request, ResponseInterface $response, int $retries): int
     {
         return pow(2, $retries) * 500000;
     }
+
+    /**
+     * @param int $retries The number of retries we made before. First time this get called it will be 0.
+     */
+    public static function defaultExceptionDelay(RequestInterface $request, Exception $e, int $retries): int
+    {
+        return pow(2, $retries) * 500000;
+    }
+
+    /**
+     * @throws \Exception if retrying returns a failed promise
+     */
+    private function retry(RequestInterface $request, callable $next, callable $first, string $chainIdentifier, int $delay): ResponseInterface
+    {
+        usleep($delay);
+
+        // Retry synchronously
+        ++$this->retryStorage[$chainIdentifier];
+        $promise = $this->handleRequest($request, $next, $first);
+
+        return $promise->wait();
+    }
 }