Add support for HTTP/2 - #13039
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.02% 22.15% -76.87%
===========================================
Files 135 140 +5
Lines 50486 51402 +916
Branches 2650 2776 +126
===========================================
- Hits 49993 11388 -38605
- Misses 370 39488 +39118
- Partials 123 526 +403
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 degrade performance by 8.18%
|
|
|
||
| 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 |
|
h2c is widely unsupported anyway (no major browser supports it), so we don't need to focus on that. If it's easy to add later, we can do so, but let's try not to expand the scope of this current work. |
|
|
Confidence Score: 0/5The PR is not safe to merge because remote HTTP/2 peers can exhaust client memory or break frame processing while configured response behavior is also ignored. Response bodies are buffered and flow-control credit is replenished without application consumption, decompression has no output bound, malformed frames can escape the protocol callback and strand active streams, and HTTP/2 responses disregard auto_decompress configuration. Files Needing Attention: aiohttp/http2/stream.py, aiohttp/http2/response.py, aiohttp/http2/connection.py, aiohttp/client.py
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def receive_data(self, data: bytes, end_stream: bool) -> None: | ||
| """Process incoming DATA frame payload.""" | ||
| self.inbound_window -= len(data) | ||
| self.response_data.extend(data) | ||
|
|
||
| # --- stream-level flow control refill --- | ||
| if self.inbound_window < self._inbound_window_initial // 2: | ||
| increment = self._inbound_window_initial - self.inbound_window | ||
| self.inbound_window = self._inbound_window_initial | ||
| # Use the connection’s helper to send the WINDOW_UPDATE frame | ||
| self.conn._send_window_update(self.stream_id, increment) |
There was a problem hiding this comment.
When an HTTP/2 server sends a large or non-terminating response, this path appends every DATA payload while replenishing flow-control credit and does not return the response until END_STREAM, causing attacker-controlled memory growth with no opportunity for the caller to consume or release partial data.
How this was verified: The receive path continually extends response_data and restores both stream and connection windows before copying the complete buffer into the response future.
| encoding = self.headers.get(CONTENT_ENCODING, None) | ||
| if encoding in {"gzip", "deflate"}: | ||
| comp = ZLibDecompressor(encoding=encoding) | ||
| body = comp.decompress_sync(body) |
There was a problem hiding this comment.
When a caller sets session-level or per-request auto_decompress=False, the HTTP/2 branch bypasses set_response_params and this constructor still decompresses gzip or deflate bodies unconditionally, causing callers to receive altered bytes instead of the encoded representation they requested.
| def _handle_rst_stream_frame( | ||
| self, flags: int, stream_id: int, payload: bytes | ||
| ) -> None: | ||
| del flags # rst doesn't use flags | ||
|
|
||
| error_code = struct.unpack("!I", payload)[0] |
There was a problem hiding this comment.
When an HTTP/2 peer sends a short RST_STREAM, GOAWAY, or WINDOW_UPDATE frame, the corresponding handler unpacks four or eight bytes without validating the payload length, causing struct.error to escape data_received, terminate the multiplexed connection, and fail every active request.
How this was verified: The dispatcher passes peer payloads directly to fixed-size struct.unpack calls without frame-specific length checks or an exception boundary.
| sslcontext.verify_mode = ssl.CERT_NONE | ||
| sslcontext.options |= ssl.OP_NO_COMPRESSION | ||
| sslcontext.set_default_verify_paths() | ||
| sslcontext.set_alpn_protocols(("http/1.1",)) | ||
|
|
||
| protocols = ["http/1.1"] | ||
| if os.getenv("AIOHTTP_ENABLE_EXPERIMENTAL_PROTOCOLS", False): | ||
| protocols += ["h2"] | ||
| sslcontext.set_alpn_protocols(tuple(protocols)) |
There was a problem hiding this comment.
The new environment-variable opt-in changes ALPN negotiation and introduces user-visible HTTP/2 behavior and limitations, but the PR adds no client reference or narrative documentation, leaving users without shipped guidance for enabling or evaluating the feature.
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!
| "propcache >= 0.2.0", | ||
| "typing_extensions >= 4.4 ; python_version < '3.13'", | ||
| "yarl >= 1.17.0, < 2.0", | ||
| "hpack >= 4.2.0" |
There was a problem hiding this comment.
This adds HTTP/2 client support and a mandatory hpack runtime dependency without the required CHANGES/{pr_or_issue}.feature.rst fragment, so generated release notes will omit the new capability and dependency change.
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!
|
If you want to skip the reviews for now, switch to a draft. |
| encoding = self.headers.get(CONTENT_ENCODING, None) | ||
| if encoding in {"gzip", "deflate"}: | ||
| comp = ZLibDecompressor(encoding=encoding) | ||
| body = comp.decompress_sync(body) |
There was a problem hiding this comment.
When an HTTP/2 server returns a highly compressed gzip or deflate body, this constructor decompresses the complete server-controlled payload synchronously without an output limit, causing memory exhaustion and blocking the event loop even if response buffering is made incremental.
How this was verified: decompress_sync is called on the complete body with its default unlimited output length.
| pos = 0 | ||
| if flags & FlagData.PADDED: | ||
| pad_length = payload[0] |
There was a problem hiding this comment.
When a peer sends an empty DATA frame with PADDED set, this handler reads payload[0] and raises IndexError; similarly, non-UTF-8 GOAWAY debug data raises UnicodeDecodeError. These exceptions escape the receive callback without closing the transport or resolving active response futures, leaving multiplexed requests hanging.
How this was verified: The handlers consume peer-controlled payloads without validation, and the receive path has no exception boundary before the only cleanup in connection_lost.
|
I noticed that, while |
HTTP/2 now integrates fully with the existing request-response machinery. Most changes are internal so the breaking changes no longer alter the user interface.
for more information, see https://pre-commit.ci
|
It seems #13152 is going to be unnecessary after all. Now that the solution is better integrated with the code I only need to implement/add:
By the way, I added web sockets to the missing features. The way the connection is initiated differs between versions. |
| import pytest | ||
| from hpack import Encoder | ||
|
|
||
| import aiohttp |
5 participants
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
AIOHTTP_ENABLE_EXPERIMENTAL_PROTOCOLS=1to allowh2negotiation via ALPN during the TLS handshake.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.
Backward compatibility
Opt-in via
AIOHTTP_ENABLE_EXPERIMENTAL_PROTOCOLS=1.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. That said, I make use of some black magic with
__getattr__to be able to conditionally switch protocols.Missing features (to the date):