no exception should be thrown on server error, but retrying those would make sense

Author: dbu

CHANGELOG.md

@@ -3,7 +3,9 @@
 ## 2.0 (unreleased)
 
 ### Changed
-- RetryPlugin will no longer retry requests when the response failed with a HTTP code < 500.
+- RetryPlugin will no longer retry exceptions when there is a response failed with a HTTP code < 500.
+- RetryPlugin also retries when no exception is thrown if the responses has HTTP code >= 500. 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.
@@ -13,6 +15,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 - [unreleased]
 

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,12 +59,31 @@ 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 $error_response_delay A callback that gets a request and response and the 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 number of retries and returns how many microseconds we should wait before trying again.
      * }
+     *
+     * @since exception_decider and error_response_decider have been added in version 2. In version 1.*, the exception_decider has been called decider.
      */
     public function __construct(array $config = [])
     {
+        $resolver = new OptionsResolver();
+        $resolver->setDefaults([
+            'retries' => 1,
+            'error_response_decider' => function (RequestInterface $request, ResponseInterface $response) {
+                // do not retry client errors
+                return $response->getStatusCode() >= 500;
+            },
+            'exception_decider' => function (RequestInterface $request, Exception $e) {
+                // do not retry client errors
+                return !$e instanceof HttpException || $e->getCode() >= 500;
+            },
+            'error_response_delay' => __CLASS__.'::defaultResponseDelay',
+            'exception_delay' => __CLASS__.'::defaultExceptionDelay',
+        ]);
+
         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');
@@ -72,21 +101,16 @@ public function __construct(array $config = [])
             unset($config['delay']);
         }
 
-        $resolver = new OptionsResolver();
-        $resolver->setDefaults([
-            'retries' => 1,
-            'exception_decider' => function (RequestInterface $request, Exception $e) {
-                // do not retry client errors
-                return !$e instanceof HttpException || $e->getCode() >= 500;
-            },
-            'exception_delay' => __CLASS__.'::defaultDelay',
-        ]);
         $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 +122,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,13 +159,8 @@ 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);
         });
     }
 
@@ -135,8 +169,29 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
      *
      * @return int
      */
-    public static function defaultDelay(RequestInterface $request, Exception $e, $retries)
+    public static function defaultErrorResponseDelay(RequestInterface $request, ResponseInterface $response, $retries)
     {
         return pow(2, $retries) * 500000;
     }
+
+    /**
+     * @param int $retries The number of retries we made before. First time this get called it will be 0.
+     *
+     * @return int
+     */
+    public static function defaultExceptionDelay(RequestInterface $request, Exception $e, $retries)
+    {
+        return pow(2, $retries) * 500000;
+    }
+
+    private function retry(RequestInterface $request, callable $next, callable $first, string $chainIdentifier, int $delay)
+    {
+        usleep($delay);
+
+        // Retry synchronously
+        ++$this->retryStorage[$chainIdentifier];
+        $promise = $this->handleRequest($request, $next, $first);
+
+        return $promise->wait();
+    }
 }