8000 bug #30513 [HttpClient] yield a last chunk for completed responses al… · symfony/symfony@18cd342 · GitHub
[go: up one dir, main page]
Skip to content

Commit 18cd342

Browse files
nicolas-grekasnicolas-grekas
committed
bug #30513 [HttpClient] yield a last chunk for completed responses also (nicolas-grekas)
This PR was merged into the 4.3-dev branch. Discussion ---------- [HttpClient] yield a last chunk for completed responses also | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - When a response completed, streaming it again yields no chunks right now. This PR makes it yield a `LastChunk` - or an `ErrorChunk` when applicable. The reasoning for the previous behavior was that streams should yield only activity from the network. But this looks more complex to use in practice. The proposed behavior is simpler to reason about I think. Commits ------- e11ef7e [HttpClient] yield a last chunk for completed responses also
2 parents 2ac5f1d + e11ef7e commit 18cd342

File tree

4 files changed

+38
-19
lines changed

4 files changed

+38
-19
lines changed

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -80,11 +80,12 @@ public function __construct(\stdClass $multi, $ch, array $options = null, string
8080
if ($onProgress = $options['on_progress']) {
8181
$url = isset($info['url']) ? ['url' => $info['url']] : [];
8282
curl_setopt($ch, CURLOPT_NOPROGRESS, false);
83-
curl_setopt($ch, CURLOPT_PROGRESSFUNCTION, static function ($ch, $dlSize, $dlNow) use ($onProgress, &$info, $url) {
83+
curl_setopt($ch, CURLOPT_PROGRESSFUNCTION, static function ($ch, $dlSize, $dlNow) use ($onProgress, &$info, $url, $multi) {
8484
try {
8585
$onProgress($dlNow, $dlSize, $url + curl_getinfo($ch) + $info);
8686
} catch (\Throwable $e) {
87-
$info['error'] = $e;
87+
$multi->handlesActivity[(int) $ch][] = null;
88+
$multi->handlesActivity[(int) $ch][] = $e;
8889

8990
return 1; // Abort the request
9091
}
@@ -109,6 +110,7 @@ public function __construct(\stdClass $multi, $ch, array $options = null, string
109110
}
110111
self::stream([$response])->current();
111112
} catch (\Throwable $e) {
113+
// Persist timeouts thrown during initialization
112114
$response->info['error'] = $e->getMessage();
113115
$response->close();
114116
throw $e;
@@ -201,13 +203,17 @@ protected function close(): void
201203
*/
202204
protected static function schedule(self $response, array &$runningResponses): void
203205
{
204-
if ('' === curl_getinfo($ch = $response->handle, CURLINFO_PRIVATE)) {
205-
// no-op - response already completed
206-
} elseif (isset($runningResponses[$i = (int) $response->multi->handle])) {
206+
if (isset($runningResponses[$i = (int) $response->multi->handle])) {
207207
$runningResponses[$i][1][$response->id] = $response;
208208
} else {
209209
$runningResponses[$i] = [$response->multi, [$response->id => $response]];
210210
}
211+
212+
if ('' === curl_getinfo($ch = $response->handle, CURLINFO_PRIVATE)) {
213+
// Response already completed
214+
$response->multi->handlesActivity[$response->id][] = null;
215+
$response->multi->handlesActivity[$response->id][] = null !== $response->info['error'] ? new TransportException($response->info['error']) : null;
216+
}
211217
}
212218

213219
/**

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -165,10 +165,6 @@ private function close(): void
165165
*/
166166
private static function schedule(self $response, array &$runningResponses): void
167167
{
168-
if (null === $response->buffer) {
169-
return;
170-
}
171-
172168
if (!isset($runningResponses[$i = $response->multi->id])) {
173169
$runningResponses[$i] = [$response->multi, []];
174170
}
@@ -178,6 +174,12 @@ private static function schedule(self $response, array &$runningResponses): void
178174
} else {
179175
$runningResponses[$i][1][$response->id] = $response;
180176
}
177+
178+
if (null === $response->buffer) {
179+
// Response already completed
180+
$response->multi->handlesActivity[$response->id][] = null;
181+
$response->multi->handlesActivity[$response->id][] = null !== $response->info['error'] ? new TransportException($response->info['error']) : null;
182+
}
181183
}
182184

183185
/**

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

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -100,14 +100,16 @@ public function getContent(bool $throw = true): string
100100
}
101101

102102
if (null === $this->content) {
103-
$content = '';
103+
$content = null;
104104
$chunk = null;
105105

106106
foreach (self::stream([$this]) as $chunk) {
107-
$content .= $chunk->getContent();
107+
if (!$chunk->isLast()) {
108+
$content .= $chunk->getContent();
109+
}
108110
}
109111

110-
if (null === $chunk) {
112+
if (null === $content) {
111113
throw new TransportException('Cannot get the content of the response twice: the request was issued with option "buffer" set to false.');
112114
}
113115

@@ -280,22 +282,21 @@ public static function stream(iterable $responses, float $timeout = null): \Gene
280282
$response->offset += \strlen($chunk);
281283
$chunk = new DataChunk($response->offset, $chunk);
282284
} elseif (null === $chunk) {
283-
if (null !== $e = $response->info['error'] ?? $multi->handlesActivity[$j][0]) {
285+
$e = $multi->handlesActivity[$j][0];
286+
unset($responses[$j], $multi->handlesActivity[$j]);
287+
$response->close();
288+
289+
if (null !== $e) {
284290
$response->info['error'] = $e->getMessage();
285291

286292
if ($e instanceof \Error) {
287-
unset($responses[$j], $multi->handlesActivity[$j]);
288-
$response->close();
289293
throw $e;
290294
}
291295

292296
$chunk = new ErrorChunk($didThrow, $response->offset, $e);
293297
} else {
294298
$chunk = new LastChunk($response->offset);
295299
}
296-
297-
unset($responses[$j]);
298-
$response->close();
299300
} elseif ($chunk instanceof ErrorChunk) {
300301
unset($responses[$j]);
301302
$isTimeout = true;

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

Lines changed: 11 additions & 1 deletion
< 87D0 td data-grid-cell-id="diff-326d22e0b638bc6de979dac9040966325af3de4c654e62433dcbabe406e92366-342-344-2" data-line-anchor="diff-326d22e0b638bc6de979dac9040966325af3de4c654e62433dcbabe406e92366R344" data-selected="false" role="gridcell" style="background-color:var(--diffBlob-additionLine-bgColor, var(--diffBlob-addition-bgColor-line));padding-right:24px" tabindex="-1" valign="top" class="focusable-grid-cell diff-text-cell right-side-diff-cell left-side">+
$chunk = null;
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,8 @@ public function testDnsError()
194194
$this->assertSame($response, $r);
195195
$this->assertNotNull($chunk->getError());
196196

197+
$this->expectException(TransportExceptionInterface::class);
197198
foreach ($client->stream($response) as $chunk) {
198-
$this->fail('Already errored responses shouldn\'t be yielded');
199199
}
200200
}
201201

@@ -340,6 +340,16 @@ public function testStream()
340340

341341
$this->assertSame($response, $r);
342342
$this->assertSame(['f', 'l'], $result);
343+
344
345+
$i = 0;
346+
347+
foreach ($client->stream($response) as $chunk) {
348+
++$i;
349+
}
350+
351+
$this->assertSame(1, $i);
352+
$this->assertTrue($chunk->isLast());
343353
}
344354

345355
public function testAddToStream()

0 commit comments

Comments
 (0)
0