-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
recent change to urllib3's is_fp_closed broke cachecontrol for Python 3 and PyPy #39
Comments
@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:
Does the lack of a cached response reveal the error you're seeing. Thanks! |
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 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. |
Hey @ionrock, should a ticket be opened in the requests tracker for this? |
@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. |
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? |
@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. |
I knew it! Thanks for confirming and following up! |
… been read before caching. I copied over the is_fp_closed from urllib3 and reversed the ordering of the try / except in order to give preference to HTTPResponses before real file pointers.
This was pushed out with 0.10.3. Thanks @requiredfield for the report! It ended up fixing a bug a pip as wel :) |
My pleasure, thanks for releasing the fix so quickly! By the way, what was the other bug this fixed? |
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 ;) |
Sounds interesting, and serendipitous :) Checked out https://github.com/pypa/pip but guess it hasn't made it from IRC to there yet. |
(for the record, looks like pypa/pip#2043 was the issue @ionrock was referring to) |
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
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!
The text was updated successfully, but these errors were encountered: