-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
There was a problem hiding this 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/http2.py
Outdated
@@ -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"] |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
80b97b9
to
b6b4ff8
Compare
Addressed your feedback @pquentin (thank you!) and added another test case for blocking timings. |
There was a problem hiding this 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.
I'm still seeing failures in
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? |
7034b9e
to
3271f2b
Compare
As far as I can tell, the two issues left are the flaky test and the coverage. I opened sethmlarson#15 to address the flaky test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged the main branch, fixing the conflict. Only need to fix two coverage issues.
src/urllib3/http2/probe.py
Outdated
raise ValueError( | ||
"Cannot reset HTTP/2 support for origin after value has been set." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not covered. Is this more of a defensive thing that can't happen if the probe is used as intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is defensive!
I opened sethmlarson#16 as a proposal to fix the remaining coverage issue. |
But don't add a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now! I do need a review for my own commits.
Your commits LGTM, let's merge this monster! |
Moved
_LockedObject
tourllib3._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:
True
orFalse
None
meaning the current connection is probing and needs to return back a result.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