recent change to urllib3's is_fp_closed broke cachecontrol for Python 3 and PyPy

[requiredfield]

FileCache stopped working for me with Python 3 and PyPy, and I think I tracked down the cause to a recent change to urllib3's is_fp_closed utility.

From https://github.com/ionrock/cachecontrol/blob/master/cachecontrol/filewrapper.py#L32

        # Is this the best way to figure out if the file has been completely
        #   consumed?
        if is_fp_closed(self.__fp):
            self.__callback(self.__buf.getvalue())

In my Python 3 and PyPy environments, is_fp_closed was never returning True. Reverting the changes in urllib3/urllib3#435 fixed it.

I tried cloning urllib3 and running the tox tests to dig in further but couldn't get the tests to run, and thought my best next step would be reporting here.

It may be that cachecontrol's code is just fine and the issue is in urllib3, but I figured I'd confirm here first. Does that look like the problem?

Thanks in advance for taking a look!

[ionrock]

@requiredfield Thanks for writing up this issue. I want to be sure I've reproduced the issue correctly. Using python 3 I've done the following:

>>> from cachecontrol import CacheControl
>>> from cachecontrol.caches.file_cache import FileCache
>>> from requests import Session
>>> s = CacheControl(Session(), FileCache('web_cache'))
>>> s.get('http://httpbin.org/cache/60')
>>> r = s.get('http://httpbin.org/cache/60')
>>> r.from_cache
False

Does the lack of a cached response reveal the error you're seeing. Thanks!

[requiredfield]

Thanks for testing this out @ionrock. Looks like you're reproducing the issue. You could also step through/instrument the code I excerpted from https://github.com/ionrock/cachecontrol/blob/master/cachecontrol/filewrapper.py#L32 to see that the is_fp_closed call never returns True.

Does urllib3/urllib3#435 look like the culprit to you too? I wanted to see if I could write some tests to expose the issue but when I tried running urllib3's tests and immediately got some tornado error I figured I'd see if anyone else could recognize this / fix more easily.

[requiredfield]

Hey @ionrock, should a ticket be opened in the requests tracker for this?

[ionrock]

@requiredfield if there is a fix in urllib3 that hasn't been vendored in requests, then definitely. Otherwise, I think it is something we can probably try to patch in the mean time. I haven't had a chance to dig into a fix just yet, but I'll take a stab this afternoon. Essentially, what I'm thinking is replicating the old behavior as it is the same as the current one.

[requiredfield]

Thanks for taking a look this afternoon, @ionrock. FWIW I wasn't able to find any fix in urllib3. Maybe worth opening an issue in requests anyway just to make sure they're aware of it?

[ionrock]

@requiredfield OK! I found it. It is the ordering of the tests in urllib3/urllib3#435. Effectively, if obj has a closed attr that is False, it exits right away even though it might also be a wrapper where obj.fp is None.

I'll see if urllib3 would accept a patch re-ordering the try/except blocks.

[requiredfield]

I knew it! Thanks for confirming and following up!

[ionrock]

This was pushed out with 0.10.3. Thanks @requiredfield for the report! It ended up fixing a bug a pip as wel :)

[requiredfield]

My pleasure, thanks for releasing the fix so quickly! By the way, what was the other bug this fixed?

[ionrock]

There was a deadlock of some sort b/c the garbage collector was waiting for the callback to be called. I happened to stumble into the IRC channel at just the right time ;)

[requiredfield]

Sounds interesting, and serendipitous :)

Checked out https://github.com/pypa/pip but guess it hasn't made it from IRC to there yet.

[requiredfield]

(for the record, looks like pypa/pip#2043 was the issue @ionrock was referring to)