psf
Use urllib3 native chunking ability
This PR is the same as #5664 but rebased on to the main branch to bring it back up to date. This PR is needed to stay compatible with future changes in urllib3 introduced by https://github.com/urllib3/urllib3/pull/2649. The comments in that PR offer a lot of detail in the changes in urllib3 but this one provided a good overview https://github.com/urllib3/urllib3/pull/2649#issuecomment-1200197578.
@sethmlarson @nateprewitt Can you please approve the workflow. I'm curious if the SSL error I had locally still shows up in this pipeline.
I ran all the checks locally this time :blush:
$ pre-commit run -a
Check Yaml...............................................................Passed
Debug Statements (Python)................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
isort....................................................................Passed
black....................................................................Passed
pyupgrade................................................................Passed
flake8...................................................................Passed
It is noteworthy that this change, while something I've been wanting for a long time, is a breaking API change. In particular, the problem is that in the current implementation urllib3's Retry is sometimes bypassed when using chunked encoding: Users have to write their own custom retry logic based on capturing errors when using chunked encoding. Therefore, this change will quietly break everybody who has written custom error handling for chunked requests versus using Retry for non-chunked requests.
@theGOTOguy is the bypassing of Retry something going on here in the request package or is this a bug in urllib3?
No, the issue wasn't with urllib3. The part of the code you removed that executes if the request is chunked clearly has at least some logic outside urllib3, and if an error happened anywhere there then it wouldn't be caught by Retry and a non-urllib3 error would be raised that then had to be handled separately. I can't seem to find my notes on the specific errors, but in short, if I was dealing with chunked requests and a flaky or overloaded server, I couldn't rely on it being handled in Retry.
To be clear, I think that the way you are handling it here is 100% the right thing to do. My understanding based on the discussion in #5664 was that a change to the errors that are thrown during the course of handling chunked requests is something that won't happen until the next major release, but if it does happen now I would suggest a test simulating chunked requests against a flaky server (#5915, anyone?) and warn about the change in behavior in the changelog.
Edit: It appears that the errors that weren't caught by urllib3's Retry were tied to either timeouts or empty responses from the server. This would be straightforward to test and verify against a realistic Werkzeug server as opposed to the existing socket-based tests.
@sethmlarson @nateprewitt We are not getting very far on this =/
I have put in an application as PSF managing member. I'm not sure if that will make a difference, but maybe then we could have a call to discuss the path forward.
It looks like we need to cut a new major release branch for both urlib3 and requests and to start preparing for the next major version?
@shadycuz Unfortunately whether or not you're a managing member won't affect this PR being merged faster. People have obligations outside of open source, I've recently bought a new house which is consuming all my open source time. I can't speak for Nate but I know with certainty this PR isn't not being worked on for lack of desire to see it succeed. Have patience, promise we will get to this when we have the time.
@nateprewitt FYI we're going to skip the failing integration tests for chunking to get that PR merged, this PR will be needed to add full support for urllib3 v2.0 to Requests.