From 951f76fba27214462f3bc6cd46c752a59e1c3a55 Mon Sep 17 00:00:00 2001 From: Gasol Wu Date: Wed, 12 Dec 2018 16:32:54 +0800 Subject: [PATCH] Reduce memory footprint when parsing HTTP result In order to resolve #82 to avoid additional memory copy of response body (from $response to $responseBody), We deperecated `parseCurlResult` method which use `substr` function to separate HTTP headers and body and create new method `parseCurlResponse` instead. We register `receiveCurlHeader` method as callback function by curl_setopt using `CURLOPT_HEADERFUNCTION` option. This changes is not intended to resolve issue #15 See https://github.com/sabre-io/http/pull/115#discussion_r241292068 --- lib/Client.php | 127 +++++++++++++++++++++++++++++++------- tests/HTTP/ClientTest.php | 57 ++++++++--------- tests/www/large.php | 7 +++ 3 files changed, 136 insertions(+), 55 deletions(-) create mode 100644 tests/www/large.php diff --git a/lib/Client.php b/lib/Client.php index 5940662..d4df42e 100644 --- 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,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,23 @@ protected function createCurlSettingsArray(RequestInterface $request): array const STATUS_CURLERROR = 1; const STATUS_HTTPERROR = 2; + private function parseResponse(string $response, $curlHandle): array + { + if ($this->beInherited) { + $response = $this->parseCurlResult($response, $curlHandle); + } else { + $resourceId = (int) $curlHandle; + if (isset($this->headerLinesMap[$resourceId])) { + $headers = $this->headerLinesMap[$resourceId]; + } else { + $headers = []; + } + $response = $this->parseCurlResponse($headers, $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 +447,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 +541,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 +561,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 +583,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 index 8d9e484..35c46b7 100644 --- a/tests/HTTP/ClientTest.php +++ b/tests/HTTP/ClientTest.php @@ -197,6 +197,24 @@ protected function getAbsoluteUrl($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 */ @@ -369,37 +387,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(); @@ -471,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. */ diff --git a/tests/www/large.php b/tests/www/large.php new file mode 100644 index 0000000..65fefb5 --- /dev/null +++ b/tests/www/large.php @@ -0,0 +1,7 @@ +