ASGI http.disconnect not handled on requests with body.

[Carlton Gibson]

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.

​https://github.com/django/django/blob/241fe59b74bb6031fa644f3ad55e6ad6a9187510/django/core/handlers/asgi.py#L189

    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. 🤔

[Mariusz Felisiak]

Thanks! Don't you think it's a bug? 🤔

[Carlton Gibson]

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 :)

[rahul negi]

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

[Carlton Gibson]

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… 🤔)

[Jeremy Lainé]

Hi Carlton, I'm not sure if the description of this bug is 100% accurate. From what I can see, http.disconnect is indeed only handled while receiving the body, so we're not going to handle this event once the (potentially empty) body is fully received. Adding "more_body": False to your proposed test makes it pass.

I had a look for how Starlette handles http.disconnect, and from what I understand the pattern seems to rely on anyio.create_taskgroup() to tear down request processing if an http.disconnect is received.

[Carlton Gibson]

With regard to detecting http.disconnect, it looks like there are two options.

First, if we gave ASGIRequest a reference to the application receive queue (via __init__) then we could add something like an is_disconnected method that could see if there was an event message pending, and if it were, whether it was a http.disconnect method. (I don't think the spec allows anything else, since the body messages have been consumed prior to instantiating the request.) Views could then call this method periodically, or before beginning an expensive process, to know to finish up early.

Second, inside handle we could launch a separate task to get on the receive queue after creating the body_file and listen for the disconnect events which we'd then use as notice to cancel the view dispatch. That would need some restructuring, since the view dispatch would need wrapping in a task (in order to have something to cancel), but I guess would be feasible. Proof-of-concept here desirable. 🤔

I think option 1 if probably simpler.

[Carlton Gibson]

​PR

[Carlton Gibson]

OK, I think this is close. Maybe extra eyes would be good on it.

With the patch we're able to handle the http.disconnnect if it comes in before the view returns.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 1d1ddffc:

Fixed #33738 -- Allowed handling ASGI http.disconnect in long-lived requests.