[HttpClient] Fix GuzzleHttpHandler consuming responses out of band - #65632

Follow-up to your closing note on #65306: the accurate "already consumed" exception added by #65311 did point at the real culprit, and it is the Guzzle handler.

Problem

streamPending() ticks Guzzle's task queue from a finally inside its stream() loop, so the queue runs while the iterator is suspended. Guzzle invokes .then() callbacks from that queue, and such a callback may wait on another promise, which drives the handler again. The nested call then consumes a second stream() iterator over the same responses, behind the back of the suspended one.

The suspended iterator resumes with stale bookkeeping, and from AsyncResponse's point of view the response was consumed out of band. Three failures follow, all reachable through the plain Guzzle API:

LogicException: Instance of "Symfony\Component\HttpClient\Response\CurlResponse" is already
consumed and cannot be managed by "Symfony\Component\HttpClient\NoPrivateNetworkHttpClient".
#0 [internal function]: Symfony\Component\HttpClient\Response\AsyncResponse::stream()
#1 Response/ResponseStream.php(40): Generator->next()
#2 GuzzleHttpHandler.php(170): Symfony\Component\HttpClient\Response\ResponseStream->next()
#3 GuzzleHttpHandler.php(104): Symfony\Component\HttpClient\GuzzleHttpHandler->streamPending()
#4 guzzlehttp/promises/src/Promise.php(251): {closure:GuzzleHttpHandler::__invoke():103}()

LogicException: Cannot change a rejected promise to fulfilled
#1 GuzzleHttpHandler.php(258): GuzzleHttp\Promise\Promise->resolve()
#2 GuzzleHttpHandler.php(221): Symfony\Component\HttpClient\GuzzleHttpHandler->resolveResponse()

plus responses stranded in $pending: never settled, they are garbage collected unconsumed and throw from their destructor, uncaught, with a stack starting at AsyncResponse::__destruct().

This is what I have been chasing in production behind NoPrivateNetworkHttpClient (~10 crashes per 50k requests): a crawler driving concurrent Guzzle promises whose callbacks wait on each other.

Reproducer

Against current 8.1 this prints 55 leaked LogicExceptions and then dies at shutdown; with the patch it prints only done.

<?php

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

use GuzzleHttp\Psr7\Request;
use Symfony\Component\HttpClient\CurlHttpClient;
use Symfony\Component\HttpClient\GuzzleHttpHandler;
use Symfony\Component\HttpClient\NoPrivateNetworkHttpClient;
use Symfony\Contracts\HttpClient\Test\TestHttpServer;

TestHttpServer::start(8057);
mt_srand(1);

$handler = new GuzzleHttpHandler(new NoPrivateNetworkHttpClient(new CurlHttpClient(), null, ['127.0.0.1']));

$urls = [
    'http://127.0.0.1:8057/302?location=http%3A%2F%2F127.0.0.1%3A8057%2F',
    'http://127.0.0.1:8057/chunked',
    'http://127.0.0.1:8057/timeout-body',
    'http://127.0.0.1:8057/json',
];

for ($round = 0; $round < 12; ++$round) {
    $promises = [];
    for ($i = 0; $i < 5; ++$i) {
        $promises[$i] = $handler(new Request('GET', $urls[array_rand($urls)]), ['timeout' => 0.2 + mt_rand(0, 4) / 10]);
    }

    // Guzzle runs these from its task queue, which the handler ticks from inside its stream() loop
    foreach ($promises as $i => $p) {
        $wait = static function () use ($promises, $i) {
            foreach (\array_slice($promises, $i + 1) as $q) {
                try {
                    $q->wait();
                } catch (\Throwable) {
                }
            }
        };
        $p->then($wait, $wait);
    }

    foreach ($promises as $p) {
        try {
            $p->wait();
        } catch (\GuzzleHttp\Exception\GuzzleException) {
            // expected: aggressive timeouts against a slow fixture server
        } catch (\Throwable $e) {
            echo 'LEAKED ', $e::class, ': ', $e->getMessage(), "\n";
        }
    }
}

echo "done\n";

Fix

The task queue now runs once the iterator is gone, so a callback that waits on another promise still makes progress. It just starts its loop when none is running. A $streaming guard keeps a nested call from starting a second loop.

Three cases cannot make progress at all, because on_headers and on_stats are called inline from the loop, as Guzzle's contract requires. The handler now reports each of them the way Guzzle's own CurlMultiHandler does:

• Waiting on any promise of the same handler from such a callback throws RequestException, naming the cause. Without that, Guzzle rejects the promise itself with "Invoking the wait callback did not resolve the promise", which names nothing and is not even a GuzzleException, so callers filtering on that interface miss it.
• Calling execute() from such a callback throws LogicException, instead of returning while transfers are still outstanding, which its own docblock says it never does.
• A promise Guzzle has already rejected in that situation is left rejected. The transfer is still tracked here, so the handler used to settle it a second time when the transfer finished, and that escaped as LogicException: Cannot change a rejected promise to fulfilled. That one also happens without the two points above, whenever an inline callback waits on its own promise.

Behaviour change

A promise waited on from on_headers or on_stats now fails with RequestException instead of silently corrupting the stream. Creating a new request from such a callback and waiting on it fails the same way. Both used to appear to work, at the cost of the corruption above.

Test

Four tests, all MockHttpClient-based and deterministic, no reflection involved:

• testWaitingFromACallbackDoesNotConsumeASecondStream asserts the invariant directly, never two stream() iterators alive at once, with a .then() callback waiting on a sibling promise. 2 live iterators before the patch, 1 after.
• testWaitingFromAnInlineCallbackFailsWithoutStrandingTheHandler, testRunningTheEventLoopFromAnInlineCallbackIsRejected and testAPromiseGuzzleRejectedFromAnInlineCallbackIsNotResolvedAfterwards cover the three cases above. The last one fails on 8.1 without any of this patch.