Skip to content
This repository was archived by the owner on Jan 30, 2020. It is now read-only.

Very inefficient decoding of large chunked messages! #112

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions src/Response.php
Original file line number Diff line number Diff line change
Expand Up @@ -505,17 +505,24 @@ protected function decodeChunkedBody($body)
{
$decBody = '';

while (trim($body)) {
if (! preg_match("/^([\da-fA-F]+)[^\r\n]*\r\n/sm", $body, $m)) {
throw new Exception\RuntimeException(
"Error parsing body - doesn't seem to be a chunked message"
);
$offset = 0;

while (true) {
if (! preg_match("/^([\da-fA-F]+)[^\r\n]*\r\n/sm", $body, $m, 0, $offset)) {
if (trim(substr($body, $offset))) {
// Message was not consumed completely!
throw new Exception\RuntimeException(
'Error parsing body - doesn\'t seem to be a chunked message'
);
}
// Message was consumed completely
break;
}

$length = hexdec(trim($m[1]));
$cut = strlen($m[0]);
$decBody .= substr($body, $cut, $length);
$body = substr($body, $cut + $length + 2);
$decBody .= substr($body, $offset + $cut, $length);
$offset += $cut + $length + 2;
}

return $decBody;
Expand Down
56 changes: 55 additions & 1 deletion test/ResponseTest.php
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?php
/**
* @see https://github.com/zendframework/zend-http for the canonical source repository
* @copyright Copyright (c) 2005-2017 Zend Technologies USA Inc. (http://www.zend.com)
* @copyright Copyright (c) 2005-2018 Zend Technologies USA Inc. (http://www.zend.com)
* @license https://github.com/zendframework/zend-http/blob/master/LICENSE.md New BSD License
*/

Expand Down Expand Up @@ -179,6 +179,60 @@ public function testChunkedResponseCaseInsensitiveZF5438()
$this->assertEquals('c0cc9d44790fa2a58078059bab1902a9', md5($res->getContent()));
}

/**
* @param int $chunksize the data size of the chunk to create
* @return string a chunk of data for embedding inside a chunked response
*/
private function makeChunk($chunksize)
{
$chunkdata = str_repeat('W', $chunksize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be inlined

return sprintf("%d\r\n%s\r\n", $chunksize, $chunkdata);
}

/**
* @param Response $response
* @return float the time that calling the getBody function took on the response
*/
private function getTimeForGetBody(Response $response)
{
$timeStart = microtime(true);
$response->getBody();
return microtime(true) - $timeStart;
}

/**
* @small
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's @small here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the test fail with the previous code? (just asking, since can't try it on my machine atm)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my machine the test does not fail at all. Neither with php7 nor php 5.6 ... small will be removed.

Copy link
Member

@Ocramius Ocramius Feb 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domoran I was asking if the test fails when removing the patch from the chunk decoding block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, without the path the ratios are on my machine 2000-5000 under PHP7 and about 250 on PHP5.6 ... Test runs > 60 secs.

I can increase the ratio here to make the test pass, but there must be some other problem. I would not expect a ratio much larger than 1, after all this is only one iteration more than before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, going up with those timings is not gonna help, so I suppose that a different approach needs to be taken.

Could you poke me on Friday? I can probably work on it while traveling.

*/
public function testChunkedResponsePerformance()
{
$response = new Response();

$headers = file_get_contents(__DIR__ . '/_files/response_chunked_head');
$response->setHeaders(Headers::fromString($headers));

// avoid flakiness, repeat test
$timings = [];
for ($i = 0; $i < 4; $i++) {
// get baseline for timing: 2000 x 1 Byte chunks
$responseData = str_repeat($this->makeChunk(1), 2000);
$response->setContent($responseData);
$time1 = $this->getTimeForGetBody($response);

// 'worst case' response, where 2000 1 Byte chunks are followed by a 10 MB Chunk
$responseData2 = $responseData . $this->makeChunk(10000000);
$response->setContent($responseData2);
$time2 = $this->getTimeForGetBody($response);

$timings[] = floor($time2 / $time1);
}

array_shift($timings); // do not measure first iteration

// make sure that the worst case packet will have an equal timing as the baseline
$errMsg = 'Chunked response is not parsing large packets efficiently! Timings:';
$this->assertLessThan(20, min($timings), $errMsg . print_r($timings, true));
}

public function testLineBreaksCompatibility()
{
$responseTestLf = $this->readResponse('response_lfonly');
Expand Down
6 changes: 6 additions & 0 deletions test/_files/response_chunked_head
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Date: Sun, 25 Jun 2006 19:55:19 GMT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest moving this into the test method, via <<<'HEADER' (and remove all bits that aren't strictly needed)

Server: Apache
X-powered-by: PHP/5.1.4-pl3-gentoo
Connection: close
Transfer-encoding: chunked
Content-type: text/html