Skip to content

Conversation

@WendellAdriel
Copy link
Contributor

As mentioned on #57973 there are some issues with the concurrency handling on Http::batch and Http::pool that 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 (via getPromise() or direct method calls like get()), the underlying
HTTP requests are initiated immediately. By the time EachPromise receives the promises array, all requests are already in flight.

Example of the problem:

Http::batch(fn ($batch) => [
    $batch->get('https://httpbin.org/delay/5'),
    $batch->get('https://httpbin.org/delay/5'),
    $batch->get('https://httpbin.org/delay/5'),
])->concurrency(1)->send();

// Expected: ~15 seconds (1 request at a time)
// Actual: ~5 seconds (all requests run in parallel)

This affects three code paths:

  1. Batch::__call() - numeric indexed requests: $batch->get()
  2. Batch::as() - keyed requests: $batch->as('key')->get()
  3. Pool::__call() - numeric indexed requests: $pool->get()
  4. Pool::as() - keyed requests: $pool->as('key')->get()

The fix

A new DeferredRequest class intercepts method calls and stores them as closures for deferred execution:

  class DeferredRequest
  {
      public function __call($method, $parameters)
      {
          $this->requests[$this->key] = fn () => $this->factory
              ->setHandler($this->handler)
              ->async()
              ->$method(...$parameters);

          return $this;
      }
  }

Updated Batch Class

  • Batch::__call(): Store closures instead of executing requests immediately
  • Batch::as(): Return DeferredRequest instead of PendingRequest
  • Batch::send(): Use a generator to yield promises on-demand when EachPromise requests them

Updated Pool Class

  • Pool::__call(): Store closures instead of executing requests immediately
  • Pool::as(): Return DeferredRequest instead of PendingRequest

Updated PendingRequest::pool() Method

  • With concurrency: Use a generator to create promises on-demand
  • Without concurrency: Create all promises first (to maintain parallel execution), then wait for all

How It Works

EachPromise accepts 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:

  1. Only N promises are created and executed at a time (where N = concurrency limit)
  2. When a promise completes, the generator creates the next one
  3. This properly throttles the HTTP requests

Examples

  Before (Broken)

  // Example 1: Batch with concurrency
  Http::batch(fn ($batch) => [
      $batch->get('https://httpbin.org/delay/2'),
      $batch->get('https://httpbin.org/delay/2'),
      $batch->get('https://httpbin.org/delay/2'),
      $batch->get('https://httpbin.org/delay/2'),
  ])->concurrency(2)->send();
  // Expected: ~4s (2 batches of 2s each)
  // Actual: ~2s (all run in parallel) ❌

  // Example 2: Pool with concurrency
  Http::pool(fn ($pool) => [
      $pool->as('a')->get('https://httpbin.org/delay/2'),
      $pool->as('b')->get('https://httpbin.org/delay/2'),
      $pool->as('c')->get('https://httpbin.org/delay/2'),
      $pool->as('d')->get('https://httpbin.org/delay/2'),
  ], 2);
  // Expected: ~4s (2 batches of 2s each)
  // Actual: ~2s (all run in parallel) ❌

  After (Fixed)

  // Example 1: Batch with concurrency
  Http::batch(fn ($batch) => [
      $batch->get('https://httpbin.org/delay/2'),
      $batch->get('https://httpbin.org/delay/2'),
      $batch->get('https://httpbin.org/delay/2'),
      $batch->get('https://httpbin.org/delay/2'),
  ])->concurrency(2)->send();
  // Takes ~4s (2 batches of 2s each) ✅

  // Example 2: Pool with concurrency
  Http::pool(fn ($pool) => [
      $pool->as('a')->get('https://httpbin.org/delay/2'),
      $pool->as('b')->get('https://httpbin.org/delay/2'),
      $pool->as('c')->get('https://httpbin.org/delay/2'),
      $pool->as('d')->get('https://httpbin.org/delay/2'),
  ], 2);
  // Takes ~4s (2 batches of 2s each) ✅

  // Example 3: Pool without concurrency (parallel execution still works)
  Http::pool(fn ($pool) => [
      $pool->as('a')->get('https://httpbin.org/delay/2'),
      $pool->as('b')->get('https://httpbin.org/delay/2'),
      $pool->as('c')->get('https://httpbin.org/delay/2'),
  ]);
  // Takes ~2s (all run in parallel) ✅

Impact of changes

  • All existing code continues to work
  • The fix only affects the internal execution timing to match the documented/expected behavior
  • No breaking changes to method signatures or return types (except as() now returns DeferredRequest, but it implements __call() for the same interface)

@WendellAdriel
Copy link
Contributor Author

@taylorotwell @cosmastech please take a look at this and let me know your thoughts

@WendellAdriel
Copy link
Contributor Author

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.

@cosmastech
Copy link
Contributor

@taylorotwell @cosmastech please take a look at this and let me know your thoughts

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()).

@WendellAdriel
Copy link
Contributor Author

@taylorotwell @cosmastech please take a look at this and let me know your thoughts

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 DeferredRequest class. if you want to run some more tests with this approach to check, it would be great @cosmastech!

@WendellAdriel
Copy link
Contributor Author

@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.

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