Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/httpcore2/_async/test_http2.py">
<violation number="1" location="tests/httpcore2/_async/test_http2.py:267">
P2: This 100 KiB payload is smaller than the stream’s effective HTTP/2 flow-control window, so the test uploads completely before reading the early response. Use a payload larger than the 16 MiB window (and update the analogous cases) so `send_body()` must read the response while blocked.</violation>
</file>
<file name="src/httpcore2/httpcore2/_sync/http2.py">
<violation number="1" location="src/httpcore2/httpcore2/_sync/http2.py:609">
P1: `reset_stream` mutates the shared h2 state without `_write_lock`, while other streams can send data or process inbound frames concurrently. Serialize this reset and its resulting flush with the connection’s write-side h2 operations to prevent concurrent state transitions and malformed output.
(Based on your team's feedback about serializing inbound HTTP/2 state processing.)</violation>
</file>
<file name="src/httpcore2/httpcore2/_async/http2.py">
<violation number="1" location="src/httpcore2/httpcore2/_async/http2.py:481">
P2: `send_body` only notices an early response when flow control forces `_wait_for_outgoing_flow()` to read; the stream window is expanded to 16 MiB, so normal smaller uploads send `END_STREAM` before the response is observed. Read response events while uploading, or coordinate upload and response processing concurrently, before ending the request stream.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| h2_closed = self._h2_state.state_machine.state == h2.connection.ConnectionState.CLOSED | ||
| if not connection._connection_error and not h2_closed: | ||
| try: | ||
| self._h2_state.reset_stream(self._stream_id, h2.errors.ErrorCodes.CANCEL) |
There was a problem hiding this comment.
P1: reset_stream mutates the shared h2 state without _write_lock, while other streams can send data or process inbound frames concurrently. Serialize this reset and its resulting flush with the connection’s write-side h2 operations to prevent concurrent state transitions and malformed output.
(Based on your team's feedback about serializing inbound HTTP/2 state processing.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/httpcore2/httpcore2/_sync/http2.py, line 609:
<comment>`reset_stream` mutates the shared h2 state without `_write_lock`, while other streams can send data or process inbound frames concurrently. Serialize this reset and its resulting flush with the connection’s write-side h2 operations to prevent concurrent state transitions and malformed output.
(Based on your team's feedback about serializing inbound HTTP/2 state processing.) </comment>
<file context>
@@ -548,33 +404,216 @@ def __exit__(
+ h2_closed = self._h2_state.state_machine.state == h2.connection.ConnectionState.CLOSED
+ if not connection._connection_error and not h2_closed:
+ try:
+ self._h2_state.reset_stream(self._stream_id, h2.errors.ErrorCodes.CANCEL)
+ except h2.exceptions.StreamClosedError:
+ pass
</file context>
There was a problem hiding this comment.
Not changing here. Every h2 mutation in this file (send_headers, send_data, end_stream, acknowledge_received_data) runs without _write_lock today; serialising h2 state access across threads is the scope of #1153, and this reset follows the existing pattern until then.
| ] | ||
| ) | ||
| async with httpcore2.AsyncHTTP2Connection(origin=origin, stream=stream) as conn: | ||
| response = await conn.request("POST", "https://example.com/", content=b"x" * 100_000) |
There was a problem hiding this comment.
P2: This 100 KiB payload is smaller than the stream’s effective HTTP/2 flow-control window, so the test uploads completely before reading the early response. Use a payload larger than the 16 MiB window (and update the analogous cases) so send_body() must read the response while blocked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/httpcore2/_async/test_http2.py, line 267:
<comment>This 100 KiB payload is smaller than the stream’s effective HTTP/2 flow-control window, so the test uploads completely before reading the early response. Use a payload larger than the 16 MiB window (and update the analogous cases) so `send_body()` must read the response while blocked.</comment>
<file context>
@@ -188,6 +190,190 @@ async def test_http2_connection_with_rst_stream() -> None:
+ ]
+ )
+ async with httpcore2.AsyncHTTP2Connection(origin=origin, stream=stream) as conn:
+ response = await conn.request("POST", "https://example.com/", content=b"x" * 100_000)
+ assert response.status == 413
+
</file context>
There was a problem hiding this comment.
Not changing. increment_flow_control_window(2**24, stream_id=...) raises our inbound window (what the server may send us). Our outbound window is the server's INITIAL_WINDOW_SIZE, 65,535 by default, which the mock never raises, so a 100 KB upload blocks at 64 KiB and reads. The three tests fail on main for exactly that reason.
| if isinstance(last_event, h2.events.StreamEnded): | ||
| self._h2_state.reset_stream(self._stream_id, h2.errors.ErrorCodes.NO_ERROR) | ||
| await self._connection._write_outgoing_data(self._request) | ||
| elif not isinstance(last_event, h2.events.StreamReset): |
There was a problem hiding this comment.
P2: send_body only notices an early response when flow control forces _wait_for_outgoing_flow() to read; the stream window is expanded to 16 MiB, so normal smaller uploads send END_STREAM before the response is observed. Read response events while uploading, or coordinate upload and response processing concurrently, before ending the request stream.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/httpcore2/httpcore2/_async/http2.py, line 481:
<comment>`send_body` only notices an early response when flow control forces `_wait_for_outgoing_flow()` to read; the stream window is expanded to 16 MiB, so normal smaller uploads send `END_STREAM` before the response is observed. Read response events while uploading, or coordinate upload and response processing concurrently, before ending the request stream.</comment>
<file context>
@@ -548,33 +404,216 @@ async def __aexit__(
+ if isinstance(last_event, h2.events.StreamEnded):
+ self._h2_state.reset_stream(self._stream_id, h2.errors.ErrorCodes.NO_ERROR)
+ await self._connection._write_outgoing_data(self._request)
+ elif not isinstance(last_event, h2.events.StreamReset):
+ self._h2_state.end_stream(self._stream_id)
+ await self._connection._write_outgoing_data(self._request)
</file context>
There was a problem hiding this comment.
Correct, and deliberate for this PR: without a reader task, an early response is only observed when something reads, and the upload path reads when flow control blocks it. Reading while uploading is the leader/follower work tracked in #849; this PR gives the stream the state that work needs.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
| else: | ||
| if flush: | ||
| try: | ||
| await connection._write_outgoing_data(self._request) |
There was a problem hiding this comment.
🟠 High _async/http2.py:620
Cancelling application work inside a streaming-response context can leave cleanup blocked indefinitely, keeping the stream permit and pool request held. PoolByteStream.aclose() calls this flush=True path under AsyncShieldCancellation, where waiting for _write_lock is unbounded and a socket write has no deadline when no write timeout is configured. Avoid I/O in the shielded close path or bound the whole cleanup, including lock acquisition.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/httpcore2/httpcore2/_async/http2.py around line 620:
Cancelling application work inside a streaming-response context can leave cleanup blocked indefinitely, keeping the stream permit and pool request held. `PoolByteStream.aclose()` calls this `flush=True` path under `AsyncShieldCancellation`, where waiting for `_write_lock` is unbounded and a socket write has no deadline when no write timeout is configured. Avoid I/O in the shielded close path or bound the whole cleanup, including lock acquisition.
| """ | ||
| if self._closed: | ||
| return | ||
| self._closed = True |
There was a problem hiding this comment.
🟡 Medium _sync/http2.py:605
Closing a response retains all unread buffered body chunks as long as the Response is retained. _receive_events() stores DataReceived events in self.events, and _release() removes the stream from the connection without clearing that list; clear the queued events during release.
Also found in 1 other location(s)
src/httpcore2/httpcore2/_async/http2.py:626
Early close no longer frees buffered response data. Incoming
DataReceivedevents now live onAsyncHTTP2Stream.events, andResponse.streamretains that same stream object._release()removes the stream from the connection but never clearsself.events, so keeping a closed response also keeps every unread buffered body chunk alive. This is particularly costly with multiplexing, where another request can queue a large body for this response before it is closed. Previously deleting the connection's_events[stream_id]released that buffer independently of the response's lifetime. Clear the queued events during release so closed responses do not retain potentially megabytes of discarded body data.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/httpcore2/httpcore2/_sync/http2.py around line 605:
Closing a response retains all unread buffered body chunks as long as the `Response` is retained. `_receive_events()` stores `DataReceived` events in `self.events`, and `_release()` removes the stream from the connection without clearing that list; clear the queued events during release.
Also found in 1 other location(s):
- src/httpcore2/httpcore2/_async/http2.py:626 -- Early close no longer frees buffered response data. Incoming `DataReceived` events now live on `AsyncHTTP2Stream.events`, and `Response.stream` retains that same stream object. `_release()` removes the stream from the connection but never clears `self.events`, so keeping a closed response also keeps every unread buffered body chunk alive. This is particularly costly with multiplexing, where another request can queue a large body for this response before it is closed. Previously deleting the connection's `_events[stream_id]` released that buffer independently of the response's lifetime. Clear the queued events during release so closed responses do not retain potentially megabytes of discarded body data.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/httpcore2/_sync/test_http2.py">
<violation number="1" location="tests/httpcore2/_sync/test_http2.py:404">
P3: This test does not verify that the suspended body iterator was closed by the response: `response.close()` already makes the connection idle, and `body.close()` runs before the assertion. Assert the iterator’s closed behavior before explicitly closing it, or instrument the stream cleanup and verify the response close performs it exactly once.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| assert next(body) == b"Hello," | ||
| body.close() | ||
|
|
||
| assert conn.is_idle() |
There was a problem hiding this comment.
P3: This test does not verify that the suspended body iterator was closed by the response: response.close() already makes the connection idle, and body.close() runs before the assertion. Assert the iterator’s closed behavior before explicitly closing it, or instrument the stream cleanup and verify the response close performs it exactly once.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/httpcore2/_sync/test_http2.py, line 404:
<comment>This test does not verify that the suspended body iterator was closed by the response: `response.close()` already makes the connection idle, and `body.close()` runs before the assertion. Assert the iterator’s closed behavior before explicitly closing it, or instrument the stream cleanup and verify the response close performs it exactly once.</comment>
<file context>
@@ -375,6 +377,67 @@ def write(self, buffer: bytes, timeout: float | None = None) -> None:
+ assert next(body) == b"Hello,"
+ body.close()
+
+ assert conn.is_idle()
+
+
</file context>
Part of #849.
Why
Between
HEADERSgoing out and the stream closing, an HTTP/2 request's state is spread across locals inhandle_async_request(),self._events[stream_id],HTTP2ConnectionByteStream(the receive half only) and_response_closed(). Nothing represents the stream, and nothing owns closing it. Every stream-level behaviour, reacting to an early response, cancelling on early close, trailers, and eventually handing the stream out asnetwork_stream, has to be threaded through the connection as a special case.What
AsyncHTTP2Streamholds everything keyed on a stream id: the queued events, sending the request, waiting for its own flow control, iterating the response body, and closing itself.handle_async_request()opens a stream, drives it through the same three traced steps as before, and returns it as the response body. The connection keeps what is shared: theh2state, the read and write locks, stream admission, event dispatch, and the pool interface.HTTP2ConnectionByteStreamgoes away.Trace events, response extensions, locking order and the pool interface are unchanged. The one new piece of shared state is
_streams: dict[int, AsyncHTTP2Stream], replacing_events.The connection and the stream stay in one module on purpose: the stream uses the connection's locks and dispatch, and that is same-module access rather than a cross-module private API.
Behaviour changes
Two, both consequences of the stream owning its cleanup. Reviewed against the tests below.
Early responses. Closes #1294. A server that answers before an upload is complete, with or without
RST_STREAM(NO_ERROR), was ignored until the upload finished, which usually meant waiting for aWINDOW_UPDATEthat never came. The stream's send path stops when the server has ended the stream, returns the queued response, and closes the stream withRST_STREAM(NO_ERROR)if the server did not. RFC 9113 §8.1.Responses closed early. Closing a response before reading its body left the stream open in
h2and on the server: the server kept sending into our 16 MB window, the data was dropped unacknowledged, and the stream counted against the concurrent-stream limit (the stall in encode/httpcore#943). The stream now sendsRST_STREAM(CANCEL). Onceh2considers the stream closed it acknowledges late data by itself, so no further accounting is needed.The cancel frame is written when the response is closed deliberately. On failure or cancellation it is queued for the next write, because every cleanup path runs under a cancellation shield and a blocked socket there would turn a cancellation into a hang (
test_h2_timeout_during_requestguards this). A stream that never reachedsend_headers()is not reset at all: a refused reset would moveh2's connection state toCLOSED.Tests
Through
conn.request()andconn.stream()against scripted frames, each confirming the connection takes a following request:413+RST_STREAM(NO_ERROR)mid-upload returns413;413without a reset makes the client sendRST_STREAM(NO_ERROR)and noEND_STREAM;RST_STREAM(REFUSED_STREAM)mid-upload raisesRemoteProtocolError;RST_STREAM(CANCEL)and leaves the connection idle;tracehook) releases its permit and leaves the connection usable;test_http2_response_closed_twicecloses through the public API instead of the removed_response_closed().The early-response tests fail on
main. Harness numbers forc16/1m/upload/h2/batch,c16/1m/upload/h2,c64/1k/h2andc8/5mare unchanged againstmain.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.