From be2175758bd88eb1a53c21a020988bbb2642c52a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Debrauwer?= Date: Thu, 9 Nov 2023 00:14:53 +0100 Subject: [PATCH] [11.x] Support retry and throw on async http client request (in a http client request pool) (#48906) * Support throw and retry in async request * Small fixes * Fix linting issues * Fix another linting issue * Throw after last retry * Provide attempt and exception to retryDelay closure * formatting --------- Co-authored-by: Taylor Otwell --- src/Illuminate/Http/Client/PendingRequest.php | 61 +++- tests/Http/HttpClientTest.php | 270 ++++++++++++++++++ 2 files changed, 330 insertions(+), 1 deletion(-) diff --git a/src/Illuminate/Http/Client/PendingRequest.php b/src/Illuminate/Http/Client/PendingRequest.php index 0ddc69f6bc35..984f8d25a658 100644 --- a/src/Illuminate/Http/Client/PendingRequest.php +++ b/src/Illuminate/Http/Client/PendingRequest.php @@ -997,9 +997,10 @@ protected function parseMultipartBodyFormat(array $data) * @param string $method * @param string $url * @param array $options + * @param int $attempt * @return \GuzzleHttp\Promise\PromiseInterface */ - protected function makePromise(string $method, string $url, array $options = []) + protected function makePromise(string $method, string $url, array $options = [], int $attempt = 1) { return $this->promise = $this->sendRequest($method, $url, $options) ->then(function (MessageInterface $message) { @@ -1011,12 +1012,70 @@ protected function makePromise(string $method, string $url, array $options = []) ->otherwise(function (TransferException $e) { if ($e instanceof ConnectException) { $this->dispatchConnectionFailedEvent(); + + return new ConnectionException($e->getMessage(), 0, $e); } return $e instanceof RequestException && $e->hasResponse() ? $this->populateResponse($this->newResponse($e->getResponse())) : $e; + }) + ->then(function (Response|ConnectionException|TransferException $response) use ($method, $url, $options, $attempt) { + return $this->handlePromiseResponse($response, $method, $url, $options, $attempt); }); } + /** + * Handle the response of an asynchronous request. + * + * @param \Illuminate\Http\Client\Response $response + * @param string $method + * @param string $url + * @param array $options + * @param int $attempt + * @return mixed + */ + protected function handlePromiseResponse(Response|ConnectionException|TransferException $response, $method, $url, $options, $attempt) + { + if ($response instanceof Response && $response->successful()) { + return $response; + } + + if ($response instanceof RequestException) { + $response = $this->populateResponse($this->newResponse($response->getResponse())); + } + + try { + $shouldRetry = $this->retryWhenCallback ? call_user_func( + $this->retryWhenCallback, + $response instanceof Response ? $response->toException() : $response, + $this + ) : true; + } catch (Exception $exception) { + return $exception; + } + + if ($attempt < $this->tries && $shouldRetry) { + $options['delay'] = value($this->retryDelay, $attempt, $response->toException()); + + return $this->makePromise($method, $url, $options, $attempt + 1); + } + + if ($response instanceof Response && + $this->throwCallback && + ($this->throwIfCallback === null || call_user_func($this->throwIfCallback, $response))) { + try { + $response->throw($this->throwCallback); + } catch (Exception $exception) { + return $exception; + } + } + + if ($this->tries > 1 && $this->retryThrow) { + return $response instanceof Response ? $response->toException() : $response; + } + + return $response; + } + /** * Send a request either synchronously or asynchronously. * diff --git a/tests/Http/HttpClientTest.php b/tests/Http/HttpClientTest.php index 7d59b2c3822d..e32f01e6a68b 100644 --- a/tests/Http/HttpClientTest.php +++ b/tests/Http/HttpClientTest.php @@ -1837,6 +1837,132 @@ public function testRequestsWillBeWaitingSleepMillisecondsReceivedBeforeRetry() ]); } + public function testRequestExceptionReturnedWhenRetriesExhaustedInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 403), + ]); + + [$exception] = $this->factory->pool(fn ($pool) => [ + $pool->retry(2, 1000, null, true)->get('http://foo.com/get'), + ]); + + $this->assertNotNull($exception); + $this->assertInstanceOf(RequestException::class, $exception); + + $this->factory->assertSentCount(2); + } + + public function testRequestExceptionIsReturnedWithoutRetriesIfRetryNotNecessaryInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 500), + ]); + + $whenAttempts = collect(); + + [$exception] = $this->factory->pool(fn ($pool) => [ + $pool->retry(2, 1000, function ($exception) use ($whenAttempts) { + $whenAttempts->push($exception); + + return $exception->response->status() === 403; + }, true)->get('http://foo.com/get'), + ]); + + $this->assertNotNull($exception); + $this->assertInstanceOf(RequestException::class, $exception); + + $this->assertCount(1, $whenAttempts); + + $this->factory->assertSentCount(1); + } + + public function testRequestExceptionIsNotReturnedWhenDisabledAndRetriesExhaustedInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 403), + ]); + + [$response] = $this->factory->pool(fn ($pool) => [ + $pool->retry(2, 1000, null, false)->get('http://foo.com/get'), + ]); + + $this->assertNotNull($response); + $this->assertInstanceOf(Response::class, $response); + $this->assertTrue($response->failed()); + + $this->factory->assertSentCount(2); + } + + public function testRequestExceptionIsNotReturnedWithoutRetriesIfRetryNotNecessaryInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 500), + ]); + + $whenAttempts = collect(); + + [$response] = $this->factory->pool(fn ($pool) => [ + $pool->retry(2, 1000, function ($exception) use ($whenAttempts) { + $whenAttempts->push($exception); + + return $exception->response->status() === 403; + }, false)->get('http://foo.com/get'), + ]); + + $this->assertNotNull($response); + $this->assertInstanceOf(Response::class, $response); + $this->assertTrue($response->failed()); + + $this->assertCount(1, $whenAttempts); + + $this->factory->assertSentCount(1); + } + + public function testRequestCanBeModifiedInRetryCallbackInPool() + { + $this->factory->fake([ + '*' => $this->factory->sequence() + ->push(['error'], 500) + ->push(['ok'], 200), + ]); + + [$response] = $this->factory->pool(fn ($pool) => [ + $pool->retry(2, 1000, function ($exception, $request) { + $this->assertInstanceOf(PendingRequest::class, $request); + + $request->withHeaders(['Foo' => 'Bar']); + + return true; + }, false)->get('http://foo.com/get'), + ]); + + $this->assertTrue($response->successful()); + + $this->factory->assertSent(function (Request $request) { + return $request->hasHeader('Foo') && $request->header('Foo') === ['Bar']; + }); + } + + public function testExceptionThrownInRetryCallbackIsReturnedWithoutRetryingInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 500), + ]); + + [$exception] = $this->factory->pool(fn ($pool) => [ + $pool->retry(2, 1000, function ($exception) { + throw new Exception('Foo bar'); + }, false)->get('http://foo.com/get'), + ]); + + $this->assertNotNull($exception); + $this->assertInstanceOf(Exception::class, $exception); + $this->assertEquals('Foo bar', $exception->getMessage()); + + $this->factory->assertSentCount(1); + } + public function testMiddlewareRunsWhenFaked() { $this->factory->fake(function (Request $request) { @@ -2081,6 +2207,150 @@ public function testRequestExceptionIsNotThrownIfTheRequestDoesNotFail() $this->assertSame('{"result":{"foo":"bar"}}', $response->body()); } + public function testRequestExceptionIsNotReturnedIfThePendingRequestIsSetToThrowOnFailureButTheResponseIsSuccessfulInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['success'], 200), + ]); + + [$response] = $this->factory->pool(fn ($pool) => [ + $pool->throw()->get('http://foo.com/get'), + ]); + + $this->assertInstanceOf(Response::class, $response); + $this->assertSame(200, $response->status()); + } + + public function testRequestExceptionIsReturnedIfThePendingRequestIsSetToThrowOnFailureInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 403), + ]); + + [$exception] = $this->factory->pool(fn ($pool) => [ + $pool->throw()->get('http://foo.com/get'), + ]); + + $this->assertNotNull($exception); + $this->assertInstanceOf(RequestException::class, $exception); + } + + public function testRequestExceptionIsReturnedIfTheThrowIfOnThePendingRequestIsSetToTrueOnFailureInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 403), + ]); + + [$exception] = $this->factory->pool(fn ($pool) => [ + $pool->throwIf(true)->get('http://foo.com/get'), + ]); + + $this->assertNotNull($exception); + $this->assertInstanceOf(RequestException::class, $exception); + } + + public function testRequestExceptionIsNotReturnedIfTheThrowIfOnThePendingRequestIsSetToFalseOnFailureInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 403), + ]); + + [$response] = $this->factory->pool(fn ($pool) => [ + $pool->throwIf(false)->get('http://foo.com/get'), + ]); + + $this->assertInstanceOf(Response::class, $response); + $this->assertSame(403, $response->status()); + } + + public function testRequestExceptionIsReturnedIfTheThrowIfClosureOnThePendingRequestReturnsTrueInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 403), + ]); + + $hitThrowCallback = collect(); + + [$exception] = $this->factory->pool(fn ($pool) => [ + $pool->throwIf(function ($response) { + $this->assertInstanceOf(Response::class, $response); + $this->assertSame(403, $response->status()); + + return true; + }, function ($response, $e) use (&$hitThrowCallback) { + $this->assertInstanceOf(Response::class, $response); + $this->assertSame(403, $response->status()); + + $this->assertInstanceOf(RequestException::class, $e); + + $hitThrowCallback->push(true); + })->get('http://foo.com/get'), + ]); + + $this->assertNotNull($exception); + $this->assertInstanceOf(RequestException::class, $exception); + $this->assertCount(1, $hitThrowCallback); + } + + public function testRequestExceptionIsNotReturnedIfTheThrowIfClosureOnThePendingRequestReturnsFalseInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 403), + ]); + + $hitThrowCallback = collect(); + + [$response] = $this->factory->pool(fn ($pool) => [ + $pool->throwIf(function ($response) { + $this->assertInstanceOf(Response::class, $response); + $this->assertSame(403, $response->status()); + + return false; + }, function ($response, $e) use (&$hitThrowCallback) { + $hitThrowCallback->push(true); + })->get('http://foo.com/get'), + ]); + + $this->assertCount(0, $hitThrowCallback); + $this->assertSame(403, $response->status()); + } + + public function testRequestExceptionIsReturnedWithCallbackIfThePendingRequestIsSetToThrowOnFailureInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 403), + ]); + + $flag = collect(); + + [$exception] = $this->factory->pool(fn ($pool) => [ + $pool->throw(function ($exception) use (&$flag) { + $flag->push(true); + })->get('http://foo.com/get'), + ]); + + $this->assertCount(1, $flag); + + $this->assertNotNull($exception); + $this->assertInstanceOf(RequestException::class, $exception); + } + + public function testRequestExceptionIsReturnedAfterLastRetryInPool() + { + $this->factory->fake([ + '*' => $this->factory->response(['error'], 403), + ]); + + [$exception] = $this->factory->pool(fn ($pool) => [ + $pool->retry(3)->throw()->get('http://foo.com/get'), + ]); + + $this->assertNotNull($exception); + $this->assertInstanceOf(RequestException::class, $exception); + + $this->factory->assertSentCount(3); + } + public function testRequestExceptionIsThrowIfConditionIsSatisfied() { $this->factory->fake([