From 53f17bb0628eecc3fad363b4969d1a068d12683b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Debrauwer?= Date: Thu, 31 Mar 2022 09:47:09 +0200 Subject: [PATCH 1/3] Add failing test --- tests/Http/HttpClientTest.php | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/Http/HttpClientTest.php b/tests/Http/HttpClientTest.php index 2509466a9c91..d1aea9c9728d 100644 --- a/tests/Http/HttpClientTest.php +++ b/tests/Http/HttpClientTest.php @@ -1184,15 +1184,24 @@ public function testRequestIsMacroable() public function testRequestExceptionIsThrownWhenRetriesExhausted() { - $this->expectException(RequestException::class); - $this->factory->fake([ '*' => $this->factory->response(['error'], 403), ]); - $this->factory + $exception = null; + + try { + $this->factory ->retry(2, 1000, null, true) ->get('http://foo.com/get'); + } catch (RequestException $e) { + $exception = $e; + } + + $this->assertNotNull($exception); + $this->assertInstanceOf(RequestException::class, $exception); + + $this->factory->assertSentCount(2); } public function testRequestExceptionIsNotThrownWhenDisabledAndRetriesExhausted() @@ -1206,6 +1215,8 @@ public function testRequestExceptionIsNotThrownWhenDisabledAndRetriesExhausted() ->get('http://foo.com/get'); $this->assertTrue($response->failed()); + + $this->factory->assertSentCount(2); } public function testMiddlewareRunsWhenFaked() From 406a51f719a5271e66246076b240e3054f9df035 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Debrauwer?= Date: Thu, 31 Mar 2022 09:48:04 +0200 Subject: [PATCH 2/3] Fix retryThrow behavior --- src/Illuminate/Http/Client/PendingRequest.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/Illuminate/Http/Client/PendingRequest.php b/src/Illuminate/Http/Client/PendingRequest.php index b784e35bc9c4..97d25b01e311 100644 --- a/src/Illuminate/Http/Client/PendingRequest.php +++ b/src/Illuminate/Http/Client/PendingRequest.php @@ -707,11 +707,19 @@ public function send(string $method, string $url, array $options = []) return $this->makePromise($method, $url, $options); } - return retry($this->tries ?? 1, function () use ($method, $url, $options) { + $attempt = 0; + + return retry($this->tries ?? 1, function () use ($method, $url, $options, &$attempt) { + $attempt++; + try { - return tap(new Response($this->sendRequest($method, $url, $options)), function ($response) { + return tap(new Response($this->sendRequest($method, $url, $options)), function ($response) use ($attempt) { $this->populateResponse($response); + if ($attempt < $this->tries && ! $response->successful()) { + $response->throw(); + } + if ($this->tries > 1 && $this->retryThrow && ! $response->successful()) { $response->throw(); } From 228e5bcdba696876be1df7ced8fd1cdfe6daf07d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Debrauwer?= Date: Thu, 31 Mar 2022 10:49:27 +0200 Subject: [PATCH 3/3] Fix code when no retry is necessary --- src/Illuminate/Http/Client/PendingRequest.php | 26 +++++---- tests/Http/HttpClientTest.php | 56 ++++++++++++++++++- 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/src/Illuminate/Http/Client/PendingRequest.php b/src/Illuminate/Http/Client/PendingRequest.php index 97d25b01e311..f987f0aed653 100644 --- a/src/Illuminate/Http/Client/PendingRequest.php +++ b/src/Illuminate/Http/Client/PendingRequest.php @@ -707,21 +707,23 @@ public function send(string $method, string $url, array $options = []) return $this->makePromise($method, $url, $options); } - $attempt = 0; - - return retry($this->tries ?? 1, function () use ($method, $url, $options, &$attempt) { - $attempt++; + $shouldRetry = true; + return retry($this->tries ?? 1, function ($attempt) use ($method, $url, $options, &$shouldRetry) { try { - return tap(new Response($this->sendRequest($method, $url, $options)), function ($response) use ($attempt) { + return tap(new Response($this->sendRequest($method, $url, $options)), function ($response) use ($attempt, &$shouldRetry) { $this->populateResponse($response); - if ($attempt < $this->tries && ! $response->successful()) { - $response->throw(); - } + if (! $response->successful()) { + $shouldRetry = $this->retryWhenCallback ? call_user_func($this->retryWhenCallback, $response->toException()) : true; - if ($this->tries > 1 && $this->retryThrow && ! $response->successful()) { - $response->throw(); + if ($attempt < $this->tries && $shouldRetry) { + $response->throw(); + } + + if ($this->tries > 1 && $this->retryThrow) { + $response->throw(); + } } $this->dispatchResponseReceivedEvent($response); @@ -731,7 +733,9 @@ public function send(string $method, string $url, array $options = []) throw new ConnectionException($e->getMessage(), 0, $e); } - }, $this->retryDelay ?? 100, $this->retryWhenCallback); + }, $this->retryDelay ?? 100, function () use (&$shouldRetry) { + return $shouldRetry; + }); } /** diff --git a/tests/Http/HttpClientTest.php b/tests/Http/HttpClientTest.php index d1aea9c9728d..5a528d4445de 100644 --- a/tests/Http/HttpClientTest.php +++ b/tests/Http/HttpClientTest.php @@ -1192,8 +1192,8 @@ public function testRequestExceptionIsThrownWhenRetriesExhausted() try { $this->factory - ->retry(2, 1000, null, true) - ->get('http://foo.com/get'); + ->retry(2, 1000, null, true) + ->get('http://foo.com/get'); } catch (RequestException $e) { $exception = $e; } @@ -1204,6 +1204,35 @@ public function testRequestExceptionIsThrownWhenRetriesExhausted() $this->factory->assertSentCount(2); } + public function testRequestExceptionIsThrownWithoutRetriesIfRetryNotNecessary() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 500), + ]); + + $exception = null; + $whenAttempts = 0; + + try { + $this->factory + ->retry(2, 1000, function ($exception) use (&$whenAttempts) { + $whenAttempts++; + + return $exception->response->status() === 403; + }, true) + ->get('http://foo.com/get'); + } catch (RequestException $e) { + $exception = $e; + } + + $this->assertNotNull($exception); + $this->assertInstanceOf(RequestException::class, $exception); + + $this->assertSame(1, $whenAttempts); + + $this->factory->assertSentCount(1); + } + public function testRequestExceptionIsNotThrownWhenDisabledAndRetriesExhausted() { $this->factory->fake([ @@ -1219,6 +1248,29 @@ public function testRequestExceptionIsNotThrownWhenDisabledAndRetriesExhausted() $this->factory->assertSentCount(2); } + public function testRequestExceptionIsNotThrownWithoutRetriesIfRetryNotNecessary() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 500), + ]); + + $whenAttempts = 0; + + $response = $this->factory + ->retry(2, 1000, function ($exception) use (&$whenAttempts) { + $whenAttempts++; + + return $exception->response->status() === 403; + }, false) + ->get('http://foo.com/get'); + + $this->assertTrue($response->failed()); + + $this->assertSame(1, $whenAttempts); + + $this->factory->assertSentCount(1); + } + public function testMiddlewareRunsWhenFaked() { $this->factory->fake(function (Request $request) {