Opened 9 years ago

Closed 9 years ago

#25725 closed Bug (fixed)

UpdateCacheMiddleware can't cache a generator response

Reported by: Matthew Somerville Owned by: Johannes Maron
Component: Core (Cache system) Version: 1.8
Severity: Normal Keywords:
Cc: info@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As generator functions have a close method, an HTTPResponse using a generator for its content dies when trying to be cached even though the generator has been consumed and read by the content setter (because the generator has been added to _closable_objects and that can't be cached). Should the content setter of an HTTPResponse spot this and close/not add to _closable_objects,or should it be documented that you can't use a generator function with an HTTPResponse and have it be cached (even though it's been read)?

from django.conf import settings
settings.configure(ALLOWED_HOSTS = ['*'])
from django.middleware import cache
from django import http

req = http.HttpRequest()
req.method = 'GET'
req.META = {'SERVER_NAME': 'localhost', 'SERVER_PORT':80}

resp = http.HttpResponse(x for x in range(1, 10))
cache.FetchFromCacheMiddleware().process_request(req)
cache.UpdateCacheMiddleware().process_response(req, resp)

gives

Can't pickle <type 'generator'>: attribute lookup __builtin__.generator failed

Change History (10)

comment:1 by Johannes Maron, 9 years ago

Cc: info@… added

They only way I see is, closing all closeable objects, while the request is being pickled. That's isn't too difficult, but may lead to strange behavior, as the objects close method could have some request specific behavior attached.
Never the less, this would be a very bad idea in the first place. If i remember Jacob's talk correctly not even all WSGI server call close. Thus relying on your objects to be closed in the Requests close method, well isn't that clever.

How about

    def __getstate__(self):
        for closable in self._closable_objects:
            try:
                    closable.close()
                except Exception:
                    pass
        return super().__getstate__()
Version 0, edited 9 years ago by Johannes Maron (next)

comment:2 by Tim Graham, 9 years ago

I'm curious why you want to use a generator with HttpResponse. Are you expecting to stream the response? Maybe we should try to avoid the crash in any case but it seems a bit unusual to me.

comment:3 by Johannes Maron, 9 years ago

I must agree, tho the iterator gets joined into a bystring anyways. I don't see why one should not be able to cash this. Despite the fact that you can pickle a generator. Question is, what to cache. The response or how to make it.

comment:4 by Matthew Somerville, 9 years ago

timgraham: This actually was an edge case of something that would by default use a StreamingHttpResponse, but would sometimes 404/error and then it would switch to e.g. HttpResponseNotFound but still have a generator-based content. Then a JSONP middleware would change the response status to 200, and that would then try and be cached by the cache middleware. We worked around it in the generic case by using a map(lambda x:x, content) - https://github.com/mysociety/mapit/commit/2a30c9eeaa939f14d5ada3bcaea71636c8cff7fb - but also consuming the generator if it wasn't a 200 would have worked too.

I do think the crash should be avoided, as the documentation says "Finally, you can pass HttpResponse an iterator rather than strings. HttpResponse will consume the iterator immediately, store its content as a string, and discard it." - the generator is not being discarded, as it's making it uncacheable, and it would be good if it could.

codingjoe: I would expect the response to be cached, not how to make it; if I've passed an iterator to HttpResponse I expect it to have been consumed and stored immediately as per the documentation.

comment:5 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:6 by Johannes Maron, 9 years ago

Has patch: set
Owner: changed from nobody to Johannes Maron
Status: newassigned

comment:7 by Tim Graham, 9 years ago

Patch needs improvement: set

comment:8 by Johannes Maron, 9 years ago

Ok, how do we want to handle this. Ether we close the iterators such as files right away or we don't close them at all.

comment:9 by Johannes Maron, 9 years ago

Patch needs improvement: unset

I updated my PR

comment:10 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In 5233b700:

Fixed #25725 -- Made HttpReponse immediately close objects.

Note: See TracTickets for help on using tickets.
Back to Top