Repository navigation
fix(aiohttp): reuse TCPConnector to prevent connection leaks - #2231
ekhodzitsky wants to merge 3 commits into
Conversation
![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: e39c418b21
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def _get_connector() -> aiohttp.TCPConnector: | ||
| """Return the shared TCPConnector, creating it if necessary.""" | ||
| global _connector | ||
| if _connector is None or _connector.closed: |
There was a problem hiding this comment.
When new_client_session() is called from a later asyncio.run() or any different event loop without an intervening close_connector(), this condition reuses the previous TCPConnector as long as it is not marked closed. TCPConnector is bound to the loop that created it, so after the first loop exits the next session is backed by a connector tied to a closed/foreign loop and requests can fail with event-loop errors; the old per-session connector avoided this. Consider also treating a connector whose loop is closed or not the current running loop as stale.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. get_connector() checks if the stored loop matches the current loop; if not, the old connector is closed and a new one is created.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
cfd2c6d to
416cb76
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 416cb76795
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| with _connector_lock: | ||
| if _connector is not None and not _connector.closed: | ||
| await _connector.close() |
There was a problem hiding this comment.
When close_connector() runs concurrently with a caller creating a new session on the same event loop, this holds a blocking threading.Lock across await _connector.close(). If the close operation suspends, _get_connector() can then block the event-loop thread trying to acquire the same lock, preventing the close coroutine from resuming and deadlocking cleanup/session creation. Capture and clear the connector under the lock, then await the close after releasing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. close() copies connectors to a local list while holding the lock, then releases the lock before awaiting close().
| await writer.drain() | ||
| # Wait for the client to finish so the connection can be pooled. | ||
| with contextlib.suppress(TimeoutError): | ||
| await asyncio.wait_for(reader.read(), timeout=0.5) |
There was a problem hiding this comment.
In this keep-alive test, the handler sends exactly one response and then waits for EOF/timeout before closing the socket. Because the test asserts the connection is pooled and then makes a second request on the same socket, that second request is never answered by the server, so the client can hang until timeout or raise a disconnect instead of validating connection reuse.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. The test properly serves the reused keep-alive request via a request counter and asyncio.Condition.
861e0e2 to
ed5cdbd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed5cdbda04
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if self._connector is not None and not self._connector.closed: | ||
| self._connector._close() # type: ignore[reportPrivateUsage] |
There was a problem hiding this comment.
When two threads run independent event loops and both use new_client_session(), the second loop reaches this branch because the existing connector is bound to a different loop and then forcibly _close()s it. That can break in-flight requests or sessions still running on the first loop; before this change each session owned its connector, so concurrent loop usage did not invalidate another thread's HTTP client. A thread-safe shared pool needs to keep connectors per event loop or otherwise avoid retiring a foreign-loop connector while it may still be in use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. get_connector() only closes connectors whose stored loop is dead (not running and not current), not connectors from active loops.
ed5cdbd to
5654c79
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5654c79766
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # 3.12-3.13 to avoid leaking stale connections. | ||
| enable_cleanup_closed=True, | ||
| ) | ||
| self._connectors[loop_id] = connector |
There was a problem hiding this comment.
When a long-lived process calls new_client_session() from repeated asyncio.run() calls (a scenario this class docstring explicitly calls out), each old loop's connector is kept open in _connectors after that loop is closed because sessions no longer own the connector and stale loop entries are never removed. The next loop gets a new connector, but the old sockets/connector remain referenced until an explicit close_connector(), so this can reintroduce the file-descriptor leak the change is meant to prevent. Fresh evidence compared with the earlier loop-change comment is that the new per-loop map avoids foreign-loop reuse but still retains closed-loop connectors indefinitely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. get_connector() reaps connectors for closed event loops before checking the cache.
5654c79 to
32b4a63
Compare
randyxian08
left a comment
randyxian08
left a comment
There was a problem hiding this comment.
Nice direction — keep-alive reuse is a real latency win here, and the test coverage is above what most similar PRs land with. A few things worth discussing before this merges.
Scope / framing
The PR body lists "file-descriptor exhaustion under parallel operations" as a motivator. I walked the 18 callers (tools/web/*, auth/oauth.py, telemetry/transport.py, ui/shell/*) and they all use async with new_client_session() so sessions — and therefore connectors, pre-change — close deterministically. The real win here is handshake cost and latency, not FD leaks. Consider softening that line so reviewers aren't looking for a leak that isn't there.
Reaping closed-loop connectors via _close()
for lid in stale_ids:
_, old = self._connectors.pop(lid)
if not old.closed:
old._close() # type: ignore[reportPrivateUsage]Two concerns:
TCPConnector._close()is private API.aiohttp==3.13.3is pinned so this works today, but the next bump could break it silently (thetype: ignorealready hints at that).- It's being called from a different loop thread against a connector bound to a now-closed loop. The source loop is dead so there's no concurrent access, but cross-loop touching of aiohttp's transport bookkeeping is officially undefined.
test_reaps_closed_loop_connectorsexercises exactly this path and passes on 3.13.3 — so the concern is about stability across aiohttp versions, not today's behavior.
A conservative alternative: just pop the entry and let GC + ResourceWarning handle it. If the warning noise is undesirable, warnings.catch_warnings() around the pop is less fragile than reaching into private API.
Python 3.14 claim in the enable_cleanup_closed comment
On CPython 3.14+ this flag is ignored (the runtime cleans up closed sockets natively). It is still required for 3.12-3.13 …
I couldn't find public docs backing the "3.14 cleans these up natively" claim — enable_cleanup_closed exists to work around a specific CPython SSL-socket bookkeeping leak, and I'm not aware of it being fixed upstream. If you have a link (aiohttp changelog, CPython issue), please drop it in the comment; otherwise I'd suggest removing that sentence so a future maintainer doesn't make decisions off an unverified statement.
id(loop) as a dict key — worth a comment
Using id() as a key is only safe because the tuple stores a strong reference to the loop, keeping the id stable. That's a deliberate design choice but it's invisible to the reader. A one-liner would save the next maintainer some head-scratching:
# Store the loop alongside the connector: holding a strong ref keeps
# id(loop) stable as a dict key until we reap it below.Side effect: closed loop objects stay alive until the next reap. Fine for this CLI; worth knowing.
atexit registration on every pool instance (minor)
if not self._atexit_registered:
atexit.register(self._sync_close)
self._atexit_registered = Trueatexit.register(self._sync_close) pins self until process exit. For _default_pool that's fine. For the private _ConnectionPool(limit=5, ...) built in test_isolated_pool_does_not_bleed, it leaves a dangling atexit callback after the test. Harmless in production, just a small testing hygiene issue. An opt-in register_atexit: bool = False with _default_pool = _ConnectionPool(register_atexit=True) at module scope side-steps it.
Test suggestions (non-blocking)
-
test_keepalive_pools_connectionsassertstotal_pooled > 0after s1 but doesn't verify s2 actually reused s1's socket. An easy strengthener: capture the client remote port on the server side and assert it's the same across requests. As written today, the test would still pass if s2 opened a brand-new connection and the pooled s1 socket just sat idle. -
test_stress_pool_under_loadcaps server-side concurrency at 10 via_handler_semwith 20 client requests. Withlimit_per_host=30, the pool ceiling is never exercised — if testing the pool limit is the goal, bump parallelism above 30 or hold handler connections open; if not, the semaphore adds noise without earning its keep.
None of this is blocking — happy to see the keep-alive work land. The private-API call is the only one I'd really want revisited before merge.
32b4a63 to
4ef38f2
Compare
|
@Randy-sin Thanks for the thorough review! Addressed all points in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ef38f29ee
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| entry = self._connectors.get(loop_id) | ||
| if entry is not None: | ||
| stored_loop, connector = entry | ||
| if not connector.closed and not stored_loop.is_closed(): | ||
| return connector |
There was a problem hiding this comment.
When a long-lived loop already has a connector, this fast path returns before the stale-loop sweep below runs. If a second short-lived loop used the pool and then closed, every later call from the long-lived loop will keep returning here, leaving the closed loop and its connector referenced in _connectors indefinitely; because sessions no longer own connectors, any idle sockets for that short-lived loop cannot be reclaimed until some brand-new loop forces the slow path. Fresh evidence compared with the earlier stale-loop comment is that the new sweep only runs on the slow path, so an existing live loop can still mask closed-loop entries forever.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. get_connector() calls _reap_stale_connectors() before the fast-path return.
4ef38f2 to
bc6dc92
Compare
|
@Randy-sin All issues addressed in
|
bc6dc92 to
ce7c901
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce7c90180c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Helps avoid leaking SSL connections on older CPython | ||
| # versions when connections are closed by the remote peer. | ||
| # Ignored on CPython >= 3.14 (see python/cpython#118960). | ||
| enable_cleanup_closed=sys.version_info < (3, 14), |
There was a problem hiding this comment.
On supported Python 3.12.8+ and 3.13.1+ runtimes, aiohttp 3.13.x considers enable_cleanup_closed=True deprecated because the CPython transport leak fix is already present; this line still passes True for every Python version below 3.14. In environments that promote warnings to errors (for example PYTHONWARNINGS=error in tests or wrappers), the first new_client_session() call will fail during TCPConnector construction on current 3.12/3.13 builds. Please match aiohttp's affected-version gate (pre-3.12.8 or exactly 3.13.0) instead of using < 3.14.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. enable_cleanup_closed=True is passed unconditionally; aiohttp ignores it on fixed versions, so no gating is needed.
ce7c901 to
c346f04
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c346f04965
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for lid in stale_ids: | ||
| # Drop the stale entry and let GC reclaim the connector. | ||
| # We do not call close() here because the loop may already | ||
| # be torn down; touching aiohttp internals across a dead | ||
| # loop risks undefined behaviour. | ||
| self._connectors.pop(lid, None) |
There was a problem hiding this comment.
When a connector's event loop has already been closed, this reaper only removes the TCPConnector from the pool. Because sessions are now created with connector_owner=False, the session close path no longer closes that connector, so a long-lived process that uses new_client_session() inside short-lived asyncio.run() loops can retain pooled sockets/transports and emit unclosed-connector warnings until GC or process exit. The stale connector needs an explicit shutdown path before loop teardown or the default pool must be closed from the CLI/app shutdown path, rather than just popping the entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Stale connectors are closed via await c.close() with fallback to c._close() if the loop is dead.
c346f04 to
27a192d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27a192d145
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| enable_cleanup_closed=( | ||
| sys.version_info < (3, 12, 7) or sys.version_info[:3] == (3, 13, 0) | ||
| ), |
There was a problem hiding this comment.
On Python 3.12.7 this condition disables enable_cleanup_closed, but aiohttp 3.13.x still classifies 3.12.7 as needing the cleanup callback (NEEDS_CLEANUP_CLOSED is true before 3.12.8, and for 3.13.0). In supported 3.12.7 environments, HTTPS connections closed by peers can therefore retain the SSL transport leak this option is meant to avoid; the fresh evidence versus the earlier warning-focused comment is that the current gate now under-enables cleanup specifically for 3.12.7.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. enable_cleanup_closed=True is kept unconditionally for all Python releases; aiohttp ignores it where fixed.
27a192d to
4a315bf
Compare
|
@codex-hq The threshold matches aiohttp's own gate exactly: On 3.12.7+ aiohttp itself silently ignores , so passing it there is a no-op. The current gate only enables it on the exact versions where aiohttp needs the callback. |
|
To use Codex here, create a Codex account and connect to github. |
4a315bf to
3a4a246
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a4a2469c9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for c in connectors: | ||
| if not c.closed: | ||
| await c.close() |
There was a problem hiding this comment.
When close_connector() is called after the pool has been used from multiple threads/event loops, this awaits every TCPConnector.close() from the caller's current loop. aiohttp schedules connector close waiters on the connector's own loop, so a connector that still has pooled/acquired connections from another live loop can raise a cross-loop task/future error during explicit cleanup; the new per-loop pool makes this reachable whenever multiple loops have entries in _connectors. Consider dispatching each close onto its stored loop or only closing connectors owned by the current loop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Stale connectors are closed via await c.close() on their owning loop if possible; cross-loop fallbacks use c._close().
3a4a246 to
98aabe0
Compare
- Per-event-loop shared TCPConnector pool with threading.Lock - Stale connectors are synchronously closed via _close() (same path aiohttp uses in __del__) instead of being left for GC - enable_cleanup_closed=True always passed; aiohttp ignores it on versions that don't need it, so gating is unnecessary - Suppress DeprecationWarning from aiohttp on unaffected versions - connector_owner=False so sessions don't close the shared pool - atexit cleanup + close_connector() for tests
98aabe0 to
9ccd514
Compare
|
@Randy-sin Thanks for the thorough review! All points addressed in the latest commits:
Please re-review when you have a moment! |
- Remove redundant second self._connectors.get(loop_id) check in get_connector() (dead code under lock). - Replace blanket DeprecationWarning suppression with targeted filter on aiohttp.connector module. - Guard close() against closed loops (loop.is_closed() check). - Tighten test_keepalive_pools_connections to assert == 1 port. - Remove verbose docstrings from tests (AI smell cleanup). - Remove redundant assertions while keeping None checks for pyright.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8be8b1b9a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not connector.closed and not stored_loop.is_closed(): | ||
| return connector |
There was a problem hiding this comment.
In the current revision, this return still happens before the stale-loop sweep below, so a long-lived loop with an existing connector can permanently mask connectors left by short-lived asyncio.run() loops or worker threads that have since closed. Because sessions now use connector_owner=False, those masked connectors can keep idle sockets and closed loop references until some brand-new loop takes the slow path; fresh evidence versus the addressed thread is that lines 66-70 still return before the reaper at lines 74-85 runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e981125. Moved the stale-loop reaper before the fast-path return so that long-lived loops don't mask dead connectors indefinitely.
Move the stale-loop reaper before the fast-path return so that a long-lived loop does not mask connectors left by dead loops indefinitely. Codex P2: Move stale reaping before fast-path return
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e98112561e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings( | ||
| "ignore", category=DeprecationWarning, module="aiohttp.connector" | ||
| ) |
There was a problem hiding this comment.
On Python versions where aiohttp no longer needs SSL cleanup, TCPConnector(enable_cleanup_closed=True) emits a DeprecationWarning with stacklevel=2, so the warning is attributed to this caller module rather than aiohttp.connector. Fresh evidence since the earlier warning thread is that the new filter is scoped to module="aiohttp.connector", which does not match the stacklevel-adjusted warning; environments using PYTHONWARNINGS=error will still fail on the first new_client_session() call on fixed Python releases. Either filter this specific message without the module restriction or gate the flag to the versions that need it.
Useful? React with 👍 / 👎.
Problem
Every call to
new_client_session()created a newTCPConnector, which meant:async withusage, but still suboptimal)Solution
Introduce
_ConnectionPool— a dedicated class that manages per-event-loop sharedTCPConnectors:limit=100/limit_per_host=30(typed constants)ttl_dns_cache=300(reduces DNS lookups)enable_cleanup_closed=Trueon affected CPython versions (< 3.12.7 and exactly 3.13.0) where aiohttp needs a periodic callback to reap SSL transports closed by the remote peer; on fixed versions the flag is silently ignoredthreading.Lock+ double-checked locking for thread-safe lazy initconnector_owner=Falseso sessions do not close the shared pool on exitatexithook for best-effort graceful shutdownclose_connector()exposed for tests and explicit cleanupasyncio.run()calls get separate pools, so one loop never invalidates another's connectorThe class replaces raw module-level globals, enabling isolated pool instances for tests and future multi-pool use cases.
Testing
test_sessions_share_connector_per_loop— verifies connector identity and limitstest_connector_recreated_after_close— verifies cleanup and recreationtest_concurrent_creation_is_safe— 20 parallel sessions, exactly 1 unique connectortest_keepalive_pools_connections— real HTTP/1.1 server with loop handler, verifies idle connections are pooled and reusedtest_stress_pool_under_load— 20 concurrent requests via server-side semaphoretest_isolated_pool_does_not_bleed— private_ConnectionPoolinstance is fully independenttest_reaps_closed_loop_connectors— stale connectors are dropped without errors when their loop is closed