urllib3: HTTPResponse.read() closes the response causing problem with io.BufferedReader

In the following text I mostly refer to python2 for clarity, but the problem also happens on py3. From some library I get an active streaming urllib3.HTTPResponse with preload_content=False. That response refers to a very large CSV file, so I want to read it line-by-line passing it through csv.reader. csv.reader requires unicode objects. HTTPResponse returns bytes objects, so I use io.TextIOWrapper. (Actually I use backports.csv to achieve compatibility with io).

Now, here is the problem:

test.csv - notice the last line is missing a trailing line separator:

abc<LF>
def

It is served with python -m SimpleHTTPServer. And the test code, even without CSV:

import io
import urllib3

http = urllib3.PoolManager()
resp = http.request('GET', 'http://localhost:8000/test.csv', preload_content=False)

for line in iter(
	io.TextIOWrapper(
		# py2's implementation of TextIOWrapper requires `read1` method which is provided by `BufferedReader` wrapper
		io.BufferedReader(
			resp
		)
	)
):
	print(repr(line))

Here is the error:

u'abc\n'
Traceback (most recent call last):
  File "test.py", line 11, in <module>
    resp
ValueError: I/O operation on closed file.

If I remove TextIOWrapper then the error is slightly different but with the same meaning:

ValueError: readline of closed file

From what I could find out, C implementation of readline checks if the file is closed. And if (and only if) the last line is not terminated then it tries to read “the rest” which results in this error, because HTTPResponse.read() closes the “file” after it is finished.

This behaviour is contrary to ordinary file behaviour (and io.open() or py3 file behaviour) which will not close the file on its will but instead will keep yielding empty string when trying to read past the end of file.

Which would be the best way to make streaming HTTPResponse work properly with io.BufferedReader?

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 15 (12 by maintainers)

Most upvoted comments

Okay, so what we have here is an API mismatch. An HTTP response body can generally be treated as a file which…

Is read-only
Is not seekable
Closes itself immediately upon reading the last byte (signaled to the io module via the .closed property)

Most of these attributes are perfectly normal and expected for a file-like object. However, self-closing is not.

	Behavior if read when closed	EOF behavior	Read on EOF behavior
File	Exception	Remains open	Returns empty byte array
HTTP response	Returns empty byte array	Closes	Returns empty byte array

Generally, a file is open when it can be interacted with. When it’s closed, a file pointer raises exceptions when it’s interacted with. Thus, we have a bit of a consistency problem. We can (and should) be able to interact with an HTTPResponse after its body has been completely read, but we’re currently acting as though the file is closed immediately upon consumption.

Essentially, we’re using .closed to telegraph that a file has been consumed, but the standard behavior for a (read-only, non-seekable) file which has been completely consumed (but which remains open) is to return an empty set of bytes. A file being closed has a semantically different meaning - it means that the file can no longer be interacted with in any way.

I propose that we change the behavior of .close() and .closed on HTTPResponse to more closely match the semantic meaning intended by IOBase. Calling .close() will result in a call to the underlying httplib response object’s .close() method, followed by a call to super().close(), while .closed will simply inherit from IOBase; if the response has not been specifically closed, it will appear open - which is the more conservative move, as it will never appear closed in a case where the httplib fp remains open (and therefore needs to be explicitly closed).

This table summarizes behavior before and after this change:

	`close()`	`closed`
Before	Close underlying HTTP fp	Checks that underlying HTTP fp has been closed
After	Close underlying HTTP fp and set status to closed	Check that status is set to closed

@nateprewitt, you’ve investigated this in the past in #977. What are your thoughts? Also CC @sigmavirus24 and @Lukasa.

haikuginger on Jan 3, 2018

Lots of good information and investigation here, nice work @haikuginger. You know when tables are used in a GitHub issue comment that shit’s getting serious. 😃

I’m in favor of the change as long as it goes in as a breaking change.

sethmlarson on Jan 3, 2018

So that attribute would be used like so:

r = urllib3.PoolManager().request("GET", "http://google.com", preload_content=False)
r.autoclose = False
for line in io.TextIOWrapper(r):
   ...

I’d be open to a PR for this change. 😃

sethmlarson on Jul 9, 2019

@gsakkis, here is the workaround I used in my project:

class ResponseWrapper(io.IOBase):
    # This is the wrapper around urllib3.HTTPResponse
    # to work-around an issue urllib3/urllib3#1305.
    #
    # Here we decouple HTTPResponse's "closed" status from ours.
    #
    # FIXME drop this wrapper after urllib3/urllib3#1305 is fixed

    def __init__(self, resp):
        self._resp = resp

    def close(self):
        self._resp.close()
        super(ResponseWrapper, self).close()

    def readable(self):
        return True

    def read(self, amt=None):
        if self._resp.closed:
            return b''
        return self._resp.read(amt)

    def readinto(self, b):
        val = self.read(len(b))
        if not val:
            return 0
        b[:len(val)] = val
        return len(val)

Hope it helps 😃

MarSoft on Jul 3, 2019

Otherwise, we can probably manage it ourselves, but I’m a little concerned about the fp getting changed underneath us. Having us maintain a closed property independent of the fp could expose us to other state issues.

@nateprewitt, understood. In general, though, since we don’t ourselves rely on the property, do actively close the connection ourselves, and provide .closed as a convenience for compatibility with io, I’m leaning towards not worrying about that potential at the moment.

I’m adding two issue labels - “solution proposed” and “proposed solution accepted” and tagging this issue with the first; if there are no significant objections in the next couple days, I’ll move it to the latter to indicate that any contributor or myself may begin work on the proposed change.

haikuginger on Jan 9, 2018