Feature: chunked encoding #58
Conversation
| $stream->on('data', array($this, 'handleData')); | ||
| $stream->on('error', array($this, 'handleError')); | ||
| $stream->on('end', array($this, 'handleEnd')); | ||
| if (isset($this->headers['transfer-encoding']) && $this->headers['transfer-encoding'] == 'chunked') { |
There was a problem hiding this comment.
Minor nitpicking: I'd like to see strict === comparision here.
There was a problem hiding this comment.
The headers should be normalized to lowercase keys and values before checking.
$normalizedHeaders = array_change_key_case($headers, CASE_LOWER);
if (isset($normalizedHeaders['transfer-encoding']) && strtolower($normalizedHeaders['transfer-encoding']) === 'chunked') {
}There was a problem hiding this comment.
Yes that is something I've thought about. Currently thinking about making lowercase the default for the entire package and releasing that in 0.5.
There was a problem hiding this comment.
That makes sense for response headers, request headers should be used as passed in by the user (imho).
There was a problem hiding this comment.
Yes absolutely, was only talking about the response headers, sorry for the confusion.
|
@WyriHaximus Sorry, i somehow missed your comment :/ 👍 on emitting the chunked extension from the decoder. |
| @@ -81,7 +81,9 @@ protected function iterateBuffer() | |||
| $this->emit('data', array( | |||
| substr($this->buffer, 0, $chunkLength), | |||
| $this, | |||
| $this->chunkedExtension | |||
| [ | |||
| 'chunkedExtension' => $this->chunkedExtension, | |||
There was a problem hiding this comment.
It should probably be chunkExtension not chunkedExtensionas i propesed in my comment.
|
FYI I've also change the |
|
@WyriHaximus: Great 👍 LGTM now |
| use React\Stream\DuplexStreamInterface; | ||
| use React\Stream\Util; | ||
|
|
||
| class ChunkedStreamDecoder |
There was a problem hiding this comment.
This should probably implement ReadableStreamInterface? (e.g. https://github.com/clue/php-utf8-react/blob/master/src/Sequencer.php#L13)
| return; | ||
| } | ||
|
|
||
| if ($this->buffer === '') { |
There was a problem hiding this comment.
Currently PHP5 tests are failing over this. Changing === to == fixes it. Investigating why.
There was a problem hiding this comment.
Found the issue, in case substr fails it will return false. Forcing the buffer back into a sting in case that happens.
|
@clue Just looked into the twitter stream and it just sends an empty line over the connection to keep it alive: |
| $this->stream->removeListener('data', array($this, 'handleData')); | ||
| $this->stream->removeListener('end', array($this, 'handleEnd')); | ||
| $this->emit('end'); | ||
| return false; |
There was a problem hiding this comment.
@clue it made most sense to me to remove the listeners and just emit end. This lets the stream intact and to be reused, although that functionality would come with a later PR implementing keep alive and/or HTTP/2 multiplexing
There was a problem hiding this comment.
This should probably still involve a proper close event as this stream is no longer valid readable after end has been emitted.
Also, this check does not account for chunked encoding trailers (which we should discard?).
There was a problem hiding this comment.
Righto will add that: https://tools.ietf.org/html/rfc2616#section-14.40 So just $this->close(); because that will also close the underlying stream?
There was a problem hiding this comment.
Something like this should do it:
$this->emit('end');
$this->close()`;
Yes, this will close the underlying stream, but IMO this is not something we should worry about right now. Connection reuse will require more effort anyway.
(Also, RFC 2616 is deprecated, you should consult https://tools.ietf.org/html/rfc7230#section-4.1.2 and in particular the following section instead)
There was a problem hiding this comment.
Yeah that is what I have locally now 👍 . Yeah we'll look into connection reuse in a later PR.
Should we just ignore the trailers, as we're also doing that with the chunk headers?
There was a problem hiding this comment.
Normally trailers are not send by the server unless you specify "trailers" in the TE header of the request so this should be safe to just never think about them (unless i'm wrong about the spec ?)
There was a problem hiding this comment.
The thing is servers are written by developers, developers tend to have a mind of their own, developers sometimes tend to ignore specs for their own reasons or advantage. Tbh I rather not take that risk of a crashing client over something take max an hour to implements 😄 . (When working on this package it wouldn't be the first time a server just blindly sends us a gzip encoded response when we didn't specify support for it at that time.)
|
Ping @clue |
| use React\Stream\Util; | ||
| use React\Stream\WritableStreamInterface; | ||
|
|
||
| class ChunkedStreamDecoder implements ReadableStreamInterface |
There was a problem hiding this comment.
Should we mark this as @internal or should consumers of this library be allowed to rely on this?
Afaict this should only be used internally and should not be part of our public API.
|
Minor remarks only. This should be squashed to a reasonable number of commits before merging, otherwise LGTM 👍 |
|
Cheers, I'll squash them into one when I get home 👍 |
|
👍 😀 |
Encountered this while writing the twitter stream example for https://blog.wyrihaximus.net/2015/11/reactphp-http-client/. Did some research and wrote up chunked transfer coding support.
Todo: