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.

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

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

Open
whataboutpereira opened this issue Oct 5, 2023 · 5 comments
Open

Comments

@whataboutpereira
Copy link

whataboutpereira commented Oct 5, 2023

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
Copy link
Author

whataboutpereira commented Oct 5, 2023

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
Copy link
Author

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 whataboutpereira changed the title requestEnd() not called in event listener immediately after all requests have completed. requestEnd() is not called in event listener in sync mode. Oct 6, 2023
@whataboutpereira
Copy link
Author

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

@kelunik
Copy link
Member

kelunik commented Oct 8, 2023

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.

@whataboutpereira
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants