Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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 Carlton Gibson)

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 Carlton Gibson, 2 years ago

Description: modified (diff)

comment:2 by Carlton Gibson, 2 years ago

Summary: Move ASGi body-file cleanup into ASGIRequestMove ASGI body-file cleanup into ASGIRequest

comment:3 by Mariusz Felisiak, 2 years ago

Cc: Jonas Lundberg added
Triage Stage: UnreviewedAccepted

comment:4 by Jonas Lundberg, 2 years ago

Owner: changed from nobody to Jonas Lundberg
Status: newassigned

comment:5 by Jonas Lundberg, 2 years ago

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?

comment:6 by Jonas Lundberg, 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.

Last edited 2 years ago by Mariusz Felisiak (previous) (diff)

comment:7 by Carlton Gibson, 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 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.


Last edited 2 years ago by Carlton Gibson (previous) (diff)

comment:8 by Carlton Gibson, 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 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!

comment:9 by Jonas Lundberg, 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.

Last edited 2 years ago by Jonas Lundberg (previous) (diff)

comment:11 by Carlton Gibson, 2 years ago

Super, thanks Jonas. I will pick up for review next week. 👍

comment:12 by Carlton Gibson, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Carlton Gibson <carlton@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In e96320c:

Fixed #33755 -- Moved ASGI body-file cleanup into request class.

comment:14 by Carlton Gibson <carlton@…>, 2 years ago

In f476c884:

Refs #33173, Refs #33755 -- Fixed ResourceWarning from unclosed files in ASGI tests.

comment:15 by Carlton Gibson <carlton.gibson@…>, 2 years ago

In 7b0ed458:

[4.1.x] Refs #33173, Refs #33755 -- Fixed ResourceWarning from unclosed files in ASGI tests.

Backport of f476c8847a0bf1a4e20becfb3dc66f4da0dbf579 from main

Note: See TracTickets for help on using tickets.
Back to Top