From 614d20ff58565fee30bc61667c5c21f9046d2e96 Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Mon, 28 Oct 2024 22:55:24 -0400 Subject: [PATCH 1/4] Fix ref leak and add tests --- Lib/test/test_asyncio/test_tasks.py | 19 +++++++++++++++++++ ...-10-28-22-35-22.gh-issue-126083.TuI--n.rst | 2 ++ Modules/_asynciomodule.c | 2 +- 3 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Library/2024-10-28-22-35-22.gh-issue-126083.TuI--n.rst diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py index a1013ab803348d..0db7b4c222abdc 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -2688,6 +2688,25 @@ def test_get_context(self): finally: loop.close() + def test_proper_refcounts(self): + class Break: + def __str__(self): + raise Exception("break") + + obj = object() + initial_refcount = sys.getrefcount(obj) + + coro = coroutine_function() + task = asyncio.Task.__new__(asyncio.Task) + + for _ in range(10000): + try: + task.__init__(coro, context=obj, name=Break()) + except: pass + del task + + self.assertEqual(sys.getrefcount(obj), initial_refcount) + def add_subclass_tests(cls): BaseTask = cls.Task diff --git a/Misc/NEWS.d/next/Library/2024-10-28-22-35-22.gh-issue-126083.TuI--n.rst b/Misc/NEWS.d/next/Library/2024-10-28-22-35-22.gh-issue-126083.TuI--n.rst new file mode 100644 index 00000000000000..5b02e41302f786 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-28-22-35-22.gh-issue-126083.TuI--n.rst @@ -0,0 +1,2 @@ +Fixed a reference leak that would happen in asyncio tasks when you would +reinitialize your task object with a non-None context. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index d4135f04e56575..c2500fbd692d4d 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -2120,7 +2120,7 @@ _asyncio_Task___init___impl(TaskObj *self, PyObject *coro, PyObject *loop, return -1; } } else { - self->task_context = Py_NewRef(context); + Py_XSETREF(self->task_context, Py_NewRef(context)); } Py_CLEAR(self->task_fut_waiter); From 8e5effea39ba65713fba171239a9b42f2a486e38 Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Tue, 29 Oct 2024 10:37:41 -0400 Subject: [PATCH 2/4] Address review comments --- Lib/test/test_asyncio/test_tasks.py | 15 +++++++++++---- ...2024-10-28-22-35-22.gh-issue-126083.TuI--n.rst | 3 +-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py index 0db7b4c222abdc..06219a0dde4b4e 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -2689,20 +2689,27 @@ def test_get_context(self): loop.close() def test_proper_refcounts(self): + # see: https://github.com/python/cpython/issues/126083 class Break: def __str__(self): - raise Exception("break") + raise RuntimeError("break") obj = object() initial_refcount = sys.getrefcount(obj) coro = coroutine_function() + loop = asyncio.new_event_loop() task = asyncio.Task.__new__(asyncio.Task) - for _ in range(10000): + for _ in range(5): try: - task.__init__(coro, context=obj, name=Break()) - except: pass + task.__init__(coro, loop=loop, context=obj, name=Break()) + except Exception as e: + # exception should only come from Break.__str__ as it's used to + # avoid having to do too much setup for this test + self.assertIs(type(e), RuntimeError) + self.assertEqual(e.args[0], "break") + coro.close() del task self.assertEqual(sys.getrefcount(obj), initial_refcount) diff --git a/Misc/NEWS.d/next/Library/2024-10-28-22-35-22.gh-issue-126083.TuI--n.rst b/Misc/NEWS.d/next/Library/2024-10-28-22-35-22.gh-issue-126083.TuI--n.rst index 5b02e41302f786..d64b7dd2fedbd6 100644 --- a/Misc/NEWS.d/next/Library/2024-10-28-22-35-22.gh-issue-126083.TuI--n.rst +++ b/Misc/NEWS.d/next/Library/2024-10-28-22-35-22.gh-issue-126083.TuI--n.rst @@ -1,2 +1 @@ -Fixed a reference leak that would happen in asyncio tasks when you would -reinitialize your task object with a non-None context. +Fixed a reference leak in :class:`asyncio.Task` objects when reinitializing the same object with a non-``None`` context. Patch by Nico Posada. From a588f7b7deb5485f1bd7c9e47db2a1307104a720 Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Tue, 29 Oct 2024 10:40:34 -0400 Subject: [PATCH 3/4] fix linter errors --- Lib/test/test_asyncio/test_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py index 06219a0dde4b4e..f2e5f263723352 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -2693,7 +2693,7 @@ def test_proper_refcounts(self): class Break: def __str__(self): raise RuntimeError("break") - + obj = object() initial_refcount = sys.getrefcount(obj) From 2e70b5198a502298dcdeeb7624d93c0290774bea Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Wed, 30 Oct 2024 09:01:43 -0400 Subject: [PATCH 4/4] Address sobolevn's review --- Lib/test/test_asyncio/test_tasks.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py index f2e5f263723352..9d2d356631b42c 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -2702,13 +2702,9 @@ def __str__(self): task = asyncio.Task.__new__(asyncio.Task) for _ in range(5): - try: + with self.assertRaisesRegex(RuntimeError, 'break'): task.__init__(coro, loop=loop, context=obj, name=Break()) - except Exception as e: - # exception should only come from Break.__str__ as it's used to - # avoid having to do too much setup for this test - self.assertIs(type(e), RuntimeError) - self.assertEqual(e.args[0], "break") + coro.close() del task