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

Probe and cache HTTP/2 support per-origin #3324

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Jan 28, 2024

Moved _LockedObject to urllib3._collections.

Adds an "HTTP/2 support" cache which does one of three things if HTTP/2 support is going to be requested from the origin:

  • Returns the cached value True or False
  • Returns None meaning the current connection is probing and needs to return back a result.
  • Blocks waiting for an active probe.

My thinking is that this HTTP/2 support probe cache can evolve into the HTTP/2 socket sharing mechanism. Need to implement probe cache resetting per test case, tests for the probe, and for when it's used by an HTTP/1.1 and HTTP/2 capable function.

Part of #3301

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! Great step in the right direction.

src/urllib3/connection.py Outdated Show resolved Hide resolved
@@ -221,11 +191,13 @@ def inject_into_urllib3() -> None:
urllib3.connection.HTTPSConnection = HTTP2Connection # type: ignore[misc]

# TODO: Offer 'http/1.1' as well, but for testing purposes this is handy.
urllib3.util.ALPN_PROTOCOLS = ["h2"]
Copy link
Member

Choose a reason for hiding this comment

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

Are you defining ALPN_PROTOCOLS outside of the ssl_ module as well in anticipation of h2c?

Copy link
Member Author

Choose a reason for hiding this comment

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

The ALPN_PROTOCOLS list in ssl_ is reexported to the urllib3.util module even before this change, so updating that list too. We should probably remove the one in utils?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, although that can be done in a followup PR.

src/urllib3/connection.py Outdated Show resolved Hide resolved
src/urllib3/connection.py Outdated Show resolved Hide resolved
src/urllib3/connection.py Outdated Show resolved Hide resolved
src/urllib3/connection.py Show resolved Hide resolved
src/urllib3/connection.py Show resolved Hide resolved
test/with_dummyserver/test_https.py Show resolved Hide resolved
pquentin
pquentin previously approved these changes Feb 19, 2024
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

This looks incredible, thanks! I really like the new direction, that I find much clearer, and that I think will make implement socket sharing easy, at least on the connection side.

LGTM if CI passes!

test/conftest.py Outdated Show resolved Hide resolved
src/urllib3/connection.py Outdated Show resolved Hide resolved
src/urllib3/connection.py Outdated Show resolved Hide resolved
src/urllib3/http2/probe.py Outdated Show resolved Hide resolved
src/urllib3/http2/probe.py Show resolved Hide resolved
src/urllib3/http2/probe.py Outdated Show resolved Hide resolved
@sethmlarson
Copy link
Member Author

sethmlarson commented Feb 20, 2024

Addressed your feedback @pquentin (thank you!) and added another test case for blocking timings.

pquentin
pquentin previously approved these changes Feb 20, 2024
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. The new test may be a bit too ambitious though. Maybe there's a way to make it work by having tracing callbacks in connection.py and make sure that we never have two threads connecting at the same time? Not sure.

src/urllib3/http2/probe.py Show resolved Hide resolved
test/with_dummyserver/test_https.py Outdated Show resolved Hide resolved
@pquentin
Copy link
Member

I'm still seeing failures in test_http2_probe_blocked_per_thread, with the times going up to 0.6 in this run: https://github.com/urllib3/urllib3/actions/runs/7984715792/job/21801981185?pr=3324

FAILED test/contrib/test_pyopenssl.py::TestHTTPS_TLSv1_2::test_http2_probe_blocked_per_thread - assert (1708495940.4867585 - 1708495939.9358299) < 0.5
FAILED test/contrib/test_pyopenssl.py::TestHTTPS_TLSv1_3::test_http2_probe_blocked_per_thread - assert (1708495955.7278147 - 1708495955.1609907) < 0.5
FAILED test/with_dummyserver/test_https.py::TestHTTPS_TLSv1_2::test_http2_probe_blocked_per_thread - assert (1708496024.1679983 - 1708496022.7132747) > (0.5 * (5 - 2))
FAILED test/with_dummyserver/test_https.py::TestHTTPS_TLSv1_3::test_http2_probe_blocked_per_thread - assert (1708496034.525668 - 1708496033.9327052) < 0.5

This continues showing that time-based tests are going to be fragile in CI. Is it still useful with a larger time, like say 1s instead of 0.5s?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants