From 0159a397094a30135f9419ec3308b3700e860d9b Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 12 Oct 2020 18:02:59 +0200 Subject: [PATCH] [HttpClient] fix reading the body after a ClientException --- .../Component/HttpClient/Internal/Canary.php | 40 +++++++++++ .../HttpClient/Response/CurlResponse.php | 69 ++++++++----------- .../HttpClient/Response/NativeResponse.php | 17 +++-- .../HttpClient/Response/ResponseTrait.php | 7 +- .../HttpClient/Tests/HttpClientTestCase.php | 1 + .../HttpClient/Test/Fixtures/web/index.php | 3 + 6 files changed, 87 insertions(+), 50 deletions(-) create mode 100644 src/Symfony/Component/HttpClient/Internal/Canary.php diff --git a/src/Symfony/Component/HttpClient/Internal/Canary.php b/src/Symfony/Component/HttpClient/Internal/Canary.php new file mode 100644 index 0000000000..3d14b5fd1a --- /dev/null +++ b/src/Symfony/Component/HttpClient/Internal/Canary.php @@ -0,0 +1,40 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpClient\Internal; + +/** + * @author Nicolas Grekas + * + * @internal + */ +final class Canary +{ + private $canceller; + + public function __construct(\Closure $canceller) + { + $this->canceller = $canceller; + } + + public function cancel() + { + if (($canceller = $this->canceller) instanceof \Closure) { + $this->canceller = null; + $canceller(); + } + } + + public function __destruct() + { + $this->cancel(); + } +} diff --git a/src/Symfony/Component/HttpClient/Response/CurlResponse.php b/src/Symfony/Component/HttpClient/Response/CurlResponse.php index a99bfff2e8..fe1c16ac8d 100644 --- a/src/Symfony/Component/HttpClient/Response/CurlResponse.php +++ b/src/Symfony/Component/HttpClient/Response/CurlResponse.php @@ -15,6 +15,7 @@ use Psr\Log\LoggerInterface; use Symfony\Component\HttpClient\Chunk\FirstChunk; use Symfony\Component\HttpClient\Chunk\InformationalChunk; use Symfony\Component\HttpClient\Exception\TransportException; +use Symfony\Component\HttpClient\Internal\Canary; use Symfony\Component\HttpClient\Internal\ClientState; use Symfony\Component\HttpClient\Internal\CurlClientState; use Symfony\Contracts\HttpClient\ResponseInterface; @@ -149,6 +150,31 @@ final class CurlResponse implements ResponseInterface // Schedule the request in a non-blocking way $multi->openHandles[$id] = [$ch, $options]; curl_multi_add_handle($multi->handle, $ch); + + $this->canary = new Canary(static function () use ($ch, $multi, $id) { + unset($multi->openHandles[$id], $multi->handlesActivity[$id]); + curl_setopt($ch, \CURLOPT_PRIVATE, '_0'); + + if (self::$performing) { + return; + } + + curl_multi_remove_handle($multi->handle, $ch); + curl_setopt_array($ch, [ + \CURLOPT_NOPROGRESS => true, + \CURLOPT_PROGRESSFUNCTION => null, + \CURLOPT_HEADERFUNCTION => null, + \CURLOPT_WRITEFUNCTION => null, + \CURLOPT_READFUNCTION => null, + \CURLOPT_INFILE => null, + ]); + + if (!$multi->openHandles) { + // Schedule DNS cache eviction for the next request + $multi->dnsCache->evictions = $multi->dnsCache->evictions ?: $multi->dnsCache->removals; + $multi->dnsCache->removals = $multi->dnsCache->hostnames = []; + } + }); } /** @@ -199,48 +225,11 @@ final class CurlResponse implements ResponseInterface public function __destruct() { - try { - if (null === $this->timeout) { - return; // Unused pushed response - } - - $this->doDestruct(); - } finally { - $multi = clone $this->multi; - - $this->close(); - - if (!$this->multi->openHandles) { - // Schedule DNS cache eviction for the next request - $this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals; - $this->multi->dnsCache->removals = $this->multi->dnsCache->hostnames = []; - } - - $this->multi = $multi; - } - } - - /** - * {@inheritdoc} - */ - private function close(): void - { - unset($this->multi->openHandles[$this->id], $this->multi->handlesActivity[$this->id]); - curl_setopt($this->handle, \CURLOPT_PRIVATE, '_0'); - - if (self::$performing) { - return; + if (null === $this->timeout) { + return; // Unused pushed response } - curl_multi_remove_handle($this->multi->handle, $this->handle); - curl_setopt_array($this->handle, [ - \CURLOPT_NOPROGRESS => true, - \CURLOPT_PROGRESSFUNCTION => null, - \CURLOPT_HEADERFUNCTION => null, - \CURLOPT_WRITEFUNCTION => null, - \CURLOPT_READFUNCTION => null, - \CURLOPT_INFILE => null, - ]); + $this->doDestruct(); } /** diff --git a/src/Symfony/Component/HttpClient/Response/NativeResponse.php b/src/Symfony/Component/HttpClient/Response/NativeResponse.php index 41eadf1a41..a3566aea32 100644 --- a/src/Symfony/Component/HttpClient/Response/NativeResponse.php +++ b/src/Symfony/Component/HttpClient/Response/NativeResponse.php @@ -14,6 +14,7 @@ namespace Symfony\Component\HttpClient\Response; use Psr\Log\LoggerInterface; use Symfony\Component\HttpClient\Chunk\FirstChunk; use Symfony\Component\HttpClient\Exception\TransportException; +use Symfony\Component\HttpClient\Internal\Canary; use Symfony\Component\HttpClient\Internal\ClientState; use Symfony\Component\HttpClient\Internal\NativeClientState; use Symfony\Contracts\HttpClient\ResponseInterface; @@ -43,7 +44,7 @@ final class NativeResponse implements ResponseInterface public function __construct(NativeClientState $multi, $context, string $url, array $options, array &$info, callable $resolveRedirect, ?callable $onProgress, ?LoggerInterface $logger) { $this->multi = $multi; - $this->id = (int) $context; + $this->id = $id = (int) $context; $this->context = $context; $this->url = $url; $this->logger = $logger; @@ -63,6 +64,10 @@ final class NativeResponse implements ResponseInterface $this->initializer = static function (self $response) { return null === $response->remaining; }; + + $this->canary = new Canary(static function () use ($multi, $id) { + unset($multi->openHandles[$id], $multi->handlesActivity[$id]); + }); } /** @@ -88,17 +93,11 @@ final class NativeResponse implements ResponseInterface try { $this->doDestruct(); } finally { - $multi = clone $this->multi; - - $this->close(); - // Clear the DNS cache when all requests completed if (0 >= --$this->multi->responseCount) { $this->multi->responseCount = 0; $this->multi->dnsCache = []; } - - $this->multi = $multi; } } @@ -192,8 +191,8 @@ final class NativeResponse implements ResponseInterface */ private function close(): void { - unset($this->multi->openHandles[$this->id], $this->multi->handlesActivity[$this->id]); - $this->handle = $this->buffer = $this->onProgress = null; + $this->canary->cancel(); + $this->handle = $this->buffer = $this->inflate = $this->onProgress = null; } /** diff --git a/src/Symfony/Component/HttpClient/Response/ResponseTrait.php b/src/Symfony/Component/HttpClient/Response/ResponseTrait.php index 9f457d334e..4a3dfce062 100644 --- a/src/Symfony/Component/HttpClient/Response/ResponseTrait.php +++ b/src/Symfony/Component/HttpClient/Response/ResponseTrait.php @@ -37,6 +37,7 @@ trait ResponseTrait { private $logger; private $headers = []; + private $canary; /** * @var callable|null A callback that initializes the two previous properties @@ -207,7 +208,11 @@ trait ResponseTrait /** * Closes the response and all its network handles. */ - abstract protected function close(): void; + private function close(): void + { + $this->canary->cancel(); + $this->inflate = null; + } /** * Adds pending responses to the activity list. diff --git a/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php b/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php index 3de724cc70..d429934b8b 100644 --- a/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php +++ b/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php @@ -134,6 +134,7 @@ abstract class HttpClientTestCase extends BaseHttpClientTestCase // The response content-type mustn't be json as that calls getContent // @see src/Symfony/Component/HttpClient/Exception/HttpExceptionTrait.php:58 $this->assertStringNotContainsString('json', $e->getResponse()->getHeaders(false)['content-type'][0] ?? ''); + unset($e); $r = new \ReflectionProperty($client, 'multi'); $r->setAccessible(true); diff --git a/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php b/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php index ed9d4e4045..68406e69c4 100644 --- a/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php +++ b/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php @@ -66,6 +66,9 @@ switch ($vars['REQUEST_URI']) { case '/404-gzipped': header('Content-Type: text/plain', true, 404); ob_start('ob_gzhandler'); + @ob_flush(); + flush(); + usleep(300000); echo 'some text'; exit;