Skip to content

gh-87634: remove locking from functools.cached_property #101890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Doc/library/functools.rst
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,14 @@ The :mod:`functools` module defines the following functions:
The cached value can be cleared by deleting the attribute. This
allows the *cached_property* method to run again.

The *cached_property* does not prevent a possible race condition in
multi-threaded usage. The getter function could run more than once on the
same instance, with the latest run setting the cached value. If the cached
property is idempotent or otherwise not harmful to run more than once on an
instance, this is fine. If synchronization is needed, implement the necessary
locking inside the decorated getter function or around the cached property
access.

Note, this decorator interferes with the operation of :pep:`412`
key-sharing dictionaries. This means that instance dictionaries
can take more space than usual.
Expand All @@ -110,6 +118,14 @@ The :mod:`functools` module defines the following functions:
def stdev(self):
return statistics.stdev(self._data)


.. versionchanged:: 3.12
Prior to Python 3.12, ``cached_property`` included an undocumented lock to
ensure that in multi-threaded usage the getter function was guaranteed to
run only once per instance. However, the lock was per-property, not
per-instance, which could result in unacceptably high lock contention. In
Python 3.12+ this locking is removed.

.. versionadded:: 3.8


Expand Down
9 changes: 9 additions & 0 deletions Doc/whatsnew/3.12.rst
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,15 @@ Changes in the Python API
around process-global resources, which are best managed from the main interpreter.
(Contributed by Dong-hee Na in :gh:`99127`.)

* The undocumented locking behavior of :func:`~functools.cached_property`
is removed, because it locked across all instances of the class, leading to high
lock contention. This means that a cached property getter function could now run
more than once for a single instance, if two threads race. For most simple
cached properties (e.g. those that are idempotent and simply calculate a value
based on other attributes of the instance) this will be fine. If
synchronization is needed, implement locking within the cached property getter
function or around multi-threaded access points.


Build Changes
=============
Expand Down
27 changes: 9 additions & 18 deletions Lib/functools.py
Original file line number Diff line number Diff line change
Expand Up @@ -959,15 +959,12 @@ def __isabstractmethod__(self):
### cached_property() - computed once per instance, cached as attribute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should also be changed. @carljm

Copy link
Member Author

@carljm carljm Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not an absolute guarantee in the presence of multi-threading race conditions, but this is still a generally accurate description of the usual behavior of a cached_property. What alternative wording would you suggest for this one-line comment that is both clear and concise? Note the public-facing documentation already contains a full and clear description of the behavior. (To be clear this is a genuine question, I'm open to changing this comment if you have a good suggestion, I'm just not sure what a clear and concise wording would be, and I think it might be OK to leave it as is.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

### cached_property() - property result cached as instance attribute

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! I've rolled that change into #106380 where I'm already touching cached_property.

################################################################################

_NOT_FOUND = object()


class cached_property:
def __init__(self, func):
self.func = func
self.attrname = None
self.__doc__ = func.__doc__
self.lock = RLock()

def __set_name__(self, owner, name):
if self.attrname is None:
Expand All @@ -992,21 +989,15 @@ def __get__(self, instance, owner=None):
f"instance to cache {self.attrname!r} property."
)
raise TypeError(msg) from None
val = cache.get(self.attrname, _NOT_FOUND)
if val is _NOT_FOUND:
with self.lock:
# check if another thread filled cache while we awaited lock
val = cache.get(self.attrname, _NOT_FOUND)
if val is _NOT_FOUND:
val = self.func(instance)
try:
cache[self.attrname] = val
except TypeError:
msg = (
f"The '__dict__' attribute on {type(instance).__name__!r} instance "
f"does not support item assignment for caching {self.attrname!r} property."
)
raise TypeError(msg) from None
val = self.func(instance)
try:
cache[self.attrname] = val
except TypeError:
msg = (
f"The '__dict__' attribute on {type(instance).__name__!r} instance "
f"does not support item assignment for caching {self.attrname!r} property."
)
raise TypeError(msg) from None
return val

__class_getitem__ = classmethod(GenericAlias)
36 changes: 0 additions & 36 deletions Lib/test/test_functools.py
Original file line number Diff line number Diff line change
Expand Up @@ -2931,21 +2931,6 @@ def get_cost(self):
cached_cost = py_functools.cached_property(get_cost)


class CachedCostItemWait:

def __init__(self, event):
self._cost = 1
self.lock = py_functools.RLock()
self.event = event

@py_functools.cached_property
def cost(self):
self.event.wait(1)
with self.lock:
self._cost += 1
return self._cost


class CachedCostItemWithSlots:
__slots__ = ('_cost')

Expand All @@ -2970,27 +2955,6 @@ def test_cached_attribute_name_differs_from_func_name(self):
self.assertEqual(item.get_cost(), 4)
self.assertEqual(item.cached_cost, 3)

@threading_helper.requires_working_threading()
def test_threaded(self):
go = threading.Event()
item = CachedCostItemWait(go)

num_threads = 3

orig_si = sys.getswitchinterval()
sys.setswitchinterval(1e-6)
try:
threads = [
threading.Thread(target=lambda: item.cost)
for k in range(num_threads)
]
with threading_helper.start_threads(threads):
go.set()
finally:
sys.setswitchinterval(orig_si)

self.assertEqual(item.cost, 2)

def test_object_with_slots(self):
item = CachedCostItemWithSlots()
with self.assertRaisesRegex(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove locking behavior from :func:`functools.cached_property`.