Skip to content

Feature: chunked encoding #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
WyriHaximus merged 46 commits into from
Sep 14, 2016
Merged

Feature: chunked encoding #58

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
b4719e1
Chunked encoding decoder
WyriHaximus Jul 4, 2016
91cb3a5
Renamed to more suitable class name
WyriHaximus Jul 4, 2016
fb14e64
Removed test debug
WyriHaximus Jul 4, 2016
d6d2c1e
Removed assert true as it wasn't useful to the test
WyriHaximus Jul 4, 2016
dd04416
Swap stream when transfer encoding is chunked
WyriHaximus Jul 4, 2016
897c4a1
Removed chunked transfer encoding from todo
WyriHaximus Jul 4, 2016
b51cdb4
Response test to ensure chunked encoding decoder is used when it should
WyriHaximus Jul 5, 2016
e1a3708
Lower case checking as suggested by @jsor https://github.com/reactphp…
WyriHaximus Jul 6, 2016
bfba037
Chunked encoding extensions as mentioned by @jsor https://github.com/…
WyriHaximus Jul 7, 2016
a3b9c80
Don't emit empty data chunks
WyriHaximus Jul 7, 2016
4ca65b3
Pass the extra's from the stream through the response
WyriHaximus Jul 8, 2016
0eab14b
Changed emitted extra to be more explicit as suggested by @jsor at ht…
WyriHaximus Aug 3, 2016
f9067ce
chunkedExtension => chunkExtension
WyriHaximus Aug 3, 2016
85b0c93
Store CRLF position into a variable, suggested by @clue at https://gi…
WyriHaximus Aug 3, 2016
03d271a
Downgraded to a ReadableStreamInterface from the duplex as suggested …
WyriHaximus Aug 3, 2016
18f044a
Removed write stream `end` event forward as suggested by @clue at htt…
WyriHaximus Aug 3, 2016
cd4ba40
Removed chunk extensions as suggested by @clue as they don't have a u…
WyriHaximus Aug 3, 2016
537de43
Enure chunk length header is in as suggested by @clue at https://gith…
WyriHaximus Aug 4, 2016
42201c8
Ensure CRLF in the middle of bodies don't cause unexpected behavior b…
WyriHaximus Aug 4, 2016
48763cf
Removed surplus arguments via @clue https://github.com/reactphp/http-…
WyriHaximus Aug 9, 2016
b32a188
Marking method internal via @clue https://github.com/reactphp/http-cl…
WyriHaximus Aug 9, 2016
6e1ebcc
Removed dead end method via @clue https://github.com/reactphp/http-cl…
WyriHaximus Aug 9, 2016
69ab9c6
Implement ReadableStreamInterface and added missing methods for it vi…
WyriHaximus Aug 9, 2016
d4eda4d
Removed transfer encoding header as suggested by @joelwurtz at https:…
WyriHaximus Aug 15, 2016
804902f
No need to unset when we're not using it from there on
WyriHaximus Aug 15, 2016
29e283b
Implement chunked encoding overflow vulnerability prevention suggeste…
WyriHaximus Aug 16, 2016
0b43386
Removed accidental committed debug code
WyriHaximus Aug 16, 2016
07f649a
Close decoder stream after emitting error via @clue https://github.co…
WyriHaximus Aug 17, 2016
3ca85d6
Implemented max content length limit, by default set at 32KB, va @clu…
WyriHaximus Aug 17, 2016
7f303d8
Don't iterate the buffer when it is smaller then three bytes (removed…
WyriHaximus Aug 17, 2016
01ecdcb
Made it smaller then two instead of three, as two can still still be …
WyriHaximus Aug 17, 2016
37dfdf9
Renamed $maxBufferLength to $maxChunkLength
WyriHaximus Aug 20, 2016
61fb8a3
Removed maximum chunk length as data is flowing through and not buffe…
WyriHaximus Aug 20, 2016
e4bf1ea
Limit chunk header length to 1024 bytes https://github.com/reactphp/h…
WyriHaximus Aug 20, 2016
69e3335
Unnecessary "
WyriHaximus Aug 20, 2016
6aa86db
When the underlying stream ends emit an error when we have unprocesse…
WyriHaximus Aug 20, 2016
eab3c71
In case a substr fails reinitialize buffer as empty string
WyriHaximus Aug 20, 2016
34ee759
Added note about chunked encoded response to readme
WyriHaximus Aug 20, 2016
4f95569
Use write instead of end to let the stream detect the end properly
WyriHaximus Aug 21, 2016
09cde22
End reading from this stream when the end of the chunked encoding has…
WyriHaximus Aug 21, 2016
d7d0473
Close stream regardless https://github.com/reactphp/http-client/pull/…
WyriHaximus Aug 23, 2016
95277ff
Prevent CRLF DOS https://github.com/reactphp/http-client/pull/58#disc…
WyriHaximus Aug 23, 2016
9d4896f
Ignore chunked encoding trailers https://github.com/reactphp/http-cli…
WyriHaximus Aug 23, 2016
8540b4e
Mark ChunkedStreamDecoder as internal
WyriHaximus Sep 13, 2016
415d790
Gave names to exception test data
WyriHaximus Sep 13, 2016
49e9e9e
Gave names to non-exception test data
WyriHaximus Sep 13, 2016
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
4 changes: 3 additions & 1 deletion src/ChunkedStreamDecoder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ protected function iterateBuffer()
$this->emit('data', array(
substr($this->buffer, 0, $chunkLength),
$this,
$this->chunkedExtension
[
'chunkedExtension' => $this->chunkedExtension,
Copy link
Member

@jsor jsor Aug 3, 2016

Choose a reason for hiding this comment

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

It should probably be chunkExtension not chunkedExtensionas i propesed in my comment.

Copy link
Member Author

@WyriHaximus WyriHaximus Aug 3, 2016

Choose a reason for hiding this comment

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

You're right 👍

]
));
Copy link
Member

@clue clue Aug 3, 2016

Choose a reason for hiding this comment

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

I'm not sold on the idea we should emit the chunk extension as part of the data message.
Do we have any specific use case for this right now?

Personally, I would rather remove this for now (reduces complexity). We can still think about introducing this once we see an actual need and we can figure out a decent API without introducing a BC break.

Also, in my projects I only emit the actual data as the other event attributes have always been unreliable across the React ecosystem and it's very easy to use a closure to add arbitrary references to the event handler anyway (e.g. https://github.com/clue/php-utf8-react/blob/master/src/Sequencer.php#L111)

Copy link
Member Author

@WyriHaximus WyriHaximus Aug 3, 2016

Choose a reason for hiding this comment

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

Very valid point, especially since there is no use case. I'll remove it

$this->remainingLength -= $chunkLength;
Copy link
Member

@clue clue Aug 9, 2016

Choose a reason for hiding this comment

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

There's a design decision here: Do we really want to emit incomplete chunks or do we honor the chunk header and then only emit complete chunks?

I'm currently undecided on this, but would likely have suggested to honor the header.

Copy link
Member Author

@WyriHaximus WyriHaximus Aug 9, 2016

Choose a reason for hiding this comment

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

The reasoning behind not honoring the header is to keep as less data floating around somewhere in a buffer and handing it over to who ever is using the client as soon as possible.

I don't have any strong preference regarding this. Can write up a benchmark to see which method performs best in various situations, but not sure if that is worth the effort 😄 .

Copy link
Member

@clue clue Aug 9, 2016

Choose a reason for hiding this comment

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

Fair point: If the header says the next chunk is 2 GB in size, we should probably not allocate a 2 GB buffer :)

Let's keep this as-is, but perhaps consider adding some documentation and/or tests? 👍

Copy link
Member Author

@WyriHaximus WyriHaximus Aug 9, 2016

Choose a reason for hiding this comment

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

Will add docs and tests 👍

Copy link
Member Author

@WyriHaximus WyriHaximus Aug 16, 2016

Choose a reason for hiding this comment

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

Tests have been added by the way

$this->buffer = substr($this->buffer, $chunkLength);
Expand Down
2 changes: 1 addition & 1 deletion src/Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public function getHeaders()
return $this->headers;
}

public function handleData($data, $stream = null, $extra = '')
public function handleData($data, $stream = null, $extra = [])
Copy link
Member

@clue clue Aug 9, 2016

Choose a reason for hiding this comment

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

Superfluous parameters can be removed again.

{
$this->emit('data', array($data, $this, $extra));
}
Expand Down
50 changes: 25 additions & 25 deletions tests/DecodeChunkedStreamTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,62 +13,62 @@ public function provideChunkedEncoding()
[
Copy link
Member

@clue clue Sep 13, 2016

Choose a reason for hiding this comment

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

Perhaps we should assign names to the data sets? The outer array can use assoc keys to describe what each data set does and eventually may ease debugging. (see also below)

Copy link
Member Author

@WyriHaximus WyriHaximus Sep 13, 2016

Choose a reason for hiding this comment

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

Yeah should have done that from the start, now I have to figure out which is which 👍

["4\r\nWiki\r\n5\r\npedia\r\ne\r\n in\r\n\r\nchunks.\r\n0\r\n\r\n"],
[
'',
'',
'',
['chunkedExtension' => ''],
['chunkedExtension' => ''],
['chunkedExtension' => ''],
],
],
[
["4\r\nWiki\r\n", "5\r\npedia\r\ne\r\n in\r\n\r\nchunks.\r\n0\r\n\r\n"],
[
'',
'',
'',
['chunkedExtension' => ''],
['chunkedExtension' => ''],
['chunkedExtension' => ''],
],
],
[
["4\r\nWiki\r\n", "5\r\n", "pedia\r\ne\r\n in\r\n\r\nchunks.\r\n0\r\n\r\n"],
[

'',
'',
'',
['chunkedExtension' => ''],
['chunkedExtension' => ''],
['chunkedExtension' => ''],
],
],
[
["4\r\nWiki\r\n", "5\r\n", "pedia\r\ne\r\n in\r\n", "\r\nchunks.\r\n0\r\n\r\n"],
[
'',
'',
'',
'',
['chunkedExtension' => ''],
['chunkedExtension' => ''],
['chunkedExtension' => ''],
['chunkedExtension' => ''],
],
],
[
["4\r\n", "Wiki\r\n", "5\r\n", "pedia\r\ne\r\n in\r\n", "\r\nchunks.\r\n0\r\n\r\n"],
[
'',
'',
'',
'',
['chunkedExtension' => ''],
['chunkedExtension' => ''],
['chunkedExtension' => ''],
['chunkedExtension' => ''],
],
],
[
["4\r\n", "Wiki\r\n", "5\r\n", "pedia\r\ne; foo=[bar,beer,pool,cue,win,won]\r\n", " in\r\n", "\r\nchunks.\r\n0\r\n\r\n"],
[
'',
'',
'foo=[bar,beer,pool,cue,win,won]',
'foo=[bar,beer,pool,cue,win,won]',
['chunkedExtension' => ''],
['chunkedExtension' => ''],
['chunkedExtension' => 'foo=[bar,beer,pool,cue,win,won]'],
['chunkedExtension' => 'foo=[bar,beer,pool,cue,win,won]'],
],
],
[
["4; foo=bar\r\n", "Wiki\r\n", "5\r\n", "pedia\r\ne\r\n", " in\r\n", "\r\nchunks.\r\n", "0\r\n\r\n"],
[
'foo=bar',
'',
'',
'',
['chunkedExtension' => 'foo=bar'],
['chunkedExtension' => ''],
['chunkedExtension' => ''],
['chunkedExtension' => ''],
],
],
];
Expand Down
2 changes: 1 addition & 1 deletion tests/ResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public function chunkedEncodingResponse()
$this->assertSame([], $exts);
$stream->write("Wiki\r\n");
$this->assertSame('Wiki', $buffer);
$this->assertSame(['abc=def'], $exts);
$this->assertSame([['chunkedExtension' => 'abc=def']], $exts);
}
}