GuzzleSender::send() doesn't handle Guzzle 8's NetworkException or ResponseTransferException #568

Summary

Since Guzzle 8 support was added in #561, GuzzleSender::send() still catches only ConnectException and RequestException. Guzzle 8 reorganised its exception hierarchy, which leaves two gaps:

1. Network failures other than connecting escape as raw Guzzle exceptions. A timeout before the response headers arrive (NetworkTimeoutException), an empty reply, or a send/receive error (NetworkException) is no longer a ConnectException, so send() lets it through instead of throwing FatalRequestException. The fatal pipeline doesn't run, $tries retries are skipped (the retry loop only catches FatalRequestException|RequestException), and application code that catches Saloon's exceptions misses it.
2. Failures after the response headers arrive come back as a successful response. ResponseTransferException (for example ResponseTimeoutException) is a RequestException that carries a response, so both send() and processPromise() return a normal Response built from the partial body: status 200, failed() is false.

On Guzzle 7 the timeouts and the empty reply all became FatalRequestException, because CurlFactory classified cURL errors 28 and 52 as ConnectException (source).

processPromise() already handles the first gap, because it treats anything that isn't a RequestException as fatal. Only the second gap affects sendAsync().

Versions

• saloonphp/saloon v4.5.0
• guzzlehttp/guzzle 8.3.0
• PHP 8.4

Reproduction

This mocks what Guzzle 8's cURL handler throws for each failure, with $tries = 3 on the connector:

<?php

require __DIR__ . '/vendor/autoload.php';

use GuzzleHttp\Exception\ConnectException;
use GuzzleHttp\Exception\NetworkException;
use GuzzleHttp\Exception\NetworkTimeoutException;
use GuzzleHttp\Exception\ResponseTimeoutException;
use GuzzleHttp\Handler\MockHandler;
use GuzzleHttp\Psr7\Request as PsrRequest;
use GuzzleHttp\Psr7\Response as PsrResponse;
use Saloon\Enums\Method;
use Saloon\Http\Connector;
use Saloon\Http\Request;

class ExampleConnector extends Connector
{
    public ?int $tries = 3;

    public function resolveBaseUrl(): string
    {
        return 'https://example.test';
    }
}

class ExampleRequest extends Request
{
    protected Method $method = Method::GET;

    public function resolveEndpoint(): string
    {
        return '/resource';
    }
}

$psrRequest = new PsrRequest('GET', 'https://example.test/resource');

// What Guzzle 8's cURL handler throws for each failure.
$cases = [
    'ConnectException' => fn () => new ConnectException('cURL error 7: connect failed', $psrRequest),
    'NetworkTimeoutException' => fn () => new NetworkTimeoutException('cURL error 28: timed out before headers', $psrRequest),
    'NetworkException' => fn () => new NetworkException('cURL error 52: empty reply', $psrRequest),
    'ResponseTimeoutException' => fn () => new ResponseTimeoutException('cURL error 28: timed out after headers', $psrRequest, new PsrResponse(200, [], '{"partial":')),
];

foreach ($cases as $label => $makeException) {
    foreach (['send', 'sendAsync'] as $mode) {
        $connector = new ExampleConnector;
        $mock = new MockHandler([$makeException(), $makeException(), $makeException()]);
        $connector->sender()->getHandlerStack()->setHandler($mock);

        try {
            $response = $mode === 'send'
                ? $connector->send(new ExampleRequest)
                : $connector->sendAsync(new ExampleRequest)->wait();

            $outcome = sprintf('returned Response (status %d, failed: %s, body: %s)', $response->status(), var_export($response->failed(), true), $response->body());
        } catch (Throwable $e) {
            $outcome = 'threw ' . get_class($e);
        }

        printf("%-25s %-9s attempts: %d  %s\n", $label, $mode, 3 - count($mock), $outcome);
    }
}

Output:

ConnectException          send      attempts: 3  threw Saloon\Exceptions\Request\FatalRequestException
ConnectException          sendAsync attempts: 1  threw Saloon\Exceptions\Request\FatalRequestException
NetworkTimeoutException   send      attempts: 1  threw GuzzleHttp\Exception\NetworkTimeoutException
NetworkTimeoutException   sendAsync attempts: 1  threw Saloon\Exceptions\Request\FatalRequestException
NetworkException          send      attempts: 1  threw GuzzleHttp\Exception\NetworkException
NetworkException          sendAsync attempts: 1  threw Saloon\Exceptions\Request\FatalRequestException
ResponseTimeoutException  send      attempts: 1  returned Response (status 200, failed: false, body: {"partial":)
ResponseTimeoutException  sendAsync attempts: 1  returned Response (status 200, failed: false, body: {"partial":)

Expected

All four cases throw FatalRequestException, and send() retries them according to $tries, as it did on Guzzle 7.

Possible fix

Treat ResponseTransferException as fatal in both methods, and have send() treat any other TransferException as fatal, which is what processPromise() already does:

--- a/src/Http/Senders/GuzzleSender.php
+++ b/src/Http/Senders/GuzzleSender.php
@@ -20,6 +20,7 @@
 use GuzzleHttp\Exception\ConnectException;
 use GuzzleHttp\Exception\RequestException;
 use GuzzleHttp\Exception\TransferException;
+use GuzzleHttp\Exception\ResponseTransferException;
 use Saloon\Exceptions\Request\FatalRequestException;
 use Saloon\Http\Senders\Factories\GuzzleMultipartBodyFactory;
 
@@ -119,6 +120,13 @@
 
             throw new FatalRequestException($exception, $pendingRequest);
         } catch (RequestException $exception) {
+            // Guzzle 8: the transfer failed after the response headers were received
+            // (e.g. a timeout or partial body), so the response is incomplete.
+
+            if ($exception instanceof ResponseTransferException) {
+                throw new FatalRequestException($exception, $pendingRequest);
+            }
+
             // Sometimes, Guzzle will throw a RequestException without a response. This
             // means that it was fatal, so we should still throw a fatal request exception.
 
@@ -129,6 +137,11 @@
             }
 
             return $this->createResponse($guzzleResponse, $pendingRequest, $request, $exception);
+        } catch (TransferException $exception) {
+            // Any other transfer failure without a response, such as Guzzle 8's
+            // NetworkException and NetworkTimeoutException. Matches processPromise().
+
+            throw new FatalRequestException($exception, $pendingRequest);
         }
     }
 
@@ -167,6 +180,13 @@
                         throw new FatalRequestException($guzzleException, $pendingRequest);
                     }
 
+                    // Guzzle 8: the transfer failed after the response headers were received
+                    // (e.g. a timeout or partial body), so the response is incomplete.
+
+                    if ($guzzleException instanceof ResponseTransferException) {
+                        throw new FatalRequestException($guzzleException, $pendingRequest);
+                    }
+
                     // Sometimes, Guzzle will throw a RequestException without a response. This
                     // means that it was fatal, so we should still throw a fatal request exception.

With this applied, the script above throws FatalRequestException in all eight rows, and send() makes 3 attempts in each case. On Guzzle 7.16, a similar script covering ConnectException, RequestException with and without a response, a 500 ServerException and a plain 200 gave identical results with and without the change. ResponseTransferException doesn't exist on Guzzle 7, and instanceof against a missing class is simply false. I haven't run Saloon's own test suite against it.

One judgement call: on Guzzle 7, only a timeout after the headers was fatal. A receive error after the headers was a RequestException with a response, so it came back as a Response. Treating every ResponseTransferException as fatal is therefore slightly broader than Guzzle 7. Checking for ResponseTimeoutException alone would match Guzzle 7 exactly, though returning a truncated body as a successful response seems worth avoiding either way.

Happy to open a PR if that's useful.