Skip to content

[HttpClient] fix management of shorter-than-requested timeouts with AmpHttpClient #36960

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

Merged
merged 1 commit into from
May 26, 2020
Merged

[HttpClient] fix management of shorter-than-requested timeouts with AmpHttpClient #36960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 29 additions & 14 deletions src/Symfony/Component/HttpClient/Response/AmpResponse.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ final class AmpResponse implements ResponseInterface
private $canceller;
private $onProgress;

private static $delay;

/**
* @internal
*/
Expand Down Expand Up @@ -171,18 +173,21 @@ private static function perform(ClientState $multi, array &$responses = null): v
*/
private static function select(ClientState $multi, float $timeout): int
{
$selected = 1;
$delay = Loop::delay(1000 * $timeout, static function () use (&$selected) {
$selected = 0;
Loop::stop();
});
Loop::run();
$start = microtime(true);
$remaining = $timeout;

if ($selected) {
Loop::cancel($delay);
}
while (true) {
self::$delay = Loop::delay(1000 * $remaining, [Loop::class, 'stop']);
Copy link
Member Author

@nicolas-grekas nicolas-grekas May 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@kelunik there is something I don't get here that you might want to check once this PR is merged. It looks like the delay defined here is not respected.

To see what I mean, checkout this PR, then add dump([$timeout, $remaining]); after the second "if" below, and run tests with ./phpunit src/Symfony/Component/HttpClient/ --filter Amp.*NotATimeout.
You'll see something like:

array:2 [
  0 => 0.9
  1 => 0.87336587905884
]

which means that the delay expired 0.87336587905884 seconds earlier than expected.
Do I miss something or could this be a bug in AMP's event loop?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added two failing tests in amphp/amp#319, one of those is expected to fail, the other isn't.

Loop::run();

return $selected;
if (null === self::$delay) {
return 1;
}

if (0 >= $remaining = $timeout - microtime(true) + $start) {
return 0;
}
}
}

private static function generateResponse(Request $request, AmpClientState $multi, string $id, array &$info, array &$headers, CancellationTokenSource $canceller, array &$options, \Closure $onProgress, &$handle, ?LoggerInterface $logger)
Expand All @@ -192,7 +197,7 @@ private static function generateResponse(Request $request, AmpClientState $multi
$request->setInformationalResponseHandler(static function (Response $response) use (&$activity, $id, &$info, &$headers) {
self::addResponseHeaders($response, $info, $headers);
$activity[$id][] = new InformationalChunk($response->getStatus(), $response->getHeaders());
Loop::defer([Loop::class, 'stop']);
self::stopLoop();
});

try {
Expand All @@ -210,7 +215,7 @@ private static function generateResponse(Request $request, AmpClientState $multi
if ('HEAD' === $response->getRequest()->getMethod() || \in_array($info['http_code'], [204, 304], true)) {
$activity[$id][] = null;
$activity[$id][] = null;
Loop::defer([Loop::class, 'stop']);
self::stopLoop();

return;
}
Expand All @@ -222,7 +227,7 @@ private static function generateResponse(Request $request, AmpClientState $multi
$body = $response->getBody();

while (true) {
Loop::defer([Loop::class, 'stop']);
self::stopLoop();

if (null === $data = yield $body->read()) {
break;
Expand All @@ -241,7 +246,7 @@ private static function generateResponse(Request $request, AmpClientState $multi
$info['download_content_length'] = $info['size_download'];
}

Loop::defer([Loop::class, 'stop']);
self::stopLoop();
}

private static function followRedirects(Request $originRequest, AmpClientState $multi, array &$info, array &$headers, CancellationTokenSource $canceller, array $options, \Closure $onProgress, &$handle, ?LoggerInterface $logger)
Expand Down Expand Up @@ -402,4 +407,14 @@ private static function getPushedResponse(Request $request, AmpClientState $mult
return $response;
}
}

private static function stopLoop(): void
{
if (null !== self::$delay) {
Loop::cancel(self::$delay);
self::$delay = null;
}

Loop::defer([Loop::class, 'stop']);
}
}
11 changes: 4 additions & 7 deletions src/Symfony/Component/HttpClient/Tests/HttpClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,15 @@
namespace Symfony\Component\HttpClient\Tests;

use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpClient\AmpHttpClient;
use Symfony\Component\HttpClient\CurlHttpClient;
use Symfony\Component\HttpClient\HttpClient;
use Symfony\Component\HttpClient\NativeHttpClient;
use Symfony\Contracts\HttpClient\HttpClientInterface;

class HttpClientTest extends TestCase
{
public function testCreateClient()
{
if (\extension_loaded('curl') && ('\\' !== \DIRECTORY_SEPARATOR || ini_get('curl.cainfo') || ini_get('openssl.cafile') || ini_get('openssl.capath')) && 0x073d00 <= curl_version()['version_number']) {
$this->assertInstanceOf(CurlHttpClient::class, HttpClient::create());
} else {
$this->assertInstanceOf(AmpHttpClient::class, HttpClient::create());
}
$this->assertInstanceOf(HttpClientInterface::class, HttpClient::create());
$this->assertNotInstanceOf(NativeHttpClient::class, HttpClient::create());
}
}
2 changes: 1 addition & 1 deletion src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
echo '<1>';
@ob_flush();
flush();
usleep(600000);
usleep(500000);
echo '<2>';
exit;

Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,7 @@ public function testTimeoutIsNotAFatalError()
usleep(300000); // wait for the previous test to release the server
$client = $this->getHttpClient(__FUNCTION__);
$response = $client->request('GET', 'http://localhost:8057/timeout-body', [
'timeout' => 0.3,
'timeout' => 0.25,
]);

try {
Expand Down