#33699 closed New feature (wontfix)

Read ASGI request body from asyncio queue on-demand

Reported by: Noxx Owned by: noneNote
Component: HTTP handling Version: dev
Severity: Normal Keywords: ASGI, async
Cc: Andrew Godwin, Carlton Gibson Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Carlton Gibson)

Was "Performance does not scale when increasing file upload sizes"

Story

Our application supports many file uploads.
In normal circumstances, those files are only a few megabytes to of around 100MB.
However, some uploads can be several GB in size (10-15GB and up).

I know, that file uploads with such a size can be handled differently,
e.g. using a seperate endpoint, allowing slow requests to be served seperately from the application etc.
and only to reference those files in a final request.
This solution would introduce some other problems regarding house-keeping, non-atomic requests etc, thus I want to ignore this workaround for now.

Performance impact:

The impact can be best observed when uploading a file which is bigger in it's size, e.g. 1GB+.
On my maschine it takes the client around 700ms to send the request to the application, but than waits around 1.5s for the final response.
Of course, those numbers are dramatically influenced additionally in production by storage speed, file size, webserver (uvicorn/daphne/..), load-balancers etc.
But the final take here is, that the server does some additional work after the client finished its request.
In a production-like environment the numbers peaks to 4s (send request) and 16s (waiting for response) for the same file size. Uploading a 3GB file, the numbers are 11s and 44s.
As you can see, the 44s are very near the default gateway timeout of Nginx and the hard-coded one of AWS load-balancers.
As soon as the server needs more than 60s to create the response, the client will get a gateway timeout error.

Workflow of file uploads:

I'm not a Django-dev, so please correct me if I'm wrong. As far as I understand, the uploaded file is processed as the following:

Given, that the application is running within an ASGI server, the whole request is received by the ASGIHandler.
It's method #read_body creates a SpooledTemporaryFile.
This temporary file contains the whole request body, including POST parameters and files.

As soon as the request has been received (this is the point the client finished uploading the file) it is than transformed into an ASGIRequest.
The ASGIRequest has #_load_post_and_files and #parse_file_upload as methods which parses the body into seperate files.
This is done by reading the body (the temporary file) and iterating over them seperated by the POST seperator done by MultiPartParser.
The generated chunks are than sent to upload handlers which process those files further on using #receive_data_chunk.
The default upload handlers provided by Django will write those files to disc again, depending on their size.

Problem

The problem here is, that the uploaded file(s) are transformed and written as well as read multiple times.
First the whole body is written into a SpooledTemporaryFile which is re-read using streams (LazyStream) just to be written once more by an upload handler.

The impact is low if the uploaded file is small, but increases dramatically if the size is increased, the file hits the disc and/or the storage is slow.

Optimization / Brainstorming

Would it be possible to reduce the workflow to a single write call?
E.g. if the ASGIHandler already splits the request body into seperate files, it would be possible to just forward the file pointers until the upload handlers needs to be called.
Those handlers would be able to either use those files as-is or to re-read them if pre-processing is needed.

In a best-case scenario, an user uploads a file whichis created as a temporary file in parallel.
As soon as the request has been finished, the file is than moved to its final location (as already implemented by upload handlers by providing #temporary_file_path)
The server would not need any time processing the request further on and would be able to sent the response within some milliseconds independent of the file size.
The roundtrip time would be reduced by 2/3 and also the gateway timeout would be fixed.

Environment

We're using Django 4.0.4 with Gunicorn 20.1.0 and Uvicorn 0.17.6.

Attachments

I've attached two flame graphs of a file upload which hopfully illustrates this issue.
One is using the internal runserver (wsgi) and one of our (stripped) application using gunicorn+uvicorn (asgi)

Attachments (2)

flame-runserver.svg (103.5 KB ) - added by Noxx 23 months ago.
flame-app.svg.tgz (41.7 KB ) - added by Noxx 23 months ago.

Download all attachments as: .zip

Change History (8)

by Noxx, 23 months ago

Attachment: flame-runserver.svg added

by Noxx, 23 months ago

Attachment: flame-app.svg.tgz added

comment:1 by Carlton Gibson, 23 months ago

Resolution: invalid
Status: newclosed

Hi. Interesting as this is, it's not really an addressable issue report at this stage. Can I ask you to use one of the support channels please. Thanks.

Likely the Forum https://forum.djangoproject.com or (if it's a feature suggestion) the DevelopersMailingList would be the best venues for a discussion.

TicketClosingReasons/UseSupportChannels

Last edited 23 months ago by Carlton Gibson (previous) (diff)

comment:2 by Carlton Gibson, 23 months ago

Cc: Andrew Godwin Carlton Gibson added
Component: File uploads/storageHTTP handling
Description: modified (diff)
Has patch: set
Keywords: ASGI async added
Resolution: invalid
Status: closednew
Summary: Performance does not scale when increasing file upload sizesRead ASGI request body from asyncio queue on-demand
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature
Version: 4.0dev

Thanks Noxx, re-opening, and accepting for review after discussion on mailing list.

The basic idea here is to read the request body as-needed, rather than pulling it into the spooled temporary file before instantiating the ASGI request.
Perhaps there will be gotchas, but that feels like the right thing to do, so... 👍

comment:3 by Carlton Gibson, 23 months ago

Owner: changed from nobody to Noxx
Status: newassigned

comment:4 by Carlton Gibson, 23 months ago

Patch needs improvement: set

comment:5 by noneNote, 14 months ago

Owner: changed from Noxx to noneNote

comment:6 by Carlton Gibson, 14 months ago

Resolution: wontfix
Status: assignedclosed

Looking at this again, with an eye to #33738 — which is how we handle http.disconnect event as they come in — it's not clear that we can actually read the body file on demand (as nice as that might be in theory).

The ASGI spec is quite clear on the required behaviour:

  • "...the body message serves ... as a trigger to actually run request code (as you should not trigger on a connection opening alone)." (Elided to bring out the force.)
  • more_body (bool) – Signifies if there is additional content to come (as part of a Request message). If True, the consuming application should wait until it gets a chunk with this set to False. If False, the request is complete and should be processed.

i.e. The application is not permitted to begin processing the request until a more_body: False is received. You can't get that unless you read the body.

On top of that, ref #33738, if you want to look for an http.disconnect, however you might do that, you need to have got the body events, which come in on the same queue, out of the way in order to do that. A handler for a long-lived connection that wanted to check that the client hadn't disconnected before processing an expensive operation on the submitted body would be stuck.

Short of a PoC, and an unlikely change to the ASGI spec here, I think we have to close this as wontfix.

Note: See TracTickets for help on using tickets.
Back to Top