From 70be3d10514b944ae232dadd95c75e22a494e4ff Mon Sep 17 00:00:00 2001 From: Deniz Berktunali Date: Wed, 25 Oct 2017 07:36:24 +0200 Subject: [PATCH 1/7] Implement Retry Middleware --- src/Rest.php | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/src/Rest.php b/src/Rest.php index b195f66..cf68c90 100644 --- a/src/Rest.php +++ b/src/Rest.php @@ -10,10 +10,14 @@ use GuzzleHttp\Client; use GuzzleHttp\Handler\CurlMultiHandler; use GuzzleHttp\HandlerStack; +use GuzzleHttp\Exception\ConnectException; +use GuzzleHttp\Exception\RequestException; +use GuzzleHttp\Middleware; use GuzzleHttp\Psr7\Request; use Maestro\Exceptions\NoMethodException; use Maestro\Exceptions\NoUrlException; use Maestro\Exceptions\PostCachingException; +use GuzzleHttp\Psr7\Response; use Maestro\Http\Methods; class Rest @@ -60,6 +64,8 @@ class Rest */ private $cacheKey = ''; + private const MAX_RETRIES = 5; + public function __construct($client = null) { $this->client = $client; @@ -259,7 +265,41 @@ private function sendRequest() } $request = new Request(...$paramsToSend); - + $handler = HandlerStack::create(); + $handler->push(Middleware::retry( + function ( + $retries, + Request $request, + Response $response = null, + RequestException $exception = null + ) { + // if we failed more than MAX_RETRIES times, we give up + if ($retries >= self::MAX_RETRIES) { + return false; + } + + // if we failed to establish a connection, we retry + if ($exception instanceof ConnectException) { + return true; + } + + if ($response) { + // if there was an server error, we retry + if ($response->getStatusCode() >= 500) + return true; + } + + // nothing did help, finally give up + return false; + } + )); + $config = [ + 'handler' => $handler + ]; + if($this->client !== null){ + $config = array_merge($this->client->getConfig(),$config); + } + $this->setClient(new Client($config)); $this->response = $this->client->send($request); $this->cacheResponseBody(); @@ -289,8 +329,34 @@ public function sendAsync() { $curl = new CurlMultiHandler(); $handler = HandlerStack::create($curl); + $handler->push(Middleware::retry( + function ( + $retries, + Request $request, + Response $response = null, + RequestException $exception = null + ) { + // if we failed more than MAX_RETRIES times, we give up + if ($retries >= self::MAX_RETRIES) { + return false; + } + + // if we failed to establish a connection, we retry + if ($exception instanceof ConnectException) { + return true; + } + + if ($response) { + // if there was an server error, we retry + if ($response->getStatusCode() >= 500) + return true; + } + + // nothing did help, finally give up + return false; + } + )); $this->setClient(new Client(['handler' => $handler])); - $request = new Request( $this->method, $this->url.$this->endPoint, From 46fabf39d344c322331116629ea140a4ea0eb07b Mon Sep 17 00:00:00 2001 From: Deniz Berktunali Date: Sun, 29 Oct 2017 15:01:21 +0100 Subject: [PATCH 2/7] Fix codeclimate issues I hope codeclimate is happy now. Personally I prefer "return early" style, but codeclimate doesn't. --- src/Rest.php | 92 ++++++++++++++++++++-------------------------------- 1 file changed, 36 insertions(+), 56 deletions(-) diff --git a/src/Rest.php b/src/Rest.php index cf68c90..018d224 100644 --- a/src/Rest.php +++ b/src/Rest.php @@ -266,38 +266,12 @@ private function sendRequest() $request = new Request(...$paramsToSend); $handler = HandlerStack::create(); - $handler->push(Middleware::retry( - function ( - $retries, - Request $request, - Response $response = null, - RequestException $exception = null - ) { - // if we failed more than MAX_RETRIES times, we give up - if ($retries >= self::MAX_RETRIES) { - return false; - } - - // if we failed to establish a connection, we retry - if ($exception instanceof ConnectException) { - return true; - } - - if ($response) { - // if there was an server error, we retry - if ($response->getStatusCode() >= 500) - return true; - } - - // nothing did help, finally give up - return false; - } - )); + $handler->push(Middleware::retry(self::getRetryDecider())); $config = [ 'handler' => $handler ]; - if($this->client !== null){ - $config = array_merge($this->client->getConfig(),$config); + if ($this->client !== null) { + $config = array_merge($this->client->getConfig(), $config); } $this->setClient(new Client($config)); $this->response = $this->client->send($request); @@ -329,33 +303,7 @@ public function sendAsync() { $curl = new CurlMultiHandler(); $handler = HandlerStack::create($curl); - $handler->push(Middleware::retry( - function ( - $retries, - Request $request, - Response $response = null, - RequestException $exception = null - ) { - // if we failed more than MAX_RETRIES times, we give up - if ($retries >= self::MAX_RETRIES) { - return false; - } - - // if we failed to establish a connection, we retry - if ($exception instanceof ConnectException) { - return true; - } - - if ($response) { - // if there was an server error, we retry - if ($response->getStatusCode() >= 500) - return true; - } - - // nothing did help, finally give up - return false; - } - )); + $handler->push(Middleware::retry(self::getRetryDecider())); $this->setClient(new Client(['handler' => $handler])); $request = new Request( $this->method, @@ -404,4 +352,36 @@ public function assoc() return $this; } + + private static function getRetryDecider(){ + return function( + $retries, + Request $request, + Response $response = null, + RequestException $exception = null + ) { + $returnValue = null; + // if we failed more than MAX_RETRIES times, we give up + if ($retries >= self::MAX_RETRIES) { + $returnValue = false; + } + + // if we failed to establish a connection, we retry + if ($exception instanceof ConnectException && $returnValue === null) { + $returnValue = true; + } + + if ($response && $returnValue === null) { + // if there was an server error, we retry + $responseCode = $response->getStatusCode(); + if ($responseCode >= 500) + $returnValue = true; + } + if ($returnValue === null) + // nothing did help, finally give up + $returnValue = false; + + return $returnValue; + }; + } } From d1c123e4352251c46c98d226e2322dd13733b4d2 Mon Sep 17 00:00:00 2001 From: Deniz Berktunali Date: Sun, 29 Oct 2017 15:14:46 +0100 Subject: [PATCH 3/7] New attempt on calming codeclimate New commit, new codeclimate issues. --- src/Rest.php | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/Rest.php b/src/Rest.php index 018d224..c37cf4a 100644 --- a/src/Rest.php +++ b/src/Rest.php @@ -353,13 +353,14 @@ public function assoc() return $this; } - private static function getRetryDecider(){ - return function( - $retries, - Request $request, - Response $response = null, - RequestException $exception = null - ) { + private static function getRetryDecider() + { + return function ( + $retries, + Request $request, + Response $response = null, + RequestException $exception = null + ) { $returnValue = null; // if we failed more than MAX_RETRIES times, we give up if ($retries >= self::MAX_RETRIES) { @@ -373,13 +374,14 @@ private static function getRetryDecider(){ if ($response && $returnValue === null) { // if there was an server error, we retry - $responseCode = $response->getStatusCode(); - if ($responseCode >= 500) + if ($response->getStatusCode() >= 500) { $returnValue = true; + } } - if ($returnValue === null) + if ($returnValue === null) { // nothing did help, finally give up $returnValue = false; + } return $returnValue; }; From c9eb25d973e968e235117d1382513a650d308ac1 Mon Sep 17 00:00:00 2001 From: Deniz Berktunali Date: Sun, 29 Oct 2017 15:23:35 +0100 Subject: [PATCH 4/7] Fix styleCI issues I'm not sure if I'm reading the analysis results correctly, but this is an attempt to fix these issues. --- src/Rest.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Rest.php b/src/Rest.php index c37cf4a..ef73ba2 100644 --- a/src/Rest.php +++ b/src/Rest.php @@ -8,16 +8,16 @@ namespace Maestro; use GuzzleHttp\Client; -use GuzzleHttp\Handler\CurlMultiHandler; -use GuzzleHttp\HandlerStack; use GuzzleHttp\Exception\ConnectException; use GuzzleHttp\Exception\RequestException; +use GuzzleHttp\Handler\CurlMultiHandler; +use GuzzleHttp\HandlerStack; use GuzzleHttp\Middleware; use GuzzleHttp\Psr7\Request; +use GuzzleHttp\Psr7\Response; use Maestro\Exceptions\NoMethodException; use Maestro\Exceptions\NoUrlException; use Maestro\Exceptions\PostCachingException; -use GuzzleHttp\Psr7\Response; use Maestro\Http\Methods; class Rest @@ -268,7 +268,7 @@ private function sendRequest() $handler = HandlerStack::create(); $handler->push(Middleware::retry(self::getRetryDecider())); $config = [ - 'handler' => $handler + 'handler' => $handler, ]; if ($this->client !== null) { $config = array_merge($this->client->getConfig(), $config); From 1b6b2eef6764b874e5aede367d0a7862fb961e2c Mon Sep 17 00:00:00 2001 From: Deniz Berktunali Date: Wed, 1 Nov 2017 13:16:22 +0100 Subject: [PATCH 5/7] Re-add MAX_RETRIES constant The constant got lost during the merge, here it is again. --- src/Rest.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Rest.php b/src/Rest.php index 75d856b..585ddaf 100644 --- a/src/Rest.php +++ b/src/Rest.php @@ -44,6 +44,11 @@ class Rest */ private $client; + /** + * @var int how many times should we retry before we give up + */ + const MAX_RETRIES = 5; + /** * {string} The response body as a string to make it cachable. */ From a386bf546aee0bcc8bda1a63ebc02b713f1bb533 Mon Sep 17 00:00:00 2001 From: Deniz Berktunali Date: Wed, 1 Nov 2017 13:44:40 +0100 Subject: [PATCH 6/7] Fix the unused local variable issue It's a ugly workaround but it should fix a codeclimate issue --- src/Rest.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Rest.php b/src/Rest.php index 585ddaf..a92d08a 100644 --- a/src/Rest.php +++ b/src/Rest.php @@ -284,6 +284,13 @@ private static function getRetryDecider() Response $response = null, RequestException $exception = null ) { + /** + * @var mixed This variable has the sole purpose of shutting codeclimate up. + * Listen codeclimate, I know it's stupid, but play stupid games, win stupid prizes, eh? + * Can be removed safely after codeclimate is configured to ignore this single instance of + * "avoid unused local variables such as $request" + */ + $garbage = $request; $returnValue = null; // if we failed more than MAX_RETRIES times, we give up if ($retries >= self::MAX_RETRIES) { From 66bdb0b8dca5d3df224ba94bce23eeb4eb5f1362 Mon Sep 17 00:00:00 2001 From: Deniz Berktunali Date: Wed, 1 Nov 2017 13:44:40 +0100 Subject: [PATCH 7/7] Revert "Fix the unused local variable issue" This reverts commit a386bf546aee0bcc8bda1a63ebc02b713f1bb533 as it does not fix the issue as expected. --- src/Rest.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/Rest.php b/src/Rest.php index a92d08a..585ddaf 100644 --- a/src/Rest.php +++ b/src/Rest.php @@ -284,13 +284,6 @@ private static function getRetryDecider() Response $response = null, RequestException $exception = null ) { - /** - * @var mixed This variable has the sole purpose of shutting codeclimate up. - * Listen codeclimate, I know it's stupid, but play stupid games, win stupid prizes, eh? - * Can be removed safely after codeclimate is configured to ignore this single instance of - * "avoid unused local variables such as $request" - */ - $garbage = $request; $returnValue = null; // if we failed more than MAX_RETRIES times, we give up if ($retries >= self::MAX_RETRIES) {