Don't heartbeat while Worker is closing #6543
Conversation
We're going to fix the race condition we were exploiting here, but a simpler one still exists.
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ±0 15 suites ±0 6h 20m 51s ⏱️ + 3m 25s For more details on these failures, see this check. Results for commit 20d3198. ± Comparison against base commit 9e4e3ab. |
| orig_comm = bcomm.comm | ||
|
|
||
| write_event = asyncio.Event() | ||
| write_queue: asyncio.Queue = asyncio.Queue() |
There was a problem hiding this comment.
nit: is it actually necessary to explicitly annotate this? That feels absurd.
There was a problem hiding this comment.
Yes, mypy actually raised an error explicitly telling me to do this. I agree it's absurd.
| # closes the worker. Heartbeats aren't sent over batched comms, so | ||
| # `freeze_batched_send` doesn't affect them. |
There was a problem hiding this comment.
FWIW I'm inclined to change this eventually and reuse the stream for heartbeats
Previously, the heartbeat could run while
closewas running. The scheduler would probably say the worker was unknown, and the worker would then try to close itself. This wouldn't actually be a big deal, sincecloseis idempotent.Mostly this just cleans up a very contrived test to instead test a more realistic scenario. It also adds a
freeze_batched_sendwhich may be useful in other places.After writing all this, I don't actually care much about the reordering of the
PeriodicCallback.stop()inWorker.close(); if anyone objects to it, happy to revert.pre-commit run --all-files