[HttpClient] throw exception when AsyncDecoratorTrait gets an already consumed response

Author: nicolas-grekas

Response/AsyncResponse.php

@@ -31,12 +31,15 @@ final class AsyncResponse implements ResponseInterface, StreamableInterface
 {
     use CommonResponseTrait;
 
+    private const FIRST_CHUNK_YIELDED = 1;
+    private const LAST_CHUNK_YIELDED = 2;
+
     private $client;
     private $response;
     private $info = ['canceled' => false];
     private $passthru;
     private $stream;
-    private $lastYielded = false;
+    private $yieldedState;
 
     /**
      * @param ?callable(ChunkInterface, AsyncContext): ?\Iterator $passthru
@@ -272,6 +275,14 @@ public static function stream(iterable $responses, float $timeout = null, string
                     continue;
                 }
 
+                if (null !== $chunk->getError()) {
+                    // no-op
+                } elseif ($chunk->isFirst()) {
+                    $r->yieldedState = self::FIRST_CHUNK_YIELDED;
+                } elseif (self::FIRST_CHUNK_YIELDED !== $r->yieldedState && null === $chunk->getInformationalStatus()) {
+                    throw new \LogicException(sprintf('Instance of "%s" is already consumed and cannot be managed by "%s". A decorated client should not call any of the response\'s methods in its "request()" method.', get_debug_type($response), $class ?? static::class));
+                }
+
                 foreach (self::passthru($r->client, $r, $chunk, $asyncMap) as $chunk) {
                     yield $r => $chunk;
                 }
@@ -282,9 +293,9 @@ public static function stream(iterable $responses, float $timeout = null, string
             }
 
             if (null === $chunk->getError() && $chunk->isLast()) {
-                $r->lastYielded = true;
+                $r->yieldedState = self::LAST_CHUNK_YIELDED;
             }
-            if (null === $chunk->getError() && !$r->lastYielded && $r->response === $response && null !== $r->client) {
+            if (null === $chunk->getError() && self::LAST_CHUNK_YIELDED !== $r->yieldedState && $r->response === $response && null !== $r->client) {
                 throw new \LogicException('A chunk passthru must yield an "isLast()" chunk before ending a stream.');
             }
 

Tests/AsyncDecoratorTraitTest.php

@@ -19,18 +19,19 @@
 use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Symfony\Contracts\HttpClient\ResponseInterface;
+use Symfony\Contracts\HttpClient\ResponseStreamInterface;
 
 class AsyncDecoratorTraitTest extends NativeHttpClientTest
 {
-    protected function getHttpClient(string $testCase, \Closure $chunkFilter = null): HttpClientInterface
+    protected function getHttpClient(string $testCase, \Closure $chunkFilter = null, HttpClientInterface $decoratedClient = null): HttpClientInterface
     {
         if ('testHandleIsRemovedOnException' === $testCase) {
             $this->markTestSkipped("AsyncDecoratorTrait doesn't cache handles");
         }
 
         $chunkFilter = $chunkFilter ?? static function (ChunkInterface $chunk, AsyncContext $context) { yield $chunk; };
 
-        return new class(parent::getHttpClient($testCase), $chunkFilter) implements HttpClientInterface {
+        return new class($decoratedClient ?? parent::getHttpClient($testCase), $chunkFilter) implements HttpClientInterface {
             use AsyncDecoratorTrait;
 
             private $chunkFilter;
@@ -303,4 +304,30 @@ public function testMultipleYieldInInitializer()
         $this->assertSame(404, $response->getStatusCode());
         $this->assertStringContainsString('injectedFoo', $response->getContent(false));
     }
+
+    public function testConsumingDecoratedClient()
+    {
+        $client = $this->getHttpClient(__FUNCTION__, null, new class(parent::getHttpClient(__FUNCTION__)) implements HttpClientInterface {
+            use AsyncDecoratorTrait;
+
+            public function request(string $method, string $url, array $options = []): ResponseInterface
+            {
+                $response = $this->client->request($method, $url, $options);
+                $response->getStatusCode(); // should  be avoided and breaks compatibility with AsyncDecoratorTrait
+
+                return $response;
+            }
+
+            public function stream($responses, float $timeout = null): ResponseStreamInterface
+            {
+                return $this->client->stream($responses, $timeout);
+            }
+        });
+
+        $response = $client->request('GET', 'http://localhost:8057/');
+
+        $this->expectException(\LogicException::class);
+        $this->expectExceptionMessage('Instance of "Symfony\Component\HttpClient\Response\NativeResponse" is already consumed and cannot be managed by "Symfony\Component\HttpClient\Response\AsyncResponse". A decorated client should not call any of the response\'s methods in its "request()" method.');
+        $response->getStatusCode();
+    }
 }