Skip to content

asyncio: hold our own strong refs for tasks and futures #9608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

SomberNight
Copy link
Member

see https://docs.python.org/3.13/library/asyncio-task.html#asyncio.create_task :

Important
Save a reference to the result of this function, to avoid a task
disappearing mid-execution. The event loop only keeps weak references
to tasks. A task that isn’t referenced elsewhere may get garbage
collected at any time, even before it’s done. For reliable
“fire-and-forget” background tasks, gather them in a collection

ref python/cpython#91887

see https://docs.python.org/3.13/library/asyncio-task.html#asyncio.create_task :

> Important
>
> Save a reference to the result of this function, to avoid a task
> disappearing mid-execution. The event loop only keeps weak references
> to tasks. A task that isn’t referenced elsewhere may get garbage
> collected at any time, even before it’s done. For reliable
> “fire-and-forget” background tasks, gather them in a collection

ref python/cpython#91887
ref beeware/toga#2814
@SomberNight
Copy link
Member Author

for example, this kind of usage is not safe (without this PR):

fut = asyncio.ensure_future(wrapped_callback())

@accumulator
Copy link
Member

Interesting read...

Does this make this existing specific strong reference storing code redundant?

electrum/electrum/util.py

Lines 1883 to 1893 in 0b3a283

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():
self.logger.error(f"cb errored. {event=}. {exc=}", exc_info=exc)
fut.add_done_callback(on_done)

@SomberNight
Copy link
Member Author

Does this make this existing specific strong reference storing code redundant?

electrum/electrum/util.py

Lines 1883 to 1893 in 0b3a283

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():
self.logger.error(f"cb errored. {event=}. {exc=}", exc_info=exc)
fut.add_done_callback(on_done)

Well, I don't know.
I had an earlier version of this commit where I undid that, yes.
But ultimately I don't know if run_coroutine_threadsafe is affected by this GC issue at all.
I tried to ask in python/cpython#88831 (comment), but never got a reply.

If it is affected, this current PR might not help it. When writing the unit test, I also tried to check if run_coroutine_threadsafe is going through the task factory and it seems it is not...

@SomberNight
Copy link
Member Author

Anyway, I'll merge this. It cannot hurt.

@SomberNight SomberNight merged commit 32b0e62 into spesmilo:master Mar 6, 2025
14 checks passed
SomberNight added a commit that referenced this pull request Apr 8, 2025
We added some code in 0b3a283
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 #9608 (comment)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants