Ensure Server.close cannot run concurrently #7313
Conversation
fjetter
force-pushed
the
server_close_concurrently
branch
from
4bc4965 to
e8737b8
Compare
| init_result_q.close() | ||
| # If we hit an exception here we need to wait for a least | ||
| # one interval for the outside to pick up this message. | ||
| # Otherwise we arrive in a race condition where the process | ||
| # cleanup wipes the queue before the exception can be | ||
| # properly handled. See also | ||
| # WorkerProcess._wait_until_connected (the 3 is for good | ||
| # measure) | ||
| sync_sleep(cls._init_msg_interval * 3) | ||
| init_result_q.join_thread() | ||
|
|
There was a problem hiding this comment.
This is a drive by. I can break this out in anothe rPR if necessary
| async def close_unsafe( | ||
| self, timeout: float | None, reason: str | None, **kwargs: Any | ||
| ) -> None: |
There was a problem hiding this comment.
I'm not happy with the signature but that's the way it is. Specifically the Nanny and the Worker have very custom signatures making this a bit ugly
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 11h 34m 58s ⏱️ + 4h 42m 30s For more details on these failures and errors, see this check. Results for commit d214271. ± Comparison against base commit 5b6351a. ♻️ This comment has been updated with latest results. |
|
Ok this approach does not work. The start method is calling close again which would require a reentrant lock. I'll rewrite the start/stop mechanics s.t. we don't need locks |
1 participant
Closes #7312
There are weird race conditions possible if start and stop is running concurrently. I think we should just prohibit doing this and lock it similarly to how we lock start. I ended up introducing similar close_unsafe methods as I did for the start_unsafe back in #5910