Always close BatchedSend write coroutines #6865
Conversation
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 37m 27s ⏱️ - 4m 28s Results for commit 9d302d7. ± Comparison against base commit e38d3a9. |
|
Nice green check mark 🙂 |
|
@graingert did I actually get the explanation right here? I don't feel like I did. |
| ) | ||
| # NOTE: Since `BatchedSend` doesn't have a handle on the running | ||
| # `_background_send` coroutine, the only thing with a reference to this | ||
| # coroutine is the event loop itself. If the event loop stops while |
There was a problem hiding this comment.
I think this comment is correct, but it's missing an edge case, the event loop doesn't even keep a reference to the coro, necessarily.
it's loop._ready, loop._scheduled, loop._selector -> Handle -> Future.callback(self) -> Future._callbacks -> Task.__step(self), -> Task._coro
Highly recommend building a fun picture in objgraph of this ^
and when you call loop.close() it wipes out all the Handles in ready, scheduled and selector
however you can also create a Future wait on it in a Task and then lose the Future: boom the coro goes with it. running the finally block on whichever poor thread upset the garbage collector.
I don't think you should change the comment, but I think it's worth having this idea written down somewhere ^
There was a problem hiding this comment.
actually on second thought maybe it is worth a change here, because maybe it explains why you can't fix this with yield asyncio.create_task(self.comm.write(...)). Or maybe It just depends on if the hammer falls before or after asyncio.get_running_loop() starts failing
|
are there any other gen.coroutines we need to to fix? |
The only other critical path coroutine is afaik, all others are "only" tests / utils_test |
|
Thanks @gjoseph92 and @graingert ! |
From #6863 (comment).
@graingert I'm not sure my explanation of this is even close to correct. Please fix as appropriate.
Closes #6551
pre-commit run --all-filestest_dont_select_closed_workerflaky #6507test_local_cluster_redundant_kwarg[True]flaky #6506test_cleanupleaked #6451test_run_spec#6549test_localcluster_start_exception#6769