diff --git a/django/core/files/storage.py b/django/core/files/storage.py index a36440ab4c..a5cded72ac 100644 --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -211,7 +211,6 @@ class FileSystemStorage(Storage): # This file has a file path that we can move. if hasattr(content, 'temporary_file_path'): file_move_safe(content.temporary_file_path(), full_path) - content.close() # This is a normal uploadedfile that we can stream. else: @@ -230,7 +229,6 @@ class FileSystemStorage(Storage): _file = os.fdopen(fd, mode) _file.write(chunk) finally: - content.close() locks.unlock(fd) if _file is not None: _file.close() diff --git a/django/core/files/uploadedfile.py b/django/core/files/uploadedfile.py index 75f25405b9..9a94894424 100644 --- a/django/core/files/uploadedfile.py +++ b/django/core/files/uploadedfile.py @@ -96,9 +96,6 @@ class InMemoryUploadedFile(UploadedFile): def open(self, mode=None): self.file.seek(0) - def close(self): - pass - def chunks(self, chunk_size=None): self.file.seek(0) yield self.read() diff --git a/django/core/handlers/base.py b/django/core/handlers/base.py index 010d5e35ee..26788fd58a 100644 --- a/django/core/handlers/base.py +++ b/django/core/handlers/base.py @@ -219,6 +219,8 @@ class BaseHandler(object): signals.got_request_exception.send(sender=self.__class__, request=request) response = self.handle_uncaught_exception(request, resolver, sys.exc_info()) + response._closable_objects.append(request) + return response def handle_uncaught_exception(self, request, resolver, exc_info): diff --git a/django/http/multipartparser.py b/django/http/multipartparser.py index 20232cb7c6..1bcace94cd 100644 --- a/django/http/multipartparser.py +++ b/django/http/multipartparser.py @@ -228,6 +228,7 @@ class MultiPartParser(object): break except SkipFile: + self._close_files() # Just use up the rest of this file... exhaust(field_stream) else: @@ -237,6 +238,7 @@ class MultiPartParser(object): # If this is neither a FIELD or a FILE, just exhaust the stream. exhaust(stream) except StopUpload as e: + self._close_files() if not e.connection_reset: exhaust(self._input_data) else: @@ -268,6 +270,14 @@ class MultiPartParser(object): """Cleanup filename from Internet Explorer full paths.""" return filename and filename[filename.rfind("\\") + 1:].strip() + def _close_files(self): + # Free up all file handles. + # FIXME: this currently assumes that upload handlers store the file as 'file' + # We should document that... (Maybe add handler.free_file to complement new_file) + for handler in self._upload_handlers: + if hasattr(handler, 'file'): + handler.file.close() + class LazyStream(six.Iterator): """ diff --git a/django/http/request.py b/django/http/request.py index 84457c5300..250db32bf2 100644 --- a/django/http/request.py +++ b/django/http/request.py @@ -5,6 +5,7 @@ import os import re import sys from io import BytesIO +from itertools import chain from pprint import pformat from django.conf import settings @@ -256,6 +257,11 @@ class HttpRequest(object): else: self._post, self._files = QueryDict('', encoding=self._encoding), MultiValueDict() + def close(self): + if hasattr(self, '_files'): + for f in chain.from_iterable(l[1] for l in self._files.lists()): + f.close() + # File-like and iterator interface. # # Expects self._stream to be set to an appropriate source of bytes by diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index 6e38d09f61..0c7c4748c8 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -504,6 +504,10 @@ File Uploads attribute is now optional. If it is omitted or given ``None`` or an empty string, a subdirectory won't be used for storing the uploaded files. +* Uploaded files are now explicitly closed before the response is delivered to + the client. Partially uploaded files are also closed as long as they are + named ``file`` in the upload handler. + Forms ^^^^^ diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py index da9410d70f..c3800cbc89 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -19,7 +19,8 @@ from django.core.cache import cache from django.core.exceptions import SuspiciousOperation from django.core.files.base import File, ContentFile from django.core.files.storage import FileSystemStorage, get_storage_class -from django.core.files.uploadedfile import SimpleUploadedFile +from django.core.files.uploadedfile import (InMemoryUploadedFile, SimpleUploadedFile, + TemporaryUploadedFile) from django.test import LiveServerTestCase, SimpleTestCase from django.test import override_settings from django.utils import six @@ -206,6 +207,23 @@ class FileStorageTests(unittest.TestCase): self.storage.delete('path/to/test.file') + def test_save_doesnt_close(self): + with TemporaryUploadedFile('test', 'text/plain', 1, 'utf8') as file: + file.write(b'1') + file.seek(0) + self.assertFalse(file.closed) + self.storage.save('path/to/test.file', file) + self.assertFalse(file.closed) + self.assertFalse(file.file.closed) + + file = InMemoryUploadedFile(six.StringIO('1'), '', 'test', + 'text/plain', 1, 'utf8') + with file: + self.assertFalse(file.closed) + self.storage.save('path/to/test.file', file) + self.assertFalse(file.closed) + self.assertFalse(file.file.closed) + def test_file_path(self): """ File storage returns the full path of a file diff --git a/tests/file_uploads/tests.py b/tests/file_uploads/tests.py index b6f8cfe17e..1e2ba488e6 100644 --- a/tests/file_uploads/tests.py +++ b/tests/file_uploads/tests.py @@ -327,6 +327,37 @@ class FileUploadTests(TestCase): self.assertEqual(got.get('file1'), 1) self.assertEqual(got.get('file2'), 2) + def test_fileuploads_closed_at_request_end(self): + file = tempfile.NamedTemporaryFile + with file() as f1, file() as f2a, file() as f2b: + response = self.client.post('/fd_closing/t/', { + 'file': f1, + 'file2': (f2a, f2b), + }) + + request = response.wsgi_request + # Check that the files got actually parsed. + self.assertTrue(hasattr(request, '_files')) + + file = request._files['file'] + self.assertTrue(file.closed) + + files = request._files.getlist('file2') + self.assertTrue(files[0].closed) + self.assertTrue(files[1].closed) + + def test_no_parsing_triggered_by_fd_closing(self): + file = tempfile.NamedTemporaryFile + with file() as f1, file() as f2a, file() as f2b: + response = self.client.post('/fd_closing/f/', { + 'file': f1, + 'file2': (f2a, f2b), + }) + + request = response.wsgi_request + # Check that the fd closing logic doesn't trigger parsing of the stream + self.assertFalse(hasattr(request, '_files')) + def test_file_error_blocking(self): """ The server should not block when there are upload errors (bug #8622). diff --git a/tests/file_uploads/urls.py b/tests/file_uploads/urls.py index ae20a2b79f..99f0cd3eb5 100644 --- a/tests/file_uploads/urls.py +++ b/tests/file_uploads/urls.py @@ -15,4 +15,5 @@ urlpatterns = [ url(r'^getlist_count/$', views.file_upload_getlist_count), url(r'^upload_errors/$', views.file_upload_errors), url(r'^filename_case/$', views.file_upload_filename_case_view), + url(r'^fd_closing/(?Pt|f)/$', views.file_upload_fd_closing), ] diff --git a/tests/file_uploads/views.py b/tests/file_uploads/views.py index a29391af6e..de195f4d39 100644 --- a/tests/file_uploads/views.py +++ b/tests/file_uploads/views.py @@ -157,3 +157,9 @@ def file_upload_content_type_extra(request): (k, smart_str(v)) for k, v in uploadedfile.content_type_extra.items() ]) return HttpResponse(json.dumps(params)) + + +def file_upload_fd_closing(request, access): + if access == 't': + request.FILES # Trigger file parsing. + return HttpResponse('')