Reduce memory footprint when parsing HTTP result

In order to resolve sabre-io#82, Remove the substr function in `parseCurlResult()` and register receiveCurlHeader as callback function by using CURLOPT_HEADERFUNCTION instead, To avoid additional memory copy of response body (from $response to $responseBody). This changes is not intended to resolve issue sabre-io#15
Gasol · Dec 13, 2018 · d41058d · d41058d
1 parent 008c80d
commit d41058d
Show file tree

Hide file tree

Showing 3 changed files with 119 additions and 35 deletions.
diff --git a/lib/Client.php b/lib/Client.php
@@ -66,17 +66,34 @@ class Client extends EventEmitter
      */
     protected $maxRedirects = 5;
 
+    protected $headerLinesMap = [];
+
+    protected $beInherited;
+
     /**
      * Initializes the client.
      */
     public function __construct()
     {
+        $this->beInherited = __CLASS__ !== get_class($this);
+
         $this->curlSettings = [
             CURLOPT_RETURNTRANSFER => true,
-            CURLOPT_HEADER => true,
             CURLOPT_NOBODY => false,
             CURLOPT_USERAGENT => 'sabre-http/'.Version::VERSION.' (http://sabre.io/)',
         ];
+        if ($this->beInherited) {
+            $this->curlSettings[CURLOPT_HEADER] = true;
+        } else {
+            $this->curlSettings[CURLOPT_HEADERFUNCTION] = [$this, 'receiveCurlHeader'];
+        }
+    }
+
+    protected function receiveCurlHeader($curlHandle, $headerLine)
+    {
+        $this->headerLinesMap[(int) $curlHandle][] = $headerLine;
+
+        return strlen($headerLine);
     }
 
     /**
@@ -203,7 +220,20 @@ public function poll(): bool
                     $errorCallback,
                     $retryCount) = $this->curlMultiMap[$resourceId];
                 unset($this->curlMultiMap[$resourceId]);
-                $curlResult = $this->parseCurlResult(curl_multi_getcontent($status['handle']), $status['handle']);
+
+                $handle = $status['handle'];
+                $content = curl_multi_getcontent($handle);
+                if ($this->beInherited) { // backward compatible
+                    $curlResult = $this->parseCurlResult($content, $handle);
+                } else {
+                    $resourceId = (int) $handle;
+                    if (isset($this->headerLinesMap[$resourceId])) {
+                        $headers = $this->headerLinesMap[$resourceId];
+                    } else {
+                        $headers = [];
+                    }
+                    $curlResult = $this->parseCurlResponse($headers, $content, $handle);
+                }
                 $retry = false;
 
                 if (self::STATUS_CURLERROR === $curlResult['status']) {
@@ -303,7 +333,17 @@ protected function doRequest(RequestInterface $request): ResponseInterface
 
         curl_setopt_array($this->curlHandle, $settings);
         $response = $this->curlExec($this->curlHandle);
-        $response = $this->parseCurlResult($response, $this->curlHandle);
+        if ($this->beInherited) { // backward compatible
+            $response = $this->parseCurlResult($response, $this->curlHandle);
+        } else {
+            $resourceId = (int) $this->curlHandle;
+            if (isset($this->headerLinesMap[$resourceId])) {
+                $headers = $this->headerLinesMap[$resourceId];
+            } else {
+                $headers = [];
+            }
+            $response = $this->parseCurlResponse($headers, $response, $this->curlHandle);
+        }
 
         if (self::STATUS_CURLERROR === $response['status']) {
             throw new ClientException($response['curl_errmsg'], $response['curl_errno']);
@@ -397,6 +437,42 @@ protected function createCurlSettingsArray(RequestInterface $request): array
     const STATUS_CURLERROR = 1;
     const STATUS_HTTPERROR = 2;
 
+    protected function parseCurlResponse(array $headerLines, string $body, $curlHandle): array
+    {
+        list(
+            $curlInfo,
+            $curlErrNo,
+            $curlErrMsg
+        ) = $this->curlStuff($curlHandle);
+
+        if ($curlErrNo) {
+            return [
+                'status' => self::STATUS_CURLERROR,
+                'curl_errno' => $curlErrNo,
+                'curl_errmsg' => $curlErrMsg,
+            ];
+        }
+
+        $response = new Response();
+        $response->setStatus($curlInfo['http_code']);
+        $response->setBody($body);
+
+        foreach ($headerLines as $header) {
+            $parts = explode(':', $header, 2);
+            if (2 === count($parts)) {
+                $response->addHeader(trim($parts[0]), trim($parts[1]));
+            }
+        }
+
+        $httpCode = $response->getStatus();
+
+        return [
+            'status' => $httpCode >= 400 ? self::STATUS_HTTPERROR : self::STATUS_SUCCESS,
+            'response' => $response,
+            'http_code' => $httpCode,
+        ];
+    }
+
     /**
      * Parses the result of a curl call in a format that's a bit more
      * convenient to work with.
@@ -412,6 +488,7 @@ protected function createCurlSettingsArray(RequestInterface $request): array
      *   * http_code - HTTP status code, as an int. Only set if Only set if
      *                 status is STATUS_SUCCESS, or STATUS_HTTPERROR
      *
+     * @deprecated Use parseCurlResponse instead
      * @param resource $curlHandle
      */
     protected function parseCurlResult(string $response, $curlHandle): array
@@ -487,7 +564,10 @@ protected function sendAsyncInternal(RequestInterface $request, callable $succes
             $this->createCurlSettingsArray($request)
         );
         curl_multi_add_handle($this->curlMultiHandle, $curl);
-        $this->curlMultiMap[(int) $curl] = [
+
+        $resourceId = (int) $curl;
+        $this->headerLinesMap[$resourceId] = [];
+        $this->curlMultiMap[$resourceId] = [
             $request,
             $success,
             $error,
@@ -506,6 +586,8 @@ protected function sendAsyncInternal(RequestInterface $request, callable $succes
      */
     protected function curlExec($curlHandle): string
     {
+        $this->headerLinesMap[(int) $curlHandle] = [];
+
         return curl_exec($curlHandle);
     }
 

diff --git a/tests/HTTP/ClientTest.php b/tests/HTTP/ClientTest.php
@@ -195,6 +195,25 @@ protected function getAbsoluteUrl($path)
         return false;
     }
 
+    /**
+     * @group ci
+     * @runInSeparateProcess
+     */
+    public function testSendToGetLargeContent()
+    {
+        ini_set('memory_limit', '50M');
+        $url = $this->getAbsoluteUrl('/large.php');
+        if (!$url) {
+            $this->markTestSkipped('Set an environment value BASEURL to continue');
+        }
+
+        $request = new Request('GET', $url);
+        $client = new Client();
+        $response = $client->send($request);
+
+        $this->assertEquals(200, $response->getStatus());
+    }
+
     /**
      * @group ci
      */
@@ -367,37 +386,6 @@ public function testParseCurlResult()
         $this->assertEquals('Foo', $result['response']->getBodyAsString());
     }
 
-    /**
-     * @runInSeparateProcess
-     */
-    public function testParseCurlLargeResult()
-    {
-        ini_set('memory_limit', '70M');
-
-        $header = "HTTP/1.1 200 OK\r\nHeader1: Val1\r\n\r\n";
-
-        $client = new ClientMock();
-        $client->on('curlStuff', function (&$return) use ($header) {
-            $return = [
-                [
-                    'header_size' => strlen($header),
-                    'http_code' => 200,
-                ],
-                0,
-                '',
-            ];
-        });
-
-        $body = str_repeat('x', 30 * pow(1024, 2));
-        $response = $header . $body;
-        $result = $client->parseCurlResult($response, 'foobar');
-
-        $this->assertEquals(Client::STATUS_SUCCESS, $result['status']);
-        $this->assertEquals(200, $result['http_code']);
-        $this->assertEquals(200, $result['response']->getStatus());
-        $this->assertEquals(31457280, strlen($result['response']->getBodyAsString()));
-    }
-
     public function testParseCurlError()
     {
         $client = new ClientMock();
@@ -469,6 +457,14 @@ class ClientMock extends Client
 {
     protected $persistedSettings = [];
 
+    /**
+     * Making this method public.
+     */
+    public function receiveCurlHeader($curlHandle, $headerLine)
+    {
+        return parent::receiveCurlHeader($curlHandle, $headerLine);
+    }
+
     /**
      * Making this method public.
      */

diff --git a/tests/www/large.php b/tests/www/large.php
@@ -0,0 +1,6 @@
+<?php
+
+$data = str_repeat('x', 32 * 1024 * 1024);
+header('Content-Length: ' . strlen($data));
+
+echo $data;