Fix HttpPayloadParser dealing with chunked response (#4630) (#4846)

* Parse the last CRLF of chunked response correctly (#4630)

If the last CRLF or only the LF are received via separate TCP segment,
HTTPPayloadParser misjudges that trailers should come after 0\r\n in the
chunked response body.

In this case, HttpPayloadParser starts waiting for trailers, but the only
remaining data to be received is CRLF. Thus, HttpPayloadParser waits trailers
indefinitely and this incurs TimeoutError in user code.

However, if the connection is keep alive disabled, this problem is not
reproduced because the server shutdown the connection explicitly after sending
all data. If the connection is closed .feed_eof is called and it helps
HttpPayloadParser finish its waiting.

Co-authored-by: JustAnotherArchivist <[email protected]>
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Andrew Svetlov <[email protected]>

CHANGES/4630.bugfix

@@ -0,0 +1 @@
+Handle the last CRLF correctly even if it is received via separate TCP segment.

CONTRIBUTORS.txt

@@ -159,6 +159,7 @@ Julia Tsemusheva
 Julien Duponchelle
 Jungkook Park
 Junjie Tao
+Junyeong Jeong
 Justas Trimailovas
 Justin Foo
 Justin Turner Arthur

aiohttp/http_parser.py

@@ -668,12 +668,23 @@ def feed_data(self,
                 # we should get another \r\n otherwise
                 # trailers needs to be skiped until \r\n\r\n
                 if self._chunk == ChunkState.PARSE_MAYBE_TRAILERS:
-                    if chunk[:2] == SEP:
+                    head = chunk[:2]
+                    if head == SEP:
                         # end of stream
                         self.payload.feed_eof()
                         return True, chunk[2:]
-                    else:
-                        self._chunk = ChunkState.PARSE_TRAILERS
+                    # Both CR and LF, or only LF may not be received yet. It is
+                    # expected that CRLF or LF will be shown at the very first
+                    # byte next time, otherwise trailers should come. The last
+                    # CRLF which marks the end of response might not be
+                    # contained in the same TCP segment which delivered the
+                    # size indicator.
+                    if not head:
+                        return False, b''
+                    if head == SEP[:1]:
+                        self._chunk_tail = head
+                        return False, b''
+                    self._chunk = ChunkState.PARSE_TRAILERS
 
                 # read and discard trailer up to the CRLF terminator
                 if self._chunk == ChunkState.PARSE_TRAILERS:

tests/test_http_parser.py

@@ -826,9 +826,6 @@ async def test_parse_chunked_payload_size_error(self, stream) -> None:
         assert isinstance(out.exception(),
                           http_exceptions.TransferEncodingError)
 
-    @pytest.mark.xfail(
-        reason="see https://github.com/aio-libs/aiohttp/issues/4630"
-    )
     async def test_parse_chunked_payload_split_end(self, protocol) -> None:
         out = aiohttp.StreamReader(protocol, loop=None)
         p = HttpPayloadParser(out, chunked=True)
@@ -838,9 +835,6 @@ async def test_parse_chunked_payload_split_end(self, protocol) -> None:
         assert out.is_eof()
         assert b'asdf' == b''.join(out._buffer)
 
-    @pytest.mark.xfail(
-        reason="see https://github.com/aio-libs/aiohttp/issues/4630"
-    )
     async def test_parse_chunked_payload_split_end2(self, protocol) -> None:
         out = aiohttp.StreamReader(protocol, loop=None)
         p = HttpPayloadParser(out, chunked=True)
@@ -861,9 +855,6 @@ async def test_parse_chunked_payload_split_end_trailers(self,
         assert out.is_eof()
         assert b'asdf' == b''.join(out._buffer)
 
-    @pytest.mark.xfail(
-        reason="see https://github.com/aio-libs/aiohttp/issues/4630"
-    )
     async def test_parse_chunked_payload_split_end_trailers2(self,
                                                              protocol) -> None:
         out = aiohttp.StreamReader(protocol, loop=None)
@@ -875,6 +866,27 @@ async def test_parse_chunked_payload_split_end_trailers2(self,
         assert out.is_eof()
         assert b'asdf' == b''.join(out._buffer)
 
+    async def test_parse_chunked_payload_split_end_trailers3(self,
+                                                             protocol) -> None:
+        out = aiohttp.StreamReader(protocol, loop=None)
+        p = HttpPayloadParser(out, chunked=True)
+        p.feed_data(b'4\r\nasdf\r\n0\r\nContent-MD5: ')
+        p.feed_data(b'912ec803b2ce49e4a541068d495ab570\r\n\r\n')
+
+        assert out.is_eof()
+        assert b'asdf' == b''.join(out._buffer)
+
+    async def test_parse_chunked_payload_split_end_trailers4(self,
+                                                             protocol) -> None:
+        out = aiohttp.StreamReader(protocol, loop=None)
+        p = HttpPayloadParser(out, chunked=True)
+        p.feed_data(b'4\r\nasdf\r\n0\r\n'
+                    b'C')
+        p.feed_data(b'ontent-MD5: 912ec803b2ce49e4a541068d495ab570\r\n\r\n')
+
+        assert out.is_eof()
+        assert b'asdf' == b''.join(out._buffer)
+
     async def test_http_payload_parser_length(self, stream) -> None:
         out = aiohttp.FlowControlDataQueue(stream,
                                            loop=asyncio.get_event_loop())