Skip to content

gh-128552: fix refcycles in eager task creation #128553

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 13 commits into from
Jan 7, 2025
Merged
7 changes: 6 additions & 1 deletion Lib/asyncio/base_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,12 @@ def create_task(self, coro, *, name=None, context=None):

task.set_name(name)

return task
try:
return task
finally:
# gh-128552: prevent a refcycle of
# task.exception().__traceback__->BaseEventLoop.create_task->task
del task

def set_task_factory(self, factory):
"""Set a task factory that will be used by loop.create_task().
Expand Down
7 changes: 6 additions & 1 deletion Lib/asyncio/taskgroups.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,12 @@ def create_task(self, coro, *, name=None, context=None):
else:
self._tasks.add(task)
task.add_done_callback(self._on_task_done)
return task
try:
return task
finally:
# gh-128552: prevent a refcycle of
# task.exception().__traceback__->TaskGroup.create_task->task
del task

# Since Python 3.8 Tasks propagate all exceptions correctly,
# except for KeyboardInterrupt and SystemExit which are
Expand Down
62 changes: 58 additions & 4 deletions Lib/test/test_asyncio/test_taskgroups.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Adapted with permission from the EdgeDB project;
# license: PSFL.

import weakref
import sys
import gc
import asyncio
Expand Down Expand Up @@ -38,7 +39,25 @@ def no_other_refs():
return [coro]


class TestTaskGroup(unittest.IsolatedAsyncioTestCase):
def set_gc_state(enabled):
was_enabled = gc.isenabled()
if enabled:
gc.enable()
else:
gc.disable()
return was_enabled


@contextlib.contextmanager
def disable_gc():
was_enabled = set_gc_state(enabled=False)
try:
yield
finally:
set_gc_state(enabled=was_enabled)


class BaseTestTaskGroup:

async def test_taskgroup_01(self):

Expand Down Expand Up @@ -832,15 +851,15 @@ async def test_taskgroup_without_parent_task(self):
with self.assertRaisesRegex(RuntimeError, "has not been entered"):
tg.create_task(coro)

def test_coro_closed_when_tg_closed(self):
async def test_coro_closed_when_tg_closed(self):
async def run_coro_after_tg_closes():
async with taskgroups.TaskGroup() as tg:
pass
coro = asyncio.sleep(0)
with self.assertRaisesRegex(RuntimeError, "is finished"):
tg.create_task(coro)
loop = asyncio.get_event_loop()
loop.run_until_complete(run_coro_after_tg_closes())

await run_coro_after_tg_closes()

async def test_cancelling_level_preserved(self):
async def raise_after(t, e):
Expand Down Expand Up @@ -965,6 +984,30 @@ async def coro_fn():
self.assertIsInstance(exc, _Done)
self.assertListEqual(gc.get_referrers(exc), no_other_refs())


async def test_exception_refcycles_parent_task_wr(self):
"""Test that TaskGroup deletes self._parent_task and create_task() deletes task"""
tg = asyncio.TaskGroup()
exc = None

class _Done(Exception):
pass

async def coro_fn():
async with tg:
raise _Done

with disable_gc():
try:
async with asyncio.TaskGroup() as tg2:
task_wr = weakref.ref(tg2.create_task(coro_fn()))
except* _Done as excs:
exc = excs.exceptions[0].exceptions[0]

self.assertIsNone(task_wr())
self.assertIsInstance(exc, _Done)
self.assertListEqual(gc.get_referrers(exc), no_other_refs())
Copy link
Contributor

@kumaraditya303 kumaraditya303 Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was later changed that coro doesn't hold ref to locals, no_other_refs should not be necessary here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah its not merged yet #125640


async def test_exception_refcycles_propagate_cancellation_error(self):
"""Test that TaskGroup deletes propagate_cancellation_error"""
tg = asyncio.TaskGroup()
Expand Down Expand Up @@ -998,5 +1041,16 @@ class MyKeyboardInterrupt(KeyboardInterrupt):
self.assertListEqual(gc.get_referrers(exc), no_other_refs())


class TestTaskGroup(BaseTestTaskGroup, unittest.IsolatedAsyncioTestCase):
loop_factory = asyncio.EventLoop

class TestEagerTaskTaskGroup(BaseTestTaskGroup, unittest.IsolatedAsyncioTestCase):
@staticmethod
def loop_factory():
loop = asyncio.EventLoop()
loop.set_task_factory(asyncio.eager_task_factory)
return loop


if __name__ == "__main__":
unittest.main()
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix cyclic garbage introduced by :meth:`asyncio.loop.create_task` and :meth:`asyncio.TaskGroup.create_task` holding a reference to the created task if it is eager.
Loading