From 8dda2d6afbd5658be6508f911ad878b6782f5730 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Mon, 13 Feb 2023 12:42:39 -0800 Subject: [PATCH 1/4] gh-87634: remove locking from functools.cached_property --- Doc/library/functools.rst | 16 ++++++++++++++++ Doc/whatsnew/3.12.rst | 9 +++++++++ Lib/functools.py | 23 +++++++++-------------- Lib/test/test_functools.py | 36 ------------------------------------ 4 files changed, 34 insertions(+), 50 deletions(-) diff --git a/Doc/library/functools.rst b/Doc/library/functools.rst index 2f0a9bd8be8815..5f1680aa251d14 100644 --- a/Doc/library/functools.rst +++ b/Doc/library/functools.rst @@ -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 (in rare cases) 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. @@ -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 diff --git a/Doc/whatsnew/3.12.rst b/Doc/whatsnew/3.12.rst index 45a5e5062d55b6..be49d1e7bc7c76 100644 --- a/Doc/whatsnew/3.12.rst +++ b/Doc/whatsnew/3.12.rst @@ -754,6 +754,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 + occasionally 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 ============= diff --git a/Lib/functools.py b/Lib/functools.py index 43ead512e1ea4e..d5a7d4eae93197 100644 --- a/Lib/functools.py +++ b/Lib/functools.py @@ -967,7 +967,6 @@ 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: @@ -994,19 +993,15 @@ def __get__(self, instance, owner=None): 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) diff --git a/Lib/test/test_functools.py b/Lib/test/test_functools.py index 730ab1f595f22c..57db96d37ee369 100644 --- a/Lib/test/test_functools.py +++ b/Lib/test/test_functools.py @@ -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') @@ -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( From 477e6b3c381c1bb8aa5aaa5b76009a2b11652ee4 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Mon, 13 Feb 2023 12:56:04 -0800 Subject: [PATCH 2/4] add separate NEWS entry --- .../next/Library/2023-02-13-12-55-48.gh-issue-87634.q-SBhJ.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2023-02-13-12-55-48.gh-issue-87634.q-SBhJ.rst diff --git a/Misc/NEWS.d/next/Library/2023-02-13-12-55-48.gh-issue-87634.q-SBhJ.rst b/Misc/NEWS.d/next/Library/2023-02-13-12-55-48.gh-issue-87634.q-SBhJ.rst new file mode 100644 index 00000000000000..a17927500bd9a5 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-02-13-12-55-48.gh-issue-87634.q-SBhJ.rst @@ -0,0 +1 @@ +Remove locking behavior from :func:`functools.cached_property`. From 31f0d54d5afb26f2f939f5478adb8517ec6a8b4b Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Mon, 13 Feb 2023 14:17:39 -0800 Subject: [PATCH 3/4] remove unnecessary check for existing value --- Lib/functools.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/Lib/functools.py b/Lib/functools.py index d5a7d4eae93197..aaf4291150fbbf 100644 --- a/Lib/functools.py +++ b/Lib/functools.py @@ -959,8 +959,6 @@ def __isabstractmethod__(self): ### cached_property() - computed once per instance, cached as attribute ################################################################################ -_NOT_FOUND = object() - class cached_property: def __init__(self, func): @@ -991,17 +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: - 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) From 141a8731204bf0244fab5d4948ada13ae7f6cd86 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Wed, 22 Feb 2023 16:26:54 -0800 Subject: [PATCH 4/4] wording changes from review comments --- Doc/library/functools.rst | 8 ++++---- Doc/whatsnew/3.12.rst | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Doc/library/functools.rst b/Doc/library/functools.rst index 79592a5a9a2a81..d467e50bc7a424 100644 --- a/Doc/library/functools.rst +++ b/Doc/library/functools.rst @@ -89,10 +89,10 @@ The :mod:`functools` module defines the following functions: 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 (in rare cases) 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. + 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 diff --git a/Doc/whatsnew/3.12.rst b/Doc/whatsnew/3.12.rst index 7a988ff1b37ea2..909c9102a405f3 100644 --- a/Doc/whatsnew/3.12.rst +++ b/Doc/whatsnew/3.12.rst @@ -763,12 +763,12 @@ Changes in the Python API * 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 - occasionally 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. + 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