Merge pull request #348 from bluetech/subset-caller-6

Improve subset hook caller & related changes

Author: bluetech

src/pluggy/_hooks.py

@@ -6,6 +6,7 @@
 import warnings
 from types import ModuleType
 from typing import (
+    AbstractSet,
     Any,
     Callable,
     Generator,
@@ -286,8 +287,7 @@ class _HookCaller:
         "name",
         "spec",
         "_hookexec",
-        "_wrappers",
-        "_nonwrappers",
+        "_hookimpls",
         "_call_history",
     )
 
@@ -300,8 +300,7 @@ def __init__(
     ) -> None:
         self.name: "Final" = name
         self._hookexec: "Final" = hook_execute
-        self._wrappers: "Final[List[HookImpl]]" = []
-        self._nonwrappers: "Final[List[HookImpl]]" = []
+        self._hookimpls: "Final[List[HookImpl]]" = []
         self._call_history: Optional[_CallHistory] = None
         self.spec: Optional[HookSpec] = None
         if specmodule_or_class is not None:
@@ -325,38 +324,38 @@ def is_historic(self) -> bool:
         return self._call_history is not None
 
     def _remove_plugin(self, plugin: _Plugin) -> None:
-        def remove(wrappers: List[HookImpl]) -> Optional[bool]:
-            for i, method in enumerate(wrappers):
-                if method.plugin == plugin:
-                    del wrappers[i]
-                    return True
-            return None
-
-        if remove(self._wrappers) is None:
-            if remove(self._nonwrappers) is None:
-                raise ValueError(f"plugin {plugin!r} not found")
+        for i, method in enumerate(self._hookimpls):
+            if method.plugin == plugin:
+                del self._hookimpls[i]
+                return
+        raise ValueError(f"plugin {plugin!r} not found")
 
     def get_hookimpls(self) -> List["HookImpl"]:
-        # Order is important for _hookexec
-        return self._nonwrappers + self._wrappers
+        return self._hookimpls.copy()
 
     def _add_hookimpl(self, hookimpl: "HookImpl") -> None:
         """Add an implementation to the callback chain."""
+        for i, method in enumerate(self._hookimpls):
+            if method.hookwrapper:
+                splitpoint = i
+                break
+        else:
+            splitpoint = len(self._hookimpls)
         if hookimpl.hookwrapper:
-            methods = self._wrappers
+            start, end = splitpoint, len(self._hookimpls)
         else:
-            methods = self._nonwrappers
+            start, end = 0, splitpoint
 
         if hookimpl.trylast:
-            methods.insert(0, hookimpl)
+            self._hookimpls.insert(start, hookimpl)
         elif hookimpl.tryfirst:
-            methods.append(hookimpl)
+            self._hookimpls.insert(end, hookimpl)
         else:
             # find last non-tryfirst method
-            i = len(methods) - 1
-            while i >= 0 and methods[i].tryfirst:
+            i = end - 1
+            while i >= start and self._hookimpls[i].tryfirst:
                 i -= 1
-            methods.insert(i + 1, hookimpl)
+            self._hookimpls.insert(i + 1, hookimpl)
 
     def __repr__(self) -> str:
         return f"<_HookCaller {self.name!r}>"
@@ -387,7 +386,7 @@ def __call__(self, *args: object, **kwargs: object) -> Any:
         ), "Cannot directly call a historic hook - use call_historic instead."
         self._verify_all_args_are_provided(kwargs)
         firstresult = self.spec.opts.get("firstresult", False) if self.spec else False
-        return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
+        return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
 
     def call_historic(
         self,
@@ -406,7 +405,7 @@ def call_historic(
         self._call_history.append((kwargs, result_callback))
         # Historizing hooks don't return results.
         # Remember firstresult isn't compatible with historic.
-        res = self._hookexec(self.name, self.get_hookimpls(), kwargs, False)
+        res = self._hookexec(self.name, self._hookimpls, kwargs, False)
         if result_callback is None:
             return
         if isinstance(res, list):
@@ -429,13 +428,12 @@ def call_extra(
             "tryfirst": False,
             "specname": None,
         }
-        hookimpls = self.get_hookimpls()
+        hookimpls = self._hookimpls.copy()
         for method in methods:
             hookimpl = HookImpl(None, "<temp>", method, opts)
             # Find last non-tryfirst nonwrapper method.
             i = len(hookimpls) - 1
-            until = len(self._nonwrappers)
-            while i >= until and hookimpls[i].tryfirst:
+            while i >= 0 and hookimpls[i].tryfirst and not hookimpls[i].hookwrapper:
                 i -= 1
             hookimpls.insert(i + 1, hookimpl)
         firstresult = self.spec.opts.get("firstresult", False) if self.spec else False
@@ -453,6 +451,53 @@ def _maybe_apply_history(self, method: "HookImpl") -> None:
                     result_callback(res[0])
 
 
+class _SubsetHookCaller(_HookCaller):
+    """A proxy to another HookCaller which manages calls to all registered
+    plugins except the ones from remove_plugins."""
+
+    # This class is unusual: in inhertits from `_HookCaller` so all of
+    # the *code* runs in the class, but it delegates all underlying *data*
+    # to the original HookCaller.
+    # `subset_hook_caller` used to be implemented by creating a full-fledged
+    # HookCaller, copying all hookimpls from the original. This had problems
+    # with memory leaks (#346) and historic calls (#347), which make a proxy
+    # approach better.
+    # An alternative implementation is to use a `_getattr__`/`__getattribute__`
+    # proxy, however that adds more overhead and is more tricky to implement.
+
+    __slots__ = (
+        "_orig",
+        "_remove_plugins",
+        "name",
+        "_hookexec",
+    )
+
+    def __init__(self, orig: _HookCaller, remove_plugins: AbstractSet[_Plugin]) -> None:
+        self._orig = orig
+        self._remove_plugins = remove_plugins
+        self.name = orig.name  # type: ignore[misc]
+        self._hookexec = orig._hookexec  # type: ignore[misc]
+
+    @property  # type: ignore[misc]
+    def _hookimpls(self) -> List["HookImpl"]:  # type: ignore[override]
+        return [
+            impl
+            for impl in self._orig._hookimpls
+            if impl.plugin not in self._remove_plugins
+        ]
+
+    @property
+    def spec(self) -> Optional["HookSpec"]:  # type: ignore[override]
+        return self._orig.spec
+
+    @property
+    def _call_history(self) -> Optional[_CallHistory]:  # type: ignore[override]
+        return self._orig._call_history
+
+    def __repr__(self) -> str:
+        return f"<_SubsetHookCaller {self.name!r}>"
+
+
 class HookImpl:
     __slots__ = (
         "function",

src/pluggy/_manager.py

@@ -25,6 +25,7 @@
     HookImpl,
     HookSpec,
     _HookCaller,
+    _SubsetHookCaller,
     _HookImplFunction,
     _HookRelay,
     _Namespace,
@@ -114,7 +115,6 @@ class PluginManager:
     def __init__(self, project_name: str) -> None:
         self.project_name: "Final" = project_name
         self._name2plugin: "Final[Dict[str, _Plugin]]" = {}
-        self._plugin2hookcallers: "Final[Dict[_Plugin, List[_HookCaller]]]" = {}
         self._plugin_distinfo: "Final[List[Tuple[_Plugin, DistFacade]]]" = []
         self.trace: "Final" = _tracing.TagTracer().get("pluginmanage")
         self.hook: "Final" = _HookRelay()
@@ -137,11 +137,17 @@ def register(self, plugin: _Plugin, name: Optional[str] = None) -> Optional[str]
         is already registered."""
         plugin_name = name or self.get_canonical_name(plugin)
 
-        if plugin_name in self._name2plugin or plugin in self._plugin2hookcallers:
+        if plugin_name in self._name2plugin:
             if self._name2plugin.get(plugin_name, -1) is None:
                 return None  # blocked plugin, return None to indicate no registration
             raise ValueError(
-                "Plugin already registered: %s=%s\n%s"
+                "Plugin name already registered: %s=%s\n%s"
+                % (plugin_name, plugin, self._name2plugin)
+            )
+
+        if plugin in self._name2plugin.values():
+            raise ValueError(
+                "Plugin already registered under a different name: %s=%s\n%s"
                 % (plugin_name, plugin, self._name2plugin)
             )
 
@@ -150,8 +156,6 @@ def register(self, plugin: _Plugin, name: Optional[str] = None) -> Optional[str]
         self._name2plugin[plugin_name] = plugin
 
         # register matching hook implementations of the plugin
-        hookcallers: List[_HookCaller] = []
-        self._plugin2hookcallers[plugin] = hookcallers
         for name in dir(plugin):
             hookimpl_opts = self.parse_hookimpl_opts(plugin, name)
             if hookimpl_opts is not None:
@@ -167,7 +171,6 @@ def register(self, plugin: _Plugin, name: Optional[str] = None) -> Optional[str]
                     self._verify_hook(hook, hookimpl)
                     hook._maybe_apply_history(hookimpl)
                 hook._add_hookimpl(hookimpl)
-                hookcallers.append(hook)
         return plugin_name
 
     def parse_hookimpl_opts(
@@ -200,14 +203,16 @@ def unregister(
         if plugin is None:
             plugin = self.get_plugin(name)
 
+        hookcallers = self.get_hookcallers(plugin)
+        if hookcallers:
+            for hookcaller in hookcallers:
+                hookcaller._remove_plugin(plugin)
+
         # if self._name2plugin[name] == None registration was blocked: ignore
         if self._name2plugin.get(name):
             assert name is not None
             del self._name2plugin[name]
 
-        for hookcaller in self._plugin2hookcallers.pop(plugin, []):
-            hookcaller._remove_plugin(plugin)
-
         return plugin
 
     def set_blocked(self, name: str) -> None:
@@ -253,11 +258,11 @@ def parse_hookspec_opts(
 
     def get_plugins(self) -> Set[Any]:
         """return the set of registered plugins."""
-        return set(self._plugin2hookcallers)
+        return set(self._name2plugin.values())
 
     def is_registered(self, plugin: _Plugin) -> bool:
         """Return ``True`` if the plugin is already registered."""
-        return plugin in self._plugin2hookcallers
+        return any(plugin == val for val in self._name2plugin.values())
 
     def get_canonical_name(self, plugin: _Plugin) -> str:
         """Return canonical name for a plugin object. Note that a plugin
@@ -372,7 +377,14 @@ def list_name_plugin(self) -> List[Tuple[str, _Plugin]]:
 
     def get_hookcallers(self, plugin: _Plugin) -> Optional[List[_HookCaller]]:
         """get all hook callers for the specified plugin."""
-        return self._plugin2hookcallers.get(plugin)
+        if self.get_name(plugin) is None:
+            return None
+        hookcallers = []
+        for hookcaller in self.hook.__dict__.values():
+            for hookimpl in hookcaller.get_hookimpls():
+                if hookimpl.plugin is plugin:
+                    hookcallers.append(hookcaller)
+        return hookcallers
 
     def add_hookcall_monitoring(
         self, before: _BeforeTrace, after: _AfterTrace
@@ -436,24 +448,13 @@ def after(
     def subset_hook_caller(
         self, name: str, remove_plugins: Iterable[_Plugin]
     ) -> _HookCaller:
-        """Return a new :py:class:`._hooks._HookCaller` instance for the named method
-        which manages calls to all registered plugins except the
-        ones from remove_plugins."""
+        """Return a proxy :py:class:`._hooks._HookCaller` instance for the named
+        method which manages calls to all registered plugins except the ones
+        from remove_plugins."""
         orig: _HookCaller = getattr(self.hook, name)
-        plugins_to_remove = [plug for plug in remove_plugins if hasattr(plug, name)]
+        plugins_to_remove = {plug for plug in remove_plugins if hasattr(plug, name)}
         if plugins_to_remove:
-            assert orig.spec is not None
-            hc = _HookCaller(
-                orig.name, orig._hookexec, orig.spec.namespace, orig.spec.opts
-            )
-            for hookimpl in orig.get_hookimpls():
-                plugin = hookimpl.plugin
-                if plugin not in plugins_to_remove:
-                    hc._add_hookimpl(hookimpl)
-                    # we also keep track of this hook caller so it
-                    # gets properly removed on plugin unregistration
-                    self._plugin2hookcallers.setdefault(plugin, []).append(hc)
-            return hc
+            return _SubsetHookCaller(orig, plugins_to_remove)
         return orig
 
 

testing/test_details.py

@@ -34,13 +34,18 @@ def x1meth2(self):
     pm = MyPluginManager(hookspec.project_name)
     pm.register(Plugin())
     pm.add_hookspecs(Spec)
-    assert not pm.hook.x1meth._nonwrappers[0].hookwrapper
-    assert not pm.hook.x1meth._nonwrappers[0].tryfirst
-    assert not pm.hook.x1meth._nonwrappers[0].trylast
-    assert not pm.hook.x1meth._nonwrappers[0].optionalhook
 
-    assert pm.hook.x1meth2._wrappers[0].tryfirst
-    assert pm.hook.x1meth2._wrappers[0].hookwrapper
+    hookimpls = pm.hook.x1meth.get_hookimpls()
+    assert len(hookimpls) == 1
+    assert not hookimpls[0].hookwrapper
+    assert not hookimpls[0].tryfirst
+    assert not hookimpls[0].trylast
+    assert not hookimpls[0].optionalhook
+
+    hookimpls = pm.hook.x1meth2.get_hookimpls()
+    assert len(hookimpls) == 1
+    assert hookimpls[0].hookwrapper
+    assert hookimpls[0].tryfirst
 
 
 def test_warn_when_deprecated_specified(recwarn) -> None:
@@ -127,6 +132,6 @@ def myhook(self):
 
     plugin = Plugin()
     pname = pm.register(plugin)
-    assert repr(pm.hook.myhook._nonwrappers[0]) == (
+    assert repr(pm.hook.myhook.get_hookimpls()[0]) == (
         f"<HookImpl plugin_name={pname!r}, plugin={plugin!r}>"
     )