Fix Ref Leak (#162)

Co-authored-by: J. Nick Koston <[email protected]>

Author: Vizonex

CHANGES/162.bugfix.rst

@@ -0,0 +1 @@
+Fixed reference leak caused by ``Py_INCREF`` because Cython has its own reference counter systems -- by :user:`Vizonex`.

src/propcache/_helpers_c.pyx

@@ -16,7 +16,8 @@ cdef extern from "Python.h":
     int PyDict_SetItem(
         object dict, object key, PyObject* value
     ) except -1
-    void Py_INCREF(PyObject*)
+    void Py_DECREF(PyObject*)
+
 
 cdef class under_cached_property:
     """Use as a class method decorator.  It operates almost exactly like
@@ -46,8 +47,7 @@ cdef class under_cached_property:
         if val == NULL:
             val = PyObject_CallOneArg(self.wrapped, inst)
             PyDict_SetItem(cache, self.name, val)
-        else:
-            Py_INCREF(val)
+            Py_DECREF(val)
         return <object>val
 
     def __set__(self, inst, value):
@@ -97,8 +97,7 @@ cdef class cached_property:
         if val is NULL:
             val = PyObject_CallOneArg(self.func, inst)
             PyDict_SetItem(cache, self.name, val)
-        else:
-            Py_INCREF(val)
+            Py_DECREF(val)
         return <object>val
 
     __class_getitem__ = classmethod(GenericAlias)

tests/test_cached_property.py

@@ -1,3 +1,4 @@
+import gc
 import sys
 from collections.abc import Callable
 from operator import not_
@@ -7,6 +8,8 @@
 
 from propcache.api import cached_property
 
+IS_PYPY = hasattr(sys, "pypy_version_info")
+
 if sys.version_info >= (3, 11):
     from typing import assert_type
 
@@ -142,3 +145,82 @@ def prop(self) -> int:
 
     a = A()
     assert A.prop.func(a) == 1
+
+
+@pytest.mark.c_extension
+@pytest.mark.skipif(IS_PYPY, reason="PyPy has no C extension")
+def test_cached_property_no_refcount_leak(propcache_module: APIProtocol) -> None:
+    """Test that cached_property does not leak references."""
+
+    class CachedPropertySentinel:
+        """A unique object we can track."""
+
+    def count_sentinels() -> int:
+        """Count the number of CachedPropertySentinel instances in gc."""
+        gc.collect()
+        return sum(
+            1 for obj in gc.get_objects() if isinstance(obj, CachedPropertySentinel)
+        )
+
+    class A:
+        def __init__(self) -> None:
+            """Init."""
+
+        @propcache_module.cached_property
+        def prop(self) -> CachedPropertySentinel:
+            """Return a sentinel object."""
+            return CachedPropertySentinel()
+
+    initial_sentinel_count = count_sentinels()
+
+    a = A()
+
+    # First access - creates and caches the object
+    result = a.prop
+    # sys.getrefcount returns 1 higher than actual (the temp ref from the call)
+    # After first access: result owns 1, __dict__ owns 1, getrefcount call owns 1 = 3
+    initial_refcount = sys.getrefcount(result)
+
+    # Should have exactly 1 Sentinel instance now
+    assert count_sentinels() == initial_sentinel_count + 1
+
+    # Second access - should return the cached object without creating new refs
+    result2 = a.prop
+    assert result is result2
+    # After second access: result owns 1, result2 owns 1, __dict__ owns 1, getrefcount call owns 1 = 4
+    # Only result2 should add 1
+    second_refcount = sys.getrefcount(result)
+    assert second_refcount == initial_refcount + 1
+
+    # Still should have exactly 1 Sentinel instance
+    assert count_sentinels() == initial_sentinel_count + 1
+
+    # Third access
+    result3 = a.prop
+    assert result is result3
+    # result2 and result3 each add 1
+    third_refcount = sys.getrefcount(result)
+    assert third_refcount == initial_refcount + 2
+
+    # Clean up local refs - should be back to just result and __dict__
+    del result2
+    del result3
+    after_cleanup_refcount = sys.getrefcount(result)
+    assert after_cleanup_refcount == initial_refcount
+
+    # Clear the cache and verify no leak when re-fetching
+    # After clearing: only result owns it
+    del a.__dict__["prop"]
+    cleared_refcount = sys.getrefcount(result)
+    assert cleared_refcount == initial_refcount - 1  # No longer in __dict__
+
+    # Re-fetch - this should create a new object, not affect old one
+    result4 = a.prop
+    assert result4 is not result  # Should be a new object
+    refetch_refcount = sys.getrefcount(result)
+    assert refetch_refcount == cleared_refcount  # Original object refcount unchanged
+
+    # Now we should have 2 Sentinel instances:
+    # - original in `result`
+    # - new one in `result4`
+    assert count_sentinels() == initial_sentinel_count + 2

tests/test_under_cached_property.py

@@ -1,3 +1,4 @@
+import gc
 import sys
 from collections.abc import Callable
 from typing import TYPE_CHECKING, Any, Protocol, TypedDict, TypeVar
@@ -6,6 +7,8 @@
 
 from propcache.api import under_cached_property
 
+IS_PYPY = hasattr(sys, "pypy_version_info")
+
 if sys.version_info >= (3, 11):
     from typing import assert_type
 
@@ -165,3 +168,85 @@ def prop(self) -> int:
 
     a = A()
     assert A.prop.wrapped(a) == 1
+
+
+@pytest.mark.c_extension
+@pytest.mark.skipif(IS_PYPY, reason="PyPy has no C extension")
+def test_under_cached_property_no_refcount_leak(propcache_module: APIProtocol) -> None:
+    """Test that under_cached_property does not leak references."""
+
+    class UnderCachedPropertySentinel:
+        """A unique object we can track."""
+
+    def count_sentinels() -> int:
+        """Count the number of UnderCachedPropertySentinel instances in gc."""
+        gc.collect()
+        return sum(
+            1
+            for obj in gc.get_objects()
+            if isinstance(obj, UnderCachedPropertySentinel)
+        )
+
+    class A:
+        def __init__(self) -> None:
+            """Init."""
+            self._cache: dict[str, Any] = {}
+
+        @propcache_module.under_cached_property
+        def prop(self) -> UnderCachedPropertySentinel:
+            """Return a sentinel object."""
+            return UnderCachedPropertySentinel()
+
+    initial_sentinel_count = count_sentinels()
+
+    a = A()
+
+    # First access - creates and caches the object
+    result = a.prop
+    # sys.getrefcount returns 1 higher than actual (the temp ref from the call)
+    # After first access: result owns 1, _cache owns 1, getrefcount call owns 1 = 3
+    initial_refcount = sys.getrefcount(result)
+
+    # Should have exactly 1 Sentinel instance now
+    assert count_sentinels() == initial_sentinel_count + 1
+
+    # Second access - should return the cached object without creating new refs
+    result2 = a.prop
+    assert result is result2
+    # After second access: result owns 1, result2 owns 1, _cache owns 1, getrefcount call owns 1 = 4
+    # Only result2 should add 1
+    second_refcount = sys.getrefcount(result)
+    assert second_refcount == initial_refcount + 1
+
+    # Still should have exactly 1 Sentinel instance
+    assert count_sentinels() == initial_sentinel_count + 1
+
+    # Third access
+    result3 = a.prop
+    assert result is result3
+    # result2 and result3 each add 1
+    third_refcount = sys.getrefcount(result)
+    assert third_refcount == initial_refcount + 2
+
+    # Clean up local refs - should be back to just result and _cache
+    del result2
+    del result3
+    after_cleanup_refcount = sys.getrefcount(result)
+    assert after_cleanup_refcount == initial_refcount
+
+    # Clear the cache and verify no leak when re-fetching
+    # After clearing: only result owns it
+    del a._cache["prop"]
+    cleared_refcount = sys.getrefcount(result)
+    assert cleared_refcount == initial_refcount - 1  # No longer in _cache
+
+    # Re-fetch - this should create a new object, not affect old one
+    result4 = a.prop
+    assert result4 is not result  # Should be a new object
+    refetch_refcount = sys.getrefcount(result)
+    assert refetch_refcount == cleared_refcount  # Original object refcount unchanged
+
+    # Now we should have 2 Sentinel instances:
+    # - original in `result`
+    # - new one in `result4`
+    assert count_sentinels() == initial_sentinel_count + 2