From 70d1e1170ea5fd229bb0ce3e51eda8bc6c52d8dc Mon Sep 17 00:00:00 2001 From: SomberNight Date: Tue, 8 Apr 2025 18:54:58 +0000 Subject: [PATCH] asyncio: clarify strong refs for run_coroutine_threadsafe We added some code in https://github.com/spesmilo/electrum/commit/0b3a28358681a0fe101bd6fc3feb5ff981fb68ce to explicitly hold strong refs for all tasks/futures. At the time I was uncertain if that also solves GC issues with asyncio.run_coroutine_threadsafe. ref https://github.com/spesmilo/electrum/pull/9608#issuecomment-2703681663 Looks like it does. run_coroutine_threadsafe *is* going through the custom task factory. See the unit test. The somewhat confusing thing is that we need a few event loop iterations for the task factory to run, due to how run_coroutine_threadsafe is implemented. And also, the task that we will hold as strong ref in the global set is not the concurrent.futures.Future that run_coroutine_threadsafe returns. So this commit simply "fixes" the unit test so that it showcases this, and removes related, older, plumbing from util.py that we now know is no longer needed because of this. --- electrum/util.py | 6 +----- tests/test_util.py | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/electrum/util.py b/electrum/util.py index 2bab55e02..cc231ba89 100644 --- a/electrum/util.py +++ b/electrum/util.py @@ -1684,7 +1684,7 @@ def _set_custom_task_factory(loop: asyncio.AbstractEventLoop): - "asyncio.create_task" - "loop.create_task" - "asyncio.ensure_future" - - what about "asyncio.run_coroutine_threadsafe"? not sure if that is safe. + - "asyncio.run_coroutine_threadsafe" related: - https://bugs.python.org/issue44665 @@ -1858,7 +1858,6 @@ class CallbackManager(Logger): Logger.__init__(self) self.callback_lock = threading.Lock() self.callbacks = defaultdict(list) # note: needs self.callback_lock - self._running_cb_futs = set() def register_callback(self, func, events): with self.callback_lock: @@ -1883,11 +1882,8 @@ class CallbackManager(Logger): for callback in callbacks: if asyncio.iscoroutinefunction(callback): # async cb fut = asyncio.run_coroutine_threadsafe(callback(*args), loop) - # keep strong references around to avoid GC issues: - self._running_cb_futs.add(fut) def on_done(fut_: concurrent.futures.Future): assert fut_.done() - self._running_cb_futs.remove(fut_) if fut_.cancelled(): self.logger.debug(f"cb cancelled. {event=}.") elif exc := fut_.exception(): diff --git a/tests/test_util.py b/tests/test_util.py index 7f9018233..7c3ba1d04 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -482,20 +482,30 @@ class TestUtil(ElectrumTestCase): async def foo(): await evt.wait() + # spawn tasks fut = asyncio.ensure_future(foo()) self.assertTrue(fut in util._running_asyncio_tasks) fut = asyncio.create_task(foo()) self.assertTrue(fut in util._running_asyncio_tasks) fut = loop.create_task(foo()) self.assertTrue(fut in util._running_asyncio_tasks) - #fut = asyncio.run_coroutine_threadsafe(foobar(), loop=loop) + fut = asyncio.run_coroutine_threadsafe(foo(), loop=loop) + # run_coroutine_threadsafe will create a different (chained) future in _running_asyncio_tasks + # (which btw will only happen a few event loop iterations later) #self.assertTrue(fut in util._running_asyncio_tasks) + # wait a few event loop iterations + for _ in range(10): + await asyncio.sleep(0) # we should have stored one ref for each above. # (though what if test framework is doing stuff ~concurrently?) - self.assertEqual(3, len(util._running_asyncio_tasks)) + self.assertEqual(4, len(util._running_asyncio_tasks)) + for task in util._running_asyncio_tasks: + self.assertEqual(foo().__qualname__, task.get_coro().__qualname__) + # let tasks finish evt.set() - for _ in range(10): # wait a few event loop iterations + # wait a few event loop iterations + for _ in range(10): await asyncio.sleep(0) # refs should be cleaned up by now: self.assertEqual(0, len(util._running_asyncio_tasks))