Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.x] Support retry and throw on async http client request (in a http client request pool) #48906

Conversation

gdebrauwer
Copy link
Contributor

@gdebrauwer gdebrauwer commented Nov 3, 2023

Issue

The Http Client allows you to add "retry" functionality and "throw" functionality to a request.

Http::retry(3, 1000)->get('https://foo.bar');

Http::throw()->get('https://foo.bar');

These are synchronous requests, but the Http Client also gives you the option to create asynchronous requests by creating a pool. The problem is asynchronous requests in a pool do not support the retry and throw functionality. If you want that functionality, then you would have to write that yourself which is not easy to do (especially if you want the retries to also happen asynchronously).

// This does not work. The request will only be tried once.
Http::pool(fn ($pool) => [$pool->retry(3, 1000)->get('https://foo.bar')]);

// This also does not work. Unless a connection issue occurs, a response will always be returned.
Http::pool(fn ($pool) => [$pool->throw()->get('https://foo.bar')]);

This issue has also been reported by others: #44955 and #41790 (comment)

Solution

This PR fixes that by adding all that behavior to an asynchronous request, so that it behaves exactly the same as a synchronous request.

We first add an extra then to the request promise. This allows us to add the same retry and throw functionality as a synchronous request.

protected function makePromise(string $method, string $url, array $options = [], int $attempt = 1)
{
    return $this->promise = $this->sendRequest($method, $url, $options)
        ->then(function (MessageInterface $message) {
            // existing code: this is executed when the request received a response
        })
        ->otherwise(function (TransferException $e) {
            // existing code: this is executed when the request did not receive response
        })
        ->then(function (Response|TransferException|ConnectionException $response) use ($method, $url, $options, $attempt) {
			// new code: this method is executed after either the "then" or "otherwise" method above this "then" method.
        })
}

First, we check if the response has a successful status code. In that case, we can return that response.

if ($response instanceof Response && $response->successful()) {
    return $response;
}

If the response is not successful, we need to check if the request needs to be retried. The execution of the "retryWhenCallback" is wrapped in a try-catch because an exception that occurs in that closure should just cause the return of that exception (exactly the same as a synchronous request, except the exception is returned instead of throw)

try {
    $shouldRetry = $this->retryWhenCallback ? call_user_func(
		$this->retryWhenCallback,
		$response instanceof Response ? $response->toException() : $response,
		$this
	) : true;
} catch (Exception $exception) {
    return $exception;
}

Next, we need to check if our current attempt does not exceed our maximum tries. If that is the case, we can start a new attempt by creating and returning a new promise by calling the makePromise method again. Guzzle also provides the option to provide a delay so the new attempt is not immediately executed if that is what is defined on the pending request.

if ($attempt < $this->tries && $shouldRetry) {
    $options['delay'] = value($this->retryDelay);

    return $this->makePromise($method, $url, $options, $attempt + 1);
}

Next, we need to check if we need to return the response or the exception. If the exception needs to be returned, we wrap the $response->throw($this->throwCallback) code in a try-catch. The $response->throw() method immediately throws the generated exception but in the case of async requests we need to return the exception instead. The try-catch enables us to do that.

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;
    }
}

Finally, if we exceed our maximum tries, we have to return the final exception (if the $response is not a response object, then it's a connection exception so we can just return that exception). Otherwise, the response should just be returned.

if ($this->tries > 1 && $this->retryThrow) {
    return $response instanceof Response ? $response->toException() : $response;
}

return $response;

Tests

All the tests in this PR are copies of existing tests but the synchronous requests are converted to asynchronous requests so we test the exact same cases.

Breaking Changes

  • When a connection issue occurs, a \Illuminate\Http\Client\ConnectionException is returned instead of the GuzzleHttp\Exception\ConnectException. This makes the behavior the same as the synchronous requests.
  • When the throw method is used on an async request in a pool, the response exception will be returned instead of always returning the response if a response is available. This makes the behavior the same as the synchronous requests.
  • When the retry method is used on an async request in a pool, the method will now work.
  • The protected makePromise method on the PendingRequest class has an extra parameter called attempt

@gdebrauwer gdebrauwer force-pushed the support-retry-and-throw-on-async-http-client-request branch from f244539 to e29ec8a Compare November 7, 2023 08:42
@taylorotwell taylorotwell merged commit be21757 into laravel:master Nov 8, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants