Uh oh!
There was an error while loading. Please reload this page.
Decode chunked transfer encoding for incoming requests - #106
Conversation
clue
left a comment
There was a problem hiding this comment.
Would love to get this in 👍
Can you look into the failing tests and the below remarks?
| use React\Stream\Util; | ||
| use Exception; | ||
| class ChunkedDecoder extends EventEmitter implements ReadableStreamInterface |
| // attach remote ip to the request as metadata | ||
| $request->remoteAddress = $conn->getRemoteAddress(); | ||
| $conn->removeListener('data', $listener); |
There was a problem hiding this comment.
Right. Removed this line in one of the latest
| $header = $request->getHeaders(); | ||
| $stream = $conn; | ||
| if (!empty($header['Transfer-Encoding']) && $header['Transfer-Encoding'] === 'chunked') { |
There was a problem hiding this comment.
Should take advantage of #103 to match case insensitive.
| $this->emit('request', array($request, $response)); | ||
| $request->emit('data', array($bodyBuffer)); | ||
| $conn->emit('data', array($bodyBuffer)); |
There was a problem hiding this comment.
Just a heads up: Will likely cause a merge conflict with #108.
1df0cad to
df840b1Comparelegionth
commented
Feb 10, 2017
Updated. Give me your feedback 👍 |
9324551 to
0e50877Comparelegionth
commented
Feb 11, 2017
Rebased to resolve merge conflicts. |
| private function handleChunkHeader($data) | ||
| { | ||
| $hexValue = strtok($this->buffer . $data, static::CRLF); | ||
| if ($this->isLineComplete($this->buffer . $data, $hexValue, strlen($hexValue))) { |
There was a problem hiding this comment.
Could this be simplified? How about using strpos() and substr() here? Also could probably use an early return instead.
| $chunk = substr($this->buffer . $data, 0, $this->chunkSize); | ||
| $this->actualChunksize = strlen($chunk); | ||
| if ($this->chunkSize == $this->actualChunksize) { |
| $this->chunkHeaderComplete = true; | ||
| $data = substr($this->buffer . $data, strlen($hexValue) + 2); | ||
| $this->buffer = ''; |
There was a problem hiding this comment.
This is really hard to keep track of. It looks like the buffer is modified in a couple of methods here? Can this buffering logic be simplified?
(see also above for strpos() etc.
| * @param string $chunk - chunk which will be emitted | ||
| * @return string - rest data string | ||
| */ | ||
| private function sendChunk($data, $chunk) |
| private function sendChunk($data, $chunk) | ||
| { | ||
| if ($this->chunkSize == 0 && $this->isLineComplete($this->buffer . $data, $chunk, $this->chunkSize)) { | ||
| $this->emit('end', array()); |
There was a problem hiding this comment.
Shouldn't this be followed by a close() call?
| if (! $this->closed) { | ||
| $this->emit('end'); | ||
| $this->close(); | ||
| } |
There was a problem hiding this comment.
What if there's still data in the buffer when the underlying stream emits an end event?
| public function testEnd() | ||
| { | ||
| $this->parser->on('end', $this->expectCallableOnce(array())); |
| public function testCompletlySplitted() | ||
| { | ||
| $this->parser->on('data', $this->expectCallableOnceWith('welt')); |
There was a problem hiding this comment.
This seems to indicate the whole chunk body is being buffered here? What if the chunk header says the chunk body will be several GiB?
| $this->input->removeListener('data', array($this, 'handleChunkHeader')); | ||
| $this->input->on('data', array($this, 'handleChunkData')); | ||
| if ($data !== '') { | ||
| $this->input->emit('data', array($data)); |
There was a problem hiding this comment.
IMO the decoder should have no control over emitting events on an external entity (which may have multiple listeners for this event).
legionth
commented
Feb 13, 2017
Will try a different approach. |
This ensures that only decoded body data will be emitted via the request object.
Resolves / closes#96