-
Notifications
You must be signed in to change notification settings - Fork 48
Maintain connection reservations incrementally in the pool #1076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a3e1a2b
b9f33ba
0ed00a0
d6582d8
58b1b65
13f976f
d718f50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,6 +112,11 @@ def __init__( | |
| self._connections: list[AsyncConnectionInterface] = [] | ||
| self._requests: list[AsyncPoolRequest] = [] | ||
|
|
||
| # Reference counts of connections held by in-flight requests, | ||
| # maintained incrementally so assignment passes never rebuild them | ||
| # by scanning the full request list. | ||
| self._request_connections: dict[AsyncConnectionInterface, int] = {} | ||
|
|
||
| # We only mutate the state of the connection pool within an 'optional_thread_lock' | ||
| # context. This holds a threading lock unless we're running in async mode, | ||
| # in which case it is a no-op. | ||
|
|
@@ -227,14 +232,17 @@ async def handle_async_request(self, request: Request) -> Response: | |
| # handle a request, but then become unavailable. | ||
| # | ||
| # In this case we clear the connection and try again. | ||
| pool_request.clear_connection() | ||
| with self._optional_thread_lock: | ||
| self._release_request_connection(pool_request) | ||
| pool_request.clear_connection() | ||
| else: | ||
| break # pragma: no cover | ||
|
|
||
| except BaseException as exc: | ||
| with self._optional_thread_lock: | ||
| # For any exception or cancellation we remove the request from | ||
| # the queue, and then re-assign requests to connections. | ||
| self._release_request_connection(pool_request) | ||
| self._requests.remove(pool_request) | ||
| closing = self._assign_requests_to_connections() | ||
|
|
||
|
|
@@ -251,6 +259,19 @@ async def handle_async_request(self, request: Request) -> Response: | |
| extensions=response.extensions, | ||
| ) | ||
|
|
||
| def _reserve_connection(self, pool_request: AsyncPoolRequest, connection: AsyncConnectionInterface) -> None: | ||
| pool_request.assign_to_connection(connection) | ||
| self._request_connections[connection] = self._request_connections.get(connection, 0) + 1 | ||
|
|
||
| def _release_request_connection(self, pool_request: AsyncPoolRequest) -> None: | ||
| connection = pool_request.connection | ||
| if connection is not None: | ||
| count = self._request_connections[connection] - 1 | ||
| if count: | ||
| self._request_connections[connection] = count | ||
| else: | ||
| del self._request_connections[connection] | ||
|
|
||
| def _assign_requests_to_connections(self) -> list[AsyncConnectionInterface]: | ||
| """ | ||
| Manage the state of the connection pool, assigning incoming | ||
|
|
@@ -267,7 +288,7 @@ def _assign_requests_to_connections(self) -> list[AsyncConnectionInterface]: | |
| # Connections currently referenced by an in-flight request, including | ||
| # connections that are in the process of being established and idle | ||
| # connections reserved by an assigned-but-not-yet-sent request. | ||
| request_connections = {r.connection for r in self._requests} | ||
| request_connections = self._request_connections | ||
|
|
||
| # First we handle cleaning up any connections that are closed | ||
| # or have expired their keep-alive, in a single pass. Reserved | ||
|
|
@@ -310,16 +331,16 @@ def _assign_requests_to_connections(self) -> list[AsyncConnectionInterface]: | |
| # it per queued request — this is what brings the loop from O(N*M) to | ||
| # O(N+M) in the common case. | ||
| # | ||
| # An idle connection already assigned to an in-flight request is | ||
| # reserved: it stays IDLE until the winning task sends on it, so | ||
| # without this exclusion the next pass would assign it again and the | ||
| # loser would churn through `ConnectionNotAvailable`. Multiplexing | ||
| # connections are exempt: they can take further requests while idle. | ||
| # An established non-multiplexing connection already assigned to an | ||
| # in-flight request is reserved. Its state may transition from IDLE to | ||
| # ACTIVE after `is_available()` returns, so use `is_connected()` here | ||
| # rather than checking its mutable idle state. Multiplexing connections | ||
| # and not-yet-connected HTTP/2 candidates remain available. | ||
| available_connections = [ | ||
| 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()) | ||
| and not (connection.is_connected() and connection in request_connections and not connection.can_multiplex()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Prompt for AI agents |
||
| ] | ||
| new_connection_budget = self._max_connections - len(self._connections) | ||
|
|
||
|
|
@@ -342,17 +363,17 @@ def _assign_requests_to_connections(self) -> list[AsyncConnectionInterface]: | |
| # to handle the request. | ||
| for idx, connection in enumerate(available_connections): | ||
| if connection.can_handle_request(origin): | ||
| pool_request.assign_to_connection(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. | ||
| if connection.is_connected() and not connection.can_multiplex(): | ||
| # Remove an established HTTP/1.1 connection before | ||
| # waking the request, which may transition it to ACTIVE. | ||
| del available_connections[idx] | ||
| self._reserve_connection(pool_request, connection) | ||
| break | ||
| else: | ||
| if new_connection_budget > 0: | ||
| connection = self.create_connection(origin) | ||
| self._connections.append(connection) | ||
| pool_request.assign_to_connection(connection) | ||
| self._reserve_connection(pool_request, connection) | ||
| new_connection_budget -= 1 | ||
| continue | ||
| for idx, connection in enumerate(available_connections): | ||
|
|
@@ -362,7 +383,7 @@ def _assign_requests_to_connections(self) -> list[AsyncConnectionInterface]: | |
| closing_connections.append(connection) | ||
| connection = self.create_connection(origin) | ||
| self._connections.append(connection) | ||
| pool_request.assign_to_connection(connection) | ||
| self._reserve_connection(pool_request, connection) | ||
| break | ||
|
|
||
| return closing_connections | ||
|
|
@@ -434,6 +455,7 @@ async def aclose(self) -> None: | |
| await self._stream.aclose() | ||
|
|
||
| with self._pool._optional_thread_lock: | ||
| self._pool._release_request_connection(self._pool_request) | ||
| self._pool._requests.remove(self._pool_request) | ||
| closing = self._pool._assign_requests_to_connections() | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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:343The new availability filter on line 343 excludes established connections from
available_connectionswhencan_multiplex()returnsFalse. Proxy connection wrappers such asAsyncTunnelHTTPConnectionandAsyncSocks5Connectiondo not override the newly addedcan_multiplex()method and inherit the defaultFalsefromAsyncConnectionInterface, 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 delegatecan_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:343src/httpcore2/httpcore2/_sync/connection_pool.py:343🚀 Reply "fix it for me" or copy this AI Prompt for your agent: