Reduce memory footprint when parsing HTTP result

.travis.yml

@@ -25,6 +25,8 @@ install:
 
 before_script:
   - composer update --prefer-source $PREFER_LOWEST
+  - PHP_BIN=$(phpenv which php)
+  - sudo $PHP_BIN -S localhost:80 -t $TRAVIS_BUILD_DIR/tests/www 2>/dev/null &
 
 script:
   - if [ $RUN_PHPSTAN == "FALSE"  ]; then ./bin/phpunit --configuration tests/phpunit.xml $WITH_COVERAGE; fi

lib/Client.php

@@ -66,17 +66,34 @@ class Client extends EventEmitter
      */
     protected $maxRedirects = 5;
 
+    protected $headerLinesMap = [];
+
     /**
      * Initializes the client.
      */
     public function __construct()
     {
+        // See https://github.com/sabre-io/http/pull/115#discussion_r241292068
+        // Preserve compatibility for sub-classes that implements their own method `parseCurlResult`
+        $separatedHeaders = __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 ($separatedHeaders) {
+            $this->curlSettings[CURLOPT_HEADERFUNCTION] = [$this, 'receiveCurlHeader'];
+        } else {
+            $this->curlSettings[CURLOPT_HEADER] = true;
+        }
+    }
+
+    protected function receiveCurlHeader($curlHandle, $headerLine)
+    {
+        $this->headerLinesMap[(int) $curlHandle][] = $headerLine;
+
+        return strlen($headerLine);
     }
 
     /**
@@ -203,7 +220,9 @@ public function poll(): bool
                     $errorCallback,
                     $retryCount) = $this->curlMultiMap[$resourceId];
                 unset($this->curlMultiMap[$resourceId]);
-                $curlResult = $this->parseCurlResult(curl_multi_getcontent($status['handle']), $status['handle']);
+
+                $curlHandle = $status['handle'];
+                $curlResult = $this->parseResponse(curl_multi_getcontent($curlHandle), $curlHandle);
                 $retry = false;
 
                 if (self::STATUS_CURLERROR === $curlResult['status']) {
@@ -303,8 +322,7 @@ protected function doRequest(RequestInterface $request): ResponseInterface
 
         curl_setopt_array($this->curlHandle, $settings);
         $response = $this->curlExec($this->curlHandle);
-        $response = $this->parseCurlResult($response, $this->curlHandle);
-
+        $response = $this->parseResponse($response, $this->curlHandle);
         if (self::STATUS_CURLERROR === $response['status']) {
             throw new ClientException($response['curl_errmsg'], $response['curl_errno']);
         }
@@ -397,6 +415,26 @@ protected function createCurlSettingsArray(RequestInterface $request): array
     const STATUS_CURLERROR = 1;
     const STATUS_HTTPERROR = 2;
 
+    private function parseResponse(string $response, $curlHandle): array
+    {
+        $settings = $this->curlSettings;
+        $separatedHeaders = isset($settings[CURLOPT_HEADERFUNCTION]) && (bool) $settings[CURLOPT_HEADERFUNCTION];
+
+        if ($separatedHeaders) {
+            $resourceId = (int) $curlHandle;
+            if (isset($this->headerLinesMap[$resourceId])) {
+                $headers = $this->headerLinesMap[$resourceId];
+            } else {
+                $headers = [];
+            }
+            $response = $this->parseCurlResponse($headers, $response, $curlHandle);
+        } else {
+            $response = $this->parseCurlResult($response, $curlHandle);
+        }
+
+        return $response;
+    }
+
     /**
      * Parses the result of a curl call in a format that's a bit more
      * convenient to work with.
@@ -412,6 +450,63 @@ 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
      *
+     * @param array    $headerLines
+     * @param string   $body
+     * @param resource $curlHandle
+     */
+    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.
+     *
+     * The method returns an array with the following elements:
+     *   * status - one of the 3 STATUS constants.
+     *   * curl_errno - A curl error number. Only set if status is
+     *                  STATUS_CURLERROR.
+     *   * curl_errmsg - A current error message. Only set if status is
+     *                   STATUS_CURLERROR.
+     *   * response - Response object. Only set if status is STATUS_SUCCESS, or
+     *                STATUS_HTTPERROR.
+     *   * 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
@@ -449,25 +544,7 @@ protected function parseCurlResult(string $response, $curlHandle): array
         // Splitting headers
         $headerBlob = explode("\r\n", $headerBlob);
 
-        $response = new Response();
-        $response->setStatus($curlInfo['http_code']);
-
-        foreach ($headerBlob as $header) {
-            $parts = explode(':', $header, 2);
-            if (2 === count($parts)) {
-                $response->addHeader(trim($parts[0]), trim($parts[1]));
-            }
-        }
-
-        $response->setBody($responseBody);
-
-        $httpCode = $response->getStatus();
-
-        return [
-            'status' => $httpCode >= 400 ? self::STATUS_HTTPERROR : self::STATUS_SUCCESS,
-            'response' => $response,
-            'http_code' => $httpCode,
-        ];
+        return $this->parseCurlResponse($headerBlob, $responseBody, $curlHandle);
     }
 
     /**
@@ -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,7 +586,14 @@ protected function sendAsyncInternal(RequestInterface $request, callable $succes
      */
     protected function curlExec($curlHandle): string
     {
-        return curl_exec($curlHandle);
+        $this->headerLinesMap[(int) $curlHandle] = [];
+
+        $result = curl_exec($curlHandle);
+        if (false === $result) {
+            $result = '';
+        }
+
+        return $result;
     }
 
     /**

tests/HTTP/ClientTest.php

@@ -185,6 +185,97 @@ public function testSend()
         $this->assertEquals(200, $response->getStatus());
     }
 
+    protected function getAbsoluteUrl($path)
+    {
+        $baseUrl = getenv('BASEURL');
+        if ($baseUrl) {
+            $path = ltrim($path, '/');
+
+            return "$baseUrl/$path";
+        }
+
+        return false;
+    }
+
+    /**
+     * @group ci
+     */
+    public function testSendToGetLargeContent()
+    {
+        $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());
+        $this->assertGreaterThan(memory_get_peak_usage(), 40 * pow(1024, 2));
+    }
+
+    /**
+     * @group ci
+     */
+    public function testSendAsync()
+    {
+        $url = $this->getAbsoluteUrl('/foo');
+        if (!$url) {
+            $this->markTestSkipped('Set an environment value BASEURL to continue');
+        }
+
+        $client = new Client();
+
+        $request = new Request('GET', $url);
+        $client->sendAsync($request, function (ResponseInterface $response) {
+            $this->assertEquals("foo\n", $response->getBody());
+            $this->assertEquals(200, $response->getStatus());
+            $this->assertEquals(4, $response->getHeader('Content-Length'));
+        }, function ($error) use ($request) {
+            $url = $request->getUrl();
+            $this->fail("Failed to GET $url");
+        });
+
+        $client->wait();
+    }
+
+    /**
+     * @group ci
+     */
+    public function testSendAsynConsecutively()
+    {
+        $url = $this->getAbsoluteUrl('/foo');
+        if (!$url) {
+            $this->markTestSkipped('Set an environment value BASEURL to continue');
+        }
+
+        $client = new Client();
+
+        $request = new Request('GET', $url);
+        $client->sendAsync($request, function (ResponseInterface $response) {
+            $this->assertEquals("foo\n", $response->getBody());
+            $this->assertEquals(200, $response->getStatus());
+            $this->assertEquals(4, $response->getHeader('Content-Length'));
+        }, function ($error) use ($request) {
+            $url = $request->getUrl();
+            $this->fail("Failed to get $url");
+        });
+
+        $url = $this->getAbsoluteUrl('/bar.php');
+        $request = new Request('GET', $url);
+        $client->sendAsync($request, function (ResponseInterface $response) {
+            $this->assertEquals("bar\n", $response->getBody());
+            $this->assertEquals(200, $response->getStatus());
+            $this->assertEquals('Bar', $response->getHeader('X-Test'));
+        }, function ($error) use ($request) {
+            $url = $request->getUrl();
+            $this->fail("Failed to get $url");
+        });
+
+        $client->wait();
+    }
+
     public function testSendClientError()
     {
         $client = new ClientMock();
@@ -367,6 +458,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.
      */

tests/phpunit.xml

@@ -1,10 +1,13 @@
 <phpunit
   colors="true"
+  beStrictAboutCoversAnnotation="true"
+  beStrictAboutOutputDuringTests="true"
+  beStrictAboutTestsThatDoNotTestAnything="true"
+  beStrictAboutTodoAnnotatedTests="true"
   bootstrap="bootstrap.php"
   convertErrorsToExceptions="true"
   convertNoticesToExceptions="true"
   convertWarningsToExceptions="true"
-  strict="true"
   >
   <testsuite name="Sabre_HTTP">
       <directory>HTTP/</directory>
@@ -15,4 +18,8 @@
        <directory suffix=".php">../lib/</directory>
     </whitelist>
   </filter>
+
+  <php>
+    <env name="BASEURL" value="http://localhost"/>
+  </php>
 </phpunit>

tests/www/bar.php

@@ -0,0 +1,5 @@
+<?php
+
+header('X-Test: Bar');
+?>
+bar

tests/www/foo

@@ -0,0 +1 @@
+foo

tests/www/large.php

@@ -0,0 +1,7 @@
+<?php
+
+$data = str_repeat('x', 32 * 1024 * 1024);
+header('Content-Length: '.strlen($data));
+header('Content-Type: text/plain');
+
+echo $data;