Exclude comm handshake from connect timeout#7698
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 20 files ± 0 20 suites ±0 10h 47m 25s ⏱️ - 20m 21s For more details on these failures, see this check. Results for commit bd2ba8f. ± Comparison against base commit 566fd1f. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
|
|
||
|
|
||
| @gen_test() | ||
| async def test_client_active_bad_port(): |
There was a problem hiding this comment.
http servers wait for a \r\n before parsing the request line and so we never get a handshake back from those servers so this test fails
convert test_single_executable_deprecated into a sync test wait_for_log_line was intermittently blocking the handshake from occuring adding the extra asyncio.gather made this more likely
Co-authored-by: Thomas Grainger <tagrain@gmail.com>
`await comm.write()` doesn't actually yield to the event loop (unless to_frames uses a thread) and so introducing `asyncio.gather` actually makes the handshake slower, which seems to be introducing more flaky test failures
graingert
force-pushed
the
remove_handshake_from_timeout
branch
from
a82f162 to
d7214e2
Compare
|
✅ From my end. @graingert feel free to merge |
|
@graingert: There's a lot of red on CI including failures like |
graingert
force-pushed
the
remove_handshake_from_timeout
branch
from
b49b28d to
08353e1
Compare
graingert
force-pushed
the
remove_handshake_from_timeout
branch
from
c949245 to
d069db4
Compare
graingert
force-pushed
the
remove_handshake_from_timeout
branch
from
73bb326 to
a1723de
Compare
changing when the worker calls self.stop() seems to cause this test to fail unless the SlowKillNanny.kill() is allowed to proceed when called during test teardown
|
I'm inclined to merge this even if it is increasing test flakiness. I do believe that this genuinely improves stability and test flakiness is mostly due to a bad test setup. |
| for pc in self.periodic_callbacks.values(): | ||
| pc.stop() | ||
|
|
||
| self.stop() |
There was a problem hiding this comment.
Moving this up here has broken P2P restarts by introducing a new race condition (or making it exponentially more likely to occur). Why do we need this change?
See #8011 (comment) for more on the P2P problem.
There was a problem hiding this comment.
I vaguely remember having a conversation with @graingert about this but can't recall the details. I believe there were a couple of connections left hanging because they were initiated during worker close and were never properly aborted. Not allowing any more connections once we are closing felt like a sane thing to do.
There was a problem hiding this comment.
I don't have any issues with reverting this change but I am a bit reluctant to revert the handshake timeout removal. I assume that this is required to keep many tests green.
If Listener.stop() runs after a connection has been accepted but before
_handle_stream gets to run, _handle_stream would crash with
ValueError('invalid operation on non-started TCPListener') when reading
self.contact_address, abandoning the accepted stream without closing it.
Since the comm handshake in connect() is deliberately not subject to the
connect timeout (dask#7698), the client would then hang forever waiting for the
server's handshake reply. abort_handshaking_comms() cannot help, as the comm
never reached on_connection().
This is what deadlocks test_RetireWorker_stress: a worker's gather_dep from a
closing worker gets stuck forever in the handshake, pinning the key in flight
state; the AMM RetireWorker policy then re-suggests the same transfer every
interval, but the recipient ignores it because the key is already in flight,
so retire_workers never completes. It also leaked one file descriptor (the
abandoned socket) every time the race hit without deadlocking.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
There is not a very specific issue this is fixing but I've come across many connection timeouts over the time and some of them are actually caused by a timed out handshake.
There is no reason for the handshake to be included in the timeout. The timeout is primarily there to ensure that the remote actually exists and we're not attempting to connect to an otherwise dead remote.
Particularly in situations when the event loop is blocked, the handshake can actually take quite a while, particularly if read/write is done sequentially as on main. However, once the connection is established, the entire operation is mostly about sending/receiving some ordinary network messages and we're not timing out such sendrcv operations anywhere else. I believe it is very generally true that the handshake is typically not slow due to network but only due to server load.