django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #33699 -- Made ASGI request read body on-demand.

Open Flauschbaellchen opened this issue 2 years ago • 6 comments

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 avatar May 18 '22 06:05 Flauschbaellchen

This PR references the discussion on https://groups.google.com/g/django-developers/c/fu6ZSmu-YJE.

Flauschbaellchen avatar May 18 '22 06:05 Flauschbaellchen

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 ⛵️!

github-actions[bot] avatar May 18 '22 06:05 github-actions[bot]

Hi @Flauschbaellchen — thanks for this! First thing is to address the CI failures so we've got a clean slate — Looks like asgi.tests.ASGITest.test_disconnect (plus the lint errors).

carltongibson avatar May 18 '22 06:05 carltongibson

@carltongibson Can I ask you to take a look into the asgi.tests.ASGITest.test_disconnect test and help me out to fix this? As the reading is now delayed until the HTTPRequest is parsing the body, the ASGIStream and its receive() is not called currently within the test - and thus no TimeoutError is raised. I think the AsyncRequestFactory/ApplicationCommunicator needs to be adjusted, but I cannot see what I'd need to change.

Flauschbaellchen avatar May 18 '22 13:05 Flauschbaellchen

@Flauschbaellchen — OK, let me take a look. (🤹 — Might be next week.)

carltongibson avatar May 18 '22 13:05 carltongibson

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. (There's a wider need to set up such benchmarks, so this would tie into that too.)

Thanks

carltongibson avatar May 19 '22 08:05 carltongibson

Closing as per ticket.

carltongibson avatar Mar 09 '23 10:03 carltongibson