Track connection pool state incrementally instead of scanning on every request - #1169
Track connection pool state incrementally instead of scanning on every request#1169Kludex wants to merge 5 commits into
Conversation
…y request Every request used to trigger two full passes over the pool, on enqueue and on release, each walking every connection through several state queries and probing every idle socket, so per-request cost grew with the number of pooled connections and throughput collapsed at high concurrency. The pool now keeps its state incrementally: connections and requests in insertion-ordered dicts, a FIFO of requests awaiting a connection, an index of idle HTTP/1.1 connections by origin (oldest first) for reuse and keepalive eviction, a per-origin set of sharable connections (HTTP/2, or not yet negotiated) and refcounted reservations. A pass only examines connections released since the previous pass and assigns queued requests, so the common case is O(1). Idle sockets are probed for a server close when a connection is about to be reused rather than on every pass, and keepalive expiry is swept only once the oldest idle connection may have expired. Origin gains a hash consistent with its equality so it can key the indexes, and a pool request creates its wakeup event only when it actually has to wait.
|
Docs preview: https://885d9135-httpx2-docs.pydantic.workers.dev |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a4770c5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(self._connections) < self._max_connections: | ||
| return self._add_connection(origin) | ||
|
|
||
| if self._idle: |
There was a problem hiding this comment.
When max_connections is exhausted by idle HTTP/2 connections, a request for a different origin cannot reuse them, but this branch considers only _idle, which contains HTTP/1.1 connections. _acquire_connection consequently returns None and the request waits until PoolTimeout even though every pool connection may be idle; include idle multiplexed connections among the replacement candidates. The mirrored async implementation has the same defect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in e1091dd: idle HTTP/2 connections now live in the same idle index, so the full-pool eviction path can replace them (covered by test_connection_pool_closes_idle_http2_connection_for_different_origin).
| if connection.can_multiplex(): | ||
| self._sharable_by_origin.setdefault(origin, {})[connection] = None | ||
| if connection.is_idle(): | ||
| self._schedule_expiry(time.monotonic()) | ||
| return |
There was a problem hiding this comment.
When an HTTP/2 connection becomes idle, this branch schedules expiration and returns without enforcing max_keepalive_connections; the enforcement below only counts _idle, which contains HTTP/1.1 connections. As a result, max_keepalive_connections=0 still retains HTTP/2 sockets, and a smaller keepalive limit can retain up to max_connections idle HTTP/2 connections. The mirrored async implementation has the same defect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in e1091dd: idle HTTP/2 connections enter the shared idle accounting, so max_keepalive_connections applies to them (covered by test_connection_pool_keepalive_limit_applies_to_http2_connections).
Idle multiplexing connections now enter the same idle index as HTTP/1.1 connections, so they count towards max_keepalive_connections, can be evicted to make room for another origin when the pool is full, are swept by the keepalive expiry, and are dropped on reuse if they became unusable without closing. They stay in the sharable set so a burst is still multiplexed. Tunnel and SOCKS5 proxy connections delegate can_multiplex() to the underlying connection so HTTP/2 tunnels keep multiplexing after their first request.
The pool now tracks each connection through a small entry record keyed by the connection's identity, so connections returned by an overridden create_connection() no longer need to be hashable, as with the previous list-based pool. The entry also carries the connection's origin, holder count and idle timestamp. A negative max_keepalive_connections is treated as zero instead of tripping the eviction loop.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… HTTP/1.1 connections A sharable connection that closed while a response still holds it is dropped when a request looks for a connection, so it does not count against max_connections until that response is closed. A connection that negotiated HTTP/1.1 is removed from the sharable set as soon as its first request releases it, even while another request still holds a speculative reservation, so later passes cannot hand it to further requests that would only fail with ConnectionNotAvailable.
The two tests that check which idle connections expire used real sleeps with margins that could be overrun on a loaded machine. They now advance a monkeypatched monotonic clock instead, which makes them deterministic and immediate.
There was a problem hiding this comment.
1 issue found across 2 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. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/httpcore2/_async/test_connection_pool.py">
<violation number="1" location="tests/httpcore2/_async/test_connection_pool.py:954">
P3: The clock is not actually deterministic because it adds the real monotonic time to the offset. Each `advance` only shifts the offset, so the absolute time at any check still includes how much wall-clock time the test actually ran. The keepalive margin is currently kept (10s expiry vs 6s advances leaves ~4s of real-time slack for the "b.com not yet expired" assertions), but under a paused or heavily loaded CI where more than ~4s elapses between the b and c requests, the younger connection would wrongly be swept and the test would flake. Returning a pure offset (starting at 0.0) makes the assertions independent of real execution time, matching the docstring claim that expiry "does not depend on real time", and lets the `keepalive_expiry`/`advance` values be read directly.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| self._offset = 0.0 | ||
|
|
||
| def __call__(self) -> float: | ||
| return self._real() + self._offset |
There was a problem hiding this comment.
P3: The clock is not actually deterministic because it adds the real monotonic time to the offset. Each advance only shifts the offset, so the absolute time at any check still includes how much wall-clock time the test actually ran. The keepalive margin is currently kept (10s expiry vs 6s advances leaves ~4s of real-time slack for the "b.com not yet expired" assertions), but under a paused or heavily loaded CI where more than ~4s elapses between the b and c requests, the younger connection would wrongly be swept and the test would flake. Returning a pure offset (starting at 0.0) makes the assertions independent of real execution time, matching the docstring claim that expiry "does not depend on real time", and lets the keepalive_expiry/advance values be read directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/httpcore2/_async/test_connection_pool.py, line 954:
<comment>The clock is not actually deterministic because it adds the real monotonic time to the offset. Each `advance` only shifts the offset, so the absolute time at any check still includes how much wall-clock time the test actually ran. The keepalive margin is currently kept (10s expiry vs 6s advances leaves ~4s of real-time slack for the "b.com not yet expired" assertions), but under a paused or heavily loaded CI where more than ~4s elapses between the b and c requests, the younger connection would wrongly be swept and the test would flake. Returning a pure offset (starting at 0.0) makes the assertions independent of real execution time, matching the docstring claim that expiry "does not depend on real time", and lets the `keepalive_expiry`/`advance` values be read directly.</comment>
<file context>
@@ -939,12 +940,33 @@ def get_extra_info(self, info: str) -> typing.Any:
+ self._offset = 0.0
+
+ def __call__(self) -> float:
+ return self._real() + self._offset
+
+ def advance(self, seconds: float) -> None:
</file context>
| return self._real() + self._offset | |
| def __call__(self) -> float: | |
| return self._offset |
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/httpcore2/httpcore2/_sync/connection_pool.py">
<violation number="1" location="src/httpcore2/httpcore2/_sync/connection_pool.py:432">
P2: When the oldest idle connection is evicted, `_expiry_due` still points to its old deadline and triggers a full idle-connection probe before any remaining connection can expire. Recompute or invalidate `_expiry_due` whenever removing the oldest idle entry to preserve incremental expiry checks.</violation>
</file>
<file name="src/httpcore2/httpcore2/_async/connection_pool.py">
<violation number="1" location="src/httpcore2/httpcore2/_async/connection_pool.py:330">
P2: When `keepalive_expiry=0`, releasing the last response leaves the connection in the pool until another pass. Run the expiry sweep after processing releases, or expire newly idle entries in that pass.</violation>
<violation number="2" location="src/httpcore2/httpcore2/_async/connection_pool.py:374">
P1: When the pool is full, an externally closed connection held by a response can block requests for other origins until that response closes. Track and remove closed entries across the pool before enforcing the connection limit, including HTTP/1.1 entries.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # either serving requests or not yet connected. One that closed while | ||
| # a response still holds it is dropped now rather than at that release, | ||
| # so it does not count against `max_connections` in the meantime. | ||
| for entry in list(self._sharable_by_origin.get(origin, ())): |
There was a problem hiding this comment.
P1: When the pool is full, an externally closed connection held by a response can block requests for other origins until that response closes. Track and remove closed entries across the pool before enforcing the connection limit, including HTTP/1.1 entries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpcore2/httpcore2/_async/connection_pool.py, line 374:
<comment>When the pool is full, an externally closed connection held by a response can block requests for other origins until that response closes. Track and remove closed entries across the pool before enforcing the connection limit, including HTTP/1.1 entries.</comment>
<file context>
@@ -253,122 +308,189 @@ async def handle_async_request(self, request: Request) -> Response:
+ # either serving requests or not yet connected. One that closed while
+ # a response still holds it is dropped now rather than at that release,
+ # so it does not count against `max_connections` in the meantime.
+ for entry in list(self._sharable_by_origin.get(origin, ())):
+ if entry.connection.is_closed():
+ self._drop_connection(entry)
</file context>
| # Enforce `max_keepalive_connections`, closing the longest idle first. | ||
| while len(self._idle) > self._max_keepalive_connections: | ||
| surplus = next(iter(self._idle)) | ||
| self._drop_connection(surplus) |
There was a problem hiding this comment.
P2: When the oldest idle connection is evicted, _expiry_due still points to its old deadline and triggers a full idle-connection probe before any remaining connection can expire. Recompute or invalidate _expiry_due whenever removing the oldest idle entry to preserve incremental expiry checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpcore2/httpcore2/_sync/connection_pool.py, line 432:
<comment>When the oldest idle connection is evicted, `_expiry_due` still points to its old deadline and triggers a full idle-connection probe before any remaining connection can expire. Recompute or invalidate `_expiry_due` whenever removing the oldest idle entry to preserve incremental expiry checks.</comment>
<file context>
@@ -253,122 +308,189 @@ def handle_request(self, request: Request) -> Response:
+ # Enforce `max_keepalive_connections`, closing the longest idle first.
+ while len(self._idle) > self._max_keepalive_connections:
+ surplus = next(iter(self._idle))
+ self._drop_connection(surplus)
+ closing_connections.append(surplus.connection)
+
</file context>
| self._examine_released_connection(entry, closing_connections) | ||
|
|
||
| # Expire idle connections, but only once the oldest may have expired. | ||
| if self._expiry_due is not None and time.monotonic() >= self._expiry_due: |
There was a problem hiding this comment.
P2: When keepalive_expiry=0, releasing the last response leaves the connection in the pool until another pass. Run the expiry sweep after processing releases, or expire newly idle entries in that pass.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpcore2/httpcore2/_async/connection_pool.py, line 330:
<comment>When `keepalive_expiry=0`, releasing the last response leaves the connection in the pool until another pass. Run the expiry sweep after processing releases, or expire newly idle entries in that pass.</comment>
<file context>
@@ -253,122 +308,189 @@ async def handle_async_request(self, request: Request) -> Response:
+ self._examine_released_connection(entry, closing_connections)
+
+ # Expire idle connections, but only once the oldest may have expired.
+ if self._expiry_due is not None and time.monotonic() >= self._expiry_due:
+ self._expire_idle_connections(closing_connections)
+
</file context>
1 participant
Summary
AsyncConnectionPool._assign_requests_to_connectionsran on every enqueue and every release and walked every pooled connection each time (several delegated state queries per connection, plus apoll()on every idle socket viahas_expired()), so per-request cost grew with the number of connections. With an unbounded pool at 512 concurrent requests, ~85% of client CPU was pool bookkeeping and throughput fell to a third of the c16 figure.The pool now keeps its state incrementally and a pass is O(1) in the common case:
deque.max_keepalive_connectionseviction (longest idle first, as before) and the "close an idle connection to another origin when the pool is full" case.http2=True) are kept per origin and checked withis_available()on lookup, so a burst against a warm HTTP/2 connection is still multiplexed within one pass.Origingets a__hash__consistent with its__eq__, andAsyncPoolRequestcreates its wakeup event only when it actually has to wait (the common path assigns before waiting, so noanyio.Eventand nosniffiocall per request)._assign_requests_to_connectionskeeps its name and cadence (once per enqueue, once per release), so the existing pass-counting tests hold. #1076 becomes redundant with this change.Benchmarks
scripts/benchmark --lib httpx2 --python main=... --python pool=..., CPython 3.14 + zuvloop, 2 vCPU, interleaved rounds (medians):Retention (rps at c512 over rps at c16) goes from 30% to 105%; per-request CPU is now flat across concurrency. Measured on the final state of the branch, after the review changes.
Tests
New tests cover: an idle connection closed by the server is discarded on reuse; staggered keepalive expiry keeps younger idle connections (HTTP/1.1 and HTTP/2); an externally closed HTTP/2 connection is discarded; and the
AsyncEvent/Eventprimitives directly, since the pool no longer exercises every branch on each request. The async tests useanyio.sleep, mapped to a threadedconcurrency.sleepby unasync.Checklist
🤖 Generated with Claude Code