Repository navigation
asyncio: Use strong references for free-flying tasks #91887
Description
Activity
You can use the new
asyncio.TaskGroupto avoid this in 3.11+It looks to me like it would be a trivial code change (keeping all active tasks in a separate, non-weak set) to gain a large benefit here.
This behavior is inherited from Futures (where it makes sense). If a future is not being kept track of, it's a bug (the developer forgot to await). However, tasks start executing, if not immediately, on their own, so not awaiting a task is not as obviously a mistake.
Interrupting actively running code because someone didn't maintain a reference (on the gc's terms and timing) is a very different beast than not executing code that the desire and timing of which is unclear (and an exception can be somewhat promptly triggered for).
I'll have time this evening to open a PR, if nobody else wants to get to it.
Reacted by Max Kühn, Elchin Sarkarov, Daniel, Malcolm Smith and Alisa SirenevaFor impact perspective, discord.py drops the task after calling create_task on it.
This means (effectively) every python based discord bot is affected by this, and every event dispatch appears to be technically at the whims of the GC.
Reacted by PearsteamRather than rhetoric about impact or how trivial the fix would be, we need a discussion on why we designed tasks this way in the first place. It doesn't look to me like it was inherited from Futures -- the variable is called
_all_tasks, not_all_futures. Without understanding this, we risk making things worse with a hasty "fix".That said, I'm not sure why we designed it this way, and (from private email) @1st1 doesn't seem to recall either. But it was definitely designed with some purpose in mind -- the code is quite complex and was updated every time an issue with it was discovered, and we've had many opportunities to change this behavior but instead chose to update the docs (gh-29163), even when asyncio itself suffered (gh-90467).
I also don't understand why the dict of all tasks is a global, since the only API that uses this (
all_tasks()) filters out tasks belonging to a different event loop. Again, I assume there's a reason for the complexity, but I don't know what it is. Is it 3rd party event loops like uvloop? Or frameworks like aiohttp?FWIW if we simply make
_all_tasksa plain dict, we would have to arrange for tasks to remove themselves from the list when they exit, possibly in a done-callback (though there's a cost to that). There's already an API to do that (_unregister_task) but it doesn't seem to be called.Agree that this should be approved by someone who is familiar with the code's design objectives, which is why I brought up two alternatives and did not create a PR right away.
I would not turn_all_tasksinto a plain dict (or set), since this might break code that assumes that finished tasks (that have a user reference somewhere) are still returned byall_tasks(). The suggested change is to have an additional_pending_tasksset. Yes, we would need a done-callback, which would introduce a slight performance penalty.
In any case, imho the current behavior is somewhat annoying or misses some fundamental functionality.Sorry, I've been doing more reading into what the current state is and other related issues since I wrote that comment and it is absolutely a lot more detailed behind the scenes (like this comment here I only got to today: #80788 (comment) ) which mirrors some of what you've sent here.
I was working with the idea that Tasks were inherited from Futures because that was how they were introduced in the original PEP-3156.
I think I have found the origin of why it was made a weakset in the first place: https://groups.google.com/g/python-tulip/c/13hfgbKrIyY/m/HpwWPGHKT6IJ
Specifically, this patch:
https://codereview.appspot.com/14295043/diff/1/tulip/tasks.pySo it was originally intended to be a registry to help recover stuck tasks and get stack frames for?
Reacted by Max Kühn and Alexander Kozlovsky(Sorry, I hit some wrong buttons.)
So it was originally intended to be a registry to help recover stuck tasks and get stack frames for?
But apparently at that point it was already the case that tasks had to be kept alive by their owner -- ISTM a weakset was used specifically to avoid keeping tasks alive longer than necessary.
So this has always been part of the design, it just wasn't made explicit in the docs. I'm still curious why we originally designed it that way. It's possible that we never consciously realized this constraint. It's also possible that, since we did make Task a subclass of Future, we assumed that tasks would always be awaited.
I am still not convinced, despite this being a common stumbling point, that we can fix this without consequences for use code.
42 remaining items
I've been meaning to come back to this issue and provide some tangible change suggestions. So I would appreciate it if things could be kept open for a while longer. :)
Let's call the example with classes
Proto,ReaderandLogFunca red herring. Yes, it shows that such a structure is possible without tasks, and I believe that asyncio's stream reader design has a similar structure, but I believe that if someone is using a stream reader without tasks they have plenty of opportunity to put in hard references, and they should.So let's focus on the shorter example with coroutines
background()andcleanup(), which demonstrates that there's a more general issue when tasks are not kept alive by the user (regardless of whether they are using stream readers).Like Ben, I don't immediately see a downside to having a global set of pending tasks either, as long as tasks are guaranteed to remove themselves from it when they complete (and without the need for
__del__).That doesn't mean there isn't a downside, but we'll never find out until we try. We'll probably have to ask @1st1 to think about this -- there might be a reason that involves uvloop or Edgestore. Possibly the fact that uvloop has its own task factory makes things more complicated -- we should definitely think about that some more. Perhaps we can use a done callback that unlinks the task when it completes?
@CendioOssman, are you interested in coming up with a PR? Or do you continue to feel that the other example must also be fixed? In that case we may have to agree to disagree, and someone else can create a PR.
Here's a concrete example of a hack to workaround this issue https://github.com/Falmarri/podman-compose/blob/8d8fa54855ce7eb73e802ef06e9f48645a30e2ac/podman_compose.py#L1229-L1237
It's possible there's a better way of structuring this, but IMO this should just be the default where I don't have to care about this. I can just start these tasks and not have to worry.
Reacted by Luccccifer and 5j9I have a feeling we need a new champion to drive a PR here. I think a global or per-loop (non-weak) set of active tasks should solve the issue, but there are a bunch of details that need sorting through -- notably how we ensure that 3rd party tasks (e.g. from uvloop) are properly inserted into and removed from the set, without requiring changes to the 3rd party library.
I have a feeling we need a new champion to drive a PR here. I think a global or per-loop (non-weak) set of active tasks should solve the issue
cc'ing @kumaraditya303, in relation to gh-104787 and #80788 (comment) - are you still planning to tackle this for 3.13?
@itamaro @kumaraditya303 Unless one of you is actively working on a PR for this, I would recommend we move this item back to TO DO. Thoughts?
Reacted by Itamar Oren@itamaro @kumaraditya303 Unless one of you is actively working on a PR for this, I would recommend we move this item back to TO DO. Thoughts?
Agreed. I haven't worked on this.
Reacted by Carol WillingMoving back to To Do for now.
Flagging this issue for discussion at the core dev sprint unless there is a champion before the sprint.
Reacted by Itamar OrenI have created a draft PR at #121264. In this PR, I have not made this feature optional. I'm open to adding the task to
_pending_tasksonly if an optional keyword argument is set.Potential for Memory Leaks
Whenever a future's state transitions from the
_PENDINGstate (due to finishing, cancelling or an exception),_finish_executionwill be triggered and the task will be removed from_pending_tasks. I've implemented the_pending_tasksset as an attribute of the event loop to ensure that no memory leaks are possible when asyncio is deinitialized. I.e. when dropping all references to the loop, and there still is a pending task, you will still get the "Task was destroyed but it is pending!" error. I think this is much more predictable and robust than the current behavior.Use of
_unregister_taskOn calling
_unregister_task, asyncio currently removes the task from_scheduled_tasks. asyncio's documentation for_unregister_tasksays "The function should be called when a task is about to finish.". Together, this is inconsistent with asyncio's main implementation, which does not remove tasks from_scheduled_taskswhen they're finishing, but only when they're deleted.
I've changed_unregister_taskto remove the task only from_pending_tasksbut not from_scheduled_tasks, which makes it consistent with the documentation. This might, of course, break old code that relies on the old behavior. The only code I could find online that uses the_unregister_taskinterface is Tornado. Tornado is consistent with the documented behavior, i.e. the new implementation.uvloop
As far as I can see,
uvloopuses asyncio'sTask. Therefore, tasks will be registered and unregistered correctly in_pending_taskswithinTask.__init__andTask._finish_execution.Reacted by 5j9- added a commit that references this issue
on Jul 12, 2024 - added a commit that references this issue
on Mar 5, 2025 - added a commit that references this issue
on May 2, 2026 - added a commit that references this issue
on Aug 31, 2026
In #88831 @vincentbernat pointed out that CPython only keeps weak references in
_all_tasks, so a reference to aTaskreturned byloop.create_taskhas to be kept to be sure the task will not be killed with a "Task was destroyed but it is pending!" at some random point in time.When shielding a task from cancellation with
await shield(something()),somethingcontinues to run when the containing coroutine is cancelled. As soon as that happens,something()is free-flying, i.e. there's no reference from user code anymore.shielditself has a bunch of circular strong references, but these shouldn't keep CPython from garbage-collecting the task. Hence, here the same problem occurs and the task might be killed unpredictably. Additionally, when running coroutines in parallel withgatherandreturn_exceptions=False, an exception in one of the coroutines will leave remaining tasks free-flying. Also in this case, the remaining tasks might be killed unpredictably.Hence, a warning in the documentation for
create_taskunfortunately does not suffice to solve the problem. Additionally, it has been brought up in #88831 that an API for fire-and-forget tasks (i.e. when the user doesn't want to keep a reference) would be nice.As solution, I suggest to either
(1) introduce a further
_pending_tasksset to keep strong references to all pending tasks. This would be the simplest solution also with respect to the API. In fact, a lot of dicussions on Stack Overflow (e.g., here, here, here) already rely on this behavior (throwing away the reference returned bycreate_task), although it's wrong currently. Since the behaviour for free-flying tasks is unpredictable currently, it should not introduce any compatibility issues when making it predictable by preventing them from being garbage-collected.(2) make sure there's always a chain of strong references from the most basic futures to the running tasks awaiting something. A quick
grepresulted in potential problems, e.g., here and here. This does not seem like a very robust approach, though.(3) introduce the concept of background tasks, i.e., tasks the user does not want to hold references to. The interface could look like suggested in #88831 (comment) . Tasks from
shieldandgathercould be automatically converted to such background tasks. Clearly, it would add complexity to the API, but the distinction between normal tasks and background tasks might potentially be beneficial also for other purposes. E.g., one might add an API call that waits for all background tasks to be completed.My preferred solution would be (1).
Linked PRs