[9.x] Fix Http Client throw boolean parameter of retry method

[gdebrauwer]

Issue

According to PR #40079 and the documentation, the throw parameter of the retry method can be used to not throw an exception when all retries have been attempted:

If all of the requests fail, an instance of Illuminate\Http\Client\RequestException will be thrown. If you would like to disable this behavior, you may provide a throw argument with a value of false. When disabled, the last response received by the client will be returned after all retries have been attempted:

$response = Http::retry(3, 100, throw: false)->post(...); 

This is currently not the case, because when throw: false, then it does not attempt any of the retries. It just throws the exception of the first attempt. This is caused by the following if-statement:

if ($this->tries > 1 && $this->retryThrow && ! $response->successful()) {
   // An exception will only be thrown if $retryThrow is true, which means that our global retry helper method
   // never triggers a retry when $retryThrow is false because it only will attempt a retry when it catches an exception.
   $response->throw();
}

This issue has also been reported by others: #41601.

Solution

To fix this, we could do the following:

// When the request failed, it should always retry if it is not the last attempt
if ($attempt < $this->tries && ! $response->successful()) {
    $response->throw();
}

// If it is the last attempt, then we have to look at the retryThrow parameter.
// In case retryThrow is true, then we need to throw the exception of the last attempt.
if ($this->tries > 1 && $this->retryThrow && ! $response->successful()) {
    $response->throw();
}

The problem with this approach is when the retryWhenCallback closure returns false (which means that it should not retry) and the throw parameter is false, then an exception is still thrown.
To fix that, we must call the retryWhenCallback before we throw any exception based on a response.

if (! $response->successful()) {
    $shouldRetry = $this->retryWhenCallback ? call_user_func($this->retryWhenCallback, $response->toException()) : true;

    // When the request failed and its an exception that requires a retry, it should retry so we should throw an exception
    if ($attempt < $this->tries && $shouldRetry) {
        $response->throw();
    }

    // If it is the last attempt, then we still just have to look at the retryThrow parameter.
    // In case retryThrow is true, then we need to throw the exception of the last attempt.
    if ($this->tries > 1 && $this->retryThrow) {
        $response->throw();
    }
}

We are almost there. The last issue we now have is when retryThrow boolean parameter is false and retryWhenCallback closure returns false for a certain exception, then it will still retry. This can be fixed by remembering the result of the retryWhenCallback closure and using it in the when closure of the global retry helper method:

$shouldRetry = true;

return retry($this->tries ?? 1, function ($attempt) use ($method, $url, $options, &$shouldRetry) {
    // ...
    $shouldRetry = $this->retryWhenCallback ? call_user_func($this->retryWhenCallback, $response->toException()) : true;
    // ...
}, $this->retryDelay ?? 100, function () use (&$shouldRetry) {
    return $shouldRetry;
});

I added the necessary tests for all these possibilities

[taylorotwell]

Thanks for your help!