HttpResponseBase.close not called when using FileResponse / WSGI "file wrapper" object

[Chris Jerdonek]

See comment #6 further down for the correct, updated description for this ticket.

This ticket is to suggest doing for StreamingHttpResponse what #25725 did for HttpReponse, namely to close the underlying content iterator after it has been iterated over.

Currently, if creating a StreamingHttpResponse from a file-like object, it doesn't seem like there's an obvious way to close the underlying file after the file has been streamed. And as one of the comments in #25725 pointed out, trying to do this in StreamingHttpResponse.close() isn't a good solution because WSGI servers can't be relied upon to call close().

I believe an alternative, more reliable solution may be to call close() immediately after the iterator has been exhausted (if hasattr(value, 'close') is true, as #25725 does). This is essentially what #25725 did for non-streaming HttpResponse objects. Here is that code:

def content(self, value):
    # Consume iterators upon assignment to allow repeated iteration.
    if hasattr(value, '__iter__') and not isinstance(value, (bytes, str)):
        content = b''.join(self.make_bytes(chunk) for chunk in value)
        if hasattr(value, 'close'):
            try:
                value.close()
            except Exception:
                pass
    else:
        content = self.make_bytes(value)

(from
​https://github.com/django/django/blob/1564e42ad397021093585147875a21dae1a3b3fc/django/http/response.py#L310-L319 )

In the streaming case, the content value argument could be wrapped something like so (inside StreamingHttpResponse._set_streaming_content(value)):

def iter_content():
    yield from value
    if hasattr(value, 'close'):
        try:
            value.close()
        except Exception:
            pass

new_value = iter_content()

Here is the current code for StreamingHttpResponse._set_streaming_content():

def _set_streaming_content(self, value):
    # Ensure we can never iterate on "value" more than once.
    self._iterator = iter(value)
    if hasattr(value, 'close'):
        self._closable_objects.append(value)

(from: ​https://github.com/django/django/blob/1564e42ad397021093585147875a21dae1a3b3fc/django/http/response.py#L376-L380)

[Carlton Gibson]

Hmmm.

There's a comment by HttpResponseBase.close():

    # The WSGI server must call this method upon completion of the request.
    # See http://blog.dscpl.com.au/2012/10/obligations-for-calling-close-on.html

We're saying that's not the case?

I'm thinking this probably needs to go via the DevelopersMailingList to get more input.

[Carlton Gibson]

cc-ing Joe on this, who was the author on #25725. (What are your thoughts Joe? 🙂)

[Chris Jerdonek]

We're saying that's not the case?

A few comments on this. My first idea to make sure my content stream was closed was to subclass StreamingHttpResponse and close my content stream inside a new StreamingHttpResponse.close() implementation, precisely because of that code comment you found. But then I found that HttpResponseBase.close() wasn't getting called for some reason. Indeed, HttpResponseBase.close() already calls close() on everything inside self._closable_objects, so it shouldn't have been necessary for me to subclass StreamingHttpResponse if HttpResponseBase.close() was already getting called.

And then I saw the comment in #25725 saying:

If i remember Jacob's talk correctly not even all WSGI server call close.

So I thought that was the coming into play.

But now I'm seeing that when I use the approach I suggested of wrapping the content stream with an iterator that closes the underlying content stream immediately after it has been exhausted, HttpResponseBase.close() does get closed. In other words, not closing the underlying content stream was causing HttpResponseBase.close() not to be called.

So there's an argument for what I'm suggesting independent of whether HttpResponseBase.close() can be relied upon to close. Put another way, implementing what I'm suggesting helps that code comment be true, at least in the case I ran into.

[Chris Jerdonek]

Some updates / more info: I'm now focusing more on the question of why HttpReponse.close() isn't being called in my case, since that could signal an underlying bug.

I believe I'm seeing this behavior only when I use / subclass from FileResponse and not from StreamingHttpResponse. Also, I'm using 1.11.20 so haven't confirmed that the issue occurs in master.

The FileResponse class has special logic when the value argument has a read() method:

def _set_streaming_content(self, value):
    if hasattr(value, 'read'):
        self.file_to_stream = value
        filelike = value
        if hasattr(filelike, 'close'):
            self._closable_objects.append(filelike)
        value = iter(lambda: filelike.read(self.block_size), b'')
    else:
        self.file_to_stream = None
    super(FileResponse, self)._set_streaming_content(value)

(from: ​https://github.com/django/django/blob/1c9cb948d7b0c264d244763b6682ab790a6b90a0/django/http/response.py#L411-L420 )

So maybe the reason things start working when I wrap the file object in an iterator is that it causes value not to have a read() method anymore, and so the special logic isn't get executed. So maybe that logic is the problem.

By the way, is it okay that the value passed to the parent class's (StreamingHttpResponse's) _set_streaming_content() method is different from the value set to self.file_to_stream?

[Chris Jerdonek]

Okay, I think I understand what's happening now.

As was remarked above, Django's code has this code comment above HttpResponseBase.close():

# The WSGI server must call this method upon completion of the request.
# See http://blog.dscpl.com.au/2012/10/obligations-for-calling-close-on.html
def close(self):

However, this comment doesn't seem to be true currently in the case that FileResponse is used with a content stream with a read() method. This is because in this case, self.file_to_stream is set to the content stream, and then Django constructs a "file wrapper" from the file (per the WSGI spec) instead of using the HttpResponse object:

if getattr(response, 'file_to_stream', None) is not None and environ.get('wsgi.file_wrapper'):
    response = environ['wsgi.file_wrapper'](response.file_to_stream)
return response

(from: ​https://github.com/django/django/blob/87f5d07eededc86f8ce1797fdfca7d4903ee0edc/django/core/handlers/wsgi.py#L151-L153 )

The implication of this is that when the WSGI server calls close() on the "file wrapper" object, it only causes close() to be called on the underlying file-like object and not on the HttpResponseBase object.

So when FileResponse is used as I've described above, it doesn't seem like HttpResponseBase.close() can be used as a hook for the end of the request. For this to happen, before Django constructs the "file wrapper" object, I think Django would need to modify the file-like object's close() method so that it calls FileResponse.close().