http-client icon indicating copy to clipboard operation
http-client copied to clipboard

requestEnd() is not called in event listener in sync mode.

Open whataboutpereira opened this issue 2 years ago • 5 comments

I converted my timings collector (based on LogHttpArchive) to the new events system and requestEnd() doesn't get called when the last request has finished.

$client = (new Amp\Http\Client\HttpClientBuilder)
    ->usingPool(Amp\Http\Client\Connection\ConnectionLimitingPool::byAuthority(4))
    ->listen(new HttpRequestTimings)
    ->build();

$semaphore = new Amp\Sync\LocalSemaphore(4);

$futures = [];

foreach ($urls as $url) {
	$futures[] = async(function () use ($client, $semaphore, $url) {
		$request = new Request($url);
		$lock = $semaphore->acquire();
		$response = $client->request($request)->getBody()->buffer();
		$lock->release();
	});
}

await($futures);

At this stage requestEnd() has not been called.

However if I add \Amp\delay(0) after await($futures), then requestEnd() is called.

Is this as intended? Should I somehow await for the HTTP client to finish?

whataboutpereira avatar Oct 05 '23 17:10 whataboutpereira

And... I see this dumbed down code works. I'll have to see what's causing it in my actual script, that didn't with old events.

whataboutpereira avatar Oct 05 '23 18:10 whataboutpereira

Okay, I misread my code a little. I've now narrowed it down into reproducible form.

requestEnd() isn't called in sync mode:

$timings = new HttpRequestTimings;

$client = (new Amp\Http\Client\HttpClientBuilder)
    ->usingPool(Amp\Http\Client\Connection\ConnectionLimitingPool::byAuthority(4))
    ->listen($timings)
    ->build();

$semaphore = new \Amp\Sync\LocalSemaphore(4);

$url = 'https://www.google.com';

$request = new Amp\Http\Client\Request($url);
$lock = $semaphore->acquire();
$response = $client->request($request)->getBody()->buffer();
$lock->release();

requestEnd() is properly called when I wrap the solitary request in async/await.

$timings = new HttpRequestTimings;

$client = (new Amp\Http\Client\HttpClientBuilder)
    ->usingPool(Amp\Http\Client\Connection\ConnectionLimitingPool::byAuthority(4))
    ->listen($timings)
    ->build();

$semaphore = new \Amp\Sync\LocalSemaphore(4);

$url = 'https://www.google.com';

$futures = [];

$futures[] = Amp\async(function () use ($client, $timings, $semaphore, $url) {
    $request = new Amp\Http\Client\Request($url);
    $lock = $semaphore->acquire();
    $response = $client->request($request)->getBody()->buffer();
    $lock->release();
});

Amp\Future\await($futures);

whataboutpereira avatar Oct 06 '23 12:10 whataboutpereira

And the cleaner working version...

$response = Amp\async(function () use ($client, $timings, $semaphore, $url) {
    $request = new Amp\Http\Client\Request($url);
    $lock = $semaphore->acquire();
    $response = $client->request($request)->getBody()->buffer();
    $lock->release();
})->await();

whataboutpereira avatar Oct 06 '23 12:10 whataboutpereira

Is this as intended? Should I somehow await for the HTTP client to finish?

Hey @whataboutpereira, this is expected behavior based on how the event works internally, but I can see this being unexpected behavior from an API user point of view.

The thing is that requestEnd() is called once the future for the trailers completes. However, that will only happen after the body ended. You're waiting for the body to be complete here and that is usually (but not always) the end of the response, but there might still be trailers to be received.

kelunik avatar Oct 08 '23 14:10 kelunik

I solved it by wrapping it in async like above. Perhaps mentioning it in the examples would be fine then. :) Thanks!

whataboutpereira avatar Oct 08 '23 20:10 whataboutpereira