django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #32798 -- Fixed ASGIHandler to run response iterators in sync context

Open mbrown1413 opened this issue 2 years ago • 2 comments

This is a simplified version of PR #14526. It's slower, but simple enough that it's easy to verify.

mbrown1413 avatar Jul 16 '21 16:07 mbrown1413

Yeah, I'd rather have an explicit error here than try to patch it with a very slow solution, or at least raise a warning.

andrewgodwin avatar Oct 11 '21 15:10 andrewgodwin

Okay thanks @andrewgodwin. It's my Q4 side-goal to have a benchmark here, so let's decide on the basis of looking at that. 👍

carltongibson avatar Oct 11 '21 15:10 carltongibson

Did some testing, and found a little problem.

My use-case is to create a large zip file on the fly and send it to the client without crashing Django due to running out of memory. On WSGI this was never an issue but with ASGI, StreamingHttpResponse was not having it. This PR did fix the issue, but if the client cancels the download request, ASGI handler would crash and lock up Django entirely.

Using the other PR mentioned here, everything works as intended.

This PR is indeed slower, but uses less memory. The other PR is roughly twice faster, but consumes a lot more memory. Like 1GB more. My testcase includes sending 2.4GB of files

T-101 avatar Nov 30 '22 09:11 T-101

Hi @T-101 — thanks for having a look.

See https://code.djangoproject.com/ticket/33735#comment:8 on the related ticket-33735 for a summary of the situation. If you wanted to pick up adding __aiter__ to StreamingHttpResponse along those lines that would be amazing.

I think I'll close this PR now (and ticket-32798) to focus on ticket-33735.

carltongibson avatar Nov 30 '22 12:11 carltongibson