Repository navigation
Add support for HTTP/2 - #13039
Add support for HTTP/2#13039Moist-Cat wants to merge 75 commits into
Conversation
This implementation is backwards compatible, functional, but still incomplete.
for more information, see https://pre-commit.ci
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| self._handler: Optional[asyncio.Protocol] = None | ||
|
|
||
| # ---- Transport callbacks forwarded to the real handler ---- | ||
| def connection_made(self, transport: asyncio.BaseTransport) -> None: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #13039 +/- ##
==========================================
- Coverage 99.12% 98.97% -0.15%
==========================================
Files 135 148 +13
Lines 53661 55932 +2271
Branches 2737 2894 +157
==========================================
+ Hits 53191 55360 +2169
- Misses 356 420 +64
- Partials 114 152 +38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Merging this PR will not alter performance1 warning
Comparing 
Footnotes
|
|
|
||
| try: | ||
| import sphinxcontrib.spelling # noqa | ||
| import sphinxcontrib.spelling |
It was necessary to add a semaphore to ensure the requests connect sequentially to the hosts and reuse connections when necessary. HTTP/2 uses a single connection per host.
for more information, see https://pre-commit.ci
|
I ran tests against remote servers (httpbin.org) to verify HTTP/2 indeed reduces latency. HTTP/2 Performance Test ResultsSystem Specs:
Test Configuration:
Batch Mean Latency (seconds)
Individual Request Latency Distribution
Statistical Analysis
A simple bar chart with the means (results vary because they are from a second test): We lose efficiency in CPU bound tasks (see #13039 (comment)) but I/O bound tasks are significantly faster. This is specially true for batch requests that require multiple TCP connections to the same host. |
|
I would like to know if the trade-offs (I/O vs CPU) are acceptable before writing the docs. |
HTTP/1.1 regression not inherent to h2. Caused by global |
Bigger blocker than the CPU/IO trade-off. h2 path returns |
|
|
|
Either inheriting from or using Regarding the To deal with |
for more information, see https://pre-commit.ci
|
|
||
| try: | ||
| from hpack import Decoder, Encoder | ||
| except ImportError: |
| try: | ||
| from aiohttp.http2.connection import Http2Connection, Http2Protocol | ||
| except ImportError: | ||
| http2 = None |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fallback_charset_resolver: _CharsetResolver = lambda r, b: "utf-8", | ||
| middlewares: Sequence[ClientMiddlewareType] = (), | ||
| ssl_shutdown_timeout: _SENTINEL | None | float = sentinel, | ||
| http2_enabled: bool = False, |
There was a problem hiding this comment.
Adding the public ClientSession(http2_enabled=...) API introduces HTTP/2 framing, multiplexing, flow-control, decompression, and resource-exhaustion risks, but THREAT_MODEL.md contains no HTTP/2 client coverage. The repository requires the threat model to be revised when a public API is added in client.py; this repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Keep flag. Default off still load-bearing. Current h2 path diverges from h1 in ways users would hit silently: response headers skip all h1 validation (CR/LF, invalid names, duplicate Content-Length), Also Two separate bugs make default-off leaky today, worth fixing either way: dispatch ignores Revisit after those land and the multiplexing lifecycle stabilizes. |
Agreed. Both, for now. Extra flag costs little. Reverting an auto-enabled protocol costs a release. One gap to close: |
Noted — fuzzer relocation left deferred per your call, not re-raised. One thing to carry across when porting: |
On
More serious: |
PR Review — Add support for HTTP/2A genuinely impressive HTTP/2 core with real progress since the last round — but the connection-lifecycle integration with the pool is broken in ways that kill in-flight requests, and the read path has no backpressure at all. What is clearly better this round, and verified in the tree:
Blocking:
Non-blocking, but worth doing: drop the Honouring earlier decisions: ✅ Resolved since last review (5)Previously-flagged issues verified fixed
🔴 Blocking
1. One failed h2 request tears down the shared connection for every other in-flight stream
|
|
I ported the fuzzers and found a few bugs:
Some of these are a bit "silly" but they show that the fuzzer is working. By the way, there is overlap between unit tests and the fuzzy tests so I'm wondering if we can somehow detect and prune them or at least include the fuzzy tests in the coverage to prevent contributors from writing unnecessary tests by hand in the future. |
|
Going to start looking at reviewing this soon. |
|
OK, I'm now reading through RFC 9113 and getting familiar with everything. First, the high level architecture. I feel like the current approach is likely to cause issues, and we should be treating streams as the focal point going forwards. So, I feel like we need an abstraction something along these lines (from a Claude suggestion): So, HTTP/1.1 is basically 1 HTTPStream per HttpConnection, HTTP/2|3 will have multiple. So, I'm feeling like we should probably have a PR that refactors the code to follow a shape like this, and then add a stacked PR on top that implement the h2 version, with all the tests and fuzzers from here included. Also, is there a reason the h2 library hasn't been used for any of the implementation? |
Yes, HTTP/1.1 can be seen as a particular case of HTTP/2. If we separate the logic in charge of the request/response exchange from the That said, I will see what I can do. |
|
I think it would be more beneficial to refactor the |
I can tell at a glance that the library is well written but the scope of the implementation isn't clear to me. I might be wrong but I believe dependencies should be replaceable and focused so I can't add add a dependency if I don't know what specific problem does it solve. |
Yeah, my concern is that the current code appears to already have several security vulnerabilities and I'm under the impression that resolving them will need significantly more code changes anyway. It's better that we refactor now to architect this better and have a cleaner approach for future maintenance that makes it easier to deal with future security fixes etc.
My understanding is that it effectively handles the HTTP/2 parsing, including connection and stream state. It's already used by httpx, for example. We can dig into it later and compare between your code and a potential h2 dependency and decide then if it's worth switching over or if we should just manage it here. On that note, I'd also point out that the same group also have hyperframe for working with HTTP/2 frames, and hpack for handling HTTP/2 headers (which you've already chosen to use). |
At a very brief glance, I get the impression h2 would replace the majority of code under http2/ currently. |
| try: | ||
| if fut.done() and not fut.cancelled(): | ||
| fut.exception() | ||
| except BaseException: |
| try: | ||
| if fut.done() and not fut.cancelled(): | ||
| fut.exception() | ||
| except BaseException: |
for more information, see https://pre-commit.ci
| host_id, req, task = pending.pop(cid) | ||
| try: | ||
| connection = task.result() | ||
| except BaseException as e: |
| try: | ||
| connection = task.result() | ||
| live[cid] = LiveConn(cid, host_id, connection, req) | ||
| except BaseException: |
| try: | ||
| connection = task.result() | ||
| live[cid] = LiveConn(cid, host_id, connection, req) | ||
| except BaseException: |
What do these changes do?
Add HTTP/2 client support.
Why
Faster I/O bound operations (e.g., many requests to the same host) via multiplexing (handling several streams/requests inside a single connection).
How
Use the environment variableVia theAIOHTTP_ENABLE_EXPERIMENTAL_PROTOCOLS=1to allowh2negotiation via ALPN during the TLS handshake.http2_enabledflag insideClientSession.ResponseHandlerwas substituted by a wrapper that conditionally switches protocols depending on the negotiated protocol.Semaphoreto avoid race conditions.This means opening many HTTP/1.1 connections in parallel is now slower because it's done sequentially. That said, to know if connections can be pooled or not it's only necessary to wait until the first connection is done. Once it's known whether the host supports HTTP/2 or not, the rest of the requests can be done in parallel so it's possible to mitigate this performance hit substantially.Solved!Backward compatibility
Opt-in via
http2_enabled.Testing
%95 coverage, benchmarks (%50 latency reduction for 99 requests, see below), and integration tests against real servers (~100).
Dependencies
hpack
Is it a substantial burden for the maintainers to support this?
Yes.
Related issue number
refs #5999
The implementation is self-contained, the changes to the current codebase are minimal and backwards compatible.
Missing features (to the date):
ProxiesResponse streamingEnsure all the high-level configuration/parameters work (or make sense for) with HTTP/2 as well(obsolete)expect100Request streamingWINDOW_UPDATEframes.Things that should be implemented/changed:
Support reusable connections
This means changing the way we handle all connections so the implementation should be done in a separate PR to avoid regressions.
Make HTTP/1.1 a particular case of HTTP/2
HTTP/1.1 connections can be seen as HTTP/2 connections that only allow a single stream. When we try to fit HTTP/2 connections inside routines that were built for HTTP/1.1 like we did here, the implementation ends up looking a bit awkward. (with "awkward" I mean that we have to add 1-2 additional if statements per class to pass the
stream_idto the HTTP/2 connector).