Revert "Maintain connection reservations incrementally in the pool" - #1197
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
|
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| # connections that are in the process of being established and idle | ||
| # connections reserved by an assigned-but-not-yet-sent request. | ||
| request_connections = self._request_connections | ||
| request_connections = {r.connection for r in self._requests} |
There was a problem hiding this comment.
Low: Queued requests trigger quadratic pool work
_assign_requests_to_connections() runs whenever requests enter, leave, or retry, so rebuilding this set from every in-flight request makes a queue of N requests perform O(N²) bookkeeping. An attacker who can trigger concurrent outbound requests to a slow origin can keep the pool saturated while growing the queue and consume disproportionate CPU before pool timeouts drain it. Keep reservation counts incrementally, or otherwise make this lookup independent of queue depth; the asynchronous implementation has the same issue.
PR overviewThis PR reverts incremental connection-reservation tracking in the HTTP connection pool, restoring reservation calculations from the current in-flight requests during request assignment. One performance-related security issue remains open: a sufficiently large queue of outbound requests to a slow origin can cause quadratic pool bookkeeping and disproportionate CPU use. Exploitation depends on an attacker being able to generate concurrent outbound requests while keeping the connection pool saturated. Open issues (1)
Fixed/addressed: 0 · PR risk: 4/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: 49fd61086d
ℹ️ 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".
| def is_idle(self) -> bool: | ||
| return self._connection.is_idle() | ||
|
|
||
| def can_multiplex(self) -> bool: | ||
| return self._connection.can_multiplex() | ||
|
|
||
| def is_closed(self) -> bool: |
There was a problem hiding this comment.
When an HTTP/2 tunnel is connected but idle, TunnelHTTPConnection now inherits ConnectionInterface.can_multiplex() == False even though its availability and idle checks delegate to the underlying HTTP/2 connection. If another thread queues after the first request reserves that connection but before it starts sending, the pool treats the connection as non-multiplexing and leaves the later request blocked until the first response closes; a long-lived streaming response can therefore make a concurrent HTTP/2 request hit its pool timeout. Restore delegation to the underlying connection; the same removal affects the async tunnel and SOCKS wrappers.
Useful? React with 👍 / 👎.
| pool_request.assign_to_connection(connection) | ||
| if connection.is_idle() and not connection.can_multiplex(): |
There was a problem hiding this comment.
When this assignment occurs while multiple requests are queued, setting the event can let the selected sync request transition the HTTP/1.1 connection from idle to active before the following is_idle() check. The condition then becomes false, leaving the non-multiplexing connection in available_connections so later requests are assigned to it too; they all wake, receive ConnectionNotAvailable, and requeue, causing concurrency-dependent churn and potentially extending pool waits. Remove an established non-multiplexing candidate before calling assign_to_connection().
Useful? React with 👍 / 👎.
| # connections that are in the process of being established and idle | ||
| # connections reserved by an assigned-but-not-yet-sent request. | ||
| request_connections = self._request_connections | ||
| request_connections = {r.connection for r in self._requests} |
There was a problem hiding this comment.
On a saturated pool, every request arrival and response close invokes this method, and this comprehension scans every in-flight request while holding the pool lock even though the assignment loop immediately stops when no connection or creation budget is available. With N concurrent requests, enqueueing and completing the batch therefore performs quadratic reservation scans, blocking sync callers and the async event loop respectively; retain incrementally updated reservation counts instead of rebuilding the set on every pass.
Useful? React with 👍 / 👎.
1 participant
I merged by mistake, I didn't properly review it. Reverting to give me time.
Reverts #1076