[9.x] Fix HTTP client pool

[wfeller]

This PR fixes 2 problems:

1. Middleware not being sent when registered inside the pool callback (1st commit):

// The middleware is never being called
Http::pool(fn (Pool $pool) => [
    $pool->withMiddleware($middleware)->get('https://example.com'),
]);

This is because the Guzzle client is created in the setHandler method on PendingRequest. Because of this, middleware are never registered on the handler stack when calling send on the request since the Guzzle client is not re-created.

2. Initial setup ignored (2nd commit):

// The token (or any other setup done on the pending request before the pool() method call) is
// not passed to the final requests in the callback
Http::withToken('secret-key')->pool(fn (Pool $pool) => [
    $pool->post('https://example.com/api', $payload_1),
    $pool->post('https://example.com/api', $payload_2),
]);

[taylorotwell]

It seems like there are code changes here that could be breaking to other applications / use cases?

[taylorotwell]

This doesn't seem to return a request instance?

[taylorotwell]

If this is a request, why is the variable $factory?

[wfeller]

It's actually a PendingRequest template that will be cloned in each pool request. I kept the name since it's like a template for each created request. Lmk if there's a more adequate name.

[driesvints]

I agree with @taylorotwell, this is highly confusing. We shouldn't change all of this imo in 9.x but in 10.x at the earliest (if we're going to do this).

[wfeller]

It seems like there are code changes here that could be breaking to other applications / use cases?

@taylorotwell, I don't think so. Do you have anything in mind?

[driesvints]

This is a major breaking change.

[wfeller]

Only if people are directly creating pools without going through the Http::pool() method. Tbh, we could also just remove the type in the constructor and it would work the same. In the end, $this->factory = ($factory ?? new Factory())->async()->setHandler($handler); will always be a PendingRequest, whether $factory is a Factory or PendingRequest.

If you want, I can split this PR into 2 PR:

1. Resolving the issue with the setHandler method on the PendingRequest (which prevents middleware from being registered inside pools)
2. Pool setup not being passed to the requests created from the pool

This way, no breaking changes in PR 1 and then you can decide what to do with PR 2.

I just need PR 1 for my use-case at the moment.

[henzeb]

Just one question, this change of yours won't break the multiCurlHandler functionality where all pending requests are executed concurrently, right?

[taylorotwell]

One thing I want to get to the bottom of before I feel comfortable with this - when Http::pool and async requests were first added by @cerbero90 ... why was it a goal to use the same client instance. It's explicitly stated in the original PR that it was intended for some reason but never explained why:

#36948

Any ideas?

[cerbero90]

@taylorotwell the reason behind the design choice of using the same client instance is related to the usage of the CurlMultiHandler.

Under the hood, async requests are handled as sub-processes of the main process started by the CurlMultiHandler.

If we were instantiating a different client for each request, we would also have one instance of CurlMultiHandler per request, making each request - in fact - synchronous.

On the other hand, having only one client instance makes sure that the same CurlMultiHandler handles all the requests as sub-processes of the same main process.

[wfeller]

Currently (without this PR), all pending requests in a pool have their own Client instance, but a shared Handler instance.

The only difference with this PR is when the guzzle client is instantiated in the pending request.

See the changes to the setHandler and buildHandlerStack methods on the PendingRequest.

[cerbero90]

Several changes have been applied to my original design.

Haven't kept track of them but as long as the underneath handler is the same, the requests should be handled asynchronously.

An easy way to manually test whether async requests are handled correctly is hitting endpoints delayed on purpose:

// the total waiting time should be ~3 seconds instead of 15

$responses = Http::pool(fn (Pool $pool) => [
    $pool->get('https://httpbin.org/delay/3')->then(/* ... */),
    $pool->get('https://httpbin.org/delay/3')->then(/* ... */),
    $pool->get('https://httpbin.org/delay/3')->then(/* ... */),
    $pool->get('https://httpbin.org/delay/3')->then(/* ... */),
    $pool->get('https://httpbin.org/delay/3')->then(/* ... */),
]);

[taylorotwell]

I've merged part 1 of this PR here: #44179

I need to address the second issue (initial setup ignored) as a separate PR.

I have confirmed that part 1 does not break the async nature of pools.

[wfeller]

Thanks, @taylorotwell!