falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.51k stars 937 forks source link

ASGI: iterating over `req.stream` hangs for chunked requests #2024

Closed vytas7 closed 2 years ago

vytas7 commented 2 years ago

When reading a streaming request without Content-Length (i.e., using "chunked" Transfer-Encoding), iterating over req.stream hangs in the case the request payload consists of more than one chunk.

It looks like this is caused by the receive loop implementation in asgi/stream.py. It only checks for the number of remaining bytes, and disregards the more_body: False hint in the case of an empty body. The logic to account for it is in place, but it is not reached due to the continue clause for an empty body chunk in the following event sequence:

RECV {'type': 'http.request', 'body': b'123456789abcdef\n', 'more_body': True}

RECV {'type': 'http.request', 'body': b'123456789abcdef\n', 'more_body': True}

RECV {'type': 'http.request', 'body': b'', 'more_body': False}

Eventually, the client (I was using httpx) times out, and an http.disconnect event is received which is handled correctly. But the request has already failed (timeout) from the client's perspective at this point.

vytas7 commented 2 years ago

The following patch fixes the case in point, but I'll need to do more testing and add regression tests:

--- a/falcon/asgi/stream.py
+++ b/falcon/asgi/stream.py
@@ -434,22 +434,20 @@ class BoundedStream:
                 pass
             else:
                 # NOTE(kgriffs): No need to yield empty body chunks.
-                if not next_chunk:
-                    continue
-
-                next_chunk_len = len(next_chunk)
-
-                if next_chunk_len <= self._bytes_remaining:
-                    self._bytes_remaining -= next_chunk_len
-                    self._pos += next_chunk_len
-                else:
-                    # NOTE(kgriffs): We received more data than expected,
-                    #   so truncate to the expected length.
-                    next_chunk = next_chunk[: self._bytes_remaining]
-                    self._pos += self._bytes_remaining
-                    self._bytes_remaining = 0
-
-                yield next_chunk
+                if next_chunk:
+                    next_chunk_len = len(next_chunk)
+
+                    if next_chunk_len <= self._bytes_remaining:
+                        self._bytes_remaining -= next_chunk_len
+                        self._pos += next_chunk_len
+                    else:
+                        # NOTE(kgriffs): We received more data than expected,
+                        #   so truncate to the expected length.
+                        next_chunk = next_chunk[: self._bytes_remaining]
+                        self._pos += self._bytes_remaining
+                        self._bytes_remaining = 0
+
+                    yield next_chunk

             # NOTE(kgriffs): Per the ASGI spec, more_body is optional
             #   and should be considered False if not present.