Opened 4 years ago

Closed 2 years ago

#15281 closed New feature (fixed)

Staticfiles sever view should use generator

Reported by: FunkyBob Owned by: jezdez
Component: contrib.staticfiles Version: master
Severity: Normal Keywords: generator
Cc: aaugustin 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 aaugustin 4 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by jezdez

  • milestone set to 1.3
  • Needs documentation unset
  • Needs tests set
  • Owner set to jezdez
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by jezdez

  • milestone changed from 1.3 to 1.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 4 years ago by jezdez

  • Owner jezdez 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 2 years ago by aaugustin (previous) (diff)

comment:4 Changed 4 years ago by jezdez

  • Patch needs improvement set

comment:5 Changed 4 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 4 years ago by aaugustin

  • 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 4 years ago by aaugustin

comment:7 Changed 4 years ago by aaugustin

  • Cc aaugustin added

comment:8 Changed 4 years ago by jezdez

  • Owner set to jezdez
  • Resolution set to fixed
  • Status changed from new to closed

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 4 years ago by jezdez

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 4 years ago by jezdez

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:11 Changed 4 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

comment:12 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

15281.patch fails to apply cleanly on to trunk

comment:13 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:2 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:3 Changed 2 years ago by aaugustin

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 2 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.
Back to Top