ASGIHandler perfoms sync writes when reading the chunks of a HTTP response's body

[Brian Atkinson]

See ​https://github.com/django/django/blob/c1a4fccf53437e480c42648f67277fb7e63ed4ab/django/core/handlers/asgi.py#L266

That write may end up resulting in IO actually hitting disk. This could result in the async loop stalling if the underlying write call is slow.

[Rayyan Khan]

I think you're right, the sync write in this async code path might cause problems if the disk I/O is slow. Maybe we could wrap the body_file.write call with sync_to_async to prevent blocking the event loop?

[Natalia Bidart]

Thank you Brian for the report! I agree that this is worth addressing, as disk IO can indeed lead to stalls.

Careful consideration will be needed for how to handle the chunked file writing without blocking the event loop. I'm wondering if we should have a sort of abstraction for the chunked file writer and allow people to override. Depending on the project's constraints or current dependencies, some projects may choose to use a 3rd party library like ​asynctempfile (unclear whether this is currently maintained), or Django channels, or something else?

We're open to a patch that addresses this while ensuring compatibility and avoiding unintended regressions. Would you like to work on this?

(I'm torn whether to categorize this as Cleanup or New Feature, for now I'm going with the former but we can adjust later on.)

[Carlton Gibson]

That's a good one. Yes. A consideration for being careful about allowed body sizes under ASGI. It'd be interesting to see what other ASGI frameworks do here.

…use a 3rd party library like ​asynctempfile (unclear whether this is currently maintained)…

It looks small and depends on ​aiofiles which is a package I'd trust. It could likely be vendored if needed, perhaps using the async version if aiofiles was available.

(I'd have to think through the severity of the blocking I/O to say whether using such would be recommended or even required. We already mention aiofiles in the docs, e.g. FileResponses.)

[shin wo jin]

I've created a pull request to resolve this ticket

pr : ​https://github.com/django/django/pull/19359

Please let me know if there's anything further needed or if additional changes are required. Thank you!

[Natalia Bidart]

Setting patch needs improvement based on ​Carlton's comment.

[shin wo jin]

Thanks for the feedback!

I've updated the patch based on Carlton’s suggestion — now using getattr for the rollover check, with conditional branching and relevant tests.

[Carlton Gibson]

I think the patch here is looking good, but it needs a test that demonstrates the fix. (There are tests added but they don't fail without the patch.) I've left comments to that effect on the PR.
With that it should be close.

[Carlton Gibson]

I’m unsetting Patch needs improvement, as I assume the intention of the last edit was to mark the PR as ready for another review.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 1fb3f57:

Fixed #36281 -- Used async-safe write in ASGIHandler.read_body().

Thanks Carlton Gibson for reviews.