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 urllib3 native chunking ability #6226

Merged
merged 4 commits into from
Apr 22, 2023
Merged

Conversation

shadycuz
Copy link
Contributor

@shadycuz shadycuz commented Sep 1, 2022

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 urllib3/urllib3#2649. The comments in that PR offer a lot of detail in the changes in urllib3 but this one provided a good overview urllib3/urllib3#2649 (comment).

@shadycuz
Copy link
Contributor Author

shadycuz commented Sep 1, 2022

@sethmlarson @nateprewitt Can you please approve the workflow. I'm curious if the SSL error I had locally still shows up in this pipeline.

@shadycuz
Copy link
Contributor Author

shadycuz commented Sep 1, 2022

All tests passed except for linting but it needs approval again 🙃

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, going to wait for @nateprewitt to merge.

@shadycuz
Copy link
Contributor Author

shadycuz commented Sep 1, 2022

@sethmlarson I have another linting error, I'll fix real quick

@shadycuz
Copy link
Contributor Author

shadycuz commented Sep 1, 2022

I ran all the checks locally this time 😊

$ 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

@theGOTOguy
Copy link
Contributor

theGOTOguy commented Sep 1, 2022

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.

@shadycuz
Copy link
Contributor Author

shadycuz commented Sep 2, 2022

@theGOTOguy is the bypassing of Retry something going on here in the request package or is this a bug in urllib3?

@theGOTOguy
Copy link
Contributor

theGOTOguy commented Sep 2, 2022

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.

@shadycuz
Copy link
Contributor Author

@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?

@sethmlarson
Copy link
Member

sethmlarson commented Sep 28, 2022

@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.

@sethmlarson
Copy link
Member

@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.

@nateprewitt nateprewitt added this to the 2.29.0 milestone Nov 16, 2022
@nateprewitt nateprewitt merged commit 26bea1e into psf:main Apr 22, 2023
@nateprewitt nateprewitt mentioned this pull request Apr 25, 2023
jaypikay pushed a commit to jaypikay/doxy that referenced this pull request Apr 27, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [requests](https://requests.readthedocs.io) ([source](https://github.com/psf/requests), [changelog](https://github.com/psf/requests/blob/master/HISTORY.md)) | dependencies | minor | `2.28.2` -> `2.29.0` |

---

### Release Notes

<details>
<summary>psf/requests</summary>

### [`v2.29.0`](https://github.com/psf/requests/blob/HEAD/HISTORY.md#&#8203;2290-2023-04-26)

[Compare Source](psf/requests@v2.28.2...v2.29.0)

**Improvements**

-   Requests now defers chunked requests to the urllib3 implementation to improve
    standardization. ([#&#8203;6226](psf/requests#6226))
-   Requests relaxes header component requirements to support bytes/str subclasses. ([#&#8203;6356](psf/requests#6356))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS42MS4wIiwidXBkYXRlZEluVmVyIjoiMzUuNjEuMCJ9-->

Co-authored-by: Renovate Bot <renovate@localhost.localdomain>
Reviewed-on: https://git.goatpr0n.de/public/doxy/pulls/5
Co-authored-by: renovate <renovate@noreply.localhost>
Co-committed-by: renovate <renovate@noreply.localhost>
matthewarmand pushed a commit to matthewarmand/requests that referenced this pull request May 10, 2023
* Fix psf#3844 
* Use urllib for chunked 

---------

Co-authored-by: James Pickering <james.pickering@xml-solutions.com>
Co-authored-by: Leon Verrall <LVerrall@slb.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants