From aeb4ddf31662e052bf74991e6e47f5dab179a8c5 Mon Sep 17 00:00:00 2001 From: HypeMC Date: Fri, 9 Oct 2020 00:15:53 +0200 Subject: [PATCH] [HttpClient] Fix CurlHttpClient memory leak --- .../HttpClient/Response/CurlResponse.php | 10 +++----- .../HttpClient/Response/NativeResponse.php | 10 +++----- .../HttpClient/Tests/HttpClientTestCase.php | 24 +++++++++++++++++++ .../HttpClient/Tests/MockHttpClientTest.php | 4 ++++ .../HttpClient/Test/Fixtures/web/index.php | 4 ++-- 5 files changed, 36 insertions(+), 16 deletions(-) diff --git a/src/Symfony/Component/HttpClient/Response/CurlResponse.php b/src/Symfony/Component/HttpClient/Response/CurlResponse.php index 29f82ac9b7..02236b466b 100644 --- a/src/Symfony/Component/HttpClient/Response/CurlResponse.php +++ b/src/Symfony/Component/HttpClient/Response/CurlResponse.php @@ -17,7 +17,6 @@ use Symfony\Component\HttpClient\Chunk\InformationalChunk; use Symfony\Component\HttpClient\Exception\TransportException; use Symfony\Component\HttpClient\Internal\ClientState; use Symfony\Component\HttpClient\Internal\CurlClientState; -use Symfony\Contracts\HttpClient\Exception\HttpExceptionInterface; use Symfony\Contracts\HttpClient\ResponseInterface; /** @@ -205,14 +204,9 @@ final class CurlResponse implements ResponseInterface return; // Unused pushed response } - $e = null; $this->doDestruct(); - } catch (HttpExceptionInterface $e) { - throw $e; } finally { - if ($e ?? false) { - throw $e; - } + $multi = clone $this->multi; $this->close(); @@ -221,6 +215,8 @@ final class CurlResponse implements ResponseInterface $this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals; $this->multi->dnsCache->removals = $this->multi->dnsCache->hostnames = []; } + + $this->multi = $multi; } } diff --git a/src/Symfony/Component/HttpClient/Response/NativeResponse.php b/src/Symfony/Component/HttpClient/Response/NativeResponse.php index e29005b58f..402210f53e 100644 --- a/src/Symfony/Component/HttpClient/Response/NativeResponse.php +++ b/src/Symfony/Component/HttpClient/Response/NativeResponse.php @@ -16,7 +16,6 @@ use Symfony\Component\HttpClient\Chunk\FirstChunk; use Symfony\Component\HttpClient\Exception\TransportException; use Symfony\Component\HttpClient\Internal\ClientState; use Symfony\Component\HttpClient\Internal\NativeClientState; -use Symfony\Contracts\HttpClient\Exception\HttpExceptionInterface; use Symfony\Contracts\HttpClient\ResponseInterface; /** @@ -87,14 +86,9 @@ final class NativeResponse implements ResponseInterface public function __destruct() { try { - $e = null; $this->doDestruct(); - } catch (HttpExceptionInterface $e) { - throw $e; } finally { - if ($e ?? false) { - throw $e; - } + $multi = clone $this->multi; $this->close(); @@ -103,6 +97,8 @@ final class NativeResponse implements ResponseInterface $this->multi->responseCount = 0; $this->multi->dnsCache = []; } + + $this->multi = $multi; } } diff --git a/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php b/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php index 94b35470e3..3de724cc70 100644 --- a/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php +++ b/src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php @@ -13,6 +13,8 @@ namespace Symfony\Component\HttpClient\Tests; use Symfony\Component\HttpClient\Exception\ClientException; use Symfony\Component\HttpClient\Exception\TransportException; +use Symfony\Component\HttpClient\Internal\ClientState; +use Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface; use Symfony\Contracts\HttpClient\Test\HttpClientTestCase as BaseHttpClientTestCase; abstract class HttpClientTestCase extends BaseHttpClientTestCase @@ -120,4 +122,26 @@ abstract class HttpClientTestCase extends BaseHttpClientTestCase throw $e; } } + + public function testHandleIsRemovedOnException() + { + $client = $this->getHttpClient(__FUNCTION__); + + try { + $client->request('GET', 'http://localhost:8057/304'); + $this->fail(RedirectionExceptionInterface::class.' expected'); + } catch (RedirectionExceptionInterface $e) { + // 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] ?? ''); + + $r = new \ReflectionProperty($client, 'multi'); + $r->setAccessible(true); + /** @var ClientState $clientState */ + $clientState = $r->getValue($client); + + $this->assertCount(0, $clientState->handlesActivity); + $this->assertCount(0, $clientState->openHandles); + } + } } diff --git a/src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php b/src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php index bfed5ce145..17e1dd1c3d 100644 --- a/src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php +++ b/src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php @@ -119,6 +119,10 @@ class MockHttpClientTest extends HttpClientTestCase $this->markTestSkipped("MockHttpClient doesn't timeout on destruct"); break; + case 'testHandleIsRemovedOnException': + $this->markTestSkipped("MockHttpClient doesn't cache handles"); + break; + case 'testGetRequest': array_unshift($headers, 'HTTP/1.1 200 OK'); $responses[] = new MockResponse($body, ['response_headers' => $headers]); diff --git a/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php b/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php index 96486ca316..32a005f404 100644 --- a/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php +++ b/src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php @@ -157,7 +157,7 @@ switch ($vars['REQUEST_URI']) { exit; case '/json': - header("Content-Type: application/json"); + header('Content-Type: application/json'); echo json_encode([ 'documents' => [ ['id' => '/json/1'], @@ -170,7 +170,7 @@ switch ($vars['REQUEST_URI']) { case '/json/1': case '/json/2': case '/json/3': - header("Content-Type: application/json"); + header('Content-Type: application/json'); echo json_encode([ 'title' => $vars['REQUEST_URI'], ]);