Skip to content
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

Use urllib for chunked requests (Resubmit) #5664

Closed
wants to merge 2 commits into from
Closed

Conversation

lmvlmv
Copy link
Contributor

@lmvlmv lmvlmv commented Nov 18, 2020

This is a resubmit of Use urllib for chunked requests #5128 which was merged and then pulled without warning.

"Resubmit of #4958 with up to date merge. Also addresses #4179.

Specifically, in my use case, chunked requests when a proxy is in use results in failure to evaluate the proxy configuration and the request being sent direct. It appears that urllib3 supports chunking as an option so this fork to lower level urllib3 code seems to no longer be required."

@nateprewitt
Copy link
Member

Hi @lmvlmv,

Thanks for reraising the issue. We're aware there's still demand for simplifying this. The reason the first PR was reverted was a lack of review, and failing to meet the original criteria we'd set to make this change.

When we first discussed this in #4392, we'd wanted a pretty rigorous testing to ensure we're not introducing regressions. We have the start of some of that but we'll need to expand it more before we're comfortable merging.

@jbgomond
Copy link

Thanks a lot :)

@dmazin
Copy link

dmazin commented Apr 21, 2021

Hi @lmvlmv,

Thanks for reraising the issue. We're aware there's still demand for simplifying this. The reason the first PR was reverted was a lack of review, and failing to meet the original criteria we'd set to make this change.

When we first discussed this in #4392, we'd wanted a pretty rigorous testing to ensure we're not introducing regressions. We have the start of some of that but we'll need to expand it more before we're comfortable merging.

@nateprewitt Is anyone currently working on said unit tests? I can take a crack at them otherwise.

@theGOTOguy
Copy link
Contributor

I know this PR is quite stale by now, but it would be really nice if requests would finally use urllib3's built-in Retry functionality for chunked requests.

However lacking the review of the original PR might have been, thousands of users of requests are independently inventing their own way to deal with this type of error: either dealing with the sporadic failure in another part of their system or rolling their own method of retrying, potentially requiring additional code paths that themselves have a maintenance and testing cost. I would argue that not having this solution in requests is doing more damage to code health downstream than accepting it until such a time that a regression is reported.

@nateprewitt
Copy link
Member

Hi @theGOTOguy, I think we all agree this would be a useful enhancement to Requests. The reason the original PR was reverted before release is the change wasn't validated in any way. I wrote up a number of tests for this previously to try to get this into the 2.26 release but switching this functionality in a non-major version will be breaking. We have exceptions that change, the actual chunking is slightly different, and there are likely other subtle changes that weren't caught in the initial check.

This is towards the top of the list of easy improvement we can make to Requests, but it's currently stuck with a dozen other changes that were originally slated for a "3.0" release. Given 3.0 is unlikely to see fruition, we have a larger issue to solve of how/if we introduce breaking changes into Requests, before we can consider merging this. We've briefly discussed following pip into a CalVer style system to allow for more regular breakages, but there are pretty limited resources to get that going right now.

@sigmavirus24
Copy link
Contributor

Also we only ever get "Shame on you for not having done this sooner with the ~5m of combined time the two of you have in any give month" or "I'd like to help!" with 0 follow-up. And then we spend most of that ~5m explaining (once again) that we want to make this change but need time because people don't engage properly with the project

@shadycuz
Copy link
Contributor

@nateprewitt @lmvlmv This PR can be closed. It's been updated and merged in #6226.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants