UpdateCacheMiddleware can't cache a generator response

[Matthew Somerville]

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

[Johannes Maron]

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__()

[Tim Graham]

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.

[Johannes Maron]

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.

[Matthew Somerville]

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.

[Johannes Maron]

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.

[Johannes Maron]

I updated my PR

[Tim Graham <timograham@…>]

In 5233b700:

Fixed #25725 -- Made HttpReponse immediately close objects.