Skip to content

Conversation

@cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Nov 29, 2025

The fact that this defaults to null feels like a major footgun. Developers will think that they are getting the benefit of concurrent requests when in fact they are just being executed in series. Within the underlying Guzzle library, passing null does mean to execute without a cap.

Is 2 the right number? I have no idea. But I think anything is better than just defaulting to serial execution. Or we could change it to pass 0 which I think would allow for unbounded concurrency.

@cosmastech cosmastech marked this pull request as draft November 29, 2025 16:36
@cosmastech cosmastech marked this pull request as ready for review November 29, 2025 16:43
Comment on lines +3530 to +3537
[$exception] = $this->factory->pool(function (Pool $pool) {
return [
$pool->get('https://laravel.com'),
];
});

$this->assertInstanceOf(StrayRequestException::class, $exception);
$this->assertEquals('Attempted request to [https://laravel.com] without a matching fake.', $exception->getMessage());
Copy link
Contributor Author

@cosmastech cosmastech Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified this test to match how we are testing other pooled exceptions in this file. The exception was being thrown because the pool was running in series rather than actually as a pool. This behavior better lines up with how it should be used (in my opinion).

@taylorotwell
Copy link
Member

I'm not sure I'm seeing them execute serially on my end.

For instance, on my machine this does not take 5 seconds:

<?php

use Illuminate\Http\Client\Pool;
use Illuminate\Support\Benchmark;
use Illuminate\Support\Facades\Http;
use Illuminate\Support\Facades\Route;

Route::get('/', function () {
    $time = Benchmark::measure(function () {
        $responses = Http::pool(fn (Pool $pool) => [
            $pool->get('http://laravel.test/test'),
            $pool->get('http://laravel.test/test'),
            $pool->get('http://laravel.test/test'),
            $pool->get('http://laravel.test/test'),
            $pool->get('http://laravel.test/test'),
        ]);

        dump(collect($responses)->map(fn ($response) => (string) $response));
    });

    dd($time);
});

Route::get('/test', function () {
    sleep(1);

    return (string) random_int(1, 100);
});

@taylorotwell taylorotwell merged commit 30ee3f8 into laravel:master Nov 29, 2025
58 checks passed
@cosmastech cosmastech deleted the patch-34 branch November 29, 2025 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants