gh-87634: deprecate cached_property locking, add lock kwarg #98123

carljm · 2022-10-09T14:56:51Z

Issue: functools.cached_property incorrectly locks the entire descriptor on class instead of per-instance locking #87634

AlexWaygood · 2022-10-09T15:04:59Z

This causes at least one deprecation warning to be emitted in the stdlib:

C:\Users\alexw\coding\cpython>python -m idlelib
Running Debug|x64 interpreter...
C:\Users\alexw\coding\cpython\Lib\platform.py:850: PendingDeprecationWarning: Locking cached_property is deprecated; use @cached_property(lock=False)
  @functools.cached_property
C:\Users\alexw\coding\cpython\Lib\platform.py:850: PendingDeprecationWarning: Locking cached_property is deprecated; use @cached_property(lock=False)
  @functools.cached_property

(Edit: looks like that's already been caught by one of the failing tests in CI :)

AlexWaygood · 2022-10-09T15:22:03Z

Lib/functools.py

@@ -20,6 +20,7 @@
 from reprlib import recursive_repr
 from _thread import RLock
 from types import GenericAlias
+from warnings import warn


Should import this inside the if lock: block in cached_property.__init__, so that users only pay the performance cost for importing the module if they're using the deprecated behaviour

AlexWaygood · 2022-10-09T15:22:03Z

Lib/functools.py

+            warn("Locking cached_property is deprecated; use @cached_property(lock=False)",
+                 PendingDeprecationWarning, 2)


Maybe use warnings._deprecated?

I don't think warnings._deprecated works here, because it assumes the thing that is deprecated is a single method/function/class with a name. What would you pass in for the name argument of warnings._deprecated in this case?

You could do this:

diff --git a/Lib/functools.py b/Lib/functools.py index 8ad150d853..5eb772e6b4 100644 --- a/Lib/functools.py +++ b/Lib/functools.py @@ -969,9 +969,12 @@ def __init__(self, func=None, *, lock=True): if func is not None: self.__doc__ = func.__doc__ if lock: - from warnings import warn - warn("Locking cached_property is deprecated; use @cached_property(lock=False)", - PendingDeprecationWarning, 2) + import warnings + warnings._deprecated( + "lock=True default behaviour of cached_property", + "Locking cached_property is deprecated; use @cached_property(lock=False)", + remove=(3, 14) + ) self.lock = RLock() if lock else None

And then you'd get this behaviour:

C:\Users\alexw\coding\cpython>python Running Debug|x64 interpreter... Python 3.12.0a0 (main, Oct 9 2022, 08:02:35) [MSC v.1932 64 bit (AMD64)] on win32 Type "help", "copyright", "credits" or "license" for more information. >>> >>> # Behaviour on 3.12: >>> >>> import functools >>> x = functools.cached_property(lambda self: 42) <stdin>:1: DeprecationWarning: Locking cached_property is deprecated; use @cached_property(lock=False) >>> >>> # Behaviour when we get to 3.14 beta if the default hasn't been changed yet: >>> >>> import sys >>> sys.version_info = 3, 14, 0, "beta" >>> del sys.modules["warnings"]; del sys.modules["functools"] >>> import functools >>> x = functools.cached_property(lambda self: 42) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "C:\Users\alexw\coding\cpython\Lib\functools.py", line 973, in __init__ warnings._deprecated( File "C:\Users\alexw\coding\cpython\Lib\warnings.py", line 511, in _deprecated raise RuntimeError(msg) RuntimeError: 'lock=True default behaviour of cached_property' was slated for removal after Python 3.14 alpha

I agree that this probably isn't the use case warnings._deprecated was designed for... but it seems to work pretty well? ;)

Also, any reason you're using PendingDeprecationWarning instead of DeprecationWarning, btw?

AlexWaygood · 2022-10-09T15:22:03Z

Lib/functools.py

+
+    def __call__(self, func=None):
+        if self.func is not None:
+            raise TypeError("'cached_property' object is not callable")


Suggested change

raise TypeError("'cached_property' object is not callable")

raise TypeError(f"{type(self).__name__!r} object is not callable")

AlexWaygood · 2022-10-09T15:22:03Z

Doc/library/functools.rst

+    cached property would execute only once per instance, but it also prevents
+    concurrent execution on different instances of the same class, causing
+    unnecessary and excessive serialization of updates. This locking behavior
+    is deprecated and can be avoided by passing ``lock=False``::


Suggested change

is deprecated and can be avoided by passing ``lock=False``::

is deprecated, and will be removed in Python 3.14. It can be avoided by

passing ``lock=False``::

AlexWaygood · 2022-10-09T15:22:03Z

Doc/library/functools.rst

   .. versionadded:: 3.8

+   .. versionchanged:: 3.12
+      The ``lock`` argument was added.
+


Suggested change

.. deprecated-removed:: 3.12 3.14

The locking behaviour of ``cached_property`` is deprecated, and will be

removed in 3.14. Use ``lock=False`` to avoid the deprecated behaviour.

AlexWaygood · 2022-10-09T15:22:03Z

Doc/whatsnew/3.12.rst

@@ -245,6 +245,17 @@ Deprecated
  :exc:`ImportWarning`).
  (Contributed by Brett Cannon in :gh:`65961`.)

+* The locking behavior of :class:`~functools.cached_property` is deprecated.


Suggested change

* The locking behavior of :class:`~functools.cached_property` is deprecated.

* The locking behavior of :class:`~functools.cached_property` is deprecated,

and will be removed in Python 3.14.

rhettinger · 2022-10-09T18:44:18Z

Ripping out the locking logic seems reasonable — as you say, it was a mistake to include it in the first place. That said, this PR's approach to deprecation is going to be rough on users and will leave a permanent mark on the API.

ISTM the most graceful thing we could do is to just fix the locking logic. I put some effort into this but it required more deep thought than I had time for. It might be a good sprint topic.

AlexWaygood · 2022-10-09T19:50:07Z

That said, this PR's approach to deprecation is going to be rough on users and will leave a permanent mark on the API [...] ISTM the most graceful thing we could do is to just fix the locking logic.

I agree that this would be the best thing to do, if we can figure out how to do it. The fallout from this PR just on the stdlib and the CPython test suite is pretty concerning. And if users want to write Python code that's compatible on both Python 3.11 and Python 3.12, while avoiding DeprecationWarnings, they'll have to do the following awkward dance:

import sys

if sys.version_info >= (3, 12):
    import functools
    cached_property = functools.partial(functools.cached_property, lock=False)
else:
    from functools import cached_property

(If we do decide to go with the approach proposed in this PR, it might be worth adding the above snippet to the "What's new in 3.12" entry.)

rhettinger · 2022-10-09T23:36:36Z

Also, if locking is going to be "left to the users", there should be an example of how to make sure a call doesn't get made more than once. The current way can be catastrophically inefficient but at least it isn't wrong.

pythongh-87634: deprecate cached_property locking, add lock kwarg

b578a4a

carljm requested a review from rhettinger as a code owner Oct 9, 2022

bedevere-bot added the awaiting review label Oct 9, 2022

AlexWaygood reviewed Oct 9, 2022

View changes

carljm added 4 commits Oct 9, 2022

Fix docs

4e4e647

Use lock=False throughout stdlib and docs

246d72e

Fix tests

7cb546e

Review comments

26c2659

carljm requested a review from vsajip as a code owner Oct 9, 2022

rhettinger removed their request for review Oct 9, 2022

gh-87634: deprecate cached_property locking, add lock kwarg #98123

gh-87634: deprecate cached_property locking, add lock kwarg #98123

carljm commented Oct 9, 2022 •

edited by bedevere-bot

AlexWaygood commented Oct 9, 2022 •

edited

AlexWaygood Oct 9, 2022

AlexWaygood Oct 9, 2022

carljm Oct 9, 2022

AlexWaygood Oct 9, 2022 •

edited

AlexWaygood Oct 9, 2022

AlexWaygood Oct 9, 2022

AlexWaygood Oct 9, 2022

AlexWaygood Oct 9, 2022 •

edited

AlexWaygood Oct 9, 2022

rhettinger commented Oct 9, 2022

AlexWaygood commented Oct 9, 2022 •

edited

rhettinger commented Oct 9, 2022

		warn("Locking cached_property is deprecated; use @cached_property(lock=False)",
		PendingDeprecationWarning, 2)

	raise TypeError("'cached_property' object is not callable")
	raise TypeError(f"{type(self).__name__!r} object is not callable")

	is deprecated and can be avoided by passing ``lock=False``::
	is deprecated, and will be removed in Python 3.14. It can be avoided by
	passing ``lock=False``::

+   .. deprecated-removed:: 3.12 3.14
+      The locking behaviour of ``cached_property`` is deprecated, and will be
+      removed in 3.14. Use ``lock=False`` to avoid the deprecated behaviour.

	* The locking behavior of :class:`~functools.cached_property` is deprecated.
	* The locking behavior of :class:`~functools.cached_property` is deprecated,
	and will be removed in Python 3.14.

gh-87634: deprecate cached_property locking, add lock kwarg #98123

Are you sure you want to change the base?

gh-87634: deprecate cached_property locking, add lock kwarg #98123

Conversation

carljm commented Oct 9, 2022 • edited by bedevere-bot

AlexWaygood commented Oct 9, 2022 • edited

AlexWaygood Oct 9, 2022

Choose a reason for hiding this comment

AlexWaygood Oct 9, 2022

Choose a reason for hiding this comment

carljm Oct 9, 2022

Choose a reason for hiding this comment

AlexWaygood Oct 9, 2022 • edited

Choose a reason for hiding this comment

AlexWaygood Oct 9, 2022

Choose a reason for hiding this comment

AlexWaygood Oct 9, 2022

Choose a reason for hiding this comment

AlexWaygood Oct 9, 2022

Choose a reason for hiding this comment

AlexWaygood Oct 9, 2022 • edited

Choose a reason for hiding this comment

AlexWaygood Oct 9, 2022

Choose a reason for hiding this comment

rhettinger commented Oct 9, 2022

AlexWaygood commented Oct 9, 2022 • edited

rhettinger commented Oct 9, 2022

carljm commented Oct 9, 2022 •

edited by bedevere-bot

AlexWaygood commented Oct 9, 2022 •

edited

AlexWaygood Oct 9, 2022 •

edited

AlexWaygood Oct 9, 2022 •

edited

AlexWaygood commented Oct 9, 2022 •

edited