Merge branch 'release/2.2.2'

Author: GrahamDumpleton

docs/bundled.rst

@@ -112,6 +112,35 @@ single shared cache.
     >>> fibonacci.cache_parameters()
     {'maxsize': 128, 'typed': False}
 
+Because the per-instance cache is stored as an attribute on the instance, an
+instance whose cached method has been called holds a
+``functools._lru_cache_wrapper`` object in its ``__dict__``, and that object
+is not picklable. If instances of the class need to be pickled, override
+``__getstate__`` to drop the cache attributes from the pickled state. The
+cache attribute names all begin with ``_lru_cache_``, so filtering on that
+prefix excludes every per-instance cache. The caches are recreated lazily on
+the next call after unpickling.
+
+::
+
+    class MyClass:
+
+        @wrapt.lru_cache
+        def compute(self, x):
+            return x * 2
+
+        def __getstate__(self):
+            return {
+                key: value
+                for key, value in self.__dict__.items()
+                if not key.startswith("_lru_cache_")
+            }
+
+To target the cache for a particular method rather than all of them, filter
+on the more specific ``_lru_cache_<name>_`` prefix, where ``<name>`` is the
+method name. For example, ``_lru_cache_compute_`` matches only the cache for
+the ``compute`` method above.
+
 Thread Synchronization
 ----------------------
 

docs/changes.rst

@@ -1,6 +1,42 @@
 Release Notes
 =============
 
+Version 2.2.2
+-------------
+
+**Bugs Fixed**
+
+* When ``@wrapt.lru_cache`` was applied to an instance method that was
+  overridden in a subclass, and the subclass method called the base class
+  method via ``super()``, a ``RecursionError`` was raised instead of the
+  base class method being invoked. The per-instance cache for each method
+  was stored as an attribute on the instance whose name was derived only
+  from the method ``__name__``, so the base and derived methods shared a
+  single cache slot. The subclass cache was therefore found again when the
+  base method was reached through ``super()``, re-entering the subclass body
+  and recursing without end. The cache attribute name now incorporates a
+  unique identifier for each decorated method so that a base method and a
+  method that overrides it use distinct per-instance caches. With thanks to
+  the reporter of `issue #342
+  <https://github.com/GrahamDumpleton/wrapt/issues/342>`_.
+
+* When ``@wrapt.lru_cache`` was applied to a method of a class deriving from
+  ``wrapt.ObjectProxy``, the per-instance cache was stored on the wrapped
+  object rather than on the proxy. This is because the proxy ``__setattr__``
+  forwards attribute assignment to the wrapped object for any name that is
+  not a recognised proxy attribute, and the cache attribute name was not one.
+  Storing the cache on the wrapped object had several consequences: the
+  wrapped object was polluted with cache attributes it never defined; the
+  cache held a reference back to the proxy through the bound method it
+  wrapped, so a wrapped object that outlived the proxy kept the proxy alive
+  and prevented its collection; wrapping an object that does not accept
+  arbitrary attributes, such as one using ``__slots__``, caused the first
+  cached call to fail with an ``AttributeError``; and two proxies sharing a
+  single wrapped object shared one cache and could return results computed
+  for the wrong proxy. The cache attribute is now stored on the proxy itself
+  using the proxy ``__self_setattr__`` method when the instance is a wrapt
+  object proxy, falling back to ``setattr`` for ordinary instances.
+
 Version 2.2.1
 -------------
 

docs/issues.rst

@@ -221,6 +221,25 @@ class whose metaclass is ``abc.ABCMeta`` will fail when used with
     # TypeError: descriptor '__subclasscheck__' for '_wrappers.ObjectProxy'
     # objects doesn't apply to a 'type' object
 
+Under the pure Python implementation of ``ObjectProxy`` (used when the
+C extension is not available, including on PyPy or when
+``WRAPT_DISABLE_EXTENSIONS`` is set in the environment), the failure
+surfaces earlier, at the ``class Proxy(...)`` statement itself, with a
+different message::
+
+    TypeError: metaclass conflict: the metaclass of a derived class
+    must be a (non-strict) subclass of the metaclasses of all its bases
+
+The pure Python ``ObjectProxy`` carries a custom metaclass, used to
+make type-level access to ``__module__`` and ``__doc__`` return strings
+rather than property objects, whereas the C extension's ``ObjectProxy``
+has plain ``type`` as its metaclass. When a second base class with
+``ABCMeta`` as its metaclass is mixed in, the pure Python build cannot
+find a common metaclass and aborts class creation. The C build
+proceeds and only fails later at the first ``isinstance()`` call. The
+underlying problem is the same in both cases; only the point at which
+it surfaces differs.
+
 The same failure occurs when the second base class is one of the
 abstract base classes exported from ``collections.abc`` (for example
 ``Hashable``, ``Iterable``, ``Container``), since they too use

src/wrapt/__init__.py

@@ -13,7 +13,7 @@ def _format_version(parts):
     )
 
 
-__version_info__ = ("2", "2", "1")
+__version_info__ = ("2", "2", "2")
 __version__ = _format_version(__version_info__)
 
 from .__wrapt__ import (

src/wrapt/caching.py

@@ -9,7 +9,7 @@
 from functools import lru_cache as _functools_lru_cache
 from functools import partial
 
-from .__wrapt__ import BoundFunctionWrapper, FunctionWrapper
+from .__wrapt__ import BaseObjectProxy, BoundFunctionWrapper, FunctionWrapper
 from .decorators import decorator
 from .synchronization import synchronized
 
@@ -71,7 +71,19 @@ def __call__(self, *args, **kwargs):
                         self.__wrapped__
                     )
 
-                    setattr(instance, cache_attr, cache)
+                    # If the instance the method is bound to is a wrapt
+                    # object proxy, a plain setattr() would fall through and
+                    # store the cache on the wrapped object rather than the
+                    # proxy. Use type() rather than isinstance() so the check
+                    # sees the real proxy type and is not fooled by the proxy
+                    # overriding __class__ to report the wrapped object's
+                    # type. Proxies expose __self_setattr__() which stores the
+                    # attribute on the proxy itself.
+
+                    if issubclass(type(instance), BaseObjectProxy):
+                        instance.__self_setattr__(cache_attr, cache)
+                    else:
+                        setattr(instance, cache_attr, cache)
 
         return cache(*args, **kwargs)
 
@@ -137,7 +149,17 @@ def __init__(self, wrapped, wrapper, **kwargs):
         if name is None:
             name = wrapped.__func__.__name__
 
-        self._self_cache_attr = "_lru_cache_" + name
+        # The cache attribute name must be unique per decorated method so
+        # that a method overridden in a subclass does not share the same
+        # per-instance cache slot as the method it overrides. If they shared
+        # a slot, a subclass method calling super() would find the subclass
+        # cache and re-enter its own body, recursing forever. The owning
+        # class is not known here, since the decorator runs on the raw
+        # function before the class exists, but each decorated method has its
+        # own wrapper instance, so id(self) is a stable discriminator unique
+        # to this method definition.
+
+        self._self_cache_attr = "_lru_cache_" + name + "_" + str(id(self))
 
     def __call__(self, *args, **kwargs):
         # Plain function or static method — single cache stored

tests/core/test_lru_cache.py

@@ -267,6 +267,161 @@ def test_access_via_instance(self):
         self.assertEqual(info.misses, 1)
 
 
+class OverrideBase:
+    def __init__(self):
+        self.base_calls = 0
+        self.derived_calls = 0
+
+    @wrapt.lru_cache
+    def compute(self, x):
+        self.base_calls += 1
+        return x * 2
+
+
+class OverrideDerived(OverrideBase):
+    @wrapt.lru_cache
+    def compute(self, x):
+        self.derived_calls += 1
+        return super().compute(x) + 1
+
+
+class TestOverriddenMethodWithSuper(unittest.TestCase):
+    def test_super_call_returns_correct_result(self):
+        obj = OverrideDerived()
+        self.assertEqual(obj.compute(10), 21)
+
+    def test_super_call_does_not_recurse(self):
+        # Regression test: a subclass method decorated with lru_cache that
+        # called the base method via super() used to recurse forever because
+        # the base and derived methods shared a single per-instance cache
+        # slot derived from the method name alone.
+        obj = OverrideDerived()
+        try:
+            result = obj.compute(10)
+        except RecursionError:
+            self.fail("super() call recursed instead of reaching base method")
+        self.assertEqual(result, 21)
+
+    def test_both_bodies_execute_once(self):
+        obj = OverrideDerived()
+        obj.compute(10)
+        self.assertEqual(obj.derived_calls, 1)
+        self.assertEqual(obj.base_calls, 1)
+
+    def test_base_and_derived_cached_independently(self):
+        obj = OverrideDerived()
+        obj.compute(10)
+        obj.compute(10)
+        # Second call is served from both caches, so neither body re-runs.
+        self.assertEqual(obj.derived_calls, 1)
+        self.assertEqual(obj.base_calls, 1)
+        info = obj.compute.cache_info()
+        self.assertEqual(info.hits, 1)
+        self.assertEqual(info.misses, 1)
+
+    def test_base_class_instance_unaffected(self):
+        obj = OverrideBase()
+        self.assertEqual(obj.compute(10), 20)
+        self.assertEqual(obj.base_calls, 1)
+        self.assertEqual(obj.derived_calls, 0)
+
+    def test_separate_instances_have_separate_caches(self):
+        obj1 = OverrideDerived()
+        obj2 = OverrideDerived()
+        obj1.compute(10)
+        obj2.compute(10)
+        self.assertEqual(obj1.derived_calls, 1)
+        self.assertEqual(obj2.derived_calls, 1)
+        self.assertEqual(obj1.base_calls, 1)
+        self.assertEqual(obj2.base_calls, 1)
+
+
+class ProxyWrapped:
+    def __init__(self, a):
+        self.a = a
+
+
+class ProxyCached(wrapt.ObjectProxy):
+    @wrapt.lru_cache
+    def compute(self, x):
+        return self.a + x
+
+
+class ProxyCachedWithState(wrapt.ObjectProxy):
+    def __init__(self, wrapped, factor):
+        super().__init__(wrapped)
+        self._self_factor = factor
+
+    @wrapt.lru_cache
+    def compute(self, x):
+        return self._self_factor * x
+
+
+class SlottedWrapped:
+    __slots__ = ("a",)
+
+    def __init__(self, a):
+        self.a = a
+
+
+class TestObjectProxySubclass(unittest.TestCase):
+    def test_returns_correct_result(self):
+        obj = ProxyCached(ProxyWrapped(1))
+        self.assertEqual(obj.compute(10), 11)
+
+    def test_caching(self):
+        obj = ProxyCached(ProxyWrapped(1))
+        obj.compute(10)
+        obj.compute(10)
+        info = obj.compute.cache_info()
+        self.assertEqual(info.hits, 1)
+        self.assertEqual(info.misses, 1)
+
+    def test_cache_not_stored_on_wrapped_object(self):
+        wrapped = ProxyWrapped(1)
+        obj = ProxyCached(wrapped)
+        obj.compute(10)
+        cache_attrs = [k for k in vars(wrapped) if k.startswith("_lru_cache_")]
+        self.assertEqual(cache_attrs, [])
+
+    def test_per_proxy_state_not_shared_for_same_wrapped_object(self):
+        # Two proxies over the same wrapped object must keep independent
+        # per-instance caches keyed to their own proxy state, not a single
+        # cache stored on the shared wrapped object.
+        wrapped = ProxyWrapped(1)
+        obj1 = ProxyCachedWithState(wrapped, 2)
+        obj2 = ProxyCachedWithState(wrapped, 10)
+        self.assertEqual(obj1.compute(5), 10)
+        self.assertEqual(obj2.compute(5), 50)
+
+    def test_proxy_garbage_collected_with_long_lived_wrapped(self):
+        # The cache must be stored on the proxy, not the wrapped object, so
+        # a proxy is not kept alive by a wrapped object that outlives it.
+        registry = []
+
+        def make_and_use():
+            backing = ProxyWrapped(7)
+            registry.append(backing)
+            proxy = ProxyCached(backing)
+            proxy.compute(4)
+            return weakref.ref(proxy)
+
+        ref = make_and_use()
+        gc.collect()
+        self.assertIsNone(ref())
+
+    def test_wrapped_object_without_dict(self):
+        # A wrapped object that does not accept arbitrary attributes (for
+        # example one using __slots__) must not cause the cache storage to
+        # fail, since the cache is stored on the proxy.
+        obj = ProxyCached(SlottedWrapped(1))
+        self.assertEqual(obj.compute(10), 11)
+        self.assertEqual(obj.compute(10), 11)
+        info = obj.compute.cache_info()
+        self.assertEqual(info.hits, 1)
+        self.assertEqual(info.misses, 1)
+
+
 class TestIntrospection(unittest.TestCase):
     def test_function_name(self):
         self.assertEqual(cached_function.__name__, "cached_function")