-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Catch errors while handling redirects #1944
Conversation
👍 |
@maxcountryman Does this fix your issue (urllib3/urllib3#206)? |
@schlamar it looks like it should. I can't test it at the moment. A little later on I'll see if I have time to give it a proper run with Photobucket. |
This looks reasonable to me. I'll let @sigmavirus24 take a look too. |
In my tests it did not block and I prefer to do something as opposed to using |
It is just not logical to read from the response in case of RuntimeError because this exception is thrown if the content is already consumed (so a read is not necessary). However, I'm willing to handle this as in your PR if you prefer it. |
While the read is not necessary, it is also not harmful. It's my own stylistic preference that except blocks not be a simple "pass". (If there's no action to take then perhaps the exception should be handled somewhere else (or not thrown at all).) It's up to Kenneth and Cory if they like it as is. I have no strenuous objections to it, I just have a bit of a pet peeve about this particular style. |
👍 |
In addition to #1939, I believe this also fixes #1952 (which I just filed - sorry about that). @schlamar Could you please add some test cases for these bugs as well? A test case for #1952 can be dug out of the (now-scrapped) pull request #1919. @sigmavirus24 Regarding " |
Very interesting. I guess this should be possible (while a test case for the original issue is actually hard, we would need a misbehaving web service for that).
I don't think this was his point. I guess you mixed two things together. My interpretation of @sigmavirus24 comments is:
|
Here's the test case for #1952 (from #1919). Add to the big
I agree #1939 is not feasible to test without adding stuff to httpbin. Maybe that should wait for #1166? A bunch of the other redirection bugs I just filed (and more to come) are going to be hard to test without adding stuff to httpbin, too. |
Thanks, I added this test. Maybe we can add a |
Your interpretation is correct. I've discussed this with several (far more experienced and knowledgeable Python developers, including core PyPy and CPython developers). This also works with your overall goal @zackw: If the error were instead a child of a try:
requests.get(...)
except requests.RequestException:
#... Work in every case. In the redirect case, before this PR, it wouldn't. I know that there are several ideas of "good" Python code that I disagree with others on (including Kenneth), so I don't push those issues normally. I haven't found a good way to preserve backwards compatibility though for those catching the RuntimeError and allowing for a RequestException. I don't think creating a new one that inherits from both is a good idea in the slightest. I'd love if either of you had the solution. In fact, I'd probably send you 🍰 :) |
While this is really interesting (never thought about how you would handle backwards compatibility when changing an exception type), it is actually a bit off topic here =) I think we should focus on this PR and maybe move this discussion into a new issue? Points standing out:
|
@schlamar it took me several months to get a feature merged and deployed on HTTPBin in order to test a bug fix. I'm not certain we should bother waiting that long. Do you have sufficient confidence that you could fake out a gzipped response from a redirect? You could take a similar approach to my test in #1963. Unfortunately, when I planned that test code, I was planning for the simplest case (mine). If there's a good way to adapt it to this PR, that would be awesome. |
@sigmavirus24 @zackw Added a test. What do you think? |
@sigmavirus24 Should I change
to
|
Now looking at this in comparison I'm +1 for the latter one =) |
Updated and rebased against master. |
Me too ;) |
Gave this one more look over. LGTM. 👍 |
@sigmavirus24 I guess you'll like the new |
I really should stop using Python 2 and only use 3. ;) |
That's what I've done with |
#1939 terrifies me. When Requests was handling decoding of transfer-encodings itself, it It was explicitly handling the case of when servers are misinforming about the transfer-encoding. This is one of the major reasons I want to be very wary of coupling too closely to urllib3. Pehaps, akin to the mutable This would unify the API we have today — and gives us the same behavior for character encodings (which was my intention, I'm not sure how this bug got introduced :P). |
I suspect that |
Yes that's right. I had to reimplement parts of requests in order to disable the decoding. |
@kennethreitz If |
But this was broken because it dropped at least one byte from the original response: https://github.com/kennethreitz/requests/issues/1249 To be able to recover gracefully from a decoding error and having the original bytes available afterwards you need basically cache the complete response because decoding could theoretically fail on the last byte (which makes streaming responses basically useless).
I don't see any other use case than disabling content decoding. And this can already be done with |
@Lukasa by all means! |
@schlamar or specifying a transfer encoding other than what the server provided. It'll just give is a more flexible architecture around this, in general. |
The question is, if we implement this, is this pull request still necessary? |
The question is, how should the proposed feature help resolving this issue =) |
Ah, I see:
Character encoding does a replace in case of an error. But in this case it would only make sense if So a rough sketch how this could be done:
However, in case of |
@schlamar if you perform a rebase this will be merged and released today! |
Done. |
Catch errors while handling redirects
✨ 🍰 ✨ |
This is my proposal which fixes #1939 completely.
@sigmavirus24 missed one possible exception in #1940 (ChunkedEncodingError) and didn't handle RuntimeError correctly (because this means that the content is already consumed).
This also addresses the issue that a decoding error is already thrown in Adapter.send by moving the content reading part at the end of Session.send.