Fixed #33699 -- Made ASGI request read body on-demand. #15704
Conversation
|
This PR references the discussion on https://groups.google.com/g/django-developers/c/fu6ZSmu-YJE. |
|
Hello @Flauschbaellchen! Thank you for your contribution 💪 As it's your first contribution be sure to check out the patch review checklist. If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket! If you have any design or process questions then you can ask in the Django forum. Welcome aboard ⛵️! |
|
Hi @Flauschbaellchen — thanks for this! First thing is to address the CI failures so we've got a clean slate — Looks like |
carltongibson
changed the title
carltongibson
changed the title
Prior to this change, the request has been written into a spooled temporary file as the HTTPRequest class depends on a byte/io-like stream to process and parse its content as a whole. As the ASGI request is given in seperate chunks, those chunks has been concatenated first using this file. However, doing so resulted in an increasing latency on the server side, as the file needed to be always written and re-read to be parsed afterwards. This was especially bad for big file uploads resulting in (Gateway) Timeouts if the Django server was running behind a reverse-proxy. This change fixes this issue by wrapping the ASGI request and providing a byte/io-like stream interface to it. Doing so, reading the request's content is delayed until it needs to be parsed and than directly delivered into the upload-handlers, resulting in a reduced latency.
Flauschbaellchen
force-pushed
the
asgi-request-body-streaming
branch
from
74b99e7
to
2ef8e76
Compare
|
@carltongibson Can I ask you to take a look into the |
|
@Flauschbaellchen — OK, let me take a look. (🤹 — Might be next week.) |
|
Hey @Flauschbaellchen — I'm picking this up now. Can I ask, could you put together a minimal app showing the upload/download example you raised in the discussion? You're creating a 1GB file; uploading; waiting for what? The same file sent back? etc. — If you could put together the minimal Django app for that, we can run it with gunicorn/uvicorn/Daphne/etc. and see the differences. Thanks |
There was a problem hiding this comment.
Can I ask you to take a look into the asgi.tests.ASGITest.test_disconnect test and help me out to fix this?
The issue is the http.disconnect handling in _receive_more_data() is not triggered until the the input queue is read from (at least once). We'll need to consider how to handle this — maybe some kind of lookahead... 🤔 — but I need to ponder more.
| message = async_to_sync(self.receive)() | ||
| if message["type"] == "http.disconnect": | ||
| # Early client disconnect. | ||
| self._has_more = False # safeguard against trying to call receive again | ||
| raise RequestAborted() |
There was a problem hiding this comment.
This isn't triggered until the input queue is read at least once.
There was a problem hiding this comment.
As this is the intended behavior, maybe a better approach would be to test ASGIStream separately for this test case as a model test instead of the current integration test?
There was a problem hiding this comment.
Yes, happy to look at a refactor.
But that integration test should pass no? (I do expect a timeout after a disconnect...?)
There was a problem hiding this comment.
We're not currently handling this correctly I think. Follow up at https://code.djangoproject.com/ticket/33738
|
Closing as per ticket. |
Prior to this change, the request has been written into a spooled temporary file
as the HTTPRequest class depends on a byte/io-like stream to process and
parse its content as a whole.
As the ASGI request is given in seperate chunks,
those chunks has been concatenated first using this file.
However, doing so resulted in an increasing latency on the server side,
as the file needed to be always written and re-read to be parsed afterwards.
This was especially bad for big file uploads resulting in (Gateway)
Timeouts if the Django server was running behind a reverse-proxy.
This change fixes this issue by wrapping the ASGI request and providing
a byte/io-like stream interface to it.
Doing so, reading the request's content is delayed until it needs to be
parsed and than directly delivered into the upload-handlers, resulting
in a reduced latency.