Code

Opened 5 years ago

Closed 3 years ago

#10627 closed (duplicate)

Middleware using len() on response.content is gobbling generators

Reported by: FunkyBob Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords: middleware
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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:

  1. In the case of conditional-get, it skips setting Content-Length header if not response._is_string.
  2. 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)

gzip.py.diff (1.2 KB) - added by FunkyBob 5 years ago.
Fix for django/middleware/gzip.py
http.py.diff (504 bytes) - added by FunkyBob 5 years ago.
Fix for django/middleware/http.py

Download all attachments as: .zip

Change History (7)

Changed 5 years ago by FunkyBob

Fix for django/middleware/gzip.py

Changed 5 years ago by FunkyBob

Fix for django/middleware/http.py

comment:1 Changed 5 years ago by FunkyBob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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:

  1. How do you determine if the content from a generator will be worth compressing, or do you just always try/never try?
  2. 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:2 Changed 5 years ago by jacob

  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of # 6527

comment:3 Changed 5 years ago by jacob

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 Changed 3 years ago by FunkyBob

  • Resolution duplicate deleted
  • Status changed from closed to 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 Changed 3 years ago by russellm

  • Resolution set to duplicate
  • Status changed from reopened to 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.