[HttpClient] Fix CurlHttpClient memory leak

Author: HypeMC

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

@@ -17,7 +17,6 @@
 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 @@ public function __destruct()
                 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 @@ public function __destruct()
                 $this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals;
                 $this->multi->dnsCache->removals = $this->multi->dnsCache->hostnames = [];
             }
+
+            $this->multi = $multi;
         }
     }
 

src/Symfony/Component/HttpClient/Response/NativeResponse.php

@@ -16,7 +16,6 @@
 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 @@ public function getInfo(string $type = null)
     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 @@ public function __destruct()
                 $this->multi->responseCount = 0;
                 $this->multi->dnsCache = [];
             }
+
+            $this->multi = $multi;
         }
     }
 

src/Symfony/Component/HttpClient/Tests/HttpClientTestCase.php

@@ -13,6 +13,8 @@
 
 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 @@ public function testTimeoutIsNotAFatalError()
             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);
+        }
+    }
 }

src/Symfony/Component/HttpClient/Tests/MockHttpClientTest.php

@@ -119,6 +119,10 @@ protected function getHttpClient(string $testCase): HttpClientInterface
                 $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]);

src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php

@@ -157,7 +157,7 @@
         exit;
 
     case '/json':
-        header("Content-Type: application/json");
+        header('Content-Type: application/json');
         echo json_encode([
             'documents' => [
                 ['id' => '/json/1'],
@@ -170,7 +170,7 @@
     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'],
         ]);