From 7d7cf40c9302261febbb146654f212cb61c5669a Mon Sep 17 00:00:00 2001 From: Rupayon Haldar <80724680+rupayon123@users.noreply.github.com> Date: Wed, 28 Oct 2026 08:43:48 -0400 Subject: [PATCH 1/3] Remove only the requested hook-call monitor --- src/pluggy/_manager.py | 68 +++++++--- testing/test_monitoring.py | 258 +++++++++++++++++++++++++++++++++++++ 2 files changed, 308 insertions(+), 18 deletions(-) create mode 100644 testing/test_monitoring.py diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index f9cb47dc..2269fd0d 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -16,6 +16,7 @@ from . import _tracing from ._callers import _multicall from ._compat import DistFacade +from ._hooks import _HookExec from ._hooks import _HookImplFunction from ._hooks import _Namespace from ._hooks import _Plugin @@ -39,6 +40,32 @@ ] +class _HookCallMonitor: + def __init__( + self, inner: _HookExec, before: _BeforeTrace, after: _AfterTrace, token: object + ) -> None: + self.inner: Final = inner + self.before: Final = before + self.after: Final = after + self.token: Final = token + + def __call__( + self, + hook_name: str, + hook_impls: Sequence[HookImpl], + caller_kwargs: Mapping[str, object], + firstresult: bool, + ) -> object | list[object]: + # An undo during a callback affects subsequent calls, not this pair. + inner = self.inner + self.before(hook_name, hook_impls, caller_kwargs) + outcome = Result.from_call( + lambda: inner(hook_name, hook_impls, caller_kwargs, firstresult) + ) + self.after(outcome, hook_name, hook_impls, caller_kwargs) + return outcome.get_result() + + def _warn_for_function(warning: Warning, function: Callable[..., object]) -> None: func = cast(types.FunctionType, function) warnings.warn_explicit( @@ -167,7 +194,7 @@ def __init__(self, project_name: str) -> None: self.trace: Final[_tracing.TagTracerSub] = _tracing.TagTracer().get( "pluginmanage" ) - self._inner_hookexec = _multicall + self._inner_hookexec: _HookExec = _multicall def _hookexec( self, @@ -587,25 +614,30 @@ def add_hookcall_monitoring( same arguments as ``before`` but also a :class:`~pluggy.Result` object which represents the result of the overall hook call. """ - oldcall = self._inner_hookexec - - def traced_hookexec( - hook_name: str, - hook_impls: Sequence[HookImpl], - caller_kwargs: Mapping[str, object], - firstresult: bool, - ) -> object | list[object]: - before(hook_name, hook_impls, caller_kwargs) - outcome = Result.from_call( - lambda: oldcall(hook_name, hook_impls, caller_kwargs, firstresult) - ) - after(outcome, hook_name, hook_impls, caller_kwargs) - return outcome.get_result() - - self._inner_hookexec = traced_hookexec + token: object | None = object() + self._inner_hookexec = _HookCallMonitor( + self._inner_hookexec, before, after, token + ) def undo() -> None: - self._inner_hookexec = oldcall + nonlocal token + if token is None: + return + current = self._inner_hookexec + outer: list[_HookCallMonitor] = [] + while isinstance(current, _HookCallMonitor): + if current.token is token: + inner = current.inner + # Keep the old chain intact for hooks already in flight. + for monitor in reversed(outer): + inner = _HookCallMonitor( + inner, monitor.before, monitor.after, monitor.token + ) + self._inner_hookexec = inner + break + outer.append(current) + current = current.inner + token = None return undo diff --git a/testing/test_monitoring.py b/testing/test_monitoring.py new file mode 100644 index 00000000..2997189a --- /dev/null +++ b/testing/test_monitoring.py @@ -0,0 +1,258 @@ +from collections.abc import Callable +from collections.abc import Mapping +from collections.abc import Sequence +import gc +from itertools import permutations +from typing import Any +import weakref + +import pytest + +from pluggy import HookImpl +from pluggy import HookimplMarker +from pluggy import PluginManager +from pluggy import Result + + +hookimpl = HookimplMarker("example") + + +class Plugin: + @hookimpl + def he_method1(self, arg: int) -> int: + return arg + + +def add_monitor(pm: PluginManager, label: str, events: list[str]) -> Callable[[], None]: + def before( + name: str, implementations: Sequence[HookImpl], kwargs: Mapping[str, Any] + ) -> None: + events.append(f"{label}:before") + + def after( + outcome: Result[Any], + name: str, + implementations: Sequence[HookImpl], + kwargs: Mapping[str, Any], + ) -> None: + events.append(f"{label}:after") + + return pm.add_hookcall_monitoring(before, after) + + +@pytest.mark.parametrize("order", list(permutations(("A", "B", "C")))) +def test_monitor_undo_order(he_pm: PluginManager, order: tuple[str, ...]) -> None: + he_pm.register(Plugin()) + events: list[str] = [] + labels = ["A", "B", "C"] + undos = {label: add_monitor(he_pm, label, events) for label in labels} + + def call() -> None: + events.clear() + assert he_pm.hook.he_method1(arg=42) == [42] + assert events == [f"{label}:before" for label in reversed(labels)] + [ + f"{label}:after" for label in labels + ] + + call() + for label in order: + undos[label]() + labels.remove(label) + call() + undos[label]() + call() + + +def test_monitor_undo_does_not_remove_new_monitor(he_pm: PluginManager) -> None: + he_pm.register(Plugin()) + events: list[str] = [] + old_undo = add_monitor(he_pm, "old", events) + old_undo() + undo = add_monitor(he_pm, "new", events) + old_undo() + assert he_pm.hook.he_method1(arg=42) == [42] + assert events == ["new:before", "new:after"] + undo() + + +@pytest.mark.parametrize("remove_before", [False, True]) +@pytest.mark.parametrize("remove_self", [False, True]) +def test_monitor_undo_during_call( + he_pm: PluginManager, remove_before: bool, remove_self: bool +) -> None: + he_pm.register(Plugin()) + events: list[str] = [] + undo_old = add_monitor(he_pm, "old", events) + + def before( + name: str, implementations: Sequence[HookImpl], kwargs: Mapping[str, Any] + ) -> None: + events.append("new:before") + if remove_before: + (undo_new if remove_self else undo_old)() + + def after( + outcome: Result[Any], + name: str, + implementations: Sequence[HookImpl], + kwargs: Mapping[str, Any], + ) -> None: + events.append("new:after") + if not remove_before: + (undo_new if remove_self else undo_old)() + + undo_new = he_pm.add_hookcall_monitoring(before, after) + assert he_pm.hook.he_method1(arg=42) == [42] + assert events == ["new:before", "old:before", "old:after", "new:after"] + events.clear() + assert he_pm.hook.he_method1(arg=42) == [42] + label = "old" if remove_self else "new" + assert events == [f"{label}:before", f"{label}:after"] + undo_old() + undo_new() + + +def test_removing_deep_monitor_preserves_in_flight_call(he_pm: PluginManager) -> None: + he_pm.register(Plugin()) + events: list[str] = [] + undo_oldest = add_monitor(he_pm, "oldest", events) + undo_middle = add_monitor(he_pm, "middle", events) + + def before( + name: str, implementations: Sequence[HookImpl], kwargs: Mapping[str, Any] + ) -> None: + events.append("newest:before") + undo_oldest() + + def after( + outcome: Result[Any], + name: str, + implementations: Sequence[HookImpl], + kwargs: Mapping[str, Any], + ) -> None: + events.append("newest:after") + + undo_newest = he_pm.add_hookcall_monitoring(before, after) + assert he_pm.hook.he_method1(arg=42) == [42] + assert events == [ + "newest:before", + "middle:before", + "oldest:before", + "oldest:after", + "middle:after", + "newest:after", + ] + events.clear() + assert he_pm.hook.he_method1(arg=42) == [42] + assert events == ["newest:before", "middle:before", "middle:after", "newest:after"] + undo_middle() + undo_newest() + + +@pytest.mark.parametrize("tracing_first", [False, True]) +def test_monitor_undo_keeps_other_tracing( + he_pm: PluginManager, tracing_first: bool +) -> None: + he_pm.register(Plugin()) + events: list[str] = [] + traces: list[str] = [] + he_pm.trace.root.setwriter(traces.append) + if tracing_first: + undo_first = he_pm.enable_tracing() + undo_second = add_monitor(he_pm, "monitor", events) + else: + undo_first = add_monitor(he_pm, "monitor", events) + undo_second = he_pm.enable_tracing() + undo_first() + assert he_pm.hook.he_method1(arg=42) == [42] + assert events == (["monitor:before", "monitor:after"] if tracing_first else []) + assert len(traces) == (0 if tracing_first else 2) + assert he_pm.trace.root.indent == 0 + undo_second() + events.clear() + traces.clear() + assert he_pm.hook.he_method1(arg=42) == [42] + assert events == traces == [] + + +@pytest.mark.parametrize("raises", [False, True]) +def test_monitor_undo_preserves_outcomes(he_pm: PluginManager, raises: bool) -> None: + error = ValueError("from hook") + + class OutcomePlugin: + @hookimpl + def he_method1(self, arg: int) -> int: + if raises: + raise error + return arg + + he_pm.register(OutcomePlugin()) + observed: list[object] = [] + + def replace_result( + outcome: Result[Any], + name: str, + implementations: Sequence[HookImpl], + kwargs: Mapping[str, Any], + ) -> None: + assert outcome.exception is (error if raises else None) + if not raises: + assert outcome.get_result() == [42] + outcome.force_result([99]) + + def observe_result( + outcome: Result[Any], + name: str, + implementations: Sequence[HookImpl], + kwargs: Mapping[str, Any], + ) -> None: + observed.append( + outcome.exception if outcome.exception is not None else outcome.get_result() + ) + + undo_replace = he_pm.add_hookcall_monitoring(lambda *args: None, replace_result) + undo_observe = he_pm.add_hookcall_monitoring(lambda *args: None, observe_result) + assert he_pm.hook.he_method1(arg=42) == [99] + assert observed == [[99]] + undo_replace() + if raises: + with pytest.raises(ValueError) as exc_info: + he_pm.hook.he_method1(arg=42) + assert exc_info.value is error + assert observed == [[99], error] + else: + assert he_pm.hook.he_method1(arg=42) == [42] + assert observed == [[99], [42]] + undo_observe() + + +def test_removed_monitors_are_released(he_pm: PluginManager) -> None: + he_pm.register(Plugin()) + events: list[str] = [] + + class Before: + def __call__( + self, + name: str, + implementations: Sequence[HookImpl], + kwargs: Mapping[str, Any], + ) -> None: + events.append("before") + + callbacks: list[weakref.ReferenceType[Before]] = [] + + def add() -> Callable[[], None]: + before = Before() + callbacks.append(weakref.ref(before)) + return he_pm.add_hookcall_monitoring(before, lambda *args: None) + + for _ in range(100): + undo_old = add() + undo_new = add() + undo_old() + undo_new() + del undo_old, undo_new + gc.collect() + assert all(callback() is None for callback in callbacks) + assert he_pm.hook.he_method1(arg=42) == [42] + assert not events From 310ebd576e419a63df2541a85748d051c3d24f70 Mon Sep 17 00:00:00 2001 From: Rupayon Haldar <80724680+rupayon123@users.noreply.github.com> Date: Wed, 28 Oct 2026 08:48:16 -0400 Subject: [PATCH 2/3] Add numbered changelog for monitor undo fix --- changelog/814.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/814.bugfix.rst diff --git a/changelog/814.bugfix.rst b/changelog/814.bugfix.rst new file mode 100644 index 00000000..13020348 --- /dev/null +++ b/changelog/814.bugfix.rst @@ -0,0 +1 @@ +Fix :meth:`~pluggy.PluginManager.add_hookcall_monitoring` undo callbacks removing other monitors or restoring removed ones when called out of order or repeatedly. From 56db3b71670eac9c32841d0d32a516638e571166 Mon Sep 17 00:00:00 2001 From: Rupayon Haldar <80724680+rupayon123@users.noreply.github.com> Date: Wed, 28 Oct 2026 12:13:00 -0400 Subject: [PATCH 3/3] Exercise monitor lifetime callbacks and clarify undo invariant --- src/pluggy/_manager.py | 18 ++++++++++-------- testing/test_monitoring.py | 3 +++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 2269fd0d..f4ec0673 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -625,18 +625,20 @@ def undo() -> None: return current = self._inner_hookexec outer: list[_HookCallMonitor] = [] - while isinstance(current, _HookCallMonitor): + while True: + # An active token is always present in the monitoring chain. + assert isinstance(current, _HookCallMonitor) if current.token is token: - inner = current.inner - # Keep the old chain intact for hooks already in flight. - for monitor in reversed(outer): - inner = _HookCallMonitor( - inner, monitor.before, monitor.after, monitor.token - ) - self._inner_hookexec = inner break outer.append(current) current = current.inner + inner = current.inner + # Keep the old chain intact for hooks already in flight. + for monitor in reversed(outer): + inner = _HookCallMonitor( + inner, monitor.before, monitor.after, monitor.token + ) + self._inner_hookexec = inner token = None return undo diff --git a/testing/test_monitoring.py b/testing/test_monitoring.py index 2997189a..f9015a71 100644 --- a/testing/test_monitoring.py +++ b/testing/test_monitoring.py @@ -249,6 +249,9 @@ def add() -> Callable[[], None]: for _ in range(100): undo_old = add() undo_new = add() + assert he_pm.hook.he_method1(arg=42) == [42] + assert events == ["before", "before"] + events.clear() undo_old() undo_new() del undo_old, undo_new