Re: [Django] #33755: Move ASGI body-file cleanup into ASGIRequest
Django
--------------------------------------+------------------------------------
Reporter: Carlton Gibson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by Mariusz Felisiak):
* cc: Jonas Lundberg (added)
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jonas Lundberg):
* owner: nobody => Jonas Lundberg
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jonas Lundberg):
Looked at this briefly and unfortunately the {{{HttpRequest.body}}}
property drops the reference to the {{{body_file}}} by overwriting it with
a {{{BytesIO}}}.
{{{
@property
def body(self):
if ...
self._stream = BytesIO(self._body)
return self._body
}}}
I'm thinking it could be safe to close the file in a finally block once it
have been read ...
{{{
diff --git a/django/http/request.py b/django/http/request.py
index 4b160bc..5e5f349 100644
--- a/django/http/request.py
+++ b/django/http/request.py
@@ -400,14 +400,16 @@ class HttpRequest:
def read(self, *args, **kwargs):
self._read_started = True
try:
return self._stream.read(*args, **kwargs)
except OSError as e:
raise UnreadablePostError(*e.args) from e
+ finally:
+ self._stream.close()
}}}
At least the test from #33754 still succeeds ;-)
Thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jonas Lundberg):
Hmm, only closing the {{{body_file}}} once read would leave it unclosed if
the body wasn't touched.
Maybe we also need to implement your suggested {{{__del__}} to handle that
case as well, closing either the {{{body_file}}} or the {{{BytesIO}}} for
read body.
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: ASGI | 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):
Hey Jonas, thanks for looking so quickly!
> Thoughts?
Initially it would be to sketch out the tests for the cases we need to
cover... β then is making them pass feasible?
> ...property drops the reference to the `body_file` by overwriting it
with a `BytesIO`
Grrr. Yes... Is the file closed when references hit zero? And can we `del`
the higher `handle()` to make sure it's not retained there? π€ βΒ Need to
have a look. Otherwise, yes, explicitly closing would be needed.
It could well be that combinations of `stream`/`read`/`body` usage mean we
have to say `wontfix` β but it'd be nice to be clear on that if we do have
to.
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: ASGI | 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):
Hi Jonas.
I was digging a bit more here:
> ...unfortunately the HttpRequest.body property drops the reference to
the body_file by overwriting it with a BytesIO.
Not closing `self._stream` looks like we should be able to fix in general
(i.e. it's not ASGI-specific π€)
This passes:
{{{
diff --git a/django/http/request.py b/django/http/request.py
index 4b160bc5f4..8354dfe57b 100644
--- a/django/http/request.py
+++ b/django/http/request.py
@@ -340,6 +340,11 @@ class HttpRequest:
self._body = self.read()
except OSError as e:
raise UnreadablePostError(*e.args) from e
+ finally:
+ try:
+ self._stream.close()
+ except:
+ pass
self._stream = BytesIO(self._body)
return self._body
}}}
This in `body`, since `read` can be used passing a chunk size, and in the
`try...except` because WSGIHandler uses a `LimitedStream` wrapper that
doesn't (currently) have a `close`.
Then, adding a simple `close()` to `ASGIRequest`...
{{{
def close(self):
super().close()
self._stream.close()
}}}
... and blocking out the `body_file.close()` in `handle()` clears up all
the ResourceWarnings except two:
* `test_static_file_response` β This because the `ASGIStaticFilesHandler`
wraps `ASGIHandler` so (likely) isn't calling `response.close` correctly.
(See also #33048, #27325)
* `test_non_unicode_query_string` β We're not then handling the `close()`
when there's a Unicode error in `create_request()`.
Both of those should be addressable though.
What do you think?
Do you fancy continuing on this?
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jonas Lundberg):
This in body, since read can be used passing a chunk size ...
Carlton, this is exactly my findings and thoughts as well :-D ... though
I'm leaning towards adding a noop {{{LimitedStream.close()}}} instead of
the {{{try...except}}} block.
Then, adding a simple close() to ASGIRequest...
Agree, this is probably enough instead of {{{__del__}}}, i.e. untouched
{{{request.body}}} will be closed and cleaned up by the response resource
closers.
I will open a PR shortly.
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/15756
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: ASGI | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Super, thanks Jonas. I will pick up for review next week. π
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Component: HTTP handling | Version: dev
Keywords: ASGI | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"e96320c91724830034033a9cb8afd9cf8c11e2fd" e96320c]:
{{{
#!CommitTicketReference repository=""
revision="e96320c91724830034033a9cb8afd9cf8c11e2fd"
Fixed #33755 -- Moved ASGI body-file cleanup into request class.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: closed
Component: HTTP handling | Version: dev
Severity: Normal | Resolution: fixed
Keywords: ASGI | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson <carlton@β¦>):
In [changeset:"f476c8847a0bf1a4e20becfb3dc66f4da0dbf579" f476c884]:
{{{
#!CommitTicketReference repository=""
revision="f476c8847a0bf1a4e20becfb3dc66f4da0dbf579"
Refs #33173, Refs #33755 -- Fixed ResourceWarning from unclosed files in
ASGI tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Carlton Gibson | Owner: Jonas
Type: | Lundberg
Cleanup/optimization | Status: closed
Component: HTTP handling | Version: dev
Severity: Normal | Resolution: fixed
Keywords: ASGI | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson <carlton.gibson@β¦>):
In [changeset:"7b0ed458d960f1b0bf5c46a74b2fef0e0def71b8" 7b0ed458]:
{{{
#!CommitTicketReference repository=""
revision="7b0ed458d960f1b0bf5c46a74b2fef0e0def71b8"
[4.1.x] Refs #33173, Refs #33755 -- Fixed ResourceWarning from unclosed
files in ASGI tests.
Backport of f476c8847a0bf1a4e20becfb3dc66f4da0dbf579 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33755#comment:15>