Opened 16 years ago
Closed 14 years ago
#10627 closed (duplicate)
Middleware using len() on response.content is gobbling generators
Reported by: | FunkyBob | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Keywords: | middleware | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
We recently found that a slow view (4 to 5 minutes) was made even worse because we couldn't give a generator to HttpResponse and have it still work.
This turned out to be because two of the built-in middlewares ( gzip and conditional-get ) assume the response.content is a string, and call len(response.content) -- which consumes the generator.
Once we bypassed these, the time to first response was seconds, and the entire process ran smoothly.
So, attached are two patches to work around this issue:
- In the case of conditional-get, it skips setting Content-Length header if not response._is_string.
- In the case of gzip, simply move the Content-Encoding test to the front, so the documentation saying gzip will be skipped on anything with a Content-Encoding header works as expected.
Attachments (2)
Change History (7)
by , 16 years ago
Attachment: | gzip.py.diff added |
---|
comment:1 by , 16 years ago
I realise there are other approaches I could have taken with gzip, but I took the "simple and safe" path.
I expect more discussion would be wanted on how to approach the problem ... such as:
- How do you determine if the content from a generator will be worth compressing, or do you just always try/never try?
- Do we want the gzip layer consuming generators and compressing, or leave them alone?
In our case, we leave it up to Apache to gzip any content we ask of it, so it's a non-issue.
comment:3 by , 16 years ago
This is part of a general set of "HttpResponse
doesn't play well with iterators" bugs (#6027, #6527, ...) that we're aware of. It's not really worth having Yet Another Bug in this series, so I've marked duplicate even those bugs don't directly address this *specific* problem. We'll fix the overall issue, though, and all these will go away.
comment:4 by , 14 years ago
Resolution: | duplicate |
---|---|
Status: | closed → reopened |
I just wanted to ping this ticket, since the patches work, pass tests on r15702, and work around the specific issues.
Importantly, the gzip patch makes the gzip middleware more closely match the documentation -- specifically, that the Response will not be processed by gzip middleware if it has Content-Encoding set.
comment:5 by , 14 years ago
Resolution: | → duplicate |
---|---|
Status: | reopened → closed |
@funkybob - none of what you say changes the fact that this ticket is a duplicate of other tickets. If you want to address this issue, please take the discussion to the original tickets.
Fix for django/middleware/gzip.py