Support both urllib3 v1 and v2 at the same time (fixes #688) #699
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #699 +/- ##
==========================================
+ Coverage 90.15% 90.25% +0.10%
==========================================
Files 27 27
Lines 1726 1745 +19
Branches 308 311 +3
==========================================
+ Hits 1556 1575 +19
Misses 135 135
Partials 35 35
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
hartwork
changed the title
|
Thanks a lot @hartwork, does this mean I should close my PR in favor of this one? Very weird to see 3.10 and 3.11 work, while the others don't :/ |
|
If there is code that is using the standard library Can you link directly to the tests and their CI failures? |
|
@vEpiphyte for all of CPython 3.7, 3.8, 3.9, pypy 3.8 the failing environments are these two:
They all fail at test I'll inline the CI traceback from 👉 Click me to unfold
|
|
Normally I would say the ConnectionResetError is probably coming from a SSL/TLS handshake issue; but the traceback shows it happens after the status line has been received? That seems very odd. |
@shifqu would that work for you? If so, it's one of the options. Alternatively, we could drop the tox changes in yours for a green CI, treat it as one then-complete building block, get it reviewed, merge it, rebase #699 onto latest |
|
@jairhenrique a was a bit surprised also 😄 Let me drop the commit and rebase onto latest |
hartwork
force-pushed
the
urllib3-hartwork
branch
2 times, most recently
from
5ea87dc to
ab3d8bf
Compare
|
@hartwork I move many tests to use mockbin when httpbin goes down and tests broken. 🤔 I don't know if this lib run an local service for tests. |
@jairhenrique if mockbin is more reliable, then that was and is an improvement.
https://github.com/kevin1024/pytest-httpbin has the answer to that. I believe if it was used everywhere in vcrpy testing, our CI should not behave flaky any more, hopefully including aiohttp. |
|
@pquentin any chance you could have a look at the trace in #699 (comment) ? If this works with Python >=3.10 but not with earlier versions, could it be this is a bug in urllib3 2.x.x? Here's an easy to-the-point way of getting to that very traceback locally: cd "$(mktemp -d)"
git clone --depth 1 --branch urllib3-hartwork https://github.com/kevin1024/vcrpy
cd vcrpy/
python3.8 -m venv venv
source venv/bin/activate
pip3 install tox
tox -e py38-urllib3-2 -- 'tests/integration/test_urllib3.py::test_post[https]'Similar for Python 3.7 or 3.9. |
|
I'll try to look at this in the next 24 hours, sorry for the silence - I was on vacation. |
|
@pquentin thank you! 👍 👍 |
|
This test works fine on older versions of Python with OpenSSL 1.1.1. However, it fails with OpenSSL 3.0 and older versions of Python, because CPython does not have great support for OpenSSL 3.0. See this mail thread or this Discuss thread for more details. That said, according to python/cpython#83001 (comment), OpenSSL 3 is supported with Python 3.9+. I'm suggesting the following:
|
|
Hi @pquentin thanks for looking into this! One thing I have trouble understanding is this: if Python 3.7/3.8(/3.9?) does not fully support OpenSSL 3, why is our CI seemingly fine with OpenSSL 3 when using urllib3 1.x.x but not 2.x.x? Any ideas? |
|
Btw here is what I found regarding the version of OpenSSL for
Does not explain anything of what we have seen but could explain things we see later, potentially. |
|
@pquentin I downgraded CI to Ubuntu 20.04 with OpenSSL 1.1.1 now for Python 3.7/3.8/3.9 on another branch to find that:
|
|
Looks like I may have jumped the gun on the release of v4.3.0 - had some folks pinging me saying master was good so I released today but I think the bug may still exist on master? |
|
Hi @kevin1024 I would expect no damage from doing that release but the urllib3 situation remains mostly unchanged (compared to 4.2.1), yes. |
Since urllib3 v2 the re-export of connection.HTTPConnection in urllib3.connectionpool was removed. In this commit we use urllib3.connection where needed. Some references to connectionpool.HTTPConnection are still there for backward compatibility. Closes #688
In #690 a quick fix was introduced to get a green ci, this change should no longer be required.
boto3 depends on botocore which in turn depends on urllib3.
.. because it turned out broken for Python <3.10.
hartwork
force-pushed
the
urllib3-hartwork
branch
from
ab3d8bf to
52efffb
Compare
|
I have added runtime dependency |
|
Seems better than the current broken state. +1 from me |
|
@shifqu @vEpiphyte @pquentin are there any concerns about merging this pull request as is? |
|
For me it would be fine; since this is really a "stop the bleeding" problem. For people who are an older python install and want to use a newer urllib3 it is hard. I would wager much of that problem would be due to transitive requests -> urllib3 dependencies and older versions of pip which cannot resolve the dependencies appropriately; but I don't think we can account for every project which has a poor version constraint or running older and older code. This is what tagged release are for though; users with older/constrained systems can install specific versions of things. At some point there is going to be a breaking change due to this; and attempting to appease all possible configurations simply isn't something for unpaid FOSS volunteers to support. Hopefully in the not-too-distant-future when the openssl stuff is sorted we can replace this: with something like this: That let's us shed the baggage of the object named VerifiedHTTPSConnection and move to HTTPSConnection. |
This was referenced Oct 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi! 👋
This builds on the work by @shifqu in #697 literally. Fixes #688, happy to adjust.
I would like to note two things:
Looking forward to your review! 🍻
CC @jairhenrique @kevin1024 @pquentin @shifqu @vEpiphyte