Move ASGI body-file cleanup into ASGIRequest

[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.

[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?

[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.

[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.

[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!

[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.

[Jonas Lundberg]

​https://github.com/django/django/pull/15756

[Carlton Gibson]

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

[Carlton Gibson <carlton@…>]

In e96320c:

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

[Carlton Gibson <carlton@…>]

In f476c884:

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

[Carlton Gibson <carlton.gibson@…>]

In 7b0ed458:

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

Backport of f476c8847a0bf1a4e20becfb3dc66f4da0dbf579 from main