Skip to content

Commit

Permalink
Create a lock on cached_property if not present (#1811)
Browse files Browse the repository at this point in the history
* Create a lock on cached_property if not present

This fixes #1804 (fixing breakage caused by use of undocumented
implementation details of functools.cached_property) by ensuring a lock
is always present on cached_property attributes, which is required to
safely support setting and deleting cached values in addition to
computing them on demand.

* Add a unit test for cached_property locking
  • Loading branch information
tari committed Oct 18, 2023
1 parent 1dfe4f3 commit 3ad075a
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 1 deletion.
11 changes: 10 additions & 1 deletion kombu/utils/objects.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

from __future__ import annotations

from threading import RLock

__all__ = ('cached_property',)

try:
Expand All @@ -25,10 +27,17 @@ def __init__(self, fget=None, fset=None, fdel=None):
# This is a backport so we set this ourselves.
self.attrname = self.func.__name__

if not hasattr(self, 'lock'):
# Prior to Python 3.12, functools.cached_property has an
# undocumented lock which is required for thread-safe __set__
# and __delete__. Create one if it isn't already present.
self.lock = RLock()

def __get__(self, instance, owner=None):
# TODO: Remove this after we drop support for Python<3.8
# or fix the signature in the cached_property package
return super().__get__(instance, owner)
with self.lock:
return super().__get__(instance, owner)

def __set__(self, instance, value):
if instance is None:
Expand Down
32 changes: 32 additions & 0 deletions t/unit/utils/test_objects.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from __future__ import annotations

from unittest import mock

from kombu.utils.objects import cached_property


Expand Down Expand Up @@ -51,3 +53,33 @@ def foo(self, value):
assert x.xx == 10

del x.foo

def test_locks_on_access(self):

class X:
@cached_property
def foo(self):
return 42

x = X()

# Getting the value acquires the lock, and may do so recursively
# on Python < 3.12 because the superclass acquires it.
with mock.patch.object(X.foo, 'lock') as mock_lock:
assert x.foo == 42
mock_lock.__enter__.assert_called()
mock_lock.__exit__.assert_called()

# Setting a value also acquires the lock.
with mock.patch.object(X.foo, 'lock') as mock_lock:
x.foo = 314
assert x.foo == 314
mock_lock.__enter__.assert_called_once()
mock_lock.__exit__.assert_called_once()

# .. as does clearing the cached value to recompute it.
with mock.patch.object(X.foo, 'lock') as mock_lock:
del x.foo
assert x.foo == 42
mock_lock.__enter__.assert_called_once()
mock_lock.__exit__.assert_called_once()

0 comments on commit 3ad075a

Please sign in to comment.