raise requests.exceptions.ReadTimeout for streaming responses

[bgardiner]

trafficstars

Currently, if there is a read timeout while streaming a response, a requests.exceptions.ConnectionError is raised in lieu of a requests.exceptions.ReadTimeout. This is unexpected and contrary to the documentation. As a user, I am being forced to catch the ConnectionError when I only actually care about the ReadTimeout situation.

This PR revises the streaming response code to properly raise a requests.exceptions.ReadTimeout, consistent with the non-streaming request behavior.

I am unable to write a test for this situation since httpbin does not support a delay for chunked-encoded responses; see requests/httpbin#479 regarding this lack of functionality.

[bgardiner]

Any thoughts on this? I think this is clearly a bug in the existing implementation...... is there something I am missing?

[bgardiner]

ping anyone want to take a look at this?

[Mr0grog]

I looks like this has been fixed in the 3.0 branch, and the comments on #2392 and #5430 note that a change like this is too much of a breaking change to ship with 2.x. So maybe this should be closed? (With an explanation for people encountering the issue, please!)

FWIW, though, I got here because I only recently had time to really dig into Requests and Urllib3 to understand what was happening here — this has been a longstanding issue in a codebase I maintain, where we were successfully catching some read timeouts but not others. I definitely understand I don’t speak for all users, but, like @VasiliPupkin256, having this fix in 2.x would not break me and would actually really help me clean up some messy code by making error handling more consistent (I currently catch both Timeout and ConnectionError, but for the latter case inspect the error string to decide what to do, which is really not good).