From 5d51c56db31c32c51154dd13cedac23fac24657f Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 18:44:40 +0100 Subject: [PATCH 01/36] gh-3108: avoid materializing f_locals by using weakrefs to code objects instead --- src/trio/_core/_generated_instrumentation.py | 7 +- src/trio/_core/_generated_io_epoll.py | 8 +- src/trio/_core/_generated_io_kqueue.py | 14 +-- src/trio/_core/_generated_io_windows.py | 20 +-- src/trio/_core/_generated_run.py | 19 ++- src/trio/_core/_ki.py | 122 +++++-------------- src/trio/_core/_run.py | 29 +++-- src/trio/_core/_tests/test_run.py | 44 +++++++ src/trio/_tools/gen_exports.py | 7 +- 9 files changed, 126 insertions(+), 144 deletions(-) diff --git a/src/trio/_core/_generated_instrumentation.py b/src/trio/_core/_generated_instrumentation.py index 568b76dffa..d03ef9db7d 100644 --- a/src/trio/_core/_generated_instrumentation.py +++ b/src/trio/_core/_generated_instrumentation.py @@ -3,10 +3,9 @@ # ************************************************************* from __future__ import annotations -import sys from typing import TYPE_CHECKING -from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED +from ._ki import enable_ki_protection from ._run import GLOBAL_RUN_CONTEXT if TYPE_CHECKING: @@ -15,6 +14,7 @@ __all__ = ["add_instrument", "remove_instrument"] +@enable_ki_protection def add_instrument(instrument: Instrument) -> None: """Start instrumenting the current run loop with the given instrument. @@ -24,13 +24,13 @@ def add_instrument(instrument: Instrument) -> None: If ``instrument`` is already active, does nothing. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.instruments.add_instrument(instrument) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def remove_instrument(instrument: Instrument) -> None: """Stop instrumenting the current run loop with the given instrument. @@ -44,7 +44,6 @@ def remove_instrument(instrument: Instrument) -> None: deactivated. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.instruments.remove_instrument(instrument) except AttributeError: diff --git a/src/trio/_core/_generated_io_epoll.py b/src/trio/_core/_generated_io_epoll.py index 9f9ad59725..41cbb40650 100644 --- a/src/trio/_core/_generated_io_epoll.py +++ b/src/trio/_core/_generated_io_epoll.py @@ -6,7 +6,7 @@ import sys from typing import TYPE_CHECKING -from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED +from ._ki import enable_ki_protection from ._run import GLOBAL_RUN_CONTEXT if TYPE_CHECKING: @@ -18,6 +18,7 @@ __all__ = ["notify_closing", "wait_readable", "wait_writable"] +@enable_ki_protection async def wait_readable(fd: int | _HasFileNo) -> None: """Block until the kernel reports that the given object is readable. @@ -40,13 +41,13 @@ async def wait_readable(fd: int | _HasFileNo) -> None: if another task calls :func:`notify_closing` while this function is still working. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.wait_readable(fd) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection async def wait_writable(fd: int | _HasFileNo) -> None: """Block until the kernel reports that the given object is writable. @@ -59,13 +60,13 @@ async def wait_writable(fd: int | _HasFileNo) -> None: if another task calls :func:`notify_closing` while this function is still working. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.wait_writable(fd) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def notify_closing(fd: int | _HasFileNo) -> None: """Notify waiters of the given object that it will be closed. @@ -91,7 +92,6 @@ def notify_closing(fd: int | _HasFileNo) -> None: step, so other tasks won't be able to tell what order they happened in anyway. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.io_manager.notify_closing(fd) except AttributeError: diff --git a/src/trio/_core/_generated_io_kqueue.py b/src/trio/_core/_generated_io_kqueue.py index eb230597b3..acbe015466 100644 --- a/src/trio/_core/_generated_io_kqueue.py +++ b/src/trio/_core/_generated_io_kqueue.py @@ -6,7 +6,7 @@ import sys from typing import TYPE_CHECKING, Callable, ContextManager -from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED +from ._ki import enable_ki_protection from ._run import GLOBAL_RUN_CONTEXT if TYPE_CHECKING: @@ -29,18 +29,19 @@ ] +@enable_ki_protection def current_kqueue() -> select.kqueue: """TODO: these are implemented, but are currently more of a sketch than anything real. See `#26 `__. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.io_manager.current_kqueue() except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def monitor_kevent( ident: int, filter: int, @@ -49,13 +50,13 @@ def monitor_kevent( anything real. See `#26 `__. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.io_manager.monitor_kevent(ident, filter) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection async def wait_kevent( ident: int, filter: int, @@ -65,7 +66,6 @@ async def wait_kevent( anything real. See `#26 `__. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.wait_kevent( ident, @@ -76,6 +76,7 @@ async def wait_kevent( raise RuntimeError("must be called from async context") from None +@enable_ki_protection async def wait_readable(fd: int | _HasFileNo) -> None: """Block until the kernel reports that the given object is readable. @@ -98,13 +99,13 @@ async def wait_readable(fd: int | _HasFileNo) -> None: if another task calls :func:`notify_closing` while this function is still working. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.wait_readable(fd) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection async def wait_writable(fd: int | _HasFileNo) -> None: """Block until the kernel reports that the given object is writable. @@ -117,13 +118,13 @@ async def wait_writable(fd: int | _HasFileNo) -> None: if another task calls :func:`notify_closing` while this function is still working. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.wait_writable(fd) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def notify_closing(fd: int | _HasFileNo) -> None: """Notify waiters of the given object that it will be closed. @@ -149,7 +150,6 @@ def notify_closing(fd: int | _HasFileNo) -> None: step, so other tasks won't be able to tell what order they happened in anyway. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.io_manager.notify_closing(fd) except AttributeError: diff --git a/src/trio/_core/_generated_io_windows.py b/src/trio/_core/_generated_io_windows.py index 1cad305a03..57e39de4b5 100644 --- a/src/trio/_core/_generated_io_windows.py +++ b/src/trio/_core/_generated_io_windows.py @@ -6,7 +6,7 @@ import sys from typing import TYPE_CHECKING, ContextManager -from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED +from ._ki import enable_ki_protection from ._run import GLOBAL_RUN_CONTEXT if TYPE_CHECKING: @@ -32,6 +32,7 @@ ] +@enable_ki_protection async def wait_readable(sock: _HasFileNo | int) -> None: """Block until the kernel reports that the given object is readable. @@ -54,13 +55,13 @@ async def wait_readable(sock: _HasFileNo | int) -> None: if another task calls :func:`notify_closing` while this function is still working. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.wait_readable(sock) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection async def wait_writable(sock: _HasFileNo | int) -> None: """Block until the kernel reports that the given object is writable. @@ -73,13 +74,13 @@ async def wait_writable(sock: _HasFileNo | int) -> None: if another task calls :func:`notify_closing` while this function is still working. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.wait_writable(sock) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def notify_closing(handle: Handle | int | _HasFileNo) -> None: """Notify waiters of the given object that it will be closed. @@ -105,33 +106,32 @@ def notify_closing(handle: Handle | int | _HasFileNo) -> None: step, so other tasks won't be able to tell what order they happened in anyway. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.io_manager.notify_closing(handle) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def register_with_iocp(handle: int | CData) -> None: """TODO: these are implemented, but are currently more of a sketch than anything real. See `#26 `__ and `#52 `__. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.io_manager.register_with_iocp(handle) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection async def wait_overlapped(handle_: int | CData, lpOverlapped: CData | int) -> object: """TODO: these are implemented, but are currently more of a sketch than anything real. See `#26 `__ and `#52 `__. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.wait_overlapped( handle_, @@ -141,6 +141,7 @@ async def wait_overlapped(handle_: int | CData, lpOverlapped: CData | int) -> ob raise RuntimeError("must be called from async context") from None +@enable_ki_protection async def write_overlapped( handle: int | CData, data: Buffer, @@ -151,7 +152,6 @@ async def write_overlapped( `__ and `#52 `__. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.write_overlapped( handle, @@ -162,6 +162,7 @@ async def write_overlapped( raise RuntimeError("must be called from async context") from None +@enable_ki_protection async def readinto_overlapped( handle: int | CData, buffer: Buffer, @@ -172,7 +173,6 @@ async def readinto_overlapped( `__ and `#52 `__. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.io_manager.readinto_overlapped( handle, @@ -183,26 +183,26 @@ async def readinto_overlapped( raise RuntimeError("must be called from async context") from None +@enable_ki_protection def current_iocp() -> int: """TODO: these are implemented, but are currently more of a sketch than anything real. See `#26 `__ and `#52 `__. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.io_manager.current_iocp() except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def monitor_completion_key() -> ContextManager[tuple[int, UnboundedQueue[object]]]: """TODO: these are implemented, but are currently more of a sketch than anything real. See `#26 `__ and `#52 `__. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.io_manager.monitor_completion_key() except AttributeError: diff --git a/src/trio/_core/_generated_run.py b/src/trio/_core/_generated_run.py index b5957a134e..aeb8e70b70 100644 --- a/src/trio/_core/_generated_run.py +++ b/src/trio/_core/_generated_run.py @@ -3,10 +3,9 @@ # ************************************************************* from __future__ import annotations -import sys from typing import TYPE_CHECKING, Any -from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED +from ._ki import enable_ki_protection from ._run import _NO_SEND, GLOBAL_RUN_CONTEXT, RunStatistics, Task if TYPE_CHECKING: @@ -33,6 +32,7 @@ ] +@enable_ki_protection def current_statistics() -> RunStatistics: """Returns ``RunStatistics``, which contains run-loop-level debugging information. @@ -56,13 +56,13 @@ def current_statistics() -> RunStatistics: other attributes vary between backends. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.current_statistics() except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def current_time() -> float: """Returns the current time according to Trio's internal clock. @@ -73,35 +73,35 @@ def current_time() -> float: RuntimeError: if not inside a call to :func:`trio.run`. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.current_time() except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def current_clock() -> Clock: """Returns the current :class:`~trio.abc.Clock`.""" - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.current_clock() except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def current_root_task() -> Task | None: """Returns the current root :class:`Task`. This is the task that is the ultimate parent of all other tasks. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.current_root_task() except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def reschedule(task: Task, next_send: Outcome[Any] = _NO_SEND) -> None: """Reschedule the given task with the given :class:`outcome.Outcome`. @@ -120,13 +120,13 @@ def reschedule(task: Task, next_send: Outcome[Any] = _NO_SEND) -> None: raise) from :func:`wait_task_rescheduled`. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.reschedule(task, next_send) except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection def spawn_system_task( async_fn: Callable[[Unpack[PosArgT]], Awaitable[object]], *args: Unpack[PosArgT], @@ -184,7 +184,6 @@ def spawn_system_task( Task: the newly spawned task """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.spawn_system_task( async_fn, @@ -196,18 +195,19 @@ def spawn_system_task( raise RuntimeError("must be called from async context") from None +@enable_ki_protection def current_trio_token() -> TrioToken: """Retrieve the :class:`TrioToken` for the current call to :func:`trio.run`. """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return GLOBAL_RUN_CONTEXT.runner.current_trio_token() except AttributeError: raise RuntimeError("must be called from async context") from None +@enable_ki_protection async def wait_all_tasks_blocked(cushion: float = 0.0) -> None: """Block until there are no runnable tasks. @@ -266,7 +266,6 @@ async def test_lock_fairness(): print("FAIL") """ - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True try: return await GLOBAL_RUN_CONTEXT.runner.wait_all_tasks_blocked(cushion) except AttributeError: diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index a8431f89db..edf643f22a 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -1,26 +1,19 @@ from __future__ import annotations -import inspect import signal import sys -from functools import wraps -from typing import TYPE_CHECKING, Final, Protocol, TypeVar +import types +import weakref +from typing import TYPE_CHECKING, Protocol, TypeVar import attrs from .._util import is_main_thread -CallableT = TypeVar("CallableT", bound="Callable[..., object]") -RetT = TypeVar("RetT") - if TYPE_CHECKING: import types from collections.abc import Callable - from typing_extensions import ParamSpec, TypeGuard - - ArgsT = ParamSpec("ArgsT") - # In ordinary single-threaded Python code, when you hit control-C, it raises # an exception and automatically does all the regular unwinding stuff. # @@ -83,18 +76,22 @@ # for any Python program that's written to catch and ignore # KeyboardInterrupt.) -# We use this special string as a unique key into the frame locals dictionary. -# The @ ensures it is not a valid identifier and can't clash with any possible -# real local name. See: https://github.com/python-trio/trio/issues/469 -LOCALS_KEY_KI_PROTECTION_ENABLED: Final = "@TRIO_KI_PROTECTION_ENABLED" +_CODE_KI_PROTECTION_STATUS_WMAP: weakref.WeakKeyDictionary[ + types.CodeType, + bool, +] = weakref.WeakKeyDictionary() # NB: according to the signal.signal docs, 'frame' can be None on entry to # this function: def ki_protection_enabled(frame: types.FrameType | None) -> bool: while frame is not None: - if LOCALS_KEY_KI_PROTECTION_ENABLED in frame.f_locals: - return bool(frame.f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED]) + try: + v = _CODE_KI_PROTECTION_STATUS_WMAP[frame.f_code] + except KeyError: + pass + else: + return bool(v) if frame.f_code.co_name == "__del__": return True frame = frame.f_back @@ -117,90 +114,27 @@ def currently_ki_protected() -> bool: return ki_protection_enabled(sys._getframe()) -# This is to support the async_generator package necessary for aclosing on <3.10 -# functions decorated @async_generator are given this magic property that's a -# reference to the object itself -# see python-trio/async_generator/async_generator/_impl.py -def legacy_isasyncgenfunction( - obj: object, -) -> TypeGuard[Callable[..., types.AsyncGeneratorType[object, object]]]: - return getattr(obj, "_async_gen_function", None) == id(obj) - - -def _ki_protection_decorator( - enabled: bool, -) -> Callable[[Callable[ArgsT, RetT]], Callable[ArgsT, RetT]]: - # The "ignore[return-value]" below is because the inspect functions cast away the - # original return type of fn, making it just CoroutineType[Any, Any, Any] etc. - # ignore[misc] is because @wraps() is passed a callable with Any in the return type. - def decorator(fn: Callable[ArgsT, RetT]) -> Callable[ArgsT, RetT]: - # In some version of Python, isgeneratorfunction returns true for - # coroutine functions, so we have to check for coroutine functions - # first. - if inspect.iscoroutinefunction(fn): - - @wraps(fn) - def wrapper(*args: ArgsT.args, **kwargs: ArgsT.kwargs) -> RetT: # type: ignore[misc] - # See the comment for regular generators below - coro = fn(*args, **kwargs) - assert coro.cr_frame is not None, "Coroutine frame should exist" - coro.cr_frame.f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = enabled - return coro # type: ignore[return-value] - - return wrapper - elif inspect.isgeneratorfunction(fn): - - @wraps(fn) - def wrapper(*args: ArgsT.args, **kwargs: ArgsT.kwargs) -> RetT: # type: ignore[misc] - # It's important that we inject this directly into the - # generator's locals, as opposed to setting it here and then - # doing 'yield from'. The reason is, if a generator is - # throw()n into, then it may magically pop to the top of the - # stack. And @contextmanager generators in particular are a - # case where we often want KI protection, and which are often - # thrown into! See: - # https://bugs.python.org/issue29590 - gen = fn(*args, **kwargs) - gen.gi_frame.f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = enabled - return gen # type: ignore[return-value] - - return wrapper - elif inspect.isasyncgenfunction(fn) or legacy_isasyncgenfunction(fn): - - @wraps(fn) # type: ignore[arg-type] - def wrapper(*args: ArgsT.args, **kwargs: ArgsT.kwargs) -> RetT: # type: ignore[misc] - # See the comment for regular generators above - agen = fn(*args, **kwargs) - agen.ag_frame.f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = enabled - return agen # type: ignore[return-value] - - return wrapper - else: - - @wraps(fn) - def wrapper(*args: ArgsT.args, **kwargs: ArgsT.kwargs) -> RetT: - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = enabled - return fn(*args, **kwargs) - - return wrapper - - return decorator +class _SupportsCode(Protocol): + __code__: types.CodeType -# pyright workaround: https://github.com/microsoft/pyright/issues/5866 -class KIProtectionSignature(Protocol): - __name__: str +_T_supports_code = TypeVar("_T_supports_code", bound=_SupportsCode) - def __call__(self, f: CallableT, /) -> CallableT: - pass +def enable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: + """Decorator to enable KI protection.""" + code = f.__code__.replace() + _CODE_KI_PROTECTION_STATUS_WMAP[code] = True + f.__code__ = code + return f -# the following `type: ignore`s are because we use ParamSpec internally, but want to allow overloads -enable_ki_protection: KIProtectionSignature = _ki_protection_decorator(True) # type: ignore[assignment] -enable_ki_protection.__name__ = "enable_ki_protection" -disable_ki_protection: KIProtectionSignature = _ki_protection_decorator(False) # type: ignore[assignment] -disable_ki_protection.__name__ = "disable_ki_protection" +def disable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: + """Dectorator to disable KI protection.""" + code = f.__code__.replace() + _CODE_KI_PROTECTION_STATUS_WMAP[code] = False + f.__code__ = code + return f @attrs.define(slots=False) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index cba7a8dec0..fe0c97bdbb 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -39,7 +39,7 @@ from ._entry_queue import EntryQueue, TrioToken from ._exceptions import Cancelled, RunFinishedError, TrioInternalError from ._instrumentation import Instruments -from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED, KIManager, enable_ki_protection +from ._ki import KIManager, disable_ki_protection, enable_ki_protection from ._parking_lot import GLOBAL_PARKING_LOT_BREAKER from ._thread_cache import start_thread_soon from ._traps import ( @@ -1668,6 +1668,16 @@ def in_main_thread() -> None: start_thread_soon(get_events, deliver) +@enable_ki_protection +async def run_coro_with_ki_protection_enabled(orig_coro: Awaitable[RetT]) -> RetT: + return await orig_coro + + +@disable_ki_protection +async def run_coro_with_ki_protection_disabled(orig_coro: Awaitable[RetT]) -> RetT: + return await orig_coro + + @attrs.define(eq=False) class Runner: clock: Clock @@ -1863,15 +1873,12 @@ def spawn_impl( except AttributeError: name = repr(name) - # very old Cython versions (<0.29.24) has the attribute, but with a value of None - if getattr(coro, "cr_frame", None) is None: - # This async function is implemented in C or Cython - async def python_wrapper(orig_coro: Awaitable[RetT]) -> RetT: - return await orig_coro - - coro = python_wrapper(coro) - assert coro.cr_frame is not None, "Coroutine frame should exist" - coro.cr_frame.f_locals.setdefault(LOCALS_KEY_KI_PROTECTION_ENABLED, system_task) + python_wrapper = ( + run_coro_with_ki_protection_enabled + if system_task + else run_coro_with_ki_protection_disabled + ) + coro = python_wrapper(coro) ###### # Set up the Task object @@ -2573,13 +2580,13 @@ def my_done_callback(run_outcome): # mode", where our core event loop gets unrolled into a series of callbacks on # the host loop. If you're doing a regular trio.run then this gets run # straight through. +@enable_ki_protection def unrolled_run( runner: Runner, async_fn: Callable[[Unpack[PosArgT]], Awaitable[object]], args: tuple[Unpack[PosArgT]], host_uses_signal_set_wakeup_fd: bool = False, ) -> Generator[float, EventResult, None]: - sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True __tracebackhide__ = True try: diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 9d2eab787d..0a78b66a6e 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2774,3 +2774,47 @@ async def spawn_tasks_in_old_nursery(task_status: _core.TaskStatus[None]) -> Non with pytest.raises(_core.TrioInternalError) as excinfo: await nursery.start(spawn_tasks_in_old_nursery) assert RaisesGroup(ValueError, ValueError).matches(excinfo.value.__cause__) + + +if sys.version_info <= (3, 11): + + def no_other_refs() -> list[object]: + return [sys._getframe(1)] + +else: + + def no_other_refs() -> list[object]: + return [] + + +@pytest.mark.skipif( + sys.implementation.name != "cpython", + reason="Only makes sense with refcounting GC", +) +async def test_ki_protection_doesnt_leave_cyclic_garbage() -> None: + class MyException(Exception): + pass + + async def demo() -> None: + async def handle_error() -> None: + try: + raise MyException + except MyException as e: + exceptions.append(e) + + exceptions: list[MyException] = [] + try: + async with _core.open_nursery() as n: + n.start_soon(handle_error) + raise ExceptionGroup("errors", exceptions) + finally: + exceptions = [] + + exc: MyException | None = None + try: + await demo() + except ExceptionGroup as excs: + exc = excs.exceptions[0] + + assert isinstance(exc, MyException) + assert gc.get_referrers(exc) == no_other_refs() diff --git a/src/trio/_tools/gen_exports.py b/src/trio/_tools/gen_exports.py index 91969d6bfe..a356bc9980 100755 --- a/src/trio/_tools/gen_exports.py +++ b/src/trio/_tools/gen_exports.py @@ -34,12 +34,11 @@ import sys -from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED +from ._ki import enable_ki_protection from ._run import GLOBAL_RUN_CONTEXT """ -TEMPLATE = """sys._getframe().f_locals[LOCALS_KEY_KI_PROTECTION_ENABLED] = True -try: +TEMPLATE = """try: return{}GLOBAL_RUN_CONTEXT.{}.{} except AttributeError: raise RuntimeError("must be called from async context") from None @@ -237,7 +236,7 @@ def gen_public_wrappers_source(file: File) -> str: is_cm = False # Remove decorators - method.decorator_list = [] + method.decorator_list = [ast.Name("enable_ki_protection")] # Create pass through arguments new_args = create_passthrough_args(method) From ea42ad0d4d0b90e86e5cd5161359d79c1da318ee Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 19:25:44 +0100 Subject: [PATCH 02/36] enable ki protection on async_generator objects --- src/trio/_core/_ki.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index edf643f22a..612f72555c 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -14,6 +14,7 @@ import types from collections.abc import Callable + from typing_extensions import TypeGuard # In ordinary single-threaded Python code, when you hit control-C, it raises # an exception and automatically does all the regular unwinding stuff. # @@ -82,6 +83,16 @@ ] = weakref.WeakKeyDictionary() +# This is to support the async_generator package necessary for aclosing on <3.10 +# functions decorated @async_generator are given this magic property that's a +# reference to the object itself +# see python-trio/async_generator/async_generator/_impl.py +def legacy_isasyncgenfunction( + obj: object, +) -> TypeGuard[Callable[..., types.AsyncGeneratorType[object, object]]]: + return getattr(obj, "_async_gen_function", None) == id(obj) + + # NB: according to the signal.signal docs, 'frame' can be None on entry to # this function: def ki_protection_enabled(frame: types.FrameType | None) -> bool: @@ -123,6 +134,10 @@ class _SupportsCode(Protocol): def enable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: """Decorator to enable KI protection.""" + + if legacy_isasyncgenfunction(f): + f = f.__wrapped__ # type: ignore + code = f.__code__.replace() _CODE_KI_PROTECTION_STATUS_WMAP[code] = True f.__code__ = code @@ -131,6 +146,10 @@ def enable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: def disable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: """Dectorator to disable KI protection.""" + + if legacy_isasyncgenfunction(f): + f = f.__wrapped__ # type: ignore + code = f.__code__.replace() _CODE_KI_PROTECTION_STATUS_WMAP[code] = False f.__code__ = code From c7555936c72880bca57bb931ca414c43995baac2 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 20:24:42 +0100 Subject: [PATCH 03/36] avoid adding an extra coroutine wrapper to Task coros --- src/trio/_core/_run.py | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index fe0c97bdbb..8d56c008be 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -83,6 +83,7 @@ StatusT_contra = TypeVar("StatusT_contra", contravariant=True) FnT = TypeVar("FnT", bound="Callable[..., Any]") +T = TypeVar("T") RetT = TypeVar("RetT") @@ -1382,6 +1383,7 @@ class Task(metaclass=NoPublicConstructor): name: str context: contextvars.Context _counter: int = attrs.field(init=False, factory=itertools.count().__next__) + _ki_protected: bool # Invariant: # - for unscheduled tasks, _next_send_fn and _next_send are both None @@ -1669,13 +1671,13 @@ def in_main_thread() -> None: @enable_ki_protection -async def run_coro_with_ki_protection_enabled(orig_coro: Awaitable[RetT]) -> RetT: - return await orig_coro +def run_with_ki_protection_enabled(f: Callable[[T], RetT], v: T) -> RetT: + return f(v) @disable_ki_protection -async def run_coro_with_ki_protection_disabled(orig_coro: Awaitable[RetT]) -> RetT: - return await orig_coro +def run_with_ki_protection_disabled(f: Callable[[T], RetT], v: T) -> RetT: + return f(v) @attrs.define(eq=False) @@ -1873,12 +1875,14 @@ def spawn_impl( except AttributeError: name = repr(name) - python_wrapper = ( - run_coro_with_ki_protection_enabled - if system_task - else run_coro_with_ki_protection_disabled - ) - coro = python_wrapper(coro) + # very old Cython versions (<0.29.24) has the attribute, but with a value of None + if getattr(coro, "cr_frame", None) is None: + # This async function is implemented in C or Cython + async def python_wrapper(orig_coro: Awaitable[RetT]) -> RetT: + return await orig_coro + + coro = python_wrapper(coro) + assert coro.cr_frame is not None, "Coroutine frame should exist" ###### # Set up the Task object @@ -1889,6 +1893,7 @@ def spawn_impl( runner=self, name=name, context=context, + ki_protected=system_task, ) self.tasks.add(task) @@ -2719,6 +2724,11 @@ def unrolled_run( next_send_fn = task._next_send_fn next_send = task._next_send + run_with = ( + run_with_ki_protection_enabled + if task._ki_protected + else run_with_ki_protection_disabled + ) task._next_send_fn = task._next_send = None final_outcome: Outcome[Any] | None = None try: @@ -2731,16 +2741,17 @@ def unrolled_run( # https://github.com/python/cpython/issues/108668 # So now we send in the Outcome object and unwrap it on the # other side. - msg = task.context.run(next_send_fn, next_send) + msg = task.context.run(run_with, next_send_fn, next_send) except StopIteration as stop_iteration: final_outcome = Value(stop_iteration.value) except BaseException as task_exc: # Store for later, removing uninteresting top frames: 1 # frame we always remove, because it's this function + # another is the run_with # catching it, and then in addition we remove however many # more Context.run adds. tb = task_exc.__traceback__ - for _ in range(1 + CONTEXT_RUN_TB_FRAMES): + for _ in range(2 + CONTEXT_RUN_TB_FRAMES): if tb is not None: # pragma: no branch tb = tb.tb_next final_outcome = Error(task_exc.with_traceback(tb)) From 6bf63f2904730f18e5e38c6265d3a39db822b646 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 20:36:09 +0100 Subject: [PATCH 04/36] fix returning the wrong object in (enable|disable)_ki_protection --- src/trio/_core/_ki.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index 612f72555c..15a62aada7 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -134,6 +134,7 @@ class _SupportsCode(Protocol): def enable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: """Decorator to enable KI protection.""" + orig = f if legacy_isasyncgenfunction(f): f = f.__wrapped__ # type: ignore @@ -141,11 +142,12 @@ def enable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: code = f.__code__.replace() _CODE_KI_PROTECTION_STATUS_WMAP[code] = True f.__code__ = code - return f + return orig def disable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: """Dectorator to disable KI protection.""" + orig = f if legacy_isasyncgenfunction(f): f = f.__wrapped__ # type: ignore @@ -153,7 +155,7 @@ def disable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: code = f.__code__.replace() _CODE_KI_PROTECTION_STATUS_WMAP[code] = False f.__code__ = code - return f + return orig @attrs.define(slots=False) From 65aa8f4005067ed1e5438858f0033937ea5394c3 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 20:39:19 +0100 Subject: [PATCH 05/36] remove KIProtectionSignature from _check_type_completeness.json --- src/trio/_tests/_check_type_completeness.json | 1 - 1 file changed, 1 deletion(-) diff --git a/src/trio/_tests/_check_type_completeness.json b/src/trio/_tests/_check_type_completeness.json index badb7cba17..72d981f89c 100644 --- a/src/trio/_tests/_check_type_completeness.json +++ b/src/trio/_tests/_check_type_completeness.json @@ -40,7 +40,6 @@ "No docstring found for class \"trio._core._local.RunVarToken\"", "No docstring found for class \"trio.lowlevel.RunVarToken\"", "No docstring found for class \"trio.lowlevel.Task\"", - "No docstring found for class \"trio._core._ki.KIProtectionSignature\"", "No docstring found for class \"trio.socket.SocketType\"", "No docstring found for class \"trio.socket.gaierror\"", "No docstring found for class \"trio.socket.herror\"", From 748f876bf568f342e95587cd0eae351ee891f34f Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 21:14:01 +0100 Subject: [PATCH 06/36] fix refcycles --- src/trio/_core/_run.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 8d56c008be..fa4820da3f 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1672,12 +1672,18 @@ def in_main_thread() -> None: @enable_ki_protection def run_with_ki_protection_enabled(f: Callable[[T], RetT], v: T) -> RetT: - return f(v) + try: + return f(v) + finally: + del v # for the case where f is coro.throw() and v is a (Base)Exception @disable_ki_protection def run_with_ki_protection_disabled(f: Callable[[T], RetT], v: T) -> RetT: - return f(v) + try: + return f(v) + finally: + del v # for the case where f is coro.throw() and v is a (Base)Exception @attrs.define(eq=False) From fccf4ff48daa6cf1fe06af612eae16f343ce95e6 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 21:21:44 +0100 Subject: [PATCH 07/36] add newsfragment --- newsfragments/3108.bugfix.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 newsfragments/3108.bugfix.rst diff --git a/newsfragments/3108.bugfix.rst b/newsfragments/3108.bugfix.rst new file mode 100644 index 0000000000..2f690266ad --- /dev/null +++ b/newsfragments/3108.bugfix.rst @@ -0,0 +1,2 @@ +avoid materializing frame.f_locals in KI protection code, this causes locals to persist in the frame.f_locals snapshot even if they are deleted on 3.12 and below + From 56696a22545025c0f1b6d3a342c59734f802a028 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 21:23:49 +0100 Subject: [PATCH 08/36] fix mypy --- src/trio/_core/_tests/test_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_core/_tests/test_run.py b/src/trio/_core/_tests/test_run.py index 0a78b66a6e..09d39bac84 100644 --- a/src/trio/_core/_tests/test_run.py +++ b/src/trio/_core/_tests/test_run.py @@ -2810,7 +2810,7 @@ async def handle_error() -> None: finally: exceptions = [] - exc: MyException | None = None + exc: Exception | None = None try: await demo() except ExceptionGroup as excs: From a7272d868034226d81e7468cd3613cf08ee0ade7 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 21:29:30 +0100 Subject: [PATCH 09/36] now that the type annotation for enable_ki_protection is fixed, we need to fix the use of Any --- src/trio/_core/_generated_run.py | 4 ++-- src/trio/_core/_run.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/trio/_core/_generated_run.py b/src/trio/_core/_generated_run.py index aeb8e70b70..67d70d9077 100644 --- a/src/trio/_core/_generated_run.py +++ b/src/trio/_core/_generated_run.py @@ -3,7 +3,7 @@ # ************************************************************* from __future__ import annotations -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING from ._ki import enable_ki_protection from ._run import _NO_SEND, GLOBAL_RUN_CONTEXT, RunStatistics, Task @@ -102,7 +102,7 @@ def current_root_task() -> Task | None: @enable_ki_protection -def reschedule(task: Task, next_send: Outcome[Any] = _NO_SEND) -> None: +def reschedule(task: Task, next_send: Outcome[object] = _NO_SEND) -> None: """Reschedule the given task with the given :class:`outcome.Outcome`. diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index fa4820da3f..0c2c3477cd 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1799,10 +1799,10 @@ def current_root_task(self) -> Task | None: ################ @_public # Type-ignore due to use of Any here. - def reschedule( # type: ignore[misc] + def reschedule( self, task: Task, - next_send: Outcome[Any] = _NO_SEND, + next_send: Outcome[object] = _NO_SEND, ) -> None: """Reschedule the given task with the given :class:`outcome.Outcome`. From 3ea6617144581a3aad1e4a5ae279a5772a0bbd2b Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sun, 13 Oct 2024 21:30:00 +0100 Subject: [PATCH 10/36] pre-commit --- newsfragments/3108.bugfix.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/newsfragments/3108.bugfix.rst b/newsfragments/3108.bugfix.rst index 2f690266ad..c6bd5828ac 100644 --- a/newsfragments/3108.bugfix.rst +++ b/newsfragments/3108.bugfix.rst @@ -1,2 +1 @@ avoid materializing frame.f_locals in KI protection code, this causes locals to persist in the frame.f_locals snapshot even if they are deleted on 3.12 and below - From 0ed35a7f70799f4979561a3f42a61b7eb218fc86 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 08:51:44 +0100 Subject: [PATCH 11/36] add test for ki protection leaking accross local functions --- src/trio/_core/_tests/test_ki.py | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index e4241fc762..271c563de2 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -4,7 +4,7 @@ import inspect import signal import threading -from typing import TYPE_CHECKING, AsyncIterator, Callable, Iterator +from typing import TYPE_CHECKING, AsyncIterator, Callable, Iterator, TypeVar import outcome import pytest @@ -515,3 +515,26 @@ async def inner() -> None: _core.run(inner) finally: threading._active[thread.ident] = original # type: ignore[attr-defined] + + +_T = TypeVar("_T") + + +def _identity(v: _T) -> _T: + return v + + +async def test_ki_does_not_leak_accross_different_calls_to_inner_functions() -> None: + assert not _core.currently_ki_protected() + + def factory(enabled: bool) -> Callable[[], bool]: + @_identity(_core.enable_ki_protection if enabled else _identity) + def decorated() -> bool: + return _core.currently_ki_protected() + + return decorated + + decorated_enabled = factory(True) + decorated_disabled = factory(False) + assert decorated_enabled() + assert not decorated_disabled() From ec30d7b33d9c5994bebb81d8537acf0889db92ac Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 08:53:51 +0100 Subject: [PATCH 12/36] add fix for ki protection leaking accross local functions --- src/trio/_core/_ki.py | 73 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 4 deletions(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index 15a62aada7..bd86b5f269 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -4,7 +4,7 @@ import sys import types import weakref -from typing import TYPE_CHECKING, Protocol, TypeVar +from typing import TYPE_CHECKING, Any, Generic, Protocol, TypeVar import attrs @@ -14,7 +14,7 @@ import types from collections.abc import Callable - from typing_extensions import TypeGuard + from typing_extensions import Self, TypeGuard # In ordinary single-threaded Python code, when you hit control-C, it raises # an exception and automatically does all the regular unwinding stuff. # @@ -77,10 +77,75 @@ # for any Python program that's written to catch and ignore # KeyboardInterrupt.) -_CODE_KI_PROTECTION_STATUS_WMAP: weakref.WeakKeyDictionary[ +_T = TypeVar("_T") + + +class _IdRef(weakref.ref[_T]): + slots = "_hash" + _hash: int + + def __new__(cls, ob: _T, callback: Callable[[Self], Any] | None = None, /) -> Self: + self: Self = weakref.ref.__new__(cls, ob, callback) + self._hash = object.__hash__(ob) + return self + + def __eq__(self, other: object) -> bool: + if self is other: + return True + + if not isinstance(other, _IdRef): + return NotImplemented + + my_obj = None + other_obj: Any = None + try: + my_obj = self() + other_obj = other() + return my_obj is not None and other_obj is not None and my_obj is other_obj + finally: + del my_obj, other_obj + + def __hash__(self) -> int: + return self._hash + + +_KT = TypeVar("_KT") +_VT = TypeVar("_VT") + + +# see also: https://github.com/python/cpython/issues/88306 +class WeakKeyIdentityDictionary(Generic[_KT, _VT]): + def __init__(self) -> None: + self._data: dict[_IdRef[_KT], _VT] = {} + + def remove( + k: _IdRef[_KT], + selfref: weakref.ref[ + WeakKeyIdentityDictionary[_KT, _VT] + ] = weakref.ref( # noqa: B008 # function-call-in-default-argument + self, + ), + ) -> None: + self = selfref() + if self is not None: + try: # noqa: SIM105 # supressible-exception + del self._data[k] + except KeyError: + pass + + self._remove = remove + + def __getitem__(self, k: _KT) -> _VT: + return self._data[_IdRef(k)] + + def __setitem__(self, k: _KT, v: _VT) -> None: + self._data[_IdRef(k, self._remove)] = v + + +_CODE_KI_PROTECTION_STATUS_WMAP: WeakKeyIdentityDictionary[ types.CodeType, bool, -] = weakref.WeakKeyDictionary() +] = WeakKeyIdentityDictionary() # This is to support the async_generator package necessary for aclosing on <3.10 From 6998a5c744efbe091dfd3eac55632f87f6b11218 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 11:40:41 +0100 Subject: [PATCH 13/36] do slots properly --- src/trio/_core/_ki.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index bd86b5f269..f6d74c54c7 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -81,7 +81,7 @@ class _IdRef(weakref.ref[_T]): - slots = "_hash" + __slots__ = ("_hash", ) _hash: int def __new__(cls, ob: _T, callback: Callable[[Self], Any] | None = None, /) -> Self: From 1f0fc6fd954de83d8e7057c7240daa72086b00d1 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 15:12:38 +0100 Subject: [PATCH 14/36] python 3.8 support --- src/trio/_core/_ki.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index f6d74c54c7..e4f921c651 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -79,9 +79,16 @@ _T = TypeVar("_T") +if TYPE_CHECKING or sys.version_info >= (3, 9): + _Ref = weakref.ref[_T] +else: -class _IdRef(weakref.ref[_T]): - __slots__ = ("_hash", ) + class _Ref(Generic[_T], weakref.ref): + __slots__ = () + + +class _IdRef(_Ref[_T]): + __slots__ = ("_hash",) _hash: int def __new__(cls, ob: _T, callback: Callable[[Self], Any] | None = None, /) -> Self: From 480aa0111a4839cccae6f629ed55f3271085b9bb Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 15:23:50 +0100 Subject: [PATCH 15/36] test reading currently_ki_protected doesn't freeze locals --- src/trio/_core/_tests/test_ki.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index 271c563de2..1941dda5cd 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -3,7 +3,9 @@ import contextlib import inspect import signal +import sys import threading +import weakref from typing import TYPE_CHECKING, AsyncIterator, Callable, Iterator, TypeVar import outcome @@ -11,6 +13,8 @@ from trio.testing import RaisesGroup +from .tutil import gc_collect_harder + try: from async_generator import async_generator, yield_ except ImportError: # pragma: no cover @@ -538,3 +542,16 @@ def decorated() -> bool: decorated_disabled = factory(False) assert decorated_enabled() assert not decorated_disabled() + + +async def test_ki_protection_check_does_not_freeze_locals() -> None: + class A: + pass + + a = A() + wr_a = weakref.ref(a) + assert not _core.currently_ki_protected() + del a + if sys.implementation.name == "pypy": + gc_collect_harder() + assert wr_a() is None From b7a11329150cfaced84b5614f069d1109aec50ae Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 15:56:31 +0100 Subject: [PATCH 16/36] cover some tricky bits of ki.py --- src/trio/_core/_ki.py | 8 ++++++++ src/trio/_core/_tests/test_ki.py | 35 ++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index e4f921c651..a998d5d324 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -112,6 +112,10 @@ def __eq__(self, other: object) -> bool: finally: del my_obj, other_obj + # we're overriding a builtin so we do need this + def __ne__(self, other: object) -> bool: + return not self == other + def __hash__(self) -> int: return self._hash @@ -148,6 +152,10 @@ def __getitem__(self, k: _KT) -> _VT: def __setitem__(self, k: _KT, v: _VT) -> None: self._data[_IdRef(k, self._remove)] = v + # unused by ki.py but needed for coverage + def __delitem__(self, k: _KT) -> None: + del self._data[_IdRef(k)] + _CODE_KI_PROTECTION_STATUS_WMAP: WeakKeyIdentityDictionary[ types.CodeType, diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index 1941dda5cd..21e91f9145 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -22,6 +22,7 @@ from ... import _core from ..._abc import Instrument +from ..._core import _ki from ..._timeouts import sleep from ..._util import signal_raise from ...testing import wait_all_tasks_blocked @@ -555,3 +556,37 @@ class A: if sys.implementation.name == "pypy": gc_collect_harder() assert wr_a() is None + + +def test_identity_weakref_internals() -> None: + """To cover the parts WeakKeyIdentityDictionary won't ever reach.""" + + class A: + def __eq__(self, other: object) -> bool: + return False + + a = A() + wr = _ki._IdRef(a) + wr_other_is_self = wr + + # dict always checks identity before equality so we need to do it here + # to cover `if self is other` + assert wr == wr_other_is_self + + # we want to cover __ne__ and `return NotImplemented` + assert wr != object() + + +def test_weak_key_identity_dict_delitem() -> None: + """We never delitem in KI, but we need to cover the KeyError in self._remove.""" + + class A: + def __eq__(self, other: object) -> bool: + return False + + a = A() + d: _ki.WeakKeyIdentityDictionary[A, bool] = _ki.WeakKeyIdentityDictionary() + d[a] = True + del d[a] + del a + gc_collect_harder() # would call sys.unraisablehook if there's a problem From 05b1fa1694063c706e4b0946dfc778b84a39f220 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 16:20:59 +0100 Subject: [PATCH 17/36] cover a potentially impossible scenario --- src/trio/_core/_ki.py | 4 ---- src/trio/_core/_tests/test_ki.py | 11 ++++++++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index a998d5d324..3cbd6b8214 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -152,10 +152,6 @@ def __getitem__(self, k: _KT) -> _VT: def __setitem__(self, k: _KT, v: _VT) -> None: self._data[_IdRef(k, self._remove)] = v - # unused by ki.py but needed for coverage - def __delitem__(self, k: _KT) -> None: - del self._data[_IdRef(k)] - _CODE_KI_PROTECTION_STATUS_WMAP: WeakKeyIdentityDictionary[ types.CodeType, diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index 21e91f9145..5b85c676c2 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -577,8 +577,8 @@ def __eq__(self, other: object) -> bool: assert wr != object() -def test_weak_key_identity_dict_delitem() -> None: - """We never delitem in KI, but we need to cover the KeyError in self._remove.""" +def test_weak_key_identity_dict_remove_callback_keyerror() -> None: + """We need to cover the KeyError in self._remove.""" class A: def __eq__(self, other: object) -> bool: @@ -586,7 +586,12 @@ def __eq__(self, other: object) -> bool: a = A() d: _ki.WeakKeyIdentityDictionary[A, bool] = _ki.WeakKeyIdentityDictionary() + d[a] = True - del d[a] + + data_copy = d._data.copy() + d._data.clear() del a + gc_collect_harder() # would call sys.unraisablehook if there's a problem + assert data_copy From f8dc61de6e9f8918061d6fcd95d774aca667c20c Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 16:34:02 +0100 Subject: [PATCH 18/36] eek out some last coverage of the eeking out coverage tests --- src/trio/_core/_tests/test_ki.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index 5b85c676c2..e679ba8d29 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -566,6 +566,7 @@ def __eq__(self, other: object) -> bool: return False a = A() + assert a != a wr = _ki._IdRef(a) wr_other_is_self = wr @@ -585,6 +586,7 @@ def __eq__(self, other: object) -> bool: return False a = A() + assert a != a d: _ki.WeakKeyIdentityDictionary[A, bool] = _ki.WeakKeyIdentityDictionary() d[a] = True From 9e6bddb4226b9cf08677c417ab8e7aa5fe3d033f Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 16:37:06 +0100 Subject: [PATCH 19/36] even more partial coverage --- src/trio/_core/_tests/test_ki.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index e679ba8d29..8fee3b6b68 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -597,3 +597,26 @@ def __eq__(self, other: object) -> bool: gc_collect_harder() # would call sys.unraisablehook if there's a problem assert data_copy + + +def test_weak_key_identity_dict_remove_callback_selfref_expired() -> None: + """We need to cover the KeyError in self._remove.""" + + class A: + def __eq__(self, other: object) -> bool: + return False + + a = A() + assert a != a + d: _ki.WeakKeyIdentityDictionary[A, bool] = _ki.WeakKeyIdentityDictionary() + + d[a] = True + + data_copy = d._data.copy() + wr_d = weakref.ref(d) + del d + gc_collect_harder() # would call sys.unraisablehook if there's a problem + assert wr_d() is None + del a + gc_collect_harder() + assert data_copy From d5e539fe20f6ba4a872bae86cd3df63f03f06560 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 18:01:55 +0100 Subject: [PATCH 20/36] Update src/trio/_core/_ki.py --- src/trio/_core/_ki.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index 3cbd6b8214..61e72dacc5 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -108,7 +108,7 @@ def __eq__(self, other: object) -> bool: try: my_obj = self() other_obj = other() - return my_obj is not None and other_obj is not None and my_obj is other_obj + return my_obj is not None and my_obj is other_obj finally: del my_obj, other_obj From 408d1ae536da1b9db7f9e29f8a986d0ae78d4ed7 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 14 Oct 2024 22:09:49 +0100 Subject: [PATCH 21/36] cleaner _IdRef.__eq__ --- src/trio/_core/_ki.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index 61e72dacc5..bb881120af 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -104,13 +104,11 @@ def __eq__(self, other: object) -> bool: return NotImplemented my_obj = None - other_obj: Any = None try: my_obj = self() - other_obj = other() - return my_obj is not None and my_obj is other_obj + return my_obj is not None and my_obj is other() finally: - del my_obj, other_obj + del my_obj # we're overriding a builtin so we do need this def __ne__(self, other: object) -> bool: From 5ba9bd81a99d6b225ed73426e47c3f71513d192b Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Tue, 15 Oct 2024 13:51:19 +0100 Subject: [PATCH 22/36] if the current_task().coro.cr_frame is in the stack ki_protection_enabled is current_task()._ki_protected --- src/trio/_core/_ki.py | 13 ++++++++++++ src/trio/_core/_run.py | 39 ++++------------------------------ src/trio/_core/_run_context.py | 15 +++++++++++++ 3 files changed, 32 insertions(+), 35 deletions(-) create mode 100644 src/trio/_core/_run_context.py diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index bb881120af..33e363f724 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -9,6 +9,7 @@ import attrs from .._util import is_main_thread +from ._run_context import GLOBAL_RUN_CONTEXT if TYPE_CHECKING: import types @@ -170,6 +171,16 @@ def legacy_isasyncgenfunction( # NB: according to the signal.signal docs, 'frame' can be None on entry to # this function: def ki_protection_enabled(frame: types.FrameType | None) -> bool: + try: + task = GLOBAL_RUN_CONTEXT.task + except AttributeError: + task_ki_protected = False + task_frame = None + else: + task_ki_protected = task._ki_protected + task_frame = task.coro.cr_frame + del task + while frame is not None: try: v = _CODE_KI_PROTECTION_STATUS_WMAP[frame.f_code] @@ -179,6 +190,8 @@ def ki_protection_enabled(frame: types.FrameType | None) -> bool: return bool(v) if frame.f_code.co_name == "__del__": return True + if frame is task_frame: + return task_ki_protected frame = frame.f_back return True diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 0c2c3477cd..96e86ff333 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -7,7 +7,6 @@ import random import select import sys -import threading import warnings from collections import deque from contextlib import AbstractAsyncContextManager, contextmanager, suppress @@ -39,8 +38,9 @@ from ._entry_queue import EntryQueue, TrioToken from ._exceptions import Cancelled, RunFinishedError, TrioInternalError from ._instrumentation import Instruments -from ._ki import KIManager, disable_ki_protection, enable_ki_protection +from ._ki import KIManager, enable_ki_protection from ._parking_lot import GLOBAL_PARKING_LOT_BREAKER +from ._run_context import GLOBAL_RUN_CONTEXT as GLOBAL_RUN_CONTEXT from ._thread_cache import start_thread_soon from ._traps import ( Abort, @@ -83,7 +83,6 @@ StatusT_contra = TypeVar("StatusT_contra", contravariant=True) FnT = TypeVar("FnT", bound="Callable[..., Any]") -T = TypeVar("T") RetT = TypeVar("RetT") @@ -1559,14 +1558,6 @@ def raise_cancel() -> NoReturn: ################################################################ -class RunContext(threading.local): - runner: Runner - task: Task - - -GLOBAL_RUN_CONTEXT: Final = RunContext() - - @attrs.frozen class RunStatistics: """An object containing run-loop-level debugging information. @@ -1670,22 +1661,6 @@ def in_main_thread() -> None: start_thread_soon(get_events, deliver) -@enable_ki_protection -def run_with_ki_protection_enabled(f: Callable[[T], RetT], v: T) -> RetT: - try: - return f(v) - finally: - del v # for the case where f is coro.throw() and v is a (Base)Exception - - -@disable_ki_protection -def run_with_ki_protection_disabled(f: Callable[[T], RetT], v: T) -> RetT: - try: - return f(v) - finally: - del v # for the case where f is coro.throw() and v is a (Base)Exception - - @attrs.define(eq=False) class Runner: clock: Clock @@ -2730,11 +2705,6 @@ def unrolled_run( next_send_fn = task._next_send_fn next_send = task._next_send - run_with = ( - run_with_ki_protection_enabled - if task._ki_protected - else run_with_ki_protection_disabled - ) task._next_send_fn = task._next_send = None final_outcome: Outcome[Any] | None = None try: @@ -2747,17 +2717,16 @@ def unrolled_run( # https://github.com/python/cpython/issues/108668 # So now we send in the Outcome object and unwrap it on the # other side. - msg = task.context.run(run_with, next_send_fn, next_send) + msg = task.context.run(next_send_fn, next_send) except StopIteration as stop_iteration: final_outcome = Value(stop_iteration.value) except BaseException as task_exc: # Store for later, removing uninteresting top frames: 1 # frame we always remove, because it's this function - # another is the run_with # catching it, and then in addition we remove however many # more Context.run adds. tb = task_exc.__traceback__ - for _ in range(2 + CONTEXT_RUN_TB_FRAMES): + for _ in range(1 + CONTEXT_RUN_TB_FRAMES): if tb is not None: # pragma: no branch tb = tb.tb_next final_outcome = Error(task_exc.with_traceback(tb)) diff --git a/src/trio/_core/_run_context.py b/src/trio/_core/_run_context.py new file mode 100644 index 0000000000..085bff9a34 --- /dev/null +++ b/src/trio/_core/_run_context.py @@ -0,0 +1,15 @@ +from __future__ import annotations + +import threading +from typing import TYPE_CHECKING, Final + +if TYPE_CHECKING: + from ._run import Runner, Task + + +class RunContext(threading.local): + runner: Runner + task: Task + + +GLOBAL_RUN_CONTEXT: Final = RunContext() From 49bd5c3be43b4c981688fff6fb48e21025675410 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Wed, 16 Oct 2024 08:09:49 +0100 Subject: [PATCH 23/36] Update newsfragments/3108.bugfix.rst --- newsfragments/3108.bugfix.rst | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/newsfragments/3108.bugfix.rst b/newsfragments/3108.bugfix.rst index c6bd5828ac..29b86d3886 100644 --- a/newsfragments/3108.bugfix.rst +++ b/newsfragments/3108.bugfix.rst @@ -1 +1,5 @@ -avoid materializing frame.f_locals in KI protection code, this causes locals to persist in the frame.f_locals snapshot even if they are deleted on 3.12 and below +Rework KeyboardInterrupt protection to track code objects, rather than frames, +as protected or not. This should substantially reduce its overhead, and also fixes +an issue with the previous implementation: locals' lifetimes will no longer be +extended by materialization in the ``frame.f_locals`` dictionary that the previous +KI protection logic needed to access to do its work. From 76b0c79643f6cb72103419587f826c2710a8ae83 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 18 Oct 2024 09:07:54 +0100 Subject: [PATCH 24/36] avoid copying code objects for ki protected function --- src/trio/_core/_ki.py | 8 ++------ src/trio/_core/_tests/test_ki.py | 8 ++++++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index 33e363f724..c484349c2d 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -226,9 +226,7 @@ def enable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: if legacy_isasyncgenfunction(f): f = f.__wrapped__ # type: ignore - code = f.__code__.replace() - _CODE_KI_PROTECTION_STATUS_WMAP[code] = True - f.__code__ = code + _CODE_KI_PROTECTION_STATUS_WMAP[f.__code__] = True return orig @@ -239,9 +237,7 @@ def disable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: if legacy_isasyncgenfunction(f): f = f.__wrapped__ # type: ignore - code = f.__code__.replace() - _CODE_KI_PROTECTION_STATUS_WMAP[code] = False - f.__code__ = code + _CODE_KI_PROTECTION_STATUS_WMAP[f.__code__] = False return orig diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index 8fee3b6b68..a9a88a04fd 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -529,6 +529,14 @@ def _identity(v: _T) -> _T: return v +@pytest.mark.xfail( + strict=True, + raises=AssertionError, + reason=( + "it was decided not to protect against this case, see discussion in: " + "https://github.com/python-trio/trio/pull/3110#discussion_r1802123644" + ), +) async def test_ki_does_not_leak_accross_different_calls_to_inner_functions() -> None: assert not _core.currently_ki_protected() From fa6cd8ec7ba9ee4789fdab95788dea2c8725b3bc Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 18 Oct 2024 11:02:37 +0100 Subject: [PATCH 25/36] Update src/trio/_core/_ki.py --- src/trio/_core/_ki.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index c484349c2d..b375e270eb 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -179,7 +179,6 @@ def ki_protection_enabled(frame: types.FrameType | None) -> bool: else: task_ki_protected = task._ki_protected task_frame = task.coro.cr_frame - del task while frame is not None: try: From be38f4ca295c7258f0aa575eb8dac8dc408c9135 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 18 Oct 2024 11:06:37 +0100 Subject: [PATCH 26/36] Update src/trio/_core/_ki.py Co-authored-by: EXPLOSION --- src/trio/_core/_ki.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index b375e270eb..4166f9a3d9 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -230,7 +230,7 @@ def enable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: def disable_ki_protection(f: _T_supports_code, /) -> _T_supports_code: - """Dectorator to disable KI protection.""" + """Decorator to disable KI protection.""" orig = f if legacy_isasyncgenfunction(f): From b21bde68856b1f53a8e47e583a87d4bc513895e0 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Wed, 23 Oct 2024 09:20:02 +0100 Subject: [PATCH 27/36] remove workaround for 3.8 --- src/trio/_core/_ki.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/trio/_core/_ki.py b/src/trio/_core/_ki.py index 02abc1c648..672501f754 100644 --- a/src/trio/_core/_ki.py +++ b/src/trio/_core/_ki.py @@ -80,15 +80,8 @@ _T = TypeVar("_T") -if TYPE_CHECKING or sys.version_info >= (3, 9): - _Ref = weakref.ref[_T] -else: - class _Ref(weakref.ref, Generic[_T]): - __slots__ = () - - -class _IdRef(_Ref[_T]): +class _IdRef(weakref.ref[_T]): __slots__ = ("_hash",) _hash: int From 0b2dd1a5aa97ba554fc753fc9c060016343b2f94 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Wed, 23 Oct 2024 09:37:26 +0100 Subject: [PATCH 28/36] Add docs and update news Co-Authored-By: oremanj --- docs/source/reference-lowlevel.rst | 34 ++++++++++++++++++++++++++++++ newsfragments/3108.bugfix.rst | 26 +++++++++++++++++++---- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/docs/source/reference-lowlevel.rst b/docs/source/reference-lowlevel.rst index 10c1ddfdc0..b395cd9e30 100644 --- a/docs/source/reference-lowlevel.rst +++ b/docs/source/reference-lowlevel.rst @@ -377,6 +377,40 @@ These transitions are accomplished using two function decorators: poorly-timed :exc:`KeyboardInterrupt` could leave the lock in an inconsistent state and cause a deadlock. + Since KeyboardInterrupt protection is tracked per code object, any attempt to + conditionally protect the same block of code in different ways is unlikely to behave + how you expect. If you try to conditionally protect a closure, it will be + unconditionally protected instead:: + + def example(protect: bool) -> bool: + def inner() -> bool: + return trio.lowlevel.currently_ki_protected() + if protect: + inner = trio.lowlevel.enable_ki_protection(inner) + return inner() + + assert example(False) == False + assert example(True) == True # once protected ... + assert example(False) == True # ... always protected + + If you really need conditional protection, you can achieve it by giving each + KI-protected instance of the closure its own code object:: + + def example(protect: bool) -> bool: + def inner() -> bool: + return trio.lowlevel.currently_ki_protected() + if protect: + inner.__code__ = inner.__code__.replace() + inner = trio.lowlevel.enable_ki_protection(inner) + return inner() + + assert example(False) == False + assert example(True) == True + assert example(False) == False + + (This isn't done by default because it carries some memory overhead and reduces + the potential for specializing optimizations in recent versions of CPython.) + .. autofunction:: currently_ki_protected diff --git a/newsfragments/3108.bugfix.rst b/newsfragments/3108.bugfix.rst index 29b86d3886..ec6d94c7fa 100644 --- a/newsfragments/3108.bugfix.rst +++ b/newsfragments/3108.bugfix.rst @@ -1,5 +1,23 @@ Rework KeyboardInterrupt protection to track code objects, rather than frames, -as protected or not. This should substantially reduce its overhead, and also fixes -an issue with the previous implementation: locals' lifetimes will no longer be -extended by materialization in the ``frame.f_locals`` dictionary that the previous -KI protection logic needed to access to do its work. +as protected or not. The new implementation no longer needs to access +``frame.f_locals`` dictionaries, so it won't artificially extend the lifetime of +local variables. Since KeyboardInterrupt protection is now imposed statically +(when a protected function is defined) rather than each time the function runs, +its previously-noticeable performance overhead should now be near zero. +The lack of a call-time wrapper has some other benefits as well: + +* :func:`inspect.iscoroutinefunction` and the like now give correct answers when + called on KI-protected functions. + +* Calling a synchronous KI-protected function no longer pushes an additional stack + frame, so tracebacks are clearer. + +* A synchronous KI-protected function invoked from C code (such as a weakref + finalizer) is now guaranteed to start executing; previously there would be a brief + window in which KeyboardInterrupt could be raised before the protection was + established. + +One minor drawback of the new approach is that it is no longer possible to apply +different KI protection rules to different instances of the same closure. See the +documentation of `@enable_ki_protection ` +for more details and a workaround. From 12329c7fe9298cde4c000a243aa1d70ec101df62 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Wed, 23 Oct 2024 09:41:02 +0100 Subject: [PATCH 29/36] wrap ki protection locals demos in async def so they work --- docs/source/reference-lowlevel.rst | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/docs/source/reference-lowlevel.rst b/docs/source/reference-lowlevel.rst index b395cd9e30..46c8b4d485 100644 --- a/docs/source/reference-lowlevel.rst +++ b/docs/source/reference-lowlevel.rst @@ -389,9 +389,12 @@ These transitions are accomplished using two function decorators: inner = trio.lowlevel.enable_ki_protection(inner) return inner() - assert example(False) == False - assert example(True) == True # once protected ... - assert example(False) == True # ... always protected + async def amain(): + assert example(False) == False + assert example(True) == True # once protected ... + assert example(False) == True # ... always protected + + trio.run(amain) If you really need conditional protection, you can achieve it by giving each KI-protected instance of the closure its own code object:: @@ -404,9 +407,12 @@ These transitions are accomplished using two function decorators: inner = trio.lowlevel.enable_ki_protection(inner) return inner() - assert example(False) == False - assert example(True) == True - assert example(False) == False + async def amain(): + assert example(False) == False + assert example(True) == True + assert example(False) == False + + trio.run(amain) (This isn't done by default because it carries some memory overhead and reduces the potential for specializing optimizations in recent versions of CPython.) From 97a9eb2da15cabec03c4064a16c8cda441d7b2ca Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Wed, 23 Oct 2024 09:43:03 +0100 Subject: [PATCH 30/36] add newsfragment for 2670 --- newsfragments/2670.bugfix.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 newsfragments/2670.bugfix.rst diff --git a/newsfragments/2670.bugfix.rst b/newsfragments/2670.bugfix.rst new file mode 100644 index 0000000000..cd5ed3b944 --- /dev/null +++ b/newsfragments/2670.bugfix.rst @@ -0,0 +1,2 @@ +:func:`inspect.iscoroutinefunction` and the like now give correct answers when +called on KI-protected functions. From 022bd3dd525f5ebdf3c3ab4d863abf51b660eae2 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 25 Oct 2024 00:42:26 +0100 Subject: [PATCH 31/36] Apply suggestions from code review --- src/trio/_core/_run.py | 2 +- src/trio/_core/_tests/test_ki.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 96e86ff333..3961a6e102 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1773,7 +1773,7 @@ def current_root_task(self) -> Task | None: # Core task handling primitives ################ - @_public # Type-ignore due to use of Any here. + @_public def reschedule( self, task: Task, diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index 38712ca455..12064aa101 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -540,7 +540,7 @@ def _identity(v: _T) -> _T: "https://github.com/python-trio/trio/pull/3110#discussion_r1802123644" ), ) -async def test_ki_does_not_leak_accross_different_calls_to_inner_functions() -> None: +async def test_ki_does_not_leak_across_different_calls_to_inner_functions() -> None: assert not _core.currently_ki_protected() def factory(enabled: bool) -> Callable[[], bool]: From 4fe4d3698f3a348de61546d86fd6bd1dcca4c007 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 25 Oct 2024 00:44:58 +0100 Subject: [PATCH 32/36] use peo614 --- src/trio/_core/_tests/test_ki.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index 12064aa101..2c7106b2ee 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -544,7 +544,7 @@ async def test_ki_does_not_leak_across_different_calls_to_inner_functions() -> N assert not _core.currently_ki_protected() def factory(enabled: bool) -> Callable[[], bool]: - @_identity(_core.enable_ki_protection if enabled else _identity) + @_core.enable_ki_protection if enabled else _identity def decorated() -> bool: return _core.currently_ki_protected() From 0c80de3f41768618da76e12104d71e6a60bb4cfc Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 25 Oct 2024 08:59:07 +0100 Subject: [PATCH 33/36] add tests for passing on inspect flags --- src/trio/_core/_tests/test_ki.py | 69 +++++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index 38712ca455..db66688833 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -29,7 +29,13 @@ from ...testing import wait_all_tasks_blocked if TYPE_CHECKING: - from collections.abc import AsyncIterator, Callable, Iterator + from collections.abc import ( + AsyncGenerator, + AsyncIterator, + Callable, + Generator, + Iterator, + ) from ..._core import Abort, RaiseCancelT @@ -631,3 +637,64 @@ def __eq__(self, other: object) -> bool: del a gc_collect_harder() assert data_copy + + +@_core.enable_ki_protection +async def _protected_async_gen_fn() -> AsyncGenerator[None, None]: + return + yield + + +@_core.enable_ki_protection +async def _protected_async_fn() -> None: + pass + + +@_core.enable_ki_protection +def _protected_gen_fn() -> Generator[None, None, None]: + return + yield + + +@_core.disable_ki_protection +async def _unprotected_async_gen_fn() -> AsyncGenerator[None, None]: + return + yield + + +@_core.disable_ki_protection +async def _unprotected_async_fn() -> None: + pass + + +@_core.disable_ki_protection +def _unprotected_gen_fn() -> Generator[None, None, None]: + return + yield + + +def _consume_function_for_coverage(fn: Callable[..., object]) -> None: + result = fn() + if inspect.isasyncgen(result): + with pytest.raises(StopAsyncIteration): + result.asend(None).send(None) + return + + assert inspect.isgenerator(result) or inspect.iscoroutine(result) + with pytest.raises(StopIteration): + result.send(None) + + +def test_enable_disable_ki_protection_passes_on_inspect_flags() -> None: + assert inspect.isasyncgenfunction(_protected_async_gen_fn) + _consume_function_for_coverage(_protected_async_gen_fn) + assert inspect.iscoroutinefunction(_protected_async_fn) + _consume_function_for_coverage(_protected_async_fn) + assert inspect.isgeneratorfunction(_protected_gen_fn) + _consume_function_for_coverage(_protected_gen_fn) + assert inspect.isasyncgenfunction(_unprotected_async_gen_fn) + _consume_function_for_coverage(_unprotected_async_gen_fn) + assert inspect.iscoroutinefunction(_unprotected_async_fn) + _consume_function_for_coverage(_unprotected_async_fn) + assert inspect.isgeneratorfunction(_unprotected_gen_fn) + _consume_function_for_coverage(_unprotected_gen_fn) From 1e10e62c633f935dea6eb400547089e61ffe31d5 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Fri, 25 Oct 2024 10:04:09 +0100 Subject: [PATCH 34/36] 'return; yield' isn't considered covered --- src/trio/_core/_tests/test_ki.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/trio/_core/_tests/test_ki.py b/src/trio/_core/_tests/test_ki.py index 4c73af57d2..d403cfa7a1 100644 --- a/src/trio/_core/_tests/test_ki.py +++ b/src/trio/_core/_tests/test_ki.py @@ -641,7 +641,6 @@ def __eq__(self, other: object) -> bool: @_core.enable_ki_protection async def _protected_async_gen_fn() -> AsyncGenerator[None, None]: - return yield @@ -652,13 +651,11 @@ async def _protected_async_fn() -> None: @_core.enable_ki_protection def _protected_gen_fn() -> Generator[None, None, None]: - return yield @_core.disable_ki_protection async def _unprotected_async_gen_fn() -> AsyncGenerator[None, None]: - return yield @@ -669,20 +666,27 @@ async def _unprotected_async_fn() -> None: @_core.disable_ki_protection def _unprotected_gen_fn() -> Generator[None, None, None]: - return yield +async def _consume_async_generator(agen: AsyncGenerator[None, None]) -> None: + try: + with pytest.raises(StopAsyncIteration): + while True: + await agen.asend(None) + finally: + await agen.aclose() + + def _consume_function_for_coverage(fn: Callable[..., object]) -> None: result = fn() if inspect.isasyncgen(result): - with pytest.raises(StopAsyncIteration): - result.asend(None).send(None) - return + result = _consume_async_generator(result) assert inspect.isgenerator(result) or inspect.iscoroutine(result) with pytest.raises(StopIteration): - result.send(None) + while True: + result.send(None) def test_enable_disable_ki_protection_passes_on_inspect_flags() -> None: From 0cd1feb8cd4dc4e5775fb3744c13c6abe16b441e Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Sat, 26 Oct 2024 10:58:18 +0100 Subject: [PATCH 35/36] Update newsfragments/3108.bugfix.rst --- newsfragments/3108.bugfix.rst | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/newsfragments/3108.bugfix.rst b/newsfragments/3108.bugfix.rst index ec6d94c7fa..22cc5223d7 100644 --- a/newsfragments/3108.bugfix.rst +++ b/newsfragments/3108.bugfix.rst @@ -17,7 +17,10 @@ The lack of a call-time wrapper has some other benefits as well: window in which KeyboardInterrupt could be raised before the protection was established. -One minor drawback of the new approach is that it is no longer possible to apply -different KI protection rules to different instances of the same closure. See the +One minor drawback of the new approach is that multiple instances of the same +closure share a single KeyboardInterrupt protection state (because they share a +single code object). That means that if you apply +`@enable_ki_protection ` to some of them +and not others, you won't get the protection semantics you asked for. See the documentation of `@enable_ki_protection ` for more details and a workaround. From 6e97f9a861f7d77e1a7acd6aa7b72972ebf59850 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 26 Oct 2024 09:59:53 +0000 Subject: [PATCH 36/36] [pre-commit.ci] auto fixes from pre-commit.com hooks --- newsfragments/3108.bugfix.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/newsfragments/3108.bugfix.rst b/newsfragments/3108.bugfix.rst index 22cc5223d7..16cf46b960 100644 --- a/newsfragments/3108.bugfix.rst +++ b/newsfragments/3108.bugfix.rst @@ -19,7 +19,7 @@ The lack of a call-time wrapper has some other benefits as well: One minor drawback of the new approach is that multiple instances of the same closure share a single KeyboardInterrupt protection state (because they share a -single code object). That means that if you apply +single code object). That means that if you apply `@enable_ki_protection ` to some of them and not others, you won't get the protection semantics you asked for. See the documentation of `@enable_ki_protection `