Re: [Django] #36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP response's body

[Django]

#36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP
response's body
--------------------------------------+------------------------------------
Reporter: Brian Atkinson | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: asgi async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by 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 [https://pypi.org/project/aiofiles/
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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP
response's body
--------------------------------------+------------------------------------
Reporter: Brian Atkinson | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: asgi async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:5>

[Django]

#36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP
response's body
--------------------------------------+------------------------------------
Reporter: Brian Atkinson | Owner: (none)

Type: Cleanup/optimization | Status: assigned

Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: asgi async | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Changes (by shin wo jin):

* has_patch: 0 => 1
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:6>

[Django]

#36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP
response's body
--------------------------------------+------------------------------------
Reporter: Brian Atkinson | Owner: (none)
Type: Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: asgi async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Changes (by Natalia Bidart):

* needs_better_patch: 0 => 1

Comment:

Setting patch needs improvement based on
[https://github.com/django/django/pull/19359#pullrequestreview-2750859057
Carlton's comment].
--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:7>

[Django]

#36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP
response's body
--------------------------------------+------------------------------------
Reporter: Brian Atkinson | Owner: (none)
Type: Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: asgi async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:8>

[Django]

#36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP
response's body
--------------------------------------+------------------------------------
Reporter: Brian Atkinson | Owner: (none)
Type: Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: asgi async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Changes (by Carlton Gibson):

* needs_better_patch: 1 => 0
* needs_tests: 0 => 1

Comment:

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:9>

[Django]

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

-------------------------------------+-------------------------------------
Reporter: Brian Atkinson | Owner: shin wo
Type: | jin

Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: asgi async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------

Changes (by shin wo jin):

* needs_better_patch: 0 => 1

* needs_tests: 1 => 0
* owner: (none) => shin wo jin

--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:10>

[Django]

#36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP
response's body
-------------------------------------+-------------------------------------
Reporter: Brian Atkinson | Owner: shin wo
Type: | jin
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: asgi async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Carlton Gibson):

* needs_better_patch: 1 => 0

Comment:

I’m unsettling Patch needs improvement, as I assume the intention of the
last edit was to mark the PR as ready for another review.
--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:11>

[Django]

#36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP
response's body
-------------------------------------+-------------------------------------
Reporter: Brian Atkinson | Owner: shin wo
Type: | jin
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:

Keywords: asgi async | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:12>

[Django]

#36281: ASGIHandler perfoms sync writes when reading the chunks of a HTTP
response's body
-------------------------------------+-------------------------------------
Reporter: Brian Atkinson | Owner: shin wo
Type: | jin

Cleanup/optimization | Status: closed

Component: HTTP handling | Version: dev

Severity: Normal | Resolution: fixed

Keywords: asgi async | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"1fb3f57e81239a75eb8f873b392e11534c041fdc" 1fb3f57]:
{{{#!CommitTicketReference repository=""
revision="1fb3f57e81239a75eb8f873b392e11534c041fdc"
Fixed #36281 -- Used async-safe write in ASGIHandler.read_body().

Thanks Carlton Gibson for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36281#comment:13>