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% 98.84% -0.19%
==========================================
Files 135 149 +14
Lines 50845 53289 +2444
Branches 2674 2863 +189
==========================================
+ Hits 50351 52675 +2324
- Misses 370 451 +81
- Partials 124 163 +39
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 performance
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _on_read_timeout(self) -> None: | ||
| exc = SocketTimeoutError("Timeout on reading data from socket") | ||
| self.set_exception(exc) |
There was a problem hiding this comment.
When an HTTP/2 peer stops sending data, the scheduled read timeout marks the protocol for closure but does not fail active streams' response futures or body readers. The request can therefore remain pending indefinitely instead of receiving SocketTimeoutError. Propagate the timeout through every active stream and terminate the connection consistently so awaiting callers are released.
Artifacts
Focused HTTP/2 timeout repro source
- Authored async repro creates an HTTP/2 stream over an idle connected transport and runs the scheduled read-timeout failure path; it demonstrates whether the awaiting request is released.
HTTP/2 request state before timeout
- Executed before-phase capture shows the created HTTP/2 request is pending before any timeout is started, establishing the comparison baseline.
HTTP/2 request state after scheduled timeout
- Executed after-phase capture shows the callback constructs SocketTimeoutError and closes the protocol state while the stream future and request remain pending, confirming the stranded-request defect.
Repeated HTTP/2 timeout repro output
- A second executed after-phase run produced the same pending stream and request state after the scheduled timeout, confirming the result is reproducible.
Existing focused HTTP/2 timeout test run
- Executed focused existing HTTP/2 timeout tests with the repository warning filter disabled; both pass while missing request-future propagation coverage.
There was a problem hiding this comment.
The timeout errors triggers at ClientResponse.start so I don't think that function is ever called. I can't reproduce the bug.
There was a problem hiding this comment.
That can be true if your test is hitting the outer request timeout, but it does not show that the HTTP/2 socket-read timeout is propagated.
ClientResponse.start() is the waiter here: aiohttp/client_reqrep.py:529 awaits protocol.read_stream(...), which awaits the stream's response_future. The timeout callback at aiohttp/http2/connection.py:779-781 runs independently and calls self.set_exception(exc). However, the HTTP/2 override of set_exception() only sets _should_close and drops the timer; it does not fail the stream's response_future, fail its body_reader, or close the transport.
Therefore:
- If response headers never arrive,
ClientResponse.start()remains pending unless an outer timeout cancels it. - If headers arrive but the body stalls,
start()has already returned and a body read remains pending. - An outer
ClientTimeoutcan produce the timeout observed atstart(), masking the missingSocketTimeoutErrorpropagation.
A focused reproduction needs an HTTP/2 server that accepts the request and then sends neither response headers nor body data, with the outer request timeout disabled or set longer than the protocol read timeout. The timeout callback does not need to be called by ClientResponse.start() for this issue to occur.
There was a problem hiding this comment.
Important
_on_read_timeout reachable. protocol.start_timeout() runs at client_reqrep.py:1025 (bodyless path) and client_reqrep.py:1551 (after write_eof). Both operate on conn.protocol, which is Http2Protocol here. _read_timeout comes from set_response_params(read_timeout=...).
Repro: ClientTimeout(sock_read=1), then hold stream open server-side without writing.
Two problems when it fires. _cancel_streams(0) kills every stream, not just stalled one. Caller sees RuntimeError("GOAWAY sent or received"), not the SocketTimeoutError you constructed — set_exception discards its argument.
Left non-blocking per your call.
There was a problem hiding this comment.
Thanks for the concrete call path and reproduction. You’re right: start_timeout() is reachable for HTTP/2, and the timeout currently has two observable defects—set_exception() drops the SocketTimeoutError, while _cancel_streams(0) causes the connection-level GOAWAY handling to fail every active stream with RuntimeError. The finding is valid; keeping it non-blocking is reasonable given the requested review priority.
| if flags & FlagData.PADDED: | ||
| # use fuzzy tests to | ||
| # verify if it's an error | ||
| pad_length = payload[0] |
There was a problem hiding this comment.
- Bug
- An authored runtime harness passed short RSTSTREAM, GOAWAY, and WINDOWUPDATE frames plus an empty PADDED DATA frame to the real HTTP/2 receiver. Each input raised struct.error or IndexError; the receiver sent RSTSTREAM but left the target stream present, open, and with its response future unresolved. Valid controls closed streams as expected, and non-UTF-8 GOAWAY debug data was accepted. This confirms malformed peer frames can strand active requests.
- Cause
- T-Rex reproduced this while running the changed behavior, but it did not return a separate root-cause sentence.
- Fix
- Update the changed code so this failing path is handled, then rerun the same T-Rex check to confirm it passes.
Artifacts
Focused HTTP/2 malformed-frame runtime repro script
- Authored and executed the direct Http2Connection harness for malformed and valid control frames; it isolates the receive paths used for the conclusion.
Malformed HTTP/2 frames leave streams pending
- Executed short RST_STREAM, GOAWAY, WINDOW_UPDATE, and empty padded DATA inputs; each causes an exception and RST_STREAM while the target stream remains open and unresolved.
Valid HTTP/2 control frames complete their expected handling
- Executed valid RST_STREAM, non-UTF-8 GOAWAY, and WINDOW_UPDATE controls; streams are closed where expected and non-UTF-8 GOAWAY handling succeeds.
HTTP/2 test suite passes despite malformed-frame gap
- Executed the complete HTTP/2 test suite with 123 passing tests; existing coverage does not detect the pending-stream behavior.
Relevant HTTP/2 receive-handler source lines
- Captured numbered source lines for frame dispatch, malformed payload parsing, and exception handling; they identify the root cause.
Artifact and diff validation status
- Captured artifact status and whitespace validation command output; no production source was modified for this review.
There was a problem hiding this comment.
Sending an empty "padded" frame violates the specification.
There was a problem hiding this comment.
You're right that an empty PADDED DATA frame is invalid; the review isn't suggesting that it should be accepted. The issue is that peer frames are untrusted, so malformed input still needs to be handled deterministically. Here, payload[0] raises before the padding validation runs; the generic handler then sends RST_STREAM but leaves the existing stream and its unresolved response future in place, stranding the request. The same concern applies to the unchecked fixed-size struct.unpack calls for RST_STREAM, GOAWAY, and WINDOW_UPDATE. These malformed-frame cases should produce the appropriate protocol error or close the affected stream rather than leaving active requests hanging.
There was a problem hiding this comment.
Agreed. RFC 9113 §6.1 makes this a connection error. Zero-length payload cannot carry Pad Length octet. Current _protocol_error() response correct.
Adjacent line needs work though. session_inbound_window -= len(data) uses unpadded length. §6.9.1 counts entire frame payload, padding included. Window drifts, transfers stall.
There was a problem hiding this comment.
Agreed. data excludes the Pad Length octet and padding, but HTTP/2 flow-control accounting uses the complete DATA frame payload length. Using len(data) undercounts padded frames, causing the receive window to drift and potentially delaying or stalling transfers.
| pad_length = payload[0] | |
| self.session_inbound_window -= len(payload) |
|
Pending tasks:
|
| try: | ||
| self._dispatch_frame(frame_type_val, flags, stream_id, payload) | ||
| except Exception as exc: | ||
| # we really don't want to swallow this exception | ||
| import traceback | ||
|
|
||
| logger.error("\n".join(traceback.format_exception(exc))) | ||
| logger.error( | ||
| "Critical error when dispatching frame: exception=%s. (frame_type=%s, flags=%d, stream_id=%d, payload=%s)", | ||
| str(exc), | ||
| FrameType(frame_type_val), | ||
| flags, | ||
| stream_id, | ||
| payload, | ||
| ) | ||
| self._send_rst_stream(stream_id, ErrorCode.PROTOCOL_ERROR) |
There was a problem hiding this comment.
Short RST_STREAM, GOAWAY, and WINDOW_UPDATE payloads, plus an empty PADDED DATA frame, raise during parsing. The generic exception handler sends an RST_STREAM but does not cancel the existing stream or resolve its response future. The runtime check observed all four inputs leave the stream registered with its response future pending. Validate payload lengths before unpacking and fail the affected stream or connection according to the frame scope.
Artifacts
Focused HTTP2 runtime harness source
- The authored narrow harness constructs HTTP/2 frames and exercises gzip expansion, pre-header data, malformed frames, and read timeout behavior; it is the executable source used for the observed results.
Focused HTTP2 runtime observed output
- Executed `/home/user/repo/venv/bin/python trex-artifacts/http2-focused-runtime.py` in `/home/user/repo` with exit code 0; it shows truncation at 262144 bytes, uncapped buffering, unresolved malformed-frame streams, and unresolved timeout waiters.
| def _on_read_timeout(self) -> None: | ||
| exc = SocketTimeoutError("Timeout on reading data from socket") | ||
| self.set_exception(exc) |
There was a problem hiding this comment.
_on_read_timeout() invokes the HTTP/2 override of set_exception(), but that override only marks the protocol for closure and drops the timer. It does not fail response futures, body-reader waiters, or the transport. The runtime check observed should_close=True while both an active response future and body read remained pending. Cancel all active and queued streams with SocketTimeoutError and close the connection when this timer fires.
Artifacts
Focused HTTP2 runtime harness source
- The authored narrow harness constructs HTTP/2 frames and exercises gzip expansion, pre-header data, malformed frames, and read timeout behavior; it is the executable source used for the observed results.
Focused HTTP2 runtime observed output
- Executed `/home/user/repo/venv/bin/python trex-artifacts/http2-focused-runtime.py` in `/home/user/repo` with exit code 0; it shows truncation at 262144 bytes, uncapped buffering, unresolved malformed-frame streams, and unresolved timeout waiters.
Yeah, sounds like a project for another day. Sounds like this is almost ready, once the next patch release is done, I'll try and loop back to review. |
|
Oh, and the fuzzers have been merged, so edit or add fuzzers there. The CI part will come once oss-fuzz update their end. |
Looks simple enough. I will try to adapt the tests. |
| if len(self._frame_buffer) < FRAME_HEADER_LENGTH + length: | ||
| break # incomplete frame; wait for more data | ||
|
|
||
| payload = bytes( | ||
| self._frame_buffer[FRAME_HEADER_LENGTH : FRAME_HEADER_LENGTH + length] |
There was a problem hiding this comment.
data_received() trusts the peer-controlled 24-bit length until that many bytes have accumulated in _frame_buffer, then creates a second payload-sized bytes copy. The receiver accepted and dispatched a 131,072-byte DATA frame while the local MAX_FRAME_SIZE was 16,384 bytes. A peer can repeatedly send frames up to the wire-format limit and cause avoidable memory and CPU exhaustion. Reject lengths above local_settings[Setting.MAX_FRAME_SIZE] immediately after parsing the frame header, before retaining the declared payload.
Artifacts
Focused HTTP/2 oversized-frame runtime harness source
- Authored harness loads the repository parser and sends an oversized DATA frame in network-sized chunks, with a guarded comparison showing the required early rejection.
Current parser accepts and copies an oversized HTTP/2 frame
- Executed current-code capture shows a 131,072-byte peer frame accumulating beyond the 16,384-byte limit and being dispatched as a copied bytes payload, proving the issue.
Guarded comparison rejects the oversized HTTP/2 frame at its header
- Executed test-only guarded comparison rejects the same 131,072-byte declaration while retaining no frame bytes, showing the expected enforcement point.
| if self.inbound_window < self._inbound_window_initial // 2: | ||
| increment = self._inbound_window_initial - self.inbound_window | ||
| self.inbound_window = self._inbound_window_initial | ||
| self.conn._send_window_update(self.stream_id, increment) | ||
|
|
||
| def receive_data(self, data: bytes, end_stream: bool, limit: int = 0) -> None: | ||
| """Process incoming DATA frame payload.""" | ||
| self.inbound_window -= len(data) | ||
| # the second time we have to pass b"" to the decompressor | ||
| # to it will end up reading two times `limit` | ||
| limit = (limit or MAX_DECOMPRESS_SIZE) // 2 | ||
|
|
||
| # --- stream-level flow control refill --- | ||
| self.maybe_reset_window() | ||
|
|
||
| if not self._headers_received: | ||
| # Buffer until we know the content-encoding. | ||
| if len(self._pending_data) + len(data) > limit: | ||
| msg = "Received too much data before headers." | ||
| logging.warning(msg) | ||
| self.conn._send_rst_stream(self.stream_id, ErrorCode.INTERNAL_ERROR) | ||
| return | ||
| self._pending_data.extend(data) | ||
| else: | ||
| # Feed data to the decompressor or directly to the reader. | ||
| if self.decompressor is not None: | ||
| try: | ||
| self.decompressor.feed_data(data) |
There was a problem hiding this comment.
receive_data() restores the stream receive window before placing DATA into body_reader, while HTTP/2 pause_reading() is a no-op. Once an application stops consuming a response, the peer continues receiving WINDOW_UPDATE credit despite the reader exceeding its high-water mark. The executed stress case buffered 2 MiB while the configured high-water mark was 131,070 bytes. Tie stream WINDOW_UPDATE messages to application consumption, or withhold credit while the reader is over its high-water mark.
Artifacts
Authored harness for unread HTTP/2 response buffering
- Loads and executes the repository's production HTTP/2 Stream implementation with local dependency stubs, then sends headers and unread DATA frames; it demonstrates the missing effective backpressure.
Baseline unread HTTP/2 response buffer run
- Executed two 32 KiB DATA frames without consuming the body; 64 KiB remains below the 131,070-byte high-water mark while one stream credit update is already sent.
Stress unread HTTP/2 response buffer run
- Executed 64 unread 32 KiB DATA frames; the buffer reaches 2 MiB, pause is requested 61 times without pausing, and stream credit is replenished for all received data — confirming unbounded growth.
| last_stream_id, error_code = struct.unpack("!I I", payload[:8]) | ||
| extra = payload[8:] | ||
| self._last_stream_id = last_stream_id | ||
| self._error_code = error_code | ||
| logger.info( | ||
| "GOAWAY received: last_stream=%d, error=%d, extra=%s", | ||
| last_stream_id, | ||
| error_code, | ||
| extra.decode(errors="replace"), | ||
| ) | ||
|
|
||
| self._cancel_streams(last_stream_id) |
There was a problem hiding this comment.
A seven-byte GOAWAY raises while unpacking its required eight-byte prefix. The generic receiver recovery then sends RST_STREAM for GOAWAY's connection-level stream ID 0, which does not close active streams or resolve their response futures. The receiver harness left stream 1 registered with its response waiter pending. Validate GOAWAY's payload length and close the connection while failing all active and queued streams when a malformed connection-level frame arrives.
Artifacts
HTTP/2 malformed-frame active-waiter receiver harness
- Creates a real HTTP/2 protocol and active stream, injects each requested malformed wire frame, and records waiter, stream, and transport state; it directly exercises the receiver.
Active waiters before malformed HTTP/2 frame delivery
- Captured baseline harness execution with each active response waiter pending before any malformed frame is delivered; it establishes the comparison state.
Active waiters after malformed HTTP/2 frame delivery
- Captured receiver execution after short RST_STREAM, short GOAWAY, short WINDOW_UPDATE, and empty PADDED DATA delivery; it shows only short GOAWAY leaves its active waiter pending.
| def _on_read_timeout(self) -> None: | ||
| exc = SocketTimeoutError("Timeout on reading data from socket") | ||
| self.set_exception(exc) | ||
| # cancel all | ||
| if self._connection is not None: | ||
| self._connection._cancel_streams(0) |
There was a problem hiding this comment.
_on_read_timeout() creates SocketTimeoutError, but set_exception() only marks the protocol for closure. _cancel_streams(0) subsequently fails every active stream with RuntimeError("GOAWAY sent or received"). The executed harness observed both pending response and body waiters receive that RuntimeError instead of the socket timeout. Pass the constructed SocketTimeoutError through the stream cancellation path.
Artifacts
Focused HTTP/2 timeout waiter harness source
- Authored asyncio harness creates a real HTTP/2 protocol and active response/body waiters, then compares the timeout setup with the production callback; it is the executable reproduction.
Before capture with active HTTP/2 waiters and constructed socket timeout
- Executed `python trex-artifacts/http2-timeout-waiter-harness.py --mode before` in `/home/user/repo`; it shows both waiters remain active when the socket-timeout exception is constructed.
After capture showing production timeout callback masks socket timeout
- Executed `python trex-artifacts/http2-timeout-waiter-harness.py --mode after` in `/home/user/repo`; it shows both active waiters receive GOAWAY RuntimeError after the real callback, verifying the masking bug.
Runtime dependency installation log
- Executed `python -m pip install -r requirements/runtime-deps.txt` in `/home/user/repo`; it shows the harness runtime dependencies were available.
| except BaseException: | ||
| # backwards compatibility | ||
| if alpn_protocol == "h2": | ||
| stream = await conn.protocol.create_stream() # type: ignore[attr-defined] |
There was a problem hiding this comment.
The HTTP/2 stream is created before request response parameters are applied, so it snapshots the protocol's previous auto_decompress=True value. The frame-level execution requested auto_decompress=False, but the stream still decompressed a 76-byte gzip response into its 2,115-byte raw body. Apply response parameters before creating the stream, or pass the request-specific decompression setting directly to it.
| self.decompressor.feed_data(data) | ||
| more = self.decompressor.feed_data(b"") | ||
| if more is True: | ||
| msg = "Overflow detected when decompressing data" | ||
| raise ContentEncodingError(msg) | ||
| except ContentEncodingError as exc: | ||
| self.body_reader.set_exception(exc) | ||
| self.conn._send_rst_stream(self.stream_id, ErrorCode.INTERNAL_ERROR) | ||
| return | ||
| else: | ||
| self.body_reader.feed_data(data) | ||
|
|
||
| if end_stream: | ||
| self.close() |
There was a problem hiding this comment.
A compressed final DATA frame can leave output pending after DeflateBuffer.feed_data(). The stream finalizes immediately after its limited drain attempt, and feed_eof() requires all decompressor output to have been drained. Ensure final DATA processing drains pending decoder output before EOF and converts any finalization failure into a body-reader exception.
| self.decompressor.feed_data(data) | ||
| more = self.decompressor.feed_data(b"") |
There was a problem hiding this comment.
HTTP/2 response data is decompressed synchronously in the receive callback. A highly compressible peer payload can require substantial CPU and output allocation before the event loop returns to other connections. Apply a practical decompression output limit and avoid processing an unbounded decompression workload synchronously on the receive path.
Current diff drops Remaining divergences worth folding into that refactor: |
|
|
|
We are going to skip expect100 as well. The main use-case seems to be asking for permission to send large payloads. |
This substitutes the environment variable used in the draft version.
I also moved the optional dependency (hpack) to aiohttp[http2]
psf-chronographer
Bot
added
the
bot:chronographer:provided
| stream = await conn.protocol.create_stream() # type: ignore[attr-defined] | ||
| req.stream_id = stream.stream_id | ||
| # release again to clear the protocol from _acquired if required | ||
| connector._release(conn._key, conn.protocol, should_close=False) | ||
| conn.protocol.set_response_params(**req._response_params) |
There was a problem hiding this comment.
create_stream() snapshots protocol._auto_decompress before this request applies its response parameters. On a shared HTTP/2 connection, requests with opposite auto_decompress values therefore receive the previous request's policy: a caller requesting raw gzip bytes can receive decoded bytes, while a caller requesting decoding can receive encoded bytes. Apply request response parameters before creating the stream, or pass the setting directly to stream construction instead of retaining request-scoped state on the shared protocol.
Artifacts
HTTP/2 gzip request policy harness
- Authored request-ordering harness that recreates the HTTP/2 client creation order and compares returned gzip bodies with each request's configured policy.
Current HTTP/2 gzip policy leakage result
- Executed policy harness using current create-stream-before-set-parameters ordering; it shows bodies use stale or another request's auto-decompression policy.
| self.maybe_reset_window() | ||
|
|
||
| if not self._headers_received: | ||
| # Buffer until we know the content-encoding. | ||
| if len(self._pending_data) + len(data) > limit: | ||
| msg = "Received too much data before headers." | ||
| logging.warning(msg) | ||
| self.conn._send_rst_stream(self.stream_id, ErrorCode.INTERNAL_ERROR) | ||
| return | ||
| self._pending_data.extend(data) | ||
| else: | ||
| # Feed data to the decompressor or directly to the reader. | ||
| if self.decompressor is not None: | ||
| try: | ||
| self.decompressor.feed_data(data) | ||
| more = self.decompressor.feed_data(b"") | ||
| if more is True: | ||
| msg = "Overflow detected when decompressing data" | ||
| raise ContentEncodingError(msg) | ||
| except ContentEncodingError as exc: | ||
| self.body_reader.set_exception(exc) | ||
| self.conn._send_rst_stream(self.stream_id, ErrorCode.INTERNAL_ERROR) | ||
| return | ||
| else: | ||
| self.body_reader.feed_data(data) |
There was a problem hiding this comment.
The stream restores its receive window before delivering DATA to body_reader, without considering reader occupancy. Because HTTP/2 pause_reading() is a no-op, a peer can continue sending an application-unread response beyond the reader high-water mark. Tie WINDOW_UPDATE credit to application consumption, or withhold credit while the reader is over its bounded high-water mark.
Artifacts
Focused HTTP/2 root recheck harness
- Authored and executed harness that injects HTTP/2 frames and stream data through the current connection and stream implementations, exercising all nine claimed paths.
Current HTTP/2 root recheck runtime results
- Executed current-code frame and stream harness showing buffer growth, policy staleness, malformed-frame waiter behavior, synchronous decompression, timeout cancellation, pre-header cap, final gzip output, and oversize rejection.
Focused HTTP/2 root recheck harness
- Authored and executed harness that injects HTTP/2 frames and stream data through the current connection and stream implementations, exercising all nine claimed paths.
Current HTTP/2 root recheck runtime results
- Executed current-code frame and stream harness showing buffer growth, policy staleness, malformed-frame waiter behavior, synchronous decompression, timeout cancellation, pre-header cap, final gzip output, and oversize rejection.
| last_stream_id, error_code = struct.unpack("!I I", payload[:8]) | ||
| extra = payload[8:] | ||
| self._last_stream_id = last_stream_id | ||
| self._error_code = error_code | ||
| logger.info( | ||
| "GOAWAY received: last_stream=%d, error=%d, extra=%s", | ||
| last_stream_id, | ||
| error_code, | ||
| extra.decode(errors="replace"), | ||
| ) | ||
|
|
||
| self._cancel_streams(last_stream_id) |
There was a problem hiding this comment.
A short GOAWAY payload reaches struct.unpack() without validating its required eight-byte prefix. The generic exception path then sends RST_STREAM for GOAWAY's stream ID 0, which does not close active streams or resolve their response futures. Reject malformed connection-level frames as connection errors and fail all active and queued streams rather than emitting RST_STREAM(0).
Artifacts
Focused HTTP/2 root recheck harness
- Authored and executed harness that injects HTTP/2 frames and stream data through the current connection and stream implementations, exercising all nine claimed paths.
Current HTTP/2 root recheck runtime results
- Executed current-code frame and stream harness showing buffer growth, policy staleness, malformed-frame waiter behavior, synchronous decompression, timeout cancellation, pre-header cap, final gzip output, and oversize rejection.
| self.decompressor.feed_data(data) | ||
| more = self.decompressor.feed_data(b"") | ||
| if more is True: | ||
| msg = "Overflow detected when decompressing data" | ||
| raise ContentEncodingError(msg) |
There was a problem hiding this comment.
DATA processing calls the synchronous decompressor directly with a default expansion limit of 2^31-1 bytes. A highly compressible peer payload can allocate a large decoded body and block unrelated event-loop work before control returns. Enforce a practical expansion budget and process decompression in bounded work units that preserve event-loop responsiveness.
Artifacts
Focused HTTP/2 root recheck harness
- Authored and executed harness that injects HTTP/2 frames and stream data through the current connection and stream implementations, exercising all nine claimed paths.
Current HTTP/2 root recheck runtime results
- Executed current-code frame and stream harness showing buffer growth, policy staleness, malformed-frame waiter behavior, synchronous decompression, timeout cancellation, pre-header cap, final gzip output, and oversize rejection.
Important Third item not done. |
Reasonable scope call. Two follow-ups. Guard at |
|
PR Review — Add support for HTTP/2A substantial, genuinely well-built HTTP/2 core — but the packaging change breaks Real progress since the last round, and worth naming specifically:
Blocking:
Non-blocking but worth doing: ✅ Resolved since last review (13)Previously-flagged issues verified fixed
🔴 Blocking
1. `hpack` is declared optional but imported unconditionally — `import aiohttp` breaks without it
|
|
Now that users can opt-in via |
Depends how much is working. If it breaks some functionality, we have no guarantee that they don't have the h2 dependency already installed as part of their project. |
I suppose the safest option is to use both the optional dependency and the flag. |
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):
ProxiesStreamingEnsure all the high-level configuration/parameters work (or make sense for) with HTTP/2 as well(obsolete)expect100request streaming