#33755 closed Cleanup/optimization (fixed)
Move ASGI body-file cleanup into ASGIRequest
Reported by: | Carlton Gibson | Owned by: | Jonas Lundberg |
---|---|---|---|
Component: | HTTP handling | Version: | dev |
Severity: | Normal | Keywords: | ASGI |
Cc: | Jonas Lundberg | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
In django/core/handlers/asgi.py
ASGIHandler.handle()
currently creates a temporary file, body_file
, and then maintains responsibility for closing that once the response is generated in an extend try...finally
. In outline:
body_file = ... try: ... finally: body_file.close()
The body_file
is passed into the request, via create_request()
and ASGIRequest.__init()
. Conceptually the request object takes ownership of body_file
at that point, and ideally it would be responsible for cleaning it up too, perhaps via a __del__
implementation.
Change History (15)
comment:1 by , 2 years ago
Description: | modified (diff) |
---|
comment:2 by , 2 years ago
Summary: | Move ASGi body-file cleanup into ASGIRequest → Move ASGI body-file cleanup into ASGIRequest |
---|
comment:3 by , 2 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:4 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 2 years ago
comment:6 by , 2 years ago
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.
comment:7 by , 2 years ago
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 aBytesIO
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.
comment:8 by , 2 years ago
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 theASGIStaticFilesHandler
wrapsASGIHandler
so (likely) isn't callingresponse.close
correctly. (See also #33048, #27325)test_non_unicode_query_string
— We're not then handling theclose()
when there's a Unicode error increate_request()
.
Both of those should be addressable though.
What do you think?
Do you fancy continuing on this?
Thanks!
comment:9 by , 2 years ago
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()
and FakePayload.close()
, instead of the try...except
block ... what do you think?
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.
comment:12 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Looked at this briefly and unfortunately the
HttpRequest.body
property drops the reference to thebody_file
by overwriting it with aBytesIO
.I'm thinking it could be safe to close the file in a finally block once it have been read ...
At least the test from #33754 still succeeds ;-)
Thoughts?