Fix AsyncioConnection race conditions causing EBADF errors (#614)#697
Draft
dkropachev wants to merge 5 commits intomasterfrom
Draft
Fix AsyncioConnection race conditions causing EBADF errors (#614)#697dkropachev wants to merge 5 commits intomasterfrom
dkropachev wants to merge 5 commits intomasterfrom
Conversation
Fix four race conditions in AsyncioConnection that cause "[Errno 9] Bad file descriptor" errors during node restarts, especially with TLS: 1. close() now waits for _close() to complete when called from outside the event loop thread, eliminating the window where is_closed=True but the socket fd is still open. 2. handle_read() sets last_error on server EOF so factory() detects dead connections instead of returning them to callers. 3. push() rejects data when the connection is already closed, preventing writes from being enqueued to a shutting-down connection. 4. handle_write() treats BrokenPipeError/ConnectionResetError as clean peer disconnections instead of defuncting, and both I/O handlers skip defunct() if the connection is already shutting down.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
ProactorEventLoop does not support remove_reader/remove_writer (raises NotImplementedError). Wrap these calls so the socket is always closed regardless, and use try/finally to ensure connected_event is always set even if cleanup fails.
On Windows, ProactorEventLoop may raise plain OSError with winerror=10054 (WSAECONNRESET) or 10053 (WSAECONNABORTED) instead of ConnectionResetError. Use ConnectionError base class plus winerror check for robust cross-platform detection.
macOS raises OSError(57, "Socket is not connected") (ENOTCONN) when writing to a peer-disconnected socket. Unlike BrokenPipeError or ConnectionResetError, this is a plain OSError — not a ConnectionError subclass — so it was not caught by the isinstance(err, ConnectionError) check. Add errno-based detection for ENOTCONN, ESHUTDOWN, ECONNRESET, and ECONNABORTED to handle these platform-specific disconnect errors cleanly instead of defuncting the connection.
- Guard SocketPairAsyncioConnection definition with ASYNCIO_AVAILABLE check to avoid subclassing NoneType when asyncio is unavailable - Add debug logging in close() except block instead of bare pass - Add inline comments to except blocks in _close() for remove_reader/remove_writer
dkropachev
force-pushed
the
fix/asyncio-close-race-614
branch
from
fe6be33 to
e4f0c6b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
close()to wait for_close()completion from non-event-loop threads, eliminating the window whereis_closed=Truebut the socket fd is still open (root cause of EBADF)last_erroron server EOF inhandle_read()sofactory()detects dead connections instead of returning themis_closed/is_defunctguard inpush()to reject writes on closed connectionsBrokenPipeError/ConnectionResetErrorinhandle_write()as clean peer disconnections instead of defuncting, and skipdefunct()in both I/O handlers if the connection is already shutting downTest plan
tests/unit/io/test_asyncio_race_614.pycover all four race conditions using real socket pairs