Middleware accessing HttpResponse.content breaks streaming HttpResponse objects.
|Reported by:||mrmachine||Owned by:||ccahoon|
|Severity:||Normal||Keywords:||stream HttpResponse Content-Length|
|Cc:||real.human@…, mmitar@…, mindsocket||Triage Stage:||Accepted|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
ConditionalGetMiddleware, GZipMiddleware, CommonMiddleware, and CsrfMiddleware all access response.content directly or indirectly. This prevents a streaming response from being initiated until any generator passed to the HttpResponse as content is consumed, which can cause a timeout when trying to stream large dynamically generated content.
I've already put together a patch based on the assumption that HttpResponse._is_string being False indicates content has been passed in as a dynamic generator and thus we shouldn't delay streaming a response to the browser while consuming the entire generator.
The patch implements the following:
- Allow middleware to assign a new generator to HttpResponse.content without consuming it (e.g. GZipMiddleware)
- Compress chunks of content yielded by HttpResponse._container progressively in GZipMiddleware to allow streaming GZipped content
- Only generate an ETag in CommonMiddleware from HttpResponse.content if HttpResponse._is_string is True
- Only check that the length of HttpResponse.content is less than 200 in GZipMiddleware if HttpResponse._is_string is True
- Only set the Content-Length header in GZipMiddleware and ConditionalGetMiddleware if HttpResponse._is_string is True
With CommonMiddleware enabled by default and breaking the streaming response functionality if ETags are enabled in settings.py, I'd consider this a bug. It can be worked around by manually specifying a bogus ETag before returning the response, which doesn't seem ideal.
With this patch, users still have the option of consuming a generator before passing it to the HttpResponse in order to enable ETag and Content-Length headers, and conditional GZipping when the content length is less than 200.
With CsrfMiddleware, the generator is only consumed when the Content-Type header is text/html or application/xhtml+xml, which may be an acceptable compromise - no streaming response for HTML content if you choose to use django.contrib.csrf.
This patch accesses HttpResponse._is_string and HttpResponse._container from external middleware classes. Perhaps these properties could be made public and/or renamed to be more logical in this context?
Change History (57)
comment:1 Changed 7 years ago by mrmachine
- milestone changed from 1.0 to post-1.0
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
- Triage Stage changed from Unreviewed to Design decision needed
Changed 7 years ago by mrmachine
Changed 7 years ago by Tai Lee <real.human@…>
Changed 7 years ago by mrmachine
Changed 6 years ago by didditdoug
Changed 6 years ago by mrmachine
Changed 4 years ago by mrmachine
comment:26 Changed 4 years ago by jacob
- Triage Stage changed from Design decision needed to Accepted
Changed 4 years ago by Chris Ghormley <chris@…>
Changed 3 years ago by aaugustin
comment:42 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>
- Resolution set to fixed
- Status changed from new to closed