From be1178784f1fb1a2dc121be84527e9ab857b229e Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Thu, 17 Oct 2024 09:40:23 +0100 Subject: [PATCH 1/9] add test for gc in foreign async generators --- src/trio/_core/_tests/test_guest_mode.py | 49 ++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/trio/_core/_tests/test_guest_mode.py b/src/trio/_core/_tests/test_guest_mode.py index 1a8b230e78..e6d0e9deef 100644 --- a/src/trio/_core/_tests/test_guest_mode.py +++ b/src/trio/_core/_tests/test_guest_mode.py @@ -11,6 +11,7 @@ import time import traceback import warnings +import weakref from functools import partial from math import inf from typing import ( @@ -664,3 +665,51 @@ async def trio_main() -> None: context.run(aiotrio_run, trio_main, host_uses_signal_set_wakeup_fd=True) assert record == {("asyncio", "asyncio"), ("trio", "trio")} + + +@restore_unraisablehook() +def test_guest_mode_asyncgens_garbage_collection() -> None: + import sniffio + + record: set[tuple[str, str, bool]] = set() + + async def agen(label: str) -> AsyncGenerator[int, None]: + class A: + pass + + a = A() + a_wr = weakref.ref(a) + assert sniffio.current_async_library() == label + try: + yield 1 + finally: + library = sniffio.current_async_library() + with contextlib.suppress(trio.Cancelled): + await sys.modules[library].sleep(0) + + del a + if sys.implementation.name == "pypy": + gc_collect_harder() + + record.add((label, library, a_wr() is None)) + + async def iterate_in_aio() -> None: + await agen("asyncio").asend(None) + + async def trio_main() -> None: + task = asyncio.ensure_future(iterate_in_aio()) + done_evt = trio.Event() + task.add_done_callback(lambda _: done_evt.set()) + with trio.fail_after(1): + await done_evt.wait() + + await agen("trio").asend(None) + + gc_collect_harder() + + # Ensure we don't pollute the thread-level context if run under + # an asyncio without contextvars support (3.6) + context = contextvars.copy_context() + context.run(aiotrio_run, trio_main, host_uses_signal_set_wakeup_fd=True) + + assert record == {("asyncio", "asyncio", True), ("trio", "trio", True)} From 0ec9f8abb117c67cf0a22739410b11706de1645b Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Thu, 17 Oct 2024 09:41:48 +0100 Subject: [PATCH 2/9] use a strong set of ids for foriegn async gens --- src/trio/_core/_asyncgens.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/trio/_core/_asyncgens.py b/src/trio/_core/_asyncgens.py index 77f1c7eced..76963a352a 100644 --- a/src/trio/_core/_asyncgens.py +++ b/src/trio/_core/_asyncgens.py @@ -36,6 +36,11 @@ class AsyncGenerators: # regular set so we don't have to deal with GC firing at # unexpected times. alive: _WEAK_ASYNC_GEN_SET | _ASYNC_GEN_SET = attrs.Factory(_WEAK_ASYNC_GEN_SET) + # The ids of foreign async generators are added to this set when first + # iterated. Usually it is not safe to refer to ids like this, but because + # we're using a finalizer we can ensure ids in this set do not outlive + # their async generator. + foreign: set[int] = attrs.Factory(set) # This collects async generators that get garbage collected during # the one-tick window between the system nursery closing and the @@ -52,10 +57,7 @@ def firstiter(agen: AsyncGeneratorType[object, NoReturn]) -> None: # An async generator first iterated outside of a Trio # task doesn't belong to Trio. Probably we're in guest # mode and the async generator belongs to our host. - # The locals dictionary is the only good place to - # remember this fact, at least until - # https://bugs.python.org/issue40916 is implemented. - agen.ag_frame.f_locals["@trio_foreign_asyncgen"] = True + self.foreign.add(id(agen)) if self.prev_hooks.firstiter is not None: self.prev_hooks.firstiter(agen) @@ -80,8 +82,9 @@ def finalize_in_trio_context( def finalizer(agen: AsyncGeneratorType[object, NoReturn]) -> None: agen_name = name_asyncgen(agen) try: - is_ours = not agen.ag_frame.f_locals.get("@trio_foreign_asyncgen") - except AttributeError: # pragma: no cover + self.foreign.remove(id(agen)) + is_ours = False + except KeyError: is_ours = True if is_ours: From 1504187949350295af8497859d8210130c851e1d Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Thu, 17 Oct 2024 19:09:13 +0100 Subject: [PATCH 3/9] Update src/trio/_core/_asyncgens.py --- src/trio/_core/_asyncgens.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/trio/_core/_asyncgens.py b/src/trio/_core/_asyncgens.py index 76963a352a..6bb6ceeedc 100644 --- a/src/trio/_core/_asyncgens.py +++ b/src/trio/_core/_asyncgens.py @@ -83,9 +83,10 @@ def finalizer(agen: AsyncGeneratorType[object, NoReturn]) -> None: agen_name = name_asyncgen(agen) try: self.foreign.remove(id(agen)) - is_ours = False except KeyError: is_ours = True + else: + is_ours = False if is_ours: runner.entry_queue.run_sync_soon( From 64ce3c4f450cd969f6241e25502266eab6e733f5 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Thu, 17 Oct 2024 19:21:03 +0100 Subject: [PATCH 4/9] add newsfragment --- newsfragments/3112.bugfix.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 newsfragments/3112.bugfix.rst diff --git a/newsfragments/3112.bugfix.rst b/newsfragments/3112.bugfix.rst new file mode 100644 index 0000000000..33a268aaae --- /dev/null +++ b/newsfragments/3112.bugfix.rst @@ -0,0 +1,5 @@ +Rework foriegn async generator finalization to track async generator +ids rather than mutating ``ag_frame.f_locals``. This fixes an issue +with the previous implementation: locals' lifetimes will no longer be +extended by materialization in the ``ag_frame.f_locals`` dictionary that +the previous finalization dispatcher logic needed to access to do its work. From f435d358f0c026d3a136912cef281e61cd8581ef Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Thu, 17 Oct 2024 19:22:36 +0100 Subject: [PATCH 5/9] Update newsfragments/3112.bugfix.rst --- newsfragments/3112.bugfix.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/newsfragments/3112.bugfix.rst b/newsfragments/3112.bugfix.rst index 33a268aaae..c34d035520 100644 --- a/newsfragments/3112.bugfix.rst +++ b/newsfragments/3112.bugfix.rst @@ -1,4 +1,4 @@ -Rework foriegn async generator finalization to track async generator +Rework foreign async generator finalization to track async generator ids rather than mutating ``ag_frame.f_locals``. This fixes an issue with the previous implementation: locals' lifetimes will no longer be extended by materialization in the ``ag_frame.f_locals`` dictionary that From b3cdced916ea39f614065a4eef6f7450084db80b Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Thu, 17 Oct 2024 20:57:38 +0100 Subject: [PATCH 6/9] move name_asyncgen later than self.foreign.remove as it's critical that self.foriegn.remove runs --- src/trio/_core/_asyncgens.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_core/_asyncgens.py b/src/trio/_core/_asyncgens.py index 6bb6ceeedc..22ae536cea 100644 --- a/src/trio/_core/_asyncgens.py +++ b/src/trio/_core/_asyncgens.py @@ -80,7 +80,6 @@ def finalize_in_trio_context( self.trailing_needs_finalize.add(agen) def finalizer(agen: AsyncGeneratorType[object, NoReturn]) -> None: - agen_name = name_asyncgen(agen) try: self.foreign.remove(id(agen)) except KeyError: @@ -88,6 +87,7 @@ def finalizer(agen: AsyncGeneratorType[object, NoReturn]) -> None: else: is_ours = False + agen_name = name_asyncgen(agen) if is_ours: runner.entry_queue.run_sync_soon( finalize_in_trio_context, From 1eae0c5166931dfc42f84b9a9d8d6a9752a51cb1 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 18 Oct 2024 08:42:31 +0100 Subject: [PATCH 7/9] remove 3.6 compat code --- src/trio/_core/_tests/test_guest_mode.py | 25 +++++++----------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/trio/_core/_tests/test_guest_mode.py b/src/trio/_core/_tests/test_guest_mode.py index e6d0e9deef..c4e161c627 100644 --- a/src/trio/_core/_tests/test_guest_mode.py +++ b/src/trio/_core/_tests/test_guest_mode.py @@ -2,7 +2,6 @@ import asyncio import contextlib -import contextvars import queue import signal import socket @@ -439,12 +438,11 @@ def aiotrio_run( pass_not_threadsafe: bool = True, **start_guest_run_kwargs: Any, ) -> T: - loop = asyncio.new_event_loop() - async def aio_main() -> T: - trio_done_fut = loop.create_future() + loop = asyncio.get_running_loop() + trio_done_fut: asyncio.Future[Outcome[T]] = loop.create_future() - def trio_done_callback(main_outcome: Outcome[object]) -> None: + def trio_done_callback(main_outcome: Outcome[T]) -> None: print(f"trio_fn finished: {main_outcome!r}") trio_done_fut.set_result(main_outcome) @@ -458,12 +456,9 @@ def trio_done_callback(main_outcome: Outcome[object]) -> None: **start_guest_run_kwargs, ) - return (await trio_done_fut).unwrap() # type: ignore[no-any-return] + return (await trio_done_fut).unwrap() - try: - return loop.run_until_complete(aio_main()) - finally: - loop.close() + return asyncio.run(aio_main()) def test_guest_mode_on_asyncio() -> None: @@ -659,10 +654,7 @@ async def trio_main() -> None: gc_collect_harder() - # Ensure we don't pollute the thread-level context if run under - # an asyncio without contextvars support (3.6) - context = contextvars.copy_context() - context.run(aiotrio_run, trio_main, host_uses_signal_set_wakeup_fd=True) + aiotrio_run(trio_main, host_uses_signal_set_wakeup_fd=True) assert record == {("asyncio", "asyncio"), ("trio", "trio")} @@ -707,9 +699,6 @@ async def trio_main() -> None: gc_collect_harder() - # Ensure we don't pollute the thread-level context if run under - # an asyncio without contextvars support (3.6) - context = contextvars.copy_context() - context.run(aiotrio_run, trio_main, host_uses_signal_set_wakeup_fd=True) + aiotrio_run(trio_main, host_uses_signal_set_wakeup_fd=True) assert record == {("asyncio", "asyncio", True), ("trio", "trio", True)} From a3c181620c28fd89909667647196bd34e995d205 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 18 Oct 2024 08:45:21 +0100 Subject: [PATCH 8/9] move sniffio import to top --- src/trio/_core/_tests/test_guest_mode.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/trio/_core/_tests/test_guest_mode.py b/src/trio/_core/_tests/test_guest_mode.py index c4e161c627..56f25fd21b 100644 --- a/src/trio/_core/_tests/test_guest_mode.py +++ b/src/trio/_core/_tests/test_guest_mode.py @@ -24,6 +24,7 @@ ) import pytest +import sniffio from outcome import Outcome import trio @@ -223,7 +224,8 @@ async def trio_main(in_host: InHost) -> str: def test_guest_mode_sniffio_integration() -> None: - from sniffio import current_async_library, thread_local as sniffio_library + current_async_library = sniffio.current_async_library + sniffio_library = sniffio.thread_local async def trio_main(in_host: InHost) -> str: async def synchronize() -> None: @@ -626,8 +628,6 @@ async def trio_main(in_host: InHost) -> None: @restore_unraisablehook() def test_guest_mode_asyncgens() -> None: - import sniffio - record = set() async def agen(label: str) -> AsyncGenerator[int, None]: @@ -661,8 +661,6 @@ async def trio_main() -> None: @restore_unraisablehook() def test_guest_mode_asyncgens_garbage_collection() -> None: - import sniffio - record: set[tuple[str, str, bool]] = set() async def agen(label: str) -> AsyncGenerator[int, None]: From fc1f5bbe7c041dbdf06841a827093710730ae795 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 18 Oct 2024 09:27:28 +0100 Subject: [PATCH 9/9] asyncio.run introduced some breakage --- src/trio/_core/_tests/test_guest_mode.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/trio/_core/_tests/test_guest_mode.py b/src/trio/_core/_tests/test_guest_mode.py index 56f25fd21b..a9b276243a 100644 --- a/src/trio/_core/_tests/test_guest_mode.py +++ b/src/trio/_core/_tests/test_guest_mode.py @@ -440,8 +440,9 @@ def aiotrio_run( pass_not_threadsafe: bool = True, **start_guest_run_kwargs: Any, ) -> T: + loop = asyncio.new_event_loop() + async def aio_main() -> T: - loop = asyncio.get_running_loop() trio_done_fut: asyncio.Future[Outcome[T]] = loop.create_future() def trio_done_callback(main_outcome: Outcome[T]) -> None: @@ -460,7 +461,12 @@ def trio_done_callback(main_outcome: Outcome[T]) -> None: return (await trio_done_fut).unwrap() - return asyncio.run(aio_main()) + try: + # can't use asyncio.run because that fails on Windows (3.8, x64, with + # Komodia LSP) + return loop.run_until_complete(aio_main()) + finally: + loop.close() def test_guest_mode_on_asyncio() -> None: