[Django] #33738: ASGI http.disconnect not handled on requests with body.
Django
------------------------------------------+------------------------
Reporter: Carlton Gibson | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: 4.0
Severity: Normal | Keywords: ASGI
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
Noticed whilst reviewing [https://github.com/django/django/pull/15704 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
[https://github.com/django/channels/blob/ae60a7d86f3655a1cc35cd9198e61fe5dcc5d1a1/channels/utils.py#L32
`await_many_dispatch()`] to keep receiving from the input queue whilst
dispatching the request. 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/33738>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Carlton Gibson | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: 4.0
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Unreviewed => Accepted
Comment:
Thanks! Don't you think it's a bug? 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:1>
Django
--------------------------------+------------------------------------
Reporter: Carlton Gibson | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Comment (by 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 :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:2>
Django
--------------------------------+------------------------------------
Reporter: Carlton Gibson | Owner: nobody
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
* type: New feature => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:3>
Django
--------------------------------+------------------------------------
Reporter: Carlton Gibson | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Comment (by rahul negi):
Replying to [ticket:33738 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
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:4>
Django
--------------------------------+------------------------------------
Reporter: Carlton Gibson | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Comment (by 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
[https://github.com/django/channels/blob/ae60a7d86f3655a1cc35cd9198e61fe5dcc5d1a1/channels/consumer.py#L56-L62
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… 🤔)
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:5>
Django
--------------------------------+------------------------------------
Reporter: Carlton Gibson | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:6>
Django
Reporter: Carlton Gibson | Owner: Dennis
| Chukwunta
Type: Bug | Status: assigned
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dennis Chukwunta):
* owner: nobody => Dennis Chukwunta
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Dennis
| Chukwunta
Type: Bug | Status: assigned
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Dennis
| Chukwunta
Type: Bug | Status: assigned
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/16603 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Dennis
| Chukwunta
Type: Bug | Status: assigned
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* needs_better_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Dennis
| Chukwunta
Type: Bug | Status: assigned
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Dennis
| Chukwunta
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* type: Bug => New feature
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Dennis
| Chukwunta
Component: HTTP handling | Version: 4.0
Keywords: ASGI | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1d1ddffc27cd55c011298cd09bfa4de3fa73cf7a" 1d1ddffc]:
{{{
#!CommitTicketReference repository=""
revision="1d1ddffc27cd55c011298cd09bfa4de3fa73cf7a"
Fixed #33738 -- Allowed handling ASGI http.disconnect in long-lived
requests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33738#comment:13>