Staticfiles sever view should use generator

[FunkyBob]

Especially for larger files, this can greatly improve the response times.

Also, there's no reason for this module to use dict-style access to the stat object.

Index: django/contrib/staticfiles/views.py
===================================================================
--- django/contrib/staticfiles/views.py	(revision 15490)
+++ django/contrib/staticfiles/views.py	(working copy)
@@ -7,7 +7,6 @@
 import os
 import posixpath
 import re
-import stat
 import urllib
 from email.Utils import parsedate_tz, mktime_tz
 
@@ -80,12 +79,12 @@
     mimetype, encoding = mimetypes.guess_type(fullpath)
     mimetype = mimetype or 'application/octet-stream'
     if not was_modified_since(request.META.get('HTTP_IF_MODIFIED_SINCE'),
-                              statobj[stat.ST_MTIME], statobj[stat.ST_SIZE]):
+                              statobj.st_mtime, statobj.st_size):
         return HttpResponseNotModified(mimetype=mimetype)
-    contents = open(fullpath, 'rb').read()
-    response = HttpResponse(contents, mimetype=mimetype)
-    response["Last-Modified"] = http_date(statobj[stat.ST_MTIME])
-    response["Content-Length"] = len(contents)
+    contents = open(fullpath, 'rb')
+    response = HttpResponse(iter(contents), mimetype=mimetype)
+    response["Last-Modified"] = http_date(statobj.st_mtime)
+    response["Content-Length"] = statobj.st_size
     if encoding:
         response["Content-Encoding"] = encoding
     return response

This does, however, make the view susceptible to generator-consuming middleware, but that's another swag of tickets.

[Jannis Leidel]

Hm, not sure actually that we can fix this in 1.3 since a lot more core things seem to be involved in that. E.g. I wasn't able to let the static view tests pass with that patch. Pushing to 1.4 since we are so close to a RC.

[Jannis Leidel]

This was fixed at the same time as #7581 in 4b27813198ae31892f1159d437e492f7745761a0.

Note that this commit ensures that the file gets closed. The last patch on this ticket relied on the garbage collector to do that.

[anonymous]

Recent trunk changes have moved the work from contrib.staticfiles.views to views.static.

Also, to address the test failures, I've adjusted them to consume the request.content only once.

The new patch is smaller [doesn't clean up the stat.ST_* usage], and makes the tests pass for me.

Index: django/views/static.py
===================================================================
--- django/views/static.py	(revision 15530)
+++ django/views/static.py	(working copy)
@@ -59,10 +59,9 @@
     if not was_modified_since(request.META.get('HTTP_IF_MODIFIED_SINCE'),
                               statobj[stat.ST_MTIME], statobj[stat.ST_SIZE]):
         return HttpResponseNotModified(mimetype=mimetype)
-    contents = open(fullpath, 'rb').read()
-    response = HttpResponse(contents, mimetype=mimetype)
+    response = HttpResponse(open(fullpath, 'rb'), mimetype=mimetype)
     response["Last-Modified"] = http_date(statobj[stat.ST_MTIME])
-    response["Content-Length"] = len(contents)
+    response["Content-Length"] = statobj.st_size
     if encoding:
         response["Content-Encoding"] = encoding
     return response
Index: tests/regressiontests/views/tests/static.py
===================================================================
--- tests/regressiontests/views/tests/static.py	(revision 15530)
+++ tests/regressiontests/views/tests/static.py	(working copy)
@@ -22,8 +22,9 @@
         for filename in media_files:
             response = self.client.get('/views/site_media/%s' % filename)
             file_path = path.join(media_dir, filename)
-            self.assertEquals(open(file_path).read(), response.content)
-            self.assertEquals(len(response.content), int(response['Content-Length']))
+            content = str(response.content)
+            self.assertEquals(open(file_path).read(), content)
+            self.assertEquals(len(content), int(response['Content-Length']))
             self.assertEquals(mimetypes.guess_type(file_path)[1], response.get('Content-Encoding', None))
 
     def test_unknown_mime_type(self):
@@ -65,9 +66,9 @@
         response = self.client.get('/views/site_media/%s' % file_name,
                                    HTTP_IF_MODIFIED_SINCE=invalid_date)
         file = open(path.join(media_dir, file_name))
-        self.assertEquals(file.read(), response.content)
-        self.assertEquals(len(response.content),
-                          int(response['Content-Length']))
+        content = str(response.content)
+        self.assertEquals(file.read(), content)
+        self.assertEquals(len(content), int(response['Content-Length']))
 
     def test_invalid_if_modified_since2(self):
         """Handle even more bogus If-Modified-Since values gracefully
@@ -80,6 +81,6 @@
         response = self.client.get('/views/site_media/%s' % file_name,
                                    HTTP_IF_MODIFIED_SINCE=invalid_date)
         file = open(path.join(media_dir, file_name))
-        self.assertEquals(file.read(), response.content)
-        self.assertEquals(len(response.content),
-                          int(response['Content-Length']))
+        content = str(response.content)
+        self.assertEquals(file.read(), content)
+        self.assertEquals(len(content), int(response['Content-Length']))

[Aymeric Augustin]

I reviewed the patch, updated it against latest trunk, and added a test for the new behavior. All tests pass.

Using statobj.st_* instead of statobj[stat.ST_*] seems worthwhile: using the attributes is possible since Python 2.2 and using the indexes is kept only for "backwards compatibility". So I included that change.

Even after the patch, I think the warning in the docs ("grossly inefficient and probably insecure") is still appropriate so I did not change it.

[Jannis Leidel]

In [15701]:

Fixed #15281 -- Made the static view use an iterator when serving a file, effectively making this less of a memory hog. Also use the appropriate attributes of the stat object instead of indexes. Thanks for the initial patch, FunkyBob and aaugustin.

[Jannis Leidel]

In [15703]:

Fixed #15531 -- Partially reverted [15701] due to compatibility issues with middlewares that modify content of responses. Thanks for the report, schinckel. Refs #15281.

[Jannis Leidel]

Reopening after having reverted the iterator change in r15703, this needs a bit more thought.

[patchhammer]

15281.patch fails to apply cleanly on to trunk

[Jacob]

Milestone 1.4 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

This was fixed at the same time as #7581 in 4b27813198ae31892f1159d437e492f7745761a0.

Note that this commit ensure the file gets closed. The last patch on this ticket relied on the garbage collector to do that.