Detach socket on create_connection cancellation to prevent fd double-close - #740
Conversation
Sorry, something went wrong.
When create_connection(sock=sock) or create_unix_connection(sock=sock) is cancelled or raises, tr._close() closes the fd via libuv but the Python socket object still believes it owns that fd number. Its __del__ later closes whatever fd the OS recycled into that slot, corrupting unrelated transports. Call sock.detach() on the error path so the Python socket sets its internal fd to -1, matching the semantics of standard asyncio where the transport always takes full ownership of the socket. Fixes MagicStack#738
|
@1st1 Please have a look! thx |
Sorry, something went wrong.
|
@fantix could you review? this seems like a critical issue under high concurrency |
Sorry, something went wrong.
|
Hey @junjzhang @6matt, perhaps one option for you would be to switch to aiofastnet? Not only it is faster, its networking source code is following python 3.14 asyncio implementation, which doesn't have this bug. And if there is any other bug, I can fix it and release much faster. I waited for more than a year to get some of my uvloop PRs merged, and they haven't been released yet. Just saying |
Sorry, something went wrong.
- Move test_create_connection_sock_cancel_detaches to _TestTCP so it runs on both uvloop and asyncio - Add test_create_connection_sock_cancel_fd_leak that reproduces the full data leak chain: cancel → fd reuse → stale close → writev to wrong socket (see MagicStack#645, aio-libs/aiohttp#10506) - Fix ConnectionAbortedError in detach test server handler Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Good call! Though, #646 fixed it in a way that breaks asyncio compatibility. I think this PR is a better approach. Let me also add a theoretical test to reproduce #645, as well as aio-libs/aiohttp#10506. |
Sorry, something went wrong.
Mirror the TCP cancel/detach and data-leak tests for the create_unix_connection(sock=) path, covering the fix in both create_connection and create_unix_connection. Also fix server handlers to close writers (Python 3.12+ wait_closed() blocks until all connections are closed) and fix flake8 blank line issue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Thanks for the background. I think the #646 approach was to make passed in socket (PSO) handling more like internally generated socket handling. When libuv generates the socket (such as through an accept call), there is no PSO so uvloop creates an internal wrapper. As a result, there are code paths that have to distinguish between a PSO and an internally-wrapped socket. The #646 change made everything just an internally wrapped socket. |
Sorry, something went wrong.
|
Yeah, what you said is indeed an ideal resolution, but it is no longer identical to asyncio - where the passed-in sock object is still accessible by the caller, no matter how much it is suggested not to be messed with. The referenced asyncio PR was to forbid such wrong usages by re-wrapping the given sock object, but that PR was never landed in upstream, that's why uvloop had all those transport._attach_sockobj() workarounds for years. |
Sorry, something went wrong.
|
Hey first of all thanks for the fix @junjzhang |
Sorry, something went wrong.
|
We hit what appears to be the same failure mode in a production disaggregated inference service using Python 3.12, uvloop 0.22.1, aiohttp 3.14.1, and Uvicorn. Under concurrent aiohttp connection attempts, retries, and cancellation, the API process first returned readiness, then its connections were reset, and the process terminated natively with SIGABRT (Aborted (core dumped)). The timing and descriptor-reuse behavior are consistent with the stale Python socket object closing an fd already closed by libuv and later recycled into another transport. The resulting outage produced thousands of downstream 503 responses. We have switched the affected proxy and Python API serving paths to the standard-library asyncio loop in our release candidate. We plan to keep uvloop disabled until a release containing dc680eb20c526d8babacaf4751a12f51431ea246 is available and passes our cancellation stress and production-shaped load checks. Thank you for fixing this and for adding the TCP and Unix socket regression coverage. An approximate release timeline would be very helpful for downstream users deciding when it is safe to re-enable uvloop. |
Sorry, something went wrong.
Summary
Fixes #738.
When create_connection(sock=sock) or create_unix_connection(sock=sock) is cancelled or raises, tr._close() closes the fd via libuv but the Python socket object still believes it owns that fd. Its __del__ later closes whatever fd the OS recycled into that slot, silently corrupting an unrelated transport.
This adds sock.detach() on the error path of both create_connection and create_unix_connection, matching the semantics of standard asyncio where the transport always takes full ownership of the socket.
Changes