-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[12.x] Fix Http::batch and Http::pool concurrency handling #57977
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
base: 12.x
Are you sure you want to change the base?
Conversation
|
@taylorotwell @cosmastech please take a look at this and let me know your thoughts |
|
BTW, there are failing tests that I need to address yet, I just wanted to open the discussion so you can see what I found and what I was thinking about to fix it. |
I had tried this sort of approach before, but I think I switched because it didn't work with chained promises. Like $pool->get('laravel.com')->then(fn ($response) => $response->json()) Not sure if it works with multiple chained calls either (like setting the user agent and then calling get()). |
The tests that were failing were related to that I think, like this one: $responses = $this->factory->pool(fn (Pool $pool) => [
$pool->withMiddleware($middleware)->post('https://example.com', ['hyped-for' => 'laravel-movie']),
]);But now I managed to fix it in my last commit by handling the method chaining issues in the |
|
@taylorotwell @cosmastech I'll be out of home for some time and today I won't be able to work on this anymore, however tomorrow if any other changes are needed, I can take a look at it. Please let me know your thoughts on the fix. |
As mentioned on #57973 there are some issues with the concurrency handling on
Http::batchandHttp::poolthat were not captured before.The issue
The issue occurs because all promises are created upfront before being passed to Guzzle's
EachPromise. When promises are created (viagetPromise()or direct method calls likeget()), the underlyingHTTP requests are initiated immediately. By the time
EachPromisereceives the promises array, all requests are already in flight.Example of the problem:
This affects three code paths:
The fix
A new
DeferredRequestclass intercepts method calls and stores them as closures for deferred execution:Updated Batch Class
Updated Pool Class
Updated PendingRequest::pool() Method
How It Works
EachPromiseaccepts an iterable (array or generator) and is designed to control concurrency by pulling items on-demand. By using a generator that yields closures and only executes them when requested, we ensure that:Examples
Impact of changes