fix(libev): guard handle_read/handle_write against close() race condition#889
fix(libev): guard handle_read/handle_write against close() race condition#889vponomaryov wants to merge 1 commit into
Conversation
|
Verified the test fix here: Test run without this fix (for comparison) is here: Comment from the "zeus" comparing the fixed test run and issue affected: @zeus posted:
@fruch , @soyacz ^ |
|
Note that the @scylladb/python-driver-maint please merge and release it. |
| elif self.is_closed or self.is_defunct: | ||
| # Socket was closed by another thread between the | ||
| # watcher firing and us calling send(). This is the | ||
| # race described in scylladb/python-driver#614. | ||
| return |
There was a problem hiding this comment.
I don't see why this is necessary. Without this, we will go into else branch, call self.defunct(err) which will do nothing, and then return.
There was a problem hiding this comment.
Yes, defunct() does have its own if self.is_defunct or self.is_closed: return guard, so no crash or incorrect state transition would occur.
But, the guard is still valuable because of the following reasons:
-
Without the guard we will get missleading log message if the error is
EBADF(which is in_PEER_DISCONNECT_ERRNOS), the code falls into the_PEER_DISCONNECT_ERRNOSbranch first (it comes beforeelse), logging"Connection %s closed by peer during write".
But the socket wasn't closed by the peer - we closed it viaclose(). -
The
_PEER_DISCONNECT_ERRNOSbranch callsself.close(), which is a no-op when already closed but still acquiresself.lock- a minor unnecessary overhead. -
The guard makes the race condition handling self-documenting.
Without it, a reader must trace through multiple branches to confirm the behavior is safe.
So, I find it rather useful than redundant.
There was a problem hiding this comment.
Ah, thanks for the explanation, I did not pay much attention to this path. Now I see a different potential issue, mentioned by Avi ( #614 (comment) )
If we are receiving EBADF, then we performed some action (in this case READ / WRITE) on a closed file descriptor. As far as I know, file descriptors can be reused. So, if another connection was opened between this one being closed and watchers exiting, it makes it possible for us to accidentaly read / write to a totally unrelated connection (or even some file opened by a user app). Am I wrong?
If this is true, then this patch hides the immediate symptom of the problem, but also hides a much more dangerous problem that could cause data corruption.
I don't know the libev connection impl that well, but maybe the proper approach is to, in close, first stop the watchers somehow and make sure that libev won't touch the fd anymore, and only then close the fd?
There was a problem hiding this comment.
Situation where we accidentally read/write to a different connection is, probably, architecturally valid for C/C++ programs using raw fds, but doesn't apply in case of Python.
The Python socket abstraction prevents it.
Python's socket.socket object tracks state internally - after .close(), any operation on that object raises EBADF regardless of whether the OS reused the fd number for something else.
The self._socket reference always points to the old (closed) Python object, never to the new socket that happened to get the same fd.
There was a problem hiding this comment.
libev get a raw fd, not PYthon socket, but the watchers themselves use the Python socket so I guess this issue really can't happen.
The watchers also seem to be correctly removed from the loop in close (connection_destroyed), so they won't keep triggering forever. I think your change should be fine.
There was a problem hiding this comment.
Yes,
defunct()does have its ownif self.is_defunct or self.is_closed: returnguard, so no crash or incorrect state transition would occur.But, the guard is still valuable because of the following reasons:
- Without the guard we will get missleading log message if the error is
EBADF(which is in_PEER_DISCONNECT_ERRNOS), the code falls into the_PEER_DISCONNECT_ERRNOSbranch first (it comes beforeelse), logging"Connection %s closed by peer during write".
But the socket wasn't closed by the peer - we closed it viaclose().- The
_PEER_DISCONNECT_ERRNOSbranch callsself.close(), which is a no-op when already closed but still acquiresself.lock- a minor unnecessary overhead.- The guard makes the race condition handling self-documenting.
Without it, a reader must trace through multiple branches to confirm the behavior is safe.So, I find it rather useful than redundant.
This is incorrect:
def defunct(self, exc):
with self.lock:
if self.is_defunct or self.is_closed:
return
self.is_defunct = True
defunct is the right approach here. If there’s an issue with it, we should fix defunct itself instead of handling all these failures separately.
There should only be two ways to close a connection:
Connection.close— for expected/normal connection shutdownConnection.defunct— for shutting down a connection due to a critical error
There is a case when socket operation can unexpectedly get one of these:
errno.EBADF, errno.ENOTCONN, errno.ESHUTDOWN,
errno.ECONNRESET, errno.ECONNABORTED,
If socket is closed/invalidated/aborted via Connection.close then Connection.defunct should ignore it and should not result in misleading errors and should be a noop.
if it closed by Connection.defunct it should preserve prior error and be effectively noop.
But if this error caused by socket being closed/invalidated/aborted by other reason we should channel it same way as any other error, which is what Connection.defunct for.
There was a problem hiding this comment.
Only thing we need to be sure is that we never close socket before setting is_closed or is_defunct , which I believe already the case.
There was a problem hiding this comment.
An improvement for the "defunct" function sounds as non-blocker for this PR - may be done separately.
There was a problem hiding this comment.
Agreed, but you still need to route error handling to defunct
| elif self.is_closed or self.is_defunct: | ||
| # Socket was closed by another thread between the watcher | ||
| # firing and us calling recv(). This is the race described | ||
| # in scylladb/python-driver#614. | ||
| return |
There was a problem hiding this comment.
Without this, if the error is in _PEER_DISCONNECT_ERRNOS (EBADF), the code would execute following:
self.last_error = ConnectionShutdown(
"Connection to %s was closed by peer" % self.endpoint)
self.close()It will overwrite the last_error with an incorrect closed by peer message when in reality close() was already called by us.
Since last_error is used by factory() to detect why a connection died, this overwrite could mask the real error.
| class LibevPeerDisconnectErrnos(unittest.TestCase): | ||
| """Verify _PEER_DISCONNECT_ERRNOS contains expected values.""" | ||
|
|
||
| def setUp(self): | ||
| _skip_if_unavailable(self) | ||
|
|
||
| def test_contains_ebadf(self): | ||
| self.assertIn(errno.EBADF, _PEER_DISCONNECT_ERRNOS) | ||
|
|
||
| def test_contains_econnreset(self): | ||
| self.assertIn(errno.ECONNRESET, _PEER_DISCONNECT_ERRNOS) | ||
|
|
||
| def test_contains_econnaborted(self): | ||
| self.assertIn(errno.ECONNABORTED, _PEER_DISCONNECT_ERRNOS) | ||
|
|
||
| def test_contains_enotconn(self): | ||
| self.assertIn(errno.ENOTCONN, _PEER_DISCONNECT_ERRNOS) | ||
|
|
||
| def test_contains_eshutdown(self): | ||
| self.assertIn(errno.ESHUTDOWN, _PEER_DISCONNECT_ERRNOS) |
There was a problem hiding this comment.
I see no point in having such tests. It just cements the current code, not testing the behavior.
|
The effect of the python driver issue was And here how much time it takes with the fix: So, numbers tell for itself. |
|
@sylwiaszunejko could you take a look at asyncio failures? Why did they start to happen, do we have an issue open? |
I thought it was due to switch to scylla version 2026.1 and #330, but I see it is supposed to be fixed by #884 |
|
@Lorak-mmk from the PR completeness point of view, should I remove that unit test class?
So, I need to rebase the PR? |
Tbh I don't care that much.
If you could then it would be great. I know you didn't touch asyncio, but let's just be sure. Change in one backend affecting another backend would not be the weirdest thing we saw in this driver. |
…tion
When close() is called from one thread, it sets is_closed=True and
closes the socket immediately. However, libev watchers are stopped
asynchronously in _loop_will_run(), so handle_read()/handle_write()
can still fire on the now-closed fd, causing EBADF errors that
surface as ConnectionShutdown('Bad file descriptor') and prevent
reconnection.
So, fix it applying following changes:
- Early-return guards at the top of handle_read() and handle_write()
that check is_closed/is_defunct before touching the socket
- Secondary is_closed/is_defunct checks in error handlers to catch
the race when close() happens between watcher dispatch and syscall
- Peer disconnect detection (EBADF, ECONNRESET, ENOTCONN, etc.)
that calls close() cleanly instead of defunct()
- last_error preservation in close() when connected_event is unset,
preventing factory() from returning dead connections
Fixes: scylladb#614
vponomaryov
force-pushed
the
fix-race-issue-614
branch
from
5dbc94b to
ad60335
Compare
|
Rebased. |
|
@dkropachev Please take a look at this. |
| elif self.is_closed or self.is_defunct: | ||
| # Socket was closed by another thread between the | ||
| # watcher firing and us calling send(). This is the | ||
| # race described in scylladb/python-driver#614. | ||
| return |
There was a problem hiding this comment.
Agreed, but you still need to route error handling to defunct
One of the tests was enabled that uses TLS with |
4 participants
When close() is called from one thread, it sets is_closed=True and closes the socket immediately. However, libev watchers are stopped asynchronously in _loop_will_run(), so handle_read()/handle_write() can still fire on the now-closed fd, causing EBADF errors that surface as ConnectionShutdown('Bad file descriptor') and prevent reconnection.
So, fix it applying following changes:
Fixes: #614
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.