Hi Carlton, thanks for your response. I've opened up a PR: https://github.com/django/django/pull/15704
I've decided to moved the "ASGIStream" wrapper code into the Django code and get rid of the dependency as it is only a small part of it. My version of the code was nearly the same as the one from a2wsgi so I used the latter one (I think that their version is a bit more battle-proofed) and added the http.disconnect exception which was missing. However, I do not know what would be an appropriate way forward: Leave it as it is, add a "thank you" comment that the code was more-or-less taken from/based on a2wsgi and/or that improvements to ASGIStream should be checked if they also apply to a2wsgi? However, we can further discuss the changes within the PR if you want. Kind regards, Noxx On Tuesday, May 17, 2022 at 7:20:18 PM UTC+2 carlton...@gmail.com wrote: > Hi Noxx. > > This is interesting, thanks for the write-up. > > As a bit of history, initially ASGIRequests would read the whole body into > memory, which was fine for an initial version, but led quickly enough to > resource concerns. Andrew followed up with the SpooledTemporaryFile > approach, as a nice next-step improvement on that. There **was** at the > time some initial work on wrapping the receive queue to consume the body > on-demand, but with the temp-file solution in hand that never got pushed > through. I can very well imagine folks in ASGI-land have had time to get > that right now. (So exciting to look at!) > > I'd very much want to dig into a dependency before deciding to take one > on. I suspect it's not **that** complex, so it's likely something we'd want > to re-implement to fit our needs exactly. (But we can see on review...) > > Please do open a PR when you're ready. > > Kind regards, > Carlton > > On Tue, 17 May 2022 at 15:20, 'Noxx' via Django developers (Contributions > to Django itself) <django-d...@googlegroups.com> wrote: > >> Hi all, >> >> I've played around a bit and would like to give you a heads up: >> >> WSGI and ASGI have only differences in how they parse incoming requests. >> As WSGI is synchronous the body is already provided as a ByteIO stream >> which is set directly on the HTTPRequest (wrapped as a LazyStream). >> >> On ASGI the body is sent in chunks. As the base of HTTPRequest - and this >> includes how to parse the body, POST, FILES etc. - is the same for ASGI and >> WSGI, those chunks need to be joined together prior to any other processing. >> to do this, all chunks are currently written into the >> SpooledTemporaryFile mentioned in my previous post and this file is then >> provided as a stream to the HTTPRequest. >> As already written, this workaround to get all chunks into one stream is >> very slow for requests which are bigger in their size as the temporary file >> is spooled to disc in the worst case. >> >> To workaround this issue, I've tried to delay the reading of the ASGI >> queue to get the next chunk (called "message" in ASGI) until the >> HTTPRequest really needs it. >> As the HTTPRequest/MultiFileParser works on a stream to read and parse >> its body, the idea is to use the ASGI queue as a stream-like object and to >> directly read it. >> This would eliminate the need to create a temporary file as a "proxy". >> >> When I was in the process of implementing this interface, I've recognized >> that the package a2wsgi already handles such use-case: >> https://github.com/abersheeran/a2wsgi/blob/master/a2wsgi/wsgi.py >> >> I've added a2wsgi as a dependency and change the ASGI handler a little >> bit. >> The changes can be reviewed on my fork: >> https://github.com/Flauschbaellchen/django/commit/006bf2d5c0b82092524ee9b65ef919637165ee01 >> >> Timings: >> In my test environment, the timing went from 4s (sending)/16s (waiting) >> to 6s/5s. >> Sending is a bit slower. This is because prior to the change the chunks >> have been read directly into the temporary file and now the parser is doing >> its business between reading new chunks. >> However, those chunks are directly parsed and feed into the upload >> handlers which dramatically improves the performance on the server side. >> Using gunicorn+uvicorn the waiting time went down to ~600ms. I don't know >> why runserver/daphne is so slow. I'll check this within the next days. >> >> I've attached a new flame graph representing my changes. >> The right-most block can be ignored - this is already the next GET >> request of the admin panel after saving the object. >> >> What's next? >> I would appreciate any feedback on my changes. >> Is this the right direction? >> What about the newly introduced dependency? - It would also be possible >> to implement/copy the code on the Django side to eliminate this dependency. >> However, on my opinion, doing things twice is never a good idea... >> Can anyone test against other environments/ASGI servers? Maybe I've >> overlooked some edge cases. >> >> The current changes depends on the version 4.0.4 of Django as on the main >> branch small changes happend to the ASGI handler. >> I'll rebase my changes prior to submitting a PR. >> >> With kind regards :) >> >> On Wednesday, May 11, 2022 at 7:47:10 PM UTC+2 Adam Johnson wrote: >> >>> Ferran - No, this is the right place. Noxx is suggesting a change to >>> Django. >>> >>> Noxx - it seems reasonable to improve Django here. You mentioned ASGI - >>> did you check what the WSGI paths do? It may be that the newer ASGI code is >>> simply less optimal. >>> >>> Would it be possible to reduce the workflow to a single write call? >>>> >>> >>> This is indeed the question. Could you investigate further and perhaps >>> come up with a patch? You're likely "the world expert" on this part of >>> Django at this point, after all the profiling you've done. >>> >>> >>> On Wed, May 11, 2022 at 5:44 PM Ferran Jovell <fer...@jovell.dev> wrote: >>> >>>> This is still the wrong place. Try django-users mailing list. >>>> >>>> El dc., 11 de maig 2022, 17:53, 'Noxx' via Django developers >>>> (Contributions to Django itself) <django-d...@googlegroups.com> va >>>> escriure: >>>> >>>>> 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: >>>>> <https://code.djangoproject.com/ticket/33699#Performanceimpact:> >>>>> >>>>> 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: >>>>> <https://code.djangoproject.com/ticket/33699#Workflowoffileuploads:> >>>>> >>>>> 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 <https://code.djangoproject.com/ticket/33699#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 >>>>> <https://code.djangoproject.com/ticket/33699#OptimizationBrainstorming> >>>>> >>>>> 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 <https://code.djangoproject.com/ticket/33699#Environment> >>>>> >>>>> We're using Django 4.0.4 with Gunicorn 20.1.0 and Uvicorn 0.17.6. >>>>> Attachments <https://code.djangoproject.com/ticket/33699#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) >>>>> >>>>> Final notes >>>>> >>>>> I'd by happy to get some thoughts and opinions on this issue and if >>>>> this is even possible to change/implement. >>>>> >>>>> Cross-posted from https://code.djangoproject.com/ticket/33699 as >>>>> ticket has been closed due to wrong place... >>>>> >>>>> -- >>>>> You received this message because you are subscribed to the Google >>>>> Groups "Django developers (Contributions to Django itself)" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an email to django-develop...@googlegroups.com. >>>>> To view this discussion on the web visit >>>>> https://groups.google.com/d/msgid/django-developers/00b47caf-2810-487f-86c5-dcf52426299an%40googlegroups.com >>>>> >>>>> <https://groups.google.com/d/msgid/django-developers/00b47caf-2810-487f-86c5-dcf52426299an%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>> . >>>>> >>>> -- >>>> You received this message because you are subscribed to the Google >>>> Groups "Django developers (Contributions to Django itself)" group. >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to django-develop...@googlegroups.com. >>>> >>> To view this discussion on the web visit >>>> https://groups.google.com/d/msgid/django-developers/CAEKh-snKLv4sd_QuoAcY7F4oo6GFM5MwMndJrZ-nz1VkXgqG%3Dg%40mail.gmail.com >>>> >>>> <https://groups.google.com/d/msgid/django-developers/CAEKh-snKLv4sd_QuoAcY7F4oo6GFM5MwMndJrZ-nz1VkXgqG%3Dg%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>> . >>>> >>> -- >> You received this message because you are subscribed to the Google Groups >> "Django developers (Contributions to Django itself)" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to django-develop...@googlegroups.com. >> > To view this discussion on the web visit >> https://groups.google.com/d/msgid/django-developers/00293396-fa86-4b40-b88e-3c1ffb990df4n%40googlegroups.com >> >> <https://groups.google.com/d/msgid/django-developers/00293396-fa86-4b40-b88e-3c1ffb990df4n%40googlegroups.com?utm_medium=email&utm_source=footer> >> . >> > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/dd91f8bf-972d-4e0b-865a-864193d924b4n%40googlegroups.com.
