Skip to content

Maintain connection reservations incrementally in the pool - #1076

Open
Kludex wants to merge 7 commits into
mainfrom
pool-incremental-reservations
Open

Maintain connection reservations incrementally in the pool#1076
Kludex wants to merge 7 commits into
mainfrom
pool-incremental-reservations

Conversation

@Kludex

@Kludex Kludex commented Jul 23, 2026

Copy link
Copy Markdown
Member

Summary

Stacked on #1075. Implements the follow-up suggested by @11kkw there: the last O(in-flight) cost per assignment pass was rebuilding request_connections from the full request list, which is quadratic in aggregate across a large burst.

Changes

  • Replace the per-pass {r.connection for r in self._requests} rebuild with a refcounted dict[connection, int] on the pool.
  • Update it at the mutation points: assignment, ConnectionNotAvailable requeue (now under the thread lock), cancellation, and response close.
  • Sync pool generated via unasync as usual.

Benchmarks

Unbounded concurrent requests against a local uvicorn server, best of 3:

n #1075 this PR
1,000 0.33s (3072 rps) 0.34s (2972 rps)
5,000 1.75s (2853 rps) 1.50s (3325 rps)
10,000 3.85s (2595 rps) 3.07s (3257 rps)

Throughput on #1075 decays as burst size grows (the residual quadratic term); with this change it stays flat, making total pool work effectively linear.

Verification

  • Full suite passes with 100% coverage; mypy/ruff/unasync clean.
  • HTTP/2 against hypercorn over TLS: 500-request cold burst and warm-idle burst multiplex on a single connection, unchanged.
  • Real services with http2=True unchanged; streaming, cancellation storms, keepalive expiry, PoolTimeout, and threaded sync Client all behave as before.

AI Disclaimer

This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.

Review in cubic

Kludex added 4 commits July 23, 2026 09:18
Assignment does not change an HTTP/1.1 connection's state, so a newly
idle connection was handed to every queued request in one pass and
re-picked by later passes until the winner sent on it. Every loser woke
up, failed with ConnectionNotAvailable, re-entered the queue, and
triggered another full O(n) assignment pass - quadratic churn at high
queue depth.

Drop a connection from the availability snapshot once assigned, and
exclude idle connections already reserved by an in-flight request when
building the snapshot. 1000 concurrent requests against a local server
drop from 5.1s to 0.9s with the default pool, and from 100s to 1.1s
with max_connections=1. HTTP/2 multiplexing is unaffected: an active h2
connection is not idle, so it stays available to additional streams.
An idle HTTP/2 connection can serve further requests while reserved, so
treating it like HTTP/1.1 could leave a queued burst waiting for the
next pool event instead of multiplexing. Add can_multiplex() to the
connection interface (False by default, True for established HTTP/2)
and only apply the reserved-idle exclusion to connections that cannot
multiplex.
Each assignment pass walked every in-flight request even when the pool
was saturated, and re-probed reserved idle connections for expiry with
an is_readable socket check on every interleaved pass. Break out of the
assignment loop once no connection is available and no new one may be
created, and skip expiry checks and surplus-keepalive eviction for
connections reserved by an assigned request - they were health-checked
at assignment time, and evicting them would hand the winning request a
closed connection.

1000 unbounded concurrent requests against a local server now complete
in 0.41s versus 0.51s sequential, compared to 0.93s before this change
and 5.1s before #1075.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

Base automatically changed from pool-single-assignment to main July 23, 2026 11:53
Comment on lines +366 to +370
self._reserve_connection(pool_request, connection)
if connection.is_idle() and not connection.can_multiplex():
# An idle HTTP/1.1 connection can only take this
# single request until it is released.
del available_connections[idx]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could there be a race here in the sync pool? _reserve_connection() wakes the waiting request, so the connection might transition from IDLE to ACTIVE before the following is_idle() check and remain in available_connections. Would it be safer to remove an established non-multiplexing connection from the candidate list first?

Suggested change
self._reserve_connection(pool_request, connection)
if connection.is_idle() and not connection.can_multiplex():
# An idle HTTP/1.1 connection can only take this
# single request until it is released.
del available_connections[idx]
if connection.is_connected() and not connection.can_multiplex():
del available_connections[idx]
self._reserve_connection(pool_request, connection)

connection
for connection in self._connections
if connection.is_available()
and not (connection.is_idle() and connection in request_connections and not connection.can_multiplex())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would is_connected() be a better fit here? It seems to keep an established reserved non-multiplexing connection excluded after it transitions from IDLE to ACTIVE, while still allowing a not-yet-connected HTTP/2 candidate. What do you think?

Suggested change
and not (connection.is_idle() and connection in request_connections and not connection.can_multiplex())
and not (
connection.is_connected()
and connection in request_connections
and not connection.can_multiplex()
)

connection
for connection in self._connections
if connection.is_available()
and not (connection.is_connected() and connection in request_connections and not connection.can_multiplex())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High _async/connection_pool.py:343

The new availability filter on line 343 excludes established connections from available_connections when can_multiplex() returns False. Proxy connection wrappers such as AsyncTunnelHTTPConnection and AsyncSocks5Connection do not override the newly added can_multiplex() method and inherit the default False from AsyncConnectionInterface, even when their inner connection has negotiated HTTP/2. As a result, once the first proxied HTTP/2 request reserves such a connection, concurrent requests cannot reuse the established HTTP/2 connection and instead queue or open extra proxy connections, defeating multiplexing. The proxy wrappers need to delegate can_multiplex() to their underlying connection so the pool filter recognizes them as multiplexing-capable.

Also found in 2 other location(s)

src/httpcore2/httpcore2/_sync/connection_pool.py:343

The new availability filter treats every proxy wrapper that inherits the default ConnectionInterface.can_multiplex() as non-multiplexing. TunnelHTTPConnection and Socks5Connection can replace their inner connection with an HTTP2Connection, but neither delegates can_multiplex() to it. Once such a proxy connection is established and reserved by one request, line 343 excludes it from available_connections, so concurrent requests cannot reuse the HTTP/2 connection and may queue or open extra proxy connections instead of multiplexing.

src/httpcore2/httpcore2/_sync/connection_pool.py:343

available_connections excludes every reserved established connection whose outer class does not override can_multiplex(). Proxy connection wrappers such as TunnelHTTPConnection and Socks5Connection can contain an HTTP2Connection but inherit the new default False, so after the first proxied HTTP/2 request is reserved, concurrent requests cannot reuse that HTTP/2 connection and instead queue or open extra connections. The wrapper needs to delegate multiplex capability to its established inner connection.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/httpcore2/httpcore2/_async/connection_pool.py around line 343:

The new availability filter on line 343 excludes established connections from `available_connections` when `can_multiplex()` returns `False`. Proxy connection wrappers such as `AsyncTunnelHTTPConnection` and `AsyncSocks5Connection` do not override the newly added `can_multiplex()` method and inherit the default `False` from `AsyncConnectionInterface`, even when their inner connection has negotiated HTTP/2. As a result, once the first proxied HTTP/2 request reserves such a connection, concurrent requests cannot reuse the established HTTP/2 connection and instead queue or open extra proxy connections, defeating multiplexing. The proxy wrappers need to delegate `can_multiplex()` to their underlying connection so the pool filter recognizes them as multiplexing-capable.

Also found in 2 other location(s):
- src/httpcore2/httpcore2/_sync/connection_pool.py:343 -- The new availability filter treats every proxy wrapper that inherits the default `ConnectionInterface.can_multiplex()` as non-multiplexing. `TunnelHTTPConnection` and `Socks5Connection` can replace their inner connection with an `HTTP2Connection`, but neither delegates `can_multiplex()` to it. Once such a proxy connection is established and reserved by one request, line 343 excludes it from `available_connections`, so concurrent requests cannot reuse the HTTP/2 connection and may queue or open extra proxy connections instead of multiplexing.
- src/httpcore2/httpcore2/_sync/connection_pool.py:343 -- `available_connections` excludes every reserved established connection whose outer class does not override `can_multiplex()`. Proxy connection wrappers such as `TunnelHTTPConnection` and `Socks5Connection` can contain an `HTTP2Connection` but inherit the new default `False`, so after the first proxied HTTP/2 request is reserved, concurrent requests cannot reuse that HTTP/2 connection and instead queue or open extra connections. The wrapper needs to delegate multiplex capability to its established inner connection.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 4 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="src/httpcore2/httpcore2/_async/connection_pool.py">

<violation number="1" location="src/httpcore2/httpcore2/_async/connection_pool.py:343">
P1: Concurrent HTTP/2 requests through an HTTP CONNECT or SOCKS proxy no longer share an established tunnel: these wrappers report `can_multiplex() == False` even after negotiating `AsyncHTTP2Connection`, so this predicate excludes the active connection. Forward `can_multiplex()` from both proxy wrappers (or otherwise base this reservation check on the negotiated delegate) before applying the non-multiplexing reservation rule.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

connection
for connection in self._connections
if connection.is_available()
and not (connection.is_connected() and connection in request_connections and not connection.can_multiplex())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Concurrent HTTP/2 requests through an HTTP CONNECT or SOCKS proxy no longer share an established tunnel: these wrappers report can_multiplex() == False even after negotiating AsyncHTTP2Connection, so this predicate excludes the active connection. Forward can_multiplex() from both proxy wrappers (or otherwise base this reservation check on the negotiated delegate) before applying the non-multiplexing reservation rule.

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 343:

<comment>Concurrent HTTP/2 requests through an HTTP CONNECT or SOCKS proxy no longer share an established tunnel: these wrappers report `can_multiplex() == False` even after negotiating `AsyncHTTP2Connection`, so this predicate excludes the active connection. Forward `can_multiplex()` from both proxy wrappers (or otherwise base this reservation check on the negotiated delegate) before applying the non-multiplexing reservation rule.</comment>

<file context>
@@ -331,16 +331,16 @@ def _assign_requests_to_connections(self) -> list[AsyncConnectionInterface]:
             for connection in self._connections
             if connection.is_available()
-            and not (connection.is_idle() and connection in request_connections and not connection.can_multiplex())
+            and not (connection.is_connected() and connection in request_connections and not connection.can_multiplex())
         ]
         new_connection_budget = self._max_connections - len(self._connections)
</file context>

Kludex added 2 commits August 2, 2026 20:57
…rvations

# Conflicts:
#	src/httpcore2/httpcore2/_async/connection_pool.py
#	src/httpcore2/httpcore2/_sync/connection_pool.py
#	tests/httpcore2/_async/test_connection_pool.py
#	tests/httpcore2/_sync/test_connection_pool.py
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

@codspeed-hq

codspeed-hq Bot commented Aug 3, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 15 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing pool-incremental-reservations (d718f50) with main (eac8ad4)

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants