10000 bug #42896 [HttpClient] Fix handling timeouts when responses are dest… · symfony/symfony@6645885 · GitHub
[go: up one dir, main page]
Skip to content

Commit 6645885

Browse files
nicolas-grekasnicolas-grekas
committed
bug #42896 [HttpClient] Fix handling timeouts when responses are destructed (nicolas-grekas)
This PR was merged into the 4.4 branch. Discussion ---------- [HttpClient] Fix handling timeouts when responses are destructed | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Spotted while giving a workshop on HttpClient at WebSummerCamp: When responses are destructed, the timeout is counted from the call to the destructor. This means that when 10 concurrent requests are destructed and they all timeout, the code waits for `10 * $timeout` before giving back control. This PR fixes it by counting the timeouts from the first timeout when they happen in chain on destruction. Commits ------- c95deea [HttpClient] Fix handling timeouts when responses are destructed
2 parents dd45efc + c95deea commit 6645885

File tree

6 files changed

+67
-5
lines changed

6 files changed

+67
-5
lines changed

‎src/Symfony/Component/HttpClient/Internal/ClientState.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,5 @@ class ClientState
2222
{
2323
public $handlesActivity = [];
2424
public $openHandles = [];
25+
public $lastTimeout;
2526
}

‎src/Symfony/Component/HttpClient/Response/ResponseTrait.php

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -233,15 +233,15 @@ abstract protected static function perform(ClientState $multi, array &$responses
233233
*/
234234
abstract protected static function select(ClientState $multi, float $timeout): int;
235235

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

242242
try {
243-
if (($response->initializer)($response)) {
244-
foreach (self::stream([$response]) as $chunk) {
243+
if (($response->initializer)($response, $timeout)) {
244+
foreach (self::stream([$response], $timeout) as $chunk) {
245245
if ($chunk->isFirst()) {
246246
break;
247247
}
@@ -304,7 +304,7 @@ private function doDestruct()
304304
$this->shouldBuffer = true;
305305

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

328+
if ($fromLastTimeout = 0.0 === $timeout && '-0' === (string) $timeout) {
329+
$timeout = null;
330+
} elseif ($fromLastTimeout = 0 > $timeout) {
331+
$timeout = -$timeout;
332+
}
333+
328334
while (true) {
329335
$hasActivity = false;
330336
$timeoutMax = 0;
@@ -340,13 +346,18 @@ public static function stream(iterable $responses, float $timeout = null): \Gene
340346
$timeoutMin = min($timeoutMin, $response->timeout, 1);
341347
$chunk = false;
342348

349+
if ($fromLastTimeout && null !== $multi->lastTimeout) {
350+
$elapsedTimeout = microtime(true) - $multi->lastTimeout;
351+
}
352+
343353
if (isset($multi->handlesActivity[$j])) {
344-
// no-op
354+
$multi->lastTimeout = null;
345355
} elseif (!isset($multi->openHandles[$j])) {
346356
unset($responses[$j]);
347357
continue;
348358
} elseif ($elapsedTimeout >= $timeoutMax) {
349359
$multi->handlesActivity[$j] = [new ErrorChunk($response->offset, sprintf('Idle timeout reached for "%s".', $response->getInfo('url')))];
360+
$multi->lastTimeout ?? $multi->lastTimeout = $lastActivity;
350361
} else {
351362
continue;
352363
}

‎src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,15 @@
1919

2020
abstract class HttpClientTestCase extends BaseHttpClientTestCase
2121
{
22+
public function testTimeoutOnDestruct()
23+
{
24+
if (!method_exists(parent::class, 'testTimeoutOnDestruct')) {
25+
$this->markTestSkipped('BaseHttpClientTestCase doesn\'t have testTimeoutOnDestruct().');
26+
}
27+
28+
parent::testTimeoutOnDestruct();
29+
}
30+
2231
public function testAcceptHeader()
2332
{
2433
$client = $this->getHttpClient(__FUNCTION__);

‎src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,10 @@ protected function getHttpClient(string $testCase): HttpClientInterface
115115
$this->markTestSkipped('Real transport required');
116116
break;
117117

118+
case 'testTimeoutOnDestruct':
119+
$this->markTestSkipped('Real transport required');
120+
break;
121+
118122
case 'testDestruct':
119123
$this->markTestSkipped("MockHttpClient doesn't timeout on destruct");
120124
break;

‎src/Symfony/Component/HttpClient/Tests/NativeHttpClientTest.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,4 +25,9 @@ public function testInformationalResponseStream()
2525
{
2626
$this->markTestSkipped('NativeHttpClient doesn\'t support informational status codes.');
2727
}
28+
29+
public function testTimeoutOnDestruct()
30+
{
31+
$this->markTestSkipped('NativeHttpClient doesn\'t support opening concurrent requests.');
32+
}
2833
}

‎src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -810,6 +810,38 @@ public function testTimeoutWithActiveConcurrentStream()
810810
}
811811
}
812812

813+
public function testTimeoutOnDestruct()
814+
{
815+
$p1 = TestHttpServer::start(8067);
816+
$p2 = TestHttpServer::start(8077);
817+
818+
$client = $this->getHttpClient(__FUNCTION__);
819+
$start = microtime(true);
820+
$responses = [];
821+
822+
$responses[] = $client->request('GET', 'http://localhost:8067/timeout-header', ['timeout' => 0.25]);
823+
$responses[] = $client->request('GET', 'http://localhost:8077/timeout-header', ['timeout' => 0.25]);
824+
$responses[] = $client->request('GET', 'http://localhost:8067/timeout-header', ['timeout' => 0.25]);
825+
$responses[] = $client->request('GET', 'http://localhost:8077/timeout-header', ['timeout' => 0.25]);
826+
827+
try {
828+
while ($response = array_shift($responses)) {
829+
try {
830+
unset($response);
831+
$this->fail(TransportExceptionInterface::class.' expected');
832+
} catch (TransportExceptionInterface $e) {
833+
}
834+
}
835+
836+
$duration = microtime(true) - $start;
837+
838+
$this->assertLessThan(1.0, $duration);
839+
} finally {
840+
$p1->stop();
841+
$p2->stop();
842+
}
843+
}
844+
813845
public function testDestruct()
814846
{
815847
$client = $this->getHttpClient(__FUNCTION__);

0 commit comments

Comments
 (0)
0