Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[9.x] Fix HTTP client pool #43789

Closed
wants to merge 4 commits into from
Closed

[9.x] Fix HTTP client pool #43789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b93cf6b
fix middleware not being sent in pooled requests
wfeller Aug 20, 2022
b32959d
keep initial request setup when pooling requests
wfeller Aug 20, 2022
d037896
remove obsolete attribute
wfeller Aug 21, 2022
137e9cd
create new collections for each cloned request
wfeller Aug 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 18 additions & 28 deletions src/Illuminate/Http/Client/PendingRequest.php
View file
Open in desktop
Expand Up @@ -42,6 +42,13 @@ class PendingRequest
*/
protected $client;

/**
* The handler function for the Guzzle client.
*
* @var callable|null
*/
protected $handler = null;

/**
* The base URL for the request.
*
Expand Down Expand Up @@ -739,7 +746,7 @@ public function pool(callable $callback)
{
$results = [];

$requests = tap(new Pool($this->factory), $callback)->getRequests();
$requests = tap(new Pool($this), $callback)->getRequests();

foreach ($requests as $key => $item) {
$results[$key] = $item instanceof static ? $item->getPromise()->wait() : $item->wait();
Expand Down Expand Up @@ -966,29 +973,7 @@ protected function populateResponse(Response $response)
*/
public function buildClient()
{
return $this->requestsReusableClient()
? $this->getReusableClient()
: $this->createClient($this->buildHandlerStack());
}

/**
* Determine if a reusable client is required.
*
* @return bool
*/
protected function requestsReusableClient()
{
return ! is_null($this->client) || $this->async;
}

/**
* Retrieve a reusable Guzzle client.
*
* @return \GuzzleHttp\Client
*/
protected function getReusableClient()
{
return $this->client = $this->client ?: $this->createClient($this->buildHandlerStack());
return $this->client ?? $this->createClient($this->buildHandlerStack());
}

/**
Expand All @@ -1012,7 +997,7 @@ public function createClient($handlerStack)
*/
public function buildHandlerStack()
{
return $this->pushHandlers(HandlerStack::create());
return $this->pushHandlers(HandlerStack::create($this->handler));
}

/**
Expand Down Expand Up @@ -1278,9 +1263,7 @@ public function setClient(Client $client)
*/
public function setHandler($handler)
{
$this->client = $this->createClient(
$this->pushHandlers(HandlerStack::create($handler))
);
$this->handler = $handler;

return $this;
}
Expand All @@ -1294,4 +1277,11 @@ public function getOptions()
{
return $this->options;
}

public function __clone() : void
{
$this->beforeSendingCallbacks = new Collection($this->beforeSendingCallbacks->all());
$this->stubCallbacks = new Collection($this->stubCallbacks->all());
$this->middleware = new Collection($this->middleware->all());
}
}
31 changes: 12 additions & 19 deletions src/Illuminate/Http/Client/Pool.php
View file
Open in desktop
Expand Up @@ -5,24 +5,17 @@
use GuzzleHttp\Utils;

/**
* @mixin \Illuminate\Http\Client\Factory
* @mixin \Illuminate\Http\Client\PendingRequest
*/
class Pool
{
/**
* The factory instance.
*
* @var \Illuminate\Http\Client\Factory
* @var \Illuminate\Http\Client\PendingRequest
*/
protected $factory;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


/**
* The handler function for the Guzzle client.
*
* @var callable
*/
protected $handler;

/**
* The pool of requests.
*
Expand All @@ -33,18 +26,18 @@ class Pool
/**
* Create a new requests pool.
*
* @param \Illuminate\Http\Client\Factory|null $factory
* @param \Illuminate\Http\Client\PendingRequest|null $factory
* @return void
*/
public function __construct(Factory $factory = null)
public function __construct(PendingRequest $factory = null)
Copy link
Member

Choose a reason for hiding this comment

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

This is a major breaking change.

Copy link
Contributor Author

@wfeller wfeller Aug 22, 2022 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
$this->factory = $factory ?: new Factory();

if (method_exists(Utils::class, 'chooseHandler')) {
$this->handler = Utils::chooseHandler();
$handler = Utils::chooseHandler();
} else {
$this->handler = \GuzzleHttp\choose_handler();
$handler = \GuzzleHttp\choose_handler();
}

$this->factory = ($factory ?? new Factory())->async()->setHandler($handler);
}

/**
Expand All @@ -55,17 +48,17 @@ public function __construct(Factory $factory = null)
*/
public function as(string $key)
{
return $this->pool[$key] = $this->asyncRequest();
return $this->pool[$key] = $this->newPendingAsyncRequest();
}

/**
* Retrieve a new async pending request.
*
* @return \Illuminate\Http\Client\PendingRequest
*/
protected function asyncRequest()
protected function newPendingAsyncRequest()
{
return $this->factory->setHandler($this->handler)->async();
return clone $this->factory;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to return a request instance?

}

/**
Expand All @@ -87,6 +80,6 @@ public function getRequests()
*/
public function __call($method, $parameters)
{
return $this->pool[] = $this->asyncRequest()->$method(...$parameters);
return $this->pool[] = $this->newPendingAsyncRequest()->$method(...$parameters);
}
}
77 changes: 77 additions & 0 deletions tests/Http/HttpClientTest.php
View file
Open in desktop
Expand Up @@ -1139,6 +1139,83 @@ public function testMultipleRequestsAreSentInThePoolWithKeys()
$this->assertSame(500, $responses['test500']->status());
}

public function testMiddlewareRunsInPool()
{
$this->factory->fake(function (Request $request) {
return $this->factory->response('Fake');
});

$history = [];

$middleware = Middleware::history($history);

$responses = $this->factory->pool(fn (Pool $pool) => [
$pool->withMiddleware($middleware)->post('https://example.com', ['hyped-for' => 'laravel-movie'])
]);

$response = $responses[0];

$this->assertSame('Fake', $response->body());

$this->assertCount(1, $history);

$this->assertSame('Fake', tap($history[0]['response']->getBody())->rewind()->getContents());

$this->assertSame(['hyped-for' => 'laravel-movie'], json_decode(tap($history[0]['request']->getBody())->rewind()->getContents(), true));
}

public function testPoolKeepsInitialSetup()
{
$this->factory->fake([
'https://example.com/endpoint' => $this->factory->response('Fake'),
]);

$history = [];

$middlewareCallCount = 0;

$responses = $this->factory
->withMiddleware(Middleware::history($history))
->withToken('super-secret')
->baseUrl('https://example.com')
->withHeaders([
'x-header' => 'value'
])
->pool(function (Pool $pool) use (&$middlewareCallCount) {
return [
$pool->withMiddleware(Middleware::mapRequest(function ($r) use (&$middlewareCallCount) {
$middlewareCallCount++;

return $r;
}))->withHeaders(['x-custom' => 0])->post('/endpoint', ['hyped-for' => 'laravel-movie-0']),
$pool->withHeaders(['x-custom' => 1])->post('/endpoint', ['hyped-for' => 'laravel-movie-1']),
];
});

$this->assertSame(1, $middlewareCallCount);

foreach ($responses as $key => $response) {
/** @var Response $response */
$this->assertSame('Fake', $response->body());

$this->assertArrayHasKey($key, $history);

$historyItem = $history[$key];

$this->assertSame('Fake', tap($historyItem['response']->getBody())->rewind()->getContents());

$request = $historyItem['request'];

$this->assertSame(['hyped-for' => 'laravel-movie-' . $key], json_decode(tap($request->getBody())->rewind()->getContents(), true));

$headers = $request->getHeaders();

$this->assertSame('Bearer super-secret', $headers['Authorization'][0]);
$this->assertSame('value', $headers['x-header'][0]);
$this->assertSame((string) $key, $headers['x-custom'][0]);
}
}

public function testTheRequestSendingAndResponseReceivedEventsAreFiredWhenARequestIsSent()
{
$events = m::mock(Dispatcher::class);
Expand Down