Manually reset the buffer when streaming request body

[ahopkins]

Resolves #2182

[codeclimate]

Code Climate has analyzed commit baf4b26 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 0.0% (86% is the threshold).

This pull request will bring the total coverage in the repository to 94.2% (0.0% change).

View more on Code Climate.

[Tronic]

@ahopkins I think this approach is flawed and you may end up corrupting the next request, if a part of it was received already. Also, I would expect problems if the actual final newline was not yet received but arrives a bit later.

At the end of the request specifically that \r\n needs to be consumed, nothing more. And if it doesn't exist in the buffer, we need to receive_more() prior to returning to handling other requests. I cannot look more closer into it right now but maybe in a few days if you don't get it sorted out by then.

[ahopkins]

@Tronic

Hmm... ok. Then need to come up with another way to contend with this so that the http1 loop stops here after completing the first request.

            # Wait for next request
            if not self.recv_buffer:
                await self._receive_more()

I'm not entirely convinced that is correct though. In my testing, that last CRLF never was consumed. The response was sent received exactly as it should be, and all trailing linebreaks of the request were properly accounted for in the received body.

I will play around with it again some more tonight, but I did spend a decent amount of time and tried with a bunch of use cases, as well as saw no problems in the test suite.

[ahopkins]

Just did a quick test, and confirmed this also seems acceptable:

            if not self.recv_buffer or self.recv_buffer == bytearray(b"\r\n"):
                await self._receive_more()

---

But, it should be noted that the recv_buffer still contains that CRLF at the conclusion of the task.

[Tronic]

Where does that newline then get consumed? Is the request receiving loop totally OK with it?

[ahopkins]

Where does that newline then get consumed?

It doesn't. After the response is delivered and we are closing out the task, it is still there. I am sure there must have been a reason for it, you clearly added it intentionally:

            if headers.getone("transfer-encoding", None) == "chunked":
                self.request_body = "chunked"
                pos -= 2  # One CRLF stays in buffer

Is the request receiving loop totally OK with it?

Yes. Although, I have a problem when two streaming requests are on the same connection. But, even with no changes the second request fails because it starts with a new line:

Traceback (most recent call last):
  File "/home/adam/Projects/Sanic/sanic/sanic/http.py", line 126, in http1
    await self.http1_request_header()
  File "/home/adam/Projects/Sanic/sanic/sanic/http.py", line 221, in http1_request_header
    raise InvalidUsage("Bad Request")
sanic.exceptions.InvalidUsage: Bad Request

bytearray(b'\r\nPOST /upload HTTP/1.1\r\nhost: localhost:9999\r\naccept: */*\r\naccept-encoding: gzip, deflate\r\nconnection: keep-alive\r\nuser-agent: python-httpx/0.15.4\r\ntransfer-encoding: chunked\r\n\r\n')

[Tronic]

While the request body is received, one extra CRLF is supposed to stay in the buffer at all times, and to provide the first one, I leave the one that follows the request header there. This is to make receiving faster because the newline that follows chunk's data is only parsed together with the next chunk header.

But once the final block is received, CRLF-0-CRLF-CRLF (newline after prior data or header, literal 0 means end, newline after the zero, and the final newline after the non-existing data) should be consumed so that nothing is left in the buffer. I believe that my code has a bug on that, and instead should consume that whole thing prior to returning from handling a request. Then the next pipelined request begins with the request string, no newlines in buffer.

If a request is not in chunked mode, no newline is ever left in the buffer.

[ahopkins]

If a request is not in chunked mode, no newline is ever left in the buffer.

Yes, this is correct. Pipelining works as expected here.

[ahopkins]

Moving the conversation to #2033