Skip to content

Commit 98e6a93

Browse files
fabpotfabpot
committed
feature #37482 [HttpClient] always yield a LastChunk in AsyncResponse on destruction (nicolas-grekas)
This PR was merged into the 5.2-dev branch. Discussion ---------- [HttpClient] always yield a LastChunk in AsyncResponse on destruction | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - This ensures that async decorators always "see" the destruction of an AsyncResponse, via an `isLast()` chunk + a "canceled" state. [The diff](https://github.com/symfony/symfony/pull/37482/files?w=1) is hard to read, here are the hints to understand what changed: - a __destructor has been added; - the passthru logic has been moved to a new method but is essentially unchanged. Commits ------- 3a4a583 [HttpClient] always yield a LastChunk in AsyncResponse on destruction
2 parents ad94b39 + 3a4a583 commit 98e6a93

File tree

2 files changed

+154
-110
lines changed

2 files changed

+154
-110
lines changed

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

Lines changed: 135 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Symfony\Component\HttpClient\Exception\TransportException;
1717
use Symfony\Contracts\HttpClient\ChunkInterface;
1818
use Symfony\Contracts\HttpClient\Exception\ExceptionInterface;
19+
use Symfony\Contracts\HttpClient\Exception\HttpExceptionInterface;
1920
use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
2021
use Symfony\Contracts\HttpClient\HttpClientInterface;
2122
use Symfony\Contracts\HttpClient\ResponseInterface;
@@ -69,7 +70,7 @@ public function getHeaders(bool $throw = true): array
6970
$headers = $this->response->getHeaders(false);
7071

7172
if ($throw) {
72-
$this->checkStatusCode($this->getInfo('http_code'));
73+
$this->checkStatusCode();
7374
}
7475

7576
return $headers;
@@ -126,31 +127,44 @@ public function cancel(): void
126127
return;
127128
}
128129

129-
$context = new AsyncContext($this->passthru, $client, $this->response, $this->info, $this->content, $this->offset);
130-
if (null === $stream = ($this->passthru)(new LastChunk(), $context)) {
131-
return;
132-
}
130+
try {
131+
foreach (self::passthru($client, $this, new LastChunk()) as $chunk) {
132+
// no-op
133+
}
133134

134-
if (!$stream instanceof \Iterator) {
135-
throw new \LogicException(sprintf('A chunk passthru must return an "Iterator", "%s" returned.', get_debug_type($stream)));
135+
$this->passthru = null;
136+
} catch (ExceptionInterface $e) {
137+
// ignore any errors when canceling
136138
}
139+
}
137140

138-
try {
139-
foreach ($stream as $chunk) {
140-
if ($chunk->isLast()) {
141-
break;
142-
}
141+
public function __destruct()
142+
{
143+
$httpException = null;
144+
145+
if ($this->initializer && null === $this->getInfo('error')) {
146+
try {
147+
$this->getHeaders(true);
148+
} catch (HttpExceptionInterface $httpException) {
149+
// no-op
143150
}
151+
}
144152

145-
$stream->next();
153+
if ($this->passthru && null === $this->getInfo('error')) {
154+
$this->info['canceled'] = true;
155+
$this->info['error'] = 'Response has been canceled.';
146156

147-
if ($stream->valid()) {
148-
throw new \LogicException('A chunk passthru cannot yield after the last chunk.');
157+
try {
158+
foreach (self::passthru($this->client, $this, new LastChunk()) as $chunk) {
159+
// no-op
160+
}
161+
} catch (ExceptionInterface $e) {
162+
// ignore any errors when destructing
149163
}
164+
}
150165

151-
$stream = $this->passthru = null;
152-
} catch (ExceptionInterface $e) {
153-
// ignore any errors when canceling
166+
if (null !== $httpException) {
167+
throw $httpException;
154168
}
155169
}
156170

@@ -201,124 +215,135 @@ public static function stream(iterable $responses, float $timeout = null, string
201215
continue;
202216
}
203217

204-
$context = new AsyncContext($r->passthru, $r->client, $r->response, $r->info, $r->content, $r->offset);
205-
if (null === $stream = ($r->passthru)($chunk, $context)) {
206-
if ($r->response === $response && (null !== $chunk->getError() || $chunk->isLast())) {
207-
throw new \LogicException('A chunk passthru cannot swallow the last chunk.');
208-
}
218+
foreach (self::passthru($r->client, $r, $chunk, $asyncMap) as $chunk) {
219+
yield $r => $chunk;
220+
}
209221

210-
continue;
222+
if ($r->response !== $response && isset($asyncMap[$response])) {
223+
break;
211224
}
212-
$chunk = null;
225+
}
213226

214-
if (!$stream instanceof \Iterator) {
215-
throw new \LogicException(sprintf('A chunk passthru must return an "Iterator", "%s" returned.', get_debug_type($stream)));
227+
if (null === $chunk->getError() && !$chunk->isLast() && $r->response === $response && null !== $r->client) {
228+
throw new \LogicException('A chunk passthru must yield an "isLast()" chunk before ending a stream.');
229+
}
230+
231+
$responses = [];
232+
foreach ($asyncMap as $response) {
233+
$r = $asyncMap[$response];
234+
235+
if (null !== $r->client) {
236+
$responses[] = $asyncMap[$response];
216237
}
238+
}
239+
}
240+
}
217241

218-
while (true) {
219-
try {
220-
if (null !== $chunk) {
221-
$stream->next();
222-
}
223-
224-
if (!$stream->valid()) {
225-
break;
226-
}
227-
} catch (\Throwable $e) {
228-
$r->info['error'] = $e->getMessage();
229-
$r->response->cancel();
230-
231-
yield $r => $chunk = new ErrorChunk($r->offset, $e);
232-
$chunk->didThrow() ?: $chunk->getContent();
233-
unset($asyncMap[$response]);
234-
break;
235-
}
242+
private static function passthru(HttpClientInterface $client, self $r, ChunkInterface $chunk, \SplObjectStorage $asyncMap = null): \Generator
243+
{
244+
$response = $r->response;
245+
$context = new AsyncContext($r->passthru, $client, $r->response, $r->info, $r->content, $r->offset);
246+
if (null === $stream = ($r->passthru)($chunk, $context)) {
247+
if ($r->response === $response && (null !== $chunk->getError() || $chunk->isLast())) {
248+
throw new \LogicException('A chunk passthru cannot swallow the last chunk.');
249+
}
236250

237-
$chunk = $stream->current();
251+
return;
252+
}
253+
$chunk = null;
238254

239-
if (!$chunk instanceof ChunkInterface) {
240-
throw new \LogicException(sprintf('A chunk passthru must yield instances of "%s", "%s" yielded.', ChunkInterface::class, get_debug_type($chunk)));
241-
}
255+
if (!$stream instanceof \Iterator) {
256+
throw new \LogicException(sprintf('A chunk passthru must return an "Iterator", "%s" returned.', get_debug_type($stream)));
257+
}
242258

243-
if (null !== $chunk->getError()) {
244-
// no-op
245-
} elseif ($chunk->isFirst()) {
246-
$e = $r->openBuffer();
247-
248-
yield $r => $chunk;
249-
250-
if (null === $e) {
251-
continue;
252-
}
253-
254-
$r->response->cancel();
255-
$chunk = new ErrorChunk($r->offset, $e);
256-
} elseif ('' !== $content = $chunk->getContent()) {
257-
if (null !== $r->shouldBuffer) {
258-
throw new \LogicException('A chunk passthru must yield an "isFirst()" chunk before any content chunk.');
259-
}
260-
261-
if (null !== $r->content && \strlen($content) !== fwrite($r->content, $content)) {
262-
$chunk = new ErrorChunk($r->offset, new TransportException(sprintf('Failed writing %d bytes to the response buffer.', \strlen($content))));
263-
$r->info['error'] = $chunk->getError();
264-
$r->response->cancel();
265-
}
266-
}
259+
while (true) {
260+
try {
261+
if (null !== $chunk) {
262+
$stream->next();
263+
}
267264

268-
if (null === $chunk->getError()) {
269-
$r->offset += \strlen($content);
265+
if (!$stream->valid()) {
266+
break;
267+
}
268+
} catch (\Throwable $e) {
269+
$r->info['error'] = $e->getMessage();
270+
$r->response->cancel();
270271

271-
yield $r => $chunk;
272+
yield $r => $chunk = new ErrorChunk($r->offset, $e);
273+
$chunk->didThrow() ?: $chunk->getContent();
274+
unset($asyncMap[$response]);
275+
break;
276+
}
272277

273-
if (!$chunk->isLast()) {
274-
continue;
275-
}
278+
$chunk = $stream->current();
276279

277-
$stream->next();
280+
if (!$chunk instanceof ChunkInterface) {
281+
throw new \LogicException(sprintf('A chunk passthru must yield instances of "%s", "%s" yielded.', ChunkInterface::class, get_debug_type($chunk)));
282+
}
278283

279-
if ($stream->valid()) {
280-
throw new \LogicException('A chunk passthru cannot yield after an "isLast()" chunk.');
281-
}
284+
if (null !== $chunk->getError()) {
285+
// no-op
286+
} elseif ($chunk->isFirst()) {
287+
$e = $r->openBuffer();
282288

283-
$r->passthru = null;
284-
} else {
285-
if ($chunk instanceof ErrorChunk) {
286-
$chunk->didThrow(false);
287-
} else {
288-
try {
289-
$chunk = new ErrorChunk($chunk->getOffset(), !$chunk->isTimeout() ?: $chunk->getError());
290-
} catch (TransportExceptionInterface $e) {
291-
$chunk = new ErrorChunk($chunk->getOffset(), $e);
292-
}
293-
}
289+
yield $r => $chunk;
294290

295-
yield $r => $chunk;
296-
$chunk->didThrow() ?: $chunk->getContent();
297-
}
291+
if ($r->initializer && null === $r->getInfo('error')) {
292+
// Ensure the HTTP status code is always checked
293+
$r->getHeaders(true);
294+
}
298295

299-
unset($asyncMap[$response]);
300-
break;
296+
if (null === $e) {
297+
continue;
301298
}
302299

303-
$stream = $context = null;
300+
$r->response->cancel();
301+
$chunk = new ErrorChunk($r->offset, $e);
302+
} elseif ('' !== $content = $chunk->getContent()) {
303+
if (null !== $r->shouldBuffer) {
304+
throw new \LogicException('A chunk passthru must yield an "isFirst()" chunk before any content chunk.');
305+
}
304306

305-
if ($r->response !== $response && isset($asyncMap[$response])) {
306-
break;
307+
if (null !== $r->content && \strlen($content) !== fwrite($r->content, $content)) {
308+
$chunk = new ErrorChunk($r->offset, new TransportException(sprintf('Failed writing %d bytes to the response buffer.', \strlen($content))));
309+
$r->info['error'] = $chunk->getError();
310+
$r->response->cancel();
307311
}
308312
}
309313

310-
if (null === $chunk->getError() && !$chunk->isLast() && $r->response === $response && null !== $r->client) {
311-
throw new \LogicException('A chunk passthru must yield an "isLast()" chunk before ending a stream.');
312-
}
314+
if (null === $chunk->getError()) {
315+
$r->offset += \strlen($content);
313316

314-
$responses = [];
315-
foreach ($asyncMap as $response) {
316-
$r = $asyncMap[$response];
317+
yield $r => $chunk;
317318

318-
if (null !== $r->client) {
319-
$responses[] = $asyncMap[$response];
319+
if (!$chunk->isLast()) {
320+
continue;
321+
}
322+
323+
$stream->next();
324+
325+
if ($stream->valid()) {
326+
throw new \LogicException('A chunk passthru cannot yield after an "isLast()" chunk.');
327+
}
328+
329+
$r->passthru = null;
330+
} else {
331+
if ($chunk instanceof ErrorChunk) {
332+
$chunk->didThrow(false);
333+
} else {
334+
try {
335+
$chunk = new ErrorChunk($chunk->getOffset(), !$chunk->isTimeout() ?: $chunk->getError());
336+
} catch (TransportExceptionInterface $e) {
337+
$chunk = new ErrorChunk($chunk->getOffset(), $e);
338+
}
320339
}
340+
341+
yield $r => $chunk;
342+
$chunk->didThrow() ?: $chunk->getContent();
321343
}
344+
345+
unset($asyncMap[$response]);
346+
break;
322347
}
323348
}
324349

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Symfony\Component\HttpClient\Response\AsyncContext;
1616
use Symfony\Component\HttpClient\Response\AsyncResponse;
1717
use Symfony\Contracts\HttpClient\ChunkInterface;
18+
use Symfony\Contracts\HttpClient\Exception\ClientExceptionInterface;
1819
use Symfony\Contracts\HttpClient\HttpClientInterface;
1920
use Symfony\Contracts\HttpClient\ResponseInterface;
2021

@@ -163,4 +164,22 @@ public function testProcessingHappensOnce()
163164
$this->assertTrue($chunk->isLast());
164165
$this->assertSame(1, $lastChunks);
165166
}
167+
168+
public function testLastChunkIsYieldOnHttpExceptionAtDestructTime()
169+
{
170+
$lastChunk = null;
171+
$client = $this->getHttpClient(__FUNCTION__, function (ChunkInterface $chunk, AsyncContext $context) use (&$lastChunk) {
172+
$lastChunk = $chunk;
173+
174+
yield $chunk;
175+
});
176+
177+
try {
178+
$client->request('GET', 'http://localhost:8057/404');
179+
$this->fail(ClientExceptionInterface::class.' expected');
180+
} catch (ClientExceptionInterface $e) {
181+
}
182+
183+
$this->assertTrue($lastChunk->isLast());
184+
}
166185
}

0 commit comments

Comments
 (0)