diff --git a/django/core/handlers/base.py b/django/core/handlers/base.py index 2304e7761d..a7b8e61f60 100644 --- a/django/core/handlers/base.py +++ b/django/core/handlers/base.py @@ -73,7 +73,7 @@ class BaseHandler: # Setup default url resolver for this thread set_urlconf(settings.ROOT_URLCONF) response = self._middleware_chain(request) - response._closable_objects.append(request) + response._resource_closers.append(request.close) if response.status_code >= 400: log_response( '%s: %s', response.reason_phrase, request.path, diff --git a/django/core/handlers/wsgi.py b/django/core/handlers/wsgi.py index f8e7a57dc8..2c1c4db241 100644 --- a/django/core/handlers/wsgi.py +++ b/django/core/handlers/wsgi.py @@ -141,6 +141,10 @@ class WSGIHandler(base.BaseHandler): ] start_response(status, response_headers) if getattr(response, 'file_to_stream', None) is not None and environ.get('wsgi.file_wrapper'): + # If `wsgi.file_wrapper` is used the WSGI server does not call + # .close on the response, but on the file wrapper. Patch it to use + # response.close instead which takes care of closing all files. + response.file_to_stream.close = response.close response = environ['wsgi.file_wrapper'](response.file_to_stream, response.block_size) return response diff --git a/django/http/response.py b/django/http/response.py index a7d98117a6..37fa10650d 100644 --- a/django/http/response.py +++ b/django/http/response.py @@ -41,7 +41,7 @@ class HttpResponseBase: # the header (required for working with legacy systems) and the header # value. Both the name of the header and its value are ASCII strings. self._headers = {} - self._closable_objects = [] + self._resource_closers = [] # This parameter is set by the handler. It's necessary to preserve the # historical behavior of request_finished. self._handler_class = None @@ -243,11 +243,13 @@ class HttpResponseBase: # The WSGI server must call this method upon completion of the request. # See http://blog.dscpl.com.au/2012/10/obligations-for-calling-close-on.html def close(self): - for closable in self._closable_objects: + for closer in self._resource_closers: try: - closable.close() + closer() except Exception: pass + # Free resources that were still referenced. + self._resource_closers.clear() self.closed = True signals.request_finished.send(sender=self._handler_class) @@ -378,7 +380,7 @@ class StreamingHttpResponse(HttpResponseBase): # Ensure we can never iterate on "value" more than once. self._iterator = iter(value) if hasattr(value, 'close'): - self._closable_objects.append(value) + self._resource_closers.append(value.close) def __iter__(self): return self.streaming_content @@ -405,7 +407,7 @@ class FileResponse(StreamingHttpResponse): self.file_to_stream = filelike = value if hasattr(filelike, 'close'): - self._closable_objects.append(filelike) + self._resource_closers.append(filelike.close) value = iter(lambda: filelike.read(self.block_size), b'') self.set_headers(filelike) super()._set_streaming_content(value) diff --git a/docs/releases/3.0.4.txt b/docs/releases/3.0.4.txt index dac9853947..c24d8f7a6a 100644 --- a/docs/releases/3.0.4.txt +++ b/docs/releases/3.0.4.txt @@ -11,3 +11,6 @@ Bugfixes * Fixed a data loss possibility when using caching from async code (:ticket:`31253`). + +* Fixed a regression in Django 3.0 that caused a file response using a + temporary file to be closed incorrectly (:ticket:`31240`). diff --git a/tests/builtin_server/tests.py b/tests/builtin_server/tests.py index 879a93bc08..71e261ddcc 100644 --- a/tests/builtin_server/tests.py +++ b/tests/builtin_server/tests.py @@ -4,6 +4,11 @@ from io import BytesIO from unittest import TestCase from wsgiref import simple_server +from django.core.servers.basehttp import get_internal_wsgi_application +from django.test import RequestFactory, override_settings + +from .views import FILE_RESPONSE_HOLDER + # If data is too large, socket will choke, so write chunks no larger than 32MB # at a time. The rationale behind the 32MB can be found in #5596#comment:4. MAX_SOCKET_CHUNK_SIZE = 32 * 1024 * 1024 # 32 MB @@ -89,6 +94,27 @@ class WSGIFileWrapperTests(TestCase): self.assertEqual(handler.stdout.getvalue().splitlines()[-1], b'Hello World!') self.assertEqual(handler.stderr.getvalue(), b'') + @override_settings(ROOT_URLCONF='builtin_server.urls') + def test_file_response_closing(self): + """ + View returning a FileResponse properly closes the file and http + response when file_wrapper is used. + """ + env = RequestFactory().get('/fileresponse/').environ + handler = FileWrapperHandler(None, BytesIO(), BytesIO(), env) + handler.run(get_internal_wsgi_application()) + # Sendfile is used only when file_wrapper has been used. + self.assertTrue(handler._used_sendfile) + # Fetch the original response object. + self.assertIn('response', FILE_RESPONSE_HOLDER) + response = FILE_RESPONSE_HOLDER['response'] + # The response and file buffers are closed. + self.assertIs(response.closed, True) + buf1, buf2 = FILE_RESPONSE_HOLDER['buffers'] + self.assertIs(buf1.closed, True) + self.assertIs(buf2.closed, True) + FILE_RESPONSE_HOLDER.clear() + class WriteChunkCounterHandler(ServerHandler): """ diff --git a/tests/builtin_server/urls.py b/tests/builtin_server/urls.py new file mode 100644 index 0000000000..c26366f1e6 --- /dev/null +++ b/tests/builtin_server/urls.py @@ -0,0 +1,7 @@ +from django.urls import path + +from . import views + +urlpatterns = [ + path('fileresponse/', views.file_response), +] diff --git a/tests/builtin_server/views.py b/tests/builtin_server/views.py new file mode 100644 index 0000000000..be7c7e94ab --- /dev/null +++ b/tests/builtin_server/views.py @@ -0,0 +1,15 @@ +from io import BytesIO + +from django.http import FileResponse + +FILE_RESPONSE_HOLDER = {} + + +def file_response(request): + f1 = BytesIO(b"test1") + f2 = BytesIO(b"test2") + response = FileResponse(f1) + response._resource_closers.append(f2.close) + FILE_RESPONSE_HOLDER['response'] = response + FILE_RESPONSE_HOLDER['buffers'] = (f1, f2) + return response