Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 18cd342

Browse filesBrowse 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
Copy full SHA for 18cd342

File tree

4 files changed

+38
-19
lines changed
Filter options

4 files changed

+38
-19
lines changed

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Response/CurlResponse.php
+11-5Lines 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

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Response/NativeResponse.php
+6-4Lines 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

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Response/ResponseTrait.php
+10-9Lines 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

Copy file name to clipboardExpand all lines: src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php
+11-1Lines changed: 11 additions & 1 deletion
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+
$chunk = null;
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)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.