From c82f30bb89cd5ea086913293522f418e7b417951 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 6 Jan 2025 18:15:23 +0000 Subject: [PATCH 01/11] gh-128552: fix refcycles in eager task creation --- Lib/asyncio/base_events.py | 27 ++++++++++++--------- Lib/asyncio/taskgroups.py | 31 +++++++++++++----------- Lib/test/test_asyncio/test_taskgroups.py | 24 +++++++++++------- 3 files changed, 47 insertions(+), 35 deletions(-) diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 9e6f6e3ee7e3ec..e6e1006af7cfbd 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -463,21 +463,24 @@ def create_task(self, coro, *, name=None, context=None): Return a task object. """ - self._check_closed() - if self._task_factory is None: - task = tasks.Task(coro, loop=self, name=name, context=context) - if task._source_traceback: - del task._source_traceback[-1] - else: - if context is None: - # Use legacy API if context is not needed - task = self._task_factory(self, coro) + try: + self._check_closed() + if self._task_factory is None: + task = tasks.Task(coro, loop=self, name=name, context=context) + if task._source_traceback: + del task._source_traceback[-1] else: - task = self._task_factory(self, coro, context=context) + if context is None: + # Use legacy API if context is not needed + task = self._task_factory(self, coro) + else: + task = self._task_factory(self, coro, context=context) - task.set_name(name) + task.set_name(name) - return task + return task + finally: + del task def set_task_factory(self, factory): """Set a task factory that will be used by loop.create_task(). diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index 9fa772ca9d02cc..b8aea2267fa8b9 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -192,20 +192,23 @@ def create_task(self, coro, *, name=None, context=None): if self._aborting: coro.close() raise RuntimeError(f"TaskGroup {self!r} is shutting down") - if context is None: - task = self._loop.create_task(coro, name=name) - else: - task = self._loop.create_task(coro, name=name, context=context) - - # optimization: Immediately call the done callback if the task is - # already done (e.g. if the coro was able to complete eagerly), - # and skip scheduling a done callback - if task.done(): - self._on_task_done(task) - else: - self._tasks.add(task) - task.add_done_callback(self._on_task_done) - return task + try: + if context is None: + task = self._loop.create_task(coro, name=name) + else: + task = self._loop.create_task(coro, name=name, context=context) + + # optimization: Immediately call the done callback if the task is + # already done (e.g. if the coro was able to complete eagerly), + # and skip scheduling a done callback + if task.done(): + self._on_task_done(task) + else: + self._tasks.add(task) + task.add_done_callback(self._on_task_done) + return task + finally: + del task # Since Python 3.8 Tasks propagate all exceptions correctly, # except for KeyboardInterrupt and SystemExit which are diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index c47bf4ec9ed64b..f0e088b205c0e6 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -12,11 +12,6 @@ from test.test_asyncio.utils import await_without_task -# To prevent a warning "test altered the execution environment" -def tearDownModule(): - asyncio._set_event_loop_policy(None) - - class MyExc(Exception): pass @@ -38,7 +33,7 @@ def no_other_refs(): return [coro] -class TestTaskGroup(unittest.IsolatedAsyncioTestCase): +class BaseTestTaskGroup: async def test_taskgroup_01(self): @@ -832,15 +827,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): @@ -998,5 +993,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() From 8a4eb568f721a6f90d277faf660973f0386f34a8 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 6 Jan 2025 18:18:21 +0000 Subject: [PATCH 02/11] Update Lib/asyncio/base_events.py --- Lib/asyncio/base_events.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index e6e1006af7cfbd..79ebe2ae5e182e 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -463,6 +463,7 @@ def create_task(self, coro, *, name=None, context=None): Return a task object. """ + task = None try: self._check_closed() if self._task_factory is None: From 35a38e4b6545f2fc2b4337425758442bc7c294c8 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 6 Jan 2025 18:21:53 +0000 Subject: [PATCH 03/11] Update Lib/asyncio/taskgroups.py --- Lib/asyncio/taskgroups.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index b8aea2267fa8b9..2819895ca08191 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -192,6 +192,7 @@ def create_task(self, coro, *, name=None, context=None): if self._aborting: coro.close() raise RuntimeError(f"TaskGroup {self!r} is shutting down") + task = None try: if context is None: task = self._loop.create_task(coro, name=name) From afd30537752853888d9dc77d0d7c78313a7ad370 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Mon, 6 Jan 2025 18:41:16 +0000 Subject: [PATCH 04/11] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Library/2025-01-06-18-41-08.gh-issue-gh-128552.fV-f8j.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-gh-128552.fV-f8j.rst diff --git a/Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-gh-128552.fV-f8j.rst b/Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-gh-128552.fV-f8j.rst new file mode 100644 index 00000000000000..cedab4f62f13d1 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-gh-128552.fV-f8j.rst @@ -0,0 +1 @@ +Fix cyclic garbage introduced by :meth:`loop.create_task` and :meth:`asyncio.TaskGroup.create_task` holding a reference to the created task. From 9700ebfedbeb5f764aaabf8b91fec06bf9e3358e Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 6 Jan 2025 18:44:19 +0000 Subject: [PATCH 05/11] Rename 2025-01-06-18-41-08.gh-issue-gh-128552.fV-f8j.rst to 2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst --- ....fV-f8j.rst => 2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename Misc/NEWS.d/next/Library/{2025-01-06-18-41-08.gh-issue-gh-128552.fV-f8j.rst => 2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst} (100%) diff --git a/Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-gh-128552.fV-f8j.rst b/Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst similarity index 100% rename from Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-gh-128552.fV-f8j.rst rename to Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst From fe83addd680a84a39288cfabc12d998fdf1449bf Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 6 Jan 2025 18:55:17 +0000 Subject: [PATCH 06/11] Update Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst --- .../next/Library/2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst b/Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst index cedab4f62f13d1..83816f775da9c5 100644 --- a/Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst +++ b/Misc/NEWS.d/next/Library/2025-01-06-18-41-08.gh-issue-128552.fV-f8j.rst @@ -1 +1 @@ -Fix cyclic garbage introduced by :meth:`loop.create_task` and :meth:`asyncio.TaskGroup.create_task` holding a reference to the created task. +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. From 7c966d0bcd5900b9f3e6a98f40233c6ef4d3b191 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Tue, 7 Jan 2025 08:06:45 +0000 Subject: [PATCH 07/11] add a version of the test with weakrefs --- Lib/test/test_asyncio/test_taskgroups.py | 43 ++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index f0e088b205c0e6..406c1c7ba249fa 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -1,6 +1,7 @@ # Adapted with permission from the EdgeDB project; # license: PSFL. +import weakref import sys import gc import asyncio @@ -33,6 +34,24 @@ def no_other_refs(): return [coro] +def set_gc_state(enabled: bool) -> bool: + was_enabled = gc.isenabled() + if enabled: + gc.enable() + else: + gc.disable() + return was_enabled + + +@contextlib.contextmanager +def disable_gc() -> Generator[None]: + was_enabled = set_gc_state(enabled=False) + try: + yield + finally: + set_gc_state(enabled=was_enabled) + + class BaseTestTaskGroup: async def test_taskgroup_01(self): @@ -960,6 +979,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()) + async def test_exception_refcycles_propagate_cancellation_error(self): """Test that TaskGroup deletes propagate_cancellation_error""" tg = asyncio.TaskGroup() From cf5cfe36e87ea7b09d5534fa3589651a7d228f5e Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Tue, 7 Jan 2025 08:59:19 +0000 Subject: [PATCH 08/11] simplify and comment --- Lib/asyncio/base_events.py | 27 ++++++++++++++------------- Lib/asyncio/taskgroups.py | 29 +++++++++++++++-------------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 79ebe2ae5e182e..830db6454d00c3 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -463,24 +463,25 @@ def create_task(self, coro, *, name=None, context=None): Return a task object. """ - task = None - try: - self._check_closed() - if self._task_factory is None: - task = tasks.Task(coro, loop=self, name=name, context=context) - if task._source_traceback: - del task._source_traceback[-1] + self._check_closed() + if self._task_factory is None: + task = tasks.Task(coro, loop=self, name=name, context=context) + if task._source_traceback: + del task._source_traceback[-1] + else: + if context is None: + # Use legacy API if context is not needed + task = self._task_factory(self, coro) else: - if context is None: - # Use legacy API if context is not needed - task = self._task_factory(self, coro) - else: - task = self._task_factory(self, coro, context=context) + task = self._task_factory(self, coro, context=context) - task.set_name(name) + task.set_name(name) + try: return task finally: + # gh-128552: prevent a refcycle of + # task.exception().__traceback__->TaskGroup.create_task->task del task def set_task_factory(self, factory): diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index 2819895ca08191..8af199d6dcc41a 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -192,23 +192,24 @@ def create_task(self, coro, *, name=None, context=None): if self._aborting: coro.close() raise RuntimeError(f"TaskGroup {self!r} is shutting down") - task = None + if context is None: + task = self._loop.create_task(coro, name=name) + else: + task = self._loop.create_task(coro, name=name, context=context) + + # optimization: Immediately call the done callback if the task is + # already done (e.g. if the coro was able to complete eagerly), + # and skip scheduling a done callback + if task.done(): + self._on_task_done(task) + else: + self._tasks.add(task) + task.add_done_callback(self._on_task_done) try: - if context is None: - task = self._loop.create_task(coro, name=name) - else: - task = self._loop.create_task(coro, name=name, context=context) - - # optimization: Immediately call the done callback if the task is - # already done (e.g. if the coro was able to complete eagerly), - # and skip scheduling a done callback - if task.done(): - self._on_task_done(task) - else: - self._tasks.add(task) - task.add_done_callback(self._on_task_done) 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, From 18b6ea982fdd14c1906c99e6da95fc6da4e47f16 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Tue, 7 Jan 2025 09:00:31 +0000 Subject: [PATCH 09/11] Update Lib/asyncio/base_events.py --- Lib/asyncio/base_events.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 830db6454d00c3..6e6e5aaac15caf 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -481,7 +481,7 @@ def create_task(self, coro, *, name=None, context=None): return task finally: # gh-128552: prevent a refcycle of - # task.exception().__traceback__->TaskGroup.create_task->task + # task.exception().__traceback__->BaseEventLoop.create_task->task del task def set_task_factory(self, factory): From ca7193540d8b984b9f7034d91ff1c74ac35f1fee Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Tue, 7 Jan 2025 11:18:17 +0000 Subject: [PATCH 10/11] restore _set_event_loop_policy(None) --- Lib/test/test_asyncio/test_taskgroups.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index 406c1c7ba249fa..02735e6335715d 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -13,6 +13,11 @@ from test.test_asyncio.utils import await_without_task +# To prevent a warning "test altered the execution environment" +def tearDownModule(): + asyncio._set_event_loop_policy(None) + + class MyExc(Exception): pass From cc39717345e7af1c9f38dc3a16e919ef9764afd6 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Tue, 7 Jan 2025 11:21:12 +0000 Subject: [PATCH 11/11] remove type annotations --- Lib/test/test_asyncio/test_taskgroups.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index 02735e6335715d..870fa8dbbf2714 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -39,7 +39,7 @@ def no_other_refs(): return [coro] -def set_gc_state(enabled: bool) -> bool: +def set_gc_state(enabled): was_enabled = gc.isenabled() if enabled: gc.enable() @@ -49,7 +49,7 @@ def set_gc_state(enabled: bool) -> bool: @contextlib.contextmanager -def disable_gc() -> Generator[None]: +def disable_gc(): was_enabled = set_gc_state(enabled=False) try: yield