Skip to content

Commit

Permalink
[HttpClient] Fix handling timeouts when responses are destructed
Browse files Browse the repository at this point in the history
  • Loading branch information
nicolas-grekas committed Sep 6, 2021
1 parent 2ed7672 commit c95deea
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 5 deletions.
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpClient/Internal/ClientState.php
Expand Up @@ -22,4 +22,5 @@ class ClientState
{
public $handlesActivity = [];
public $openHandles = [];
public $lastTimeout;
}
21 changes: 16 additions & 5 deletions src/Symfony/Component/HttpClient/Response/ResponseTrait.php
Expand Up @@ -233,15 +233,15 @@ abstract protected static function perform(ClientState $multi, array &$responses
*/
abstract protected static function select(ClientState $multi, float $timeout): int;

private static function initialize(self $response): void
private static function initialize(self $response, float $timeout = null): void
{
if (null !== $response->info['error']) {
throw new TransportException($response->info['error']);
}

try {
if (($response->initializer)($response)) {
foreach (self::stream([$response]) as $chunk) {
if (($response->initializer)($response, $timeout)) {
foreach (self::stream([$response], $timeout) as $chunk) {
if ($chunk->isFirst()) {
break;
}
Expand Down Expand Up @@ -304,7 +304,7 @@ private function doDestruct()
$this->shouldBuffer = true;

if ($this->initializer && null === $this->info['error']) {
self::initialize($this);
self::initialize($this, -0.0);
$this->checkStatusCode();
}
}
Expand All @@ -325,6 +325,12 @@ public static function stream(iterable $responses, float $timeout = null): \Gene
$lastActivity = microtime(true);
$elapsedTimeout = 0;

if ($fromLastTimeout = 0.0 === $timeout && '-0' === (string) $timeout) {
$timeout = null;
} elseif ($fromLastTimeout = 0 > $timeout) {
$timeout = -$timeout;
}

while (true) {
$hasActivity = false;
$timeoutMax = 0;
Expand All @@ -340,13 +346,18 @@ public static function stream(iterable $responses, float $timeout = null): \Gene
$timeoutMin = min($timeoutMin, $response->timeout, 1);
$chunk = false;

if ($fromLastTimeout && null !== $multi->lastTimeout) {
$elapsedTimeout = microtime(true) - $multi->lastTimeout;
}

if (isset($multi->handlesActivity[$j])) {
// no-op
$multi->lastTimeout = null;
} elseif (!isset($multi->openHandles[$j])) {
unset($responses[$j]);
continue;
} elseif ($elapsedTimeout >= $timeoutMax) {
$multi->handlesActivity[$j] = [new ErrorChunk($response->offset, sprintf('Idle timeout reached for "%s".', $response->getInfo('url')))];
$multi->lastTimeout ?? $multi->lastTimeout = $lastActivity;
} else {
continue;
}
Expand Down
9 changes: 9 additions & 0 deletions src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php
Expand Up @@ -19,6 +19,15 @@

abstract class HttpClientTestCase extends BaseHttpClientTestCase
{
public function testTimeoutOnDestruct()
{
if (!method_exists(parent::class, 'testTimeoutOnDestruct')) {
$this->markTestSkipped('BaseHttpClientTestCase doesn\'t have testTimeoutOnDestruct().');
}

parent::testTimeoutOnDestruct();
}

public function testAcceptHeader()
{
$client = $this->getHttpClient(__FUNCTION__);
Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php
Expand Up @@ -115,6 +115,10 @@ protected function getHttpClient(string $testCase): HttpClientInterface
$this->markTestSkipped('Real transport required');
break;

case 'testTimeoutOnDestruct':
$this->markTestSkipped('Real transport required');
break;

case 'testDestruct':
$this->markTestSkipped("MockHttpClient doesn't timeout on destruct");
break;
Expand Down
Expand Up @@ -25,4 +25,9 @@ public function testInformationalResponseStream()
{
$this->markTestSkipped('NativeHttpClient doesn\'t support informational status codes.');
}

public function testTimeoutOnDestruct()
{
$this->markTestSkipped('NativeHttpClient doesn\'t support opening concurrent requests.');
}
}
32 changes: 32 additions & 0 deletions src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php
Expand Up @@ -810,6 +810,38 @@ public function testTimeoutWithActiveConcurrentStream()
}
}

public function testTimeoutOnDestruct()
{
$p1 = TestHttpServer::start(8067);
$p2 = TestHttpServer::start(8077);

$client = $this->getHttpClient(__FUNCTION__);
$start = microtime(true);
$responses = [];

$responses[] = $client->request('GET', 'http://localhost:8067/timeout-header', ['timeout' => 0.25]);
$responses[] = $client->request('GET', 'http://localhost:8077/timeout-header', ['timeout' => 0.25]);
$responses[] = $client->request('GET', 'http://localhost:8067/timeout-header', ['timeout' => 0.25]);
$responses[] = $client->request('GET', 'http://localhost:8077/timeout-header', ['timeout' => 0.25]);

try {
while ($response = array_shift($responses)) {
try {
unset($response);
$this->fail(TransportExceptionInterface::class.' expected');
} catch (TransportExceptionInterface $e) {
}
}

$duration = microtime(true) - $start;

$this->assertLessThan(1.0, $duration);
} finally {
$p1->stop();
$p2->stop();
}
}

public function testDestruct()
{
$client = $this->getHttpClient(__FUNCTION__);
Expand Down

0 comments on commit c95deea

Please sign in to comment.