Opened 6 years ago

Closed 4 years ago

#15281 closed New feature (fixed)

Staticfiles sever view should use generator

Reported by: FunkyBob Owned by: Jannis Leidel
Component: contrib.staticfiles Version: master
Severity: Normal Keywords: generator
Cc: Aymeric Augustin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

15281.patch (3.9 KB) - added by Aymeric Augustin 6 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by Jannis Leidel

milestone: 1.3
Needs tests: set
Owner: set to Jannis Leidel
Triage Stage: UnreviewedAccepted

comment:2 Changed 6 years ago by Jannis Leidel

milestone: 1.31.4

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.

comment:3 Changed 6 years ago by Jannis Leidel

Owner: Jannis Leidel deleted

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.

Last edited 4 years ago by Aymeric Augustin (previous) (diff)

comment:4 Changed 6 years ago by Jannis Leidel

Patch needs improvement: set

comment:5 Changed 6 years ago by 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']))

comment:6 Changed 6 years ago by Aymeric Augustin

Needs tests: unset
Patch needs improvement: unset

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.

Changed 6 years ago by Aymeric Augustin

Attachment: 15281.patch added

comment:7 Changed 6 years ago by Aymeric Augustin

Cc: Aymeric Augustin added

comment:8 Changed 6 years ago by Jannis Leidel

Owner: set to Jannis Leidel
Resolution: fixed
Status: newclosed

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.

comment:9 Changed 6 years ago by 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.

comment:10 Changed 6 years ago by Jannis Leidel

Resolution: fixed
Status: closedreopened

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

comment:11 Changed 6 years ago by Łukasz Rekucki

Severity: Normal
Type: New feature

comment:12 Changed 6 years ago by patchhammer

Easy pickings: unset
Patch needs improvement: set

15281.patch fails to apply cleanly on to trunk

comment:13 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:2 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:3 Changed 4 years ago by 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.

comment:4 Changed 4 years ago by Aymeric Augustin

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.
Back to Top