Opened 3 months ago
Last modified 7 weeks ago
#33738 new Bug
ASGI http.disconnect not handled on requests with body.
Reported by: | Carlton Gibson | Owned by: | nobody |
---|---|---|---|
Component: | HTTP handling | Version: | 4.0 |
Severity: | Normal | Keywords: | ASGI |
Cc: | Noxx, Andrew Godwin | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Noticed whilst reviewing PR 15704 for #33699, we're not handling the ASGI http.disconnect
message correctly. Since it's only dealt with whilst reading the request body, http.disconnect
is not processed on a request that includes a body.
async def read_body(self, receive): """Reads an HTTP body from an ASGI connection.""" # Use the tempfile that auto rolls-over to a disk file as it fills up. body_file = tempfile.SpooledTemporaryFile( max_size=settings.FILE_UPLOAD_MAX_MEMORY_SIZE, mode="w+b" ) while True: message = await receive() if message["type"] == "http.disconnect": # This is the only place `http.disconnect` is handled. body_file.close() # Early client disconnect. raise RequestAborted() # Add a body chunk from the message, if provided. if "body" in message: body_file.write(message["body"]) # Quit out if that's the end. if not message.get("more_body", False): break body_file.seek(0) return body_file
http.disconnect
is designed for long-polling — so we imagine a client opening a request, with a request body, and then disconnecting before the response is generated.
The protocol server (Daphne/uvicorn/...) will send the http.diconnect
message, but it's not handled.
This test fails on main (at 9f5548952906c6ea97200c016734b4f519520a64 — 4.2 pre-alpha)
diff --git a/tests/asgi/tests.py b/tests/asgi/tests.py index ef7b55724e..a68ca8a473 100644 --- a/tests/asgi/tests.py +++ b/tests/asgi/tests.py @@ -188,6 +188,18 @@ class ASGITest(SimpleTestCase): with self.assertRaises(asyncio.TimeoutError): await communicator.receive_output() + async def test_disconnect_with_body(self): + application = get_asgi_application() + scope = self.async_request_factory._base_scope(path="/") + communicator = ApplicationCommunicator(application, scope) + await communicator.send_input({ + "type": "http.request", + "body": b"some body", + }) + await communicator.send_input({"type": "http.disconnect"}) + with self.assertRaises(asyncio.TimeoutError): + await communicator.receive_output() + async def test_wrong_connection_type(self): application = get_asgi_application() scope = self.async_request_factory._base_scope(path="/", type="other")
To handle this correctly it looks like we'd need something like Channel's `await_many_dispatch()` to keep receiving from the input queue whilst dispatching the request. 🤔
Change History (5)
comment:1 Changed 3 months ago by
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 Changed 3 months ago by
Don't you think it's a bug? 🤔
I had it down as such, but it's never worked, and once I started thinking about the fix I thought it's probably a non-minor adjustment so... (But happy if you want to re-classify :)
comment:3 Changed 3 months ago by
Type: | New feature → Bug |
---|
comment:4 Changed 7 weeks ago by
Replying to Carlton Gibson:
async def read_body(self, receive): """Reads an HTTP body from an ASGI connection.""" # Use the tempfile that auto rolls-over to a disk file as it fills up. body_file = tempfile.SpooledTemporaryFile( max_size=settings.FILE_UPLOAD_MAX_MEMORY_SIZE, mode="w+b" ) while True: message = await receive() if message["type"] == "http.disconnect": # This is the only place `http.disconnect` is handled. body_file.close() # Early client disconnect. raise RequestAborted() # Add a body chunk from the message, if provided. if "body" in message: body_file.write(message["body"]) # Quit out if that's the end. if not message.get("more_body", False): break body_file.seek(0) return body_file
I'm following this ticket for some days and trying to understand the code base learned more about the asynchronous processes and long-polling,
IDK this is a solution or not,
if we perform a loop only while receiving the message and break it after X interval of time like
while True: message = await receive() message_queue.append(message) if time_waited == X : break
and do store it in a queue and then perform a loop on the queue data to read the message?
Please correct me if I'm wrong or if this sounds immature,
thanks
comment:5 Changed 7 weeks ago by
Hi Rahul.
... if we perform a loop only while receiving the message and break it after X interval of time...
This won't work I'm afraid. We still need to be able to handle the incoming http.disconnect
at any time.
It's not 100% straightforward but something like Channels' await_many_dispatch()
will be needed. See the usage here: it's able to route multiple messages.
(I don't think the example there is 100% properly handling the disconnect, as a kind of interrupt there either, since the dispatch task will only process one message at a time, where we're looking to handle a disconnect whilst processing a long-running request… 🤔)
Thanks! Don't you think it's a bug? 🤔