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 :)

Lib/functools.py

AlexWaygood · 2022-10-09T15:11:47Z

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?

You could do this

Ok, agreed, using a full noun phrase as the name does seem to work fine. I can make this change.

any reason you're using PendingDeprecationWarning instead of DeprecationWarning

I thought the general preference was to provide one release of PendingDeprecationWarning before moving to DeprecationWarning in the next release, and then removing in the release after that?

Lib/functools.py

Doc/library/functools.rst

Doc/whatsnew/3.12.rst

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.

carljm · 2022-10-14T22:45:51Z

I agree this deprecation is unpleasant, but I still think it might be the best option available. The transition pain is short term; the more important question is which long-term state we prefer to reach.

I think the need for locking around a cached_property in practice is very rare, even if threading is used. I've personally used cached_property many, many times over 15+ years and I can't think of a single case where I would have wanted or needed locking. The classic (and documented) scenario for a cached_property, and the one I have always used it in, is when a property value depends only on some other immutable attributes of the instance, but requires some expensive computation to produce from those inputs. In this scenario there is never a correctness issue if the property runs twice on a given instance, and there is only a performance issue if the computation is massively expensive and the likelihood of same-instance races quite high.

Do we know of any real-world use of cached_property in which the locking is necessary for correctness? One bit of signal here is that none of the stdlib uses fall into that category.

"Fixing the locking" is not without tradeoffs, even if we reach a solution that we believe is correct. For one thing, RLock instances can't be pickled, and according to the latest comment on the issue, there can be situations with some libraries where this will block use of cached properties. (I'm not sure why these libraries end up pickling the class as an object instead of a reference, but presuming there is some good reason for that, I don't see any clear way to fix this problem.) And the per-instance locking code will be complex and a likely source of future bugs.

So I believe that the lock-free version of cached_property is more desirable than "fixed" locking, as an end state. If we do develop correct per-instance locking code, I would much rather extract that logic out into a separate @lock decorator which can be composed with a lock-free cached_property (or with any other method.)

Even if the deprecation of locking cached_property is too painful, I would still prefer to land this PR, or something similar, without the deprecation, so that a lock-free cached property is available in the standard library.

this PR's approach to deprecation is going to be rough on users and will leave a permanent mark on the API.

I don't think the mark on the API has to be permanent, though cleaning it up would require another deprecation of the lock kwarg at some point many releases in the future. So it's certainly a mark that will stick around for a while, at best.

* main: (37 commits) pythongh-98251: Allow venv to pass along PYTHON* variables to pip and ensurepip when they do not impact path resolution (pythonGH-98259) Bpo-41246: IOCP Proactor avoid callback code duplication (python#21399) bpo-46364: Use sockets for stdin of asyncio only on AIX (python#30596) pythongh-98178: syslog() is not thread-safe on macOS (python#98213) Mark all targets in `Doc/Makefile` as `PHONY` (pythonGH-98189) pythongh-97982: Factorize PyUnicode_Count() and unicode_count() code (python#98025) pythongh-96265: Formatting changes for faq/general (python#98129) tutorial: remove "with single quotes" (python#98204) pythongh-97669: Remove Tools/scripts/startuptime.py (python#98214) signalmodule.c uses _PyErr_WriteUnraisableMsg() (python#98217) pythongh-97669: Fix test_tools reference leak (python#98216) pythongh-97669: Create Tools/patchcheck/ directory (python#98186) pythongh-65046: Link to logging cookbook from asyncio docs (python#98207) Formatting fixes in contextlib docs (python#98111) pythongh-95276: Add callable entry to the glossary (python#95738) pythongh-96130: Rephrase use of "typecheck" verb for clarity (python#98144) Fix some incorrect indentation around the main switch (python#98177) pythongh-98172: Fix formatting in `except*` docs (python#98173) pythongh-97982: Remove asciilib_count() (python#98164) pythongh-95756: Free and NULL-out code caches when needed (pythonGH-98181) ...

rhettinger · 2022-10-15T22:02:26Z

I agree this deprecation is unpleasant, but I still think it might be the best option available.
...
I don't think the mark on the API has to be permanent, though cleaning it up would require another deprecation of the lock kwarg at some point many releases in the future.

From a user point of view, it would be better to rip out the functionality all at once than to force EVERY user of cached_property() to change their code TWICE, once to add the lock parameter and again to remove that parameter when it too becomes deprecated. All at once is clearly better than deprecation, but it is a violation of the deprecation policy and would need an SC sign-off.

Also, it is still possible to just fix the code. It will just take someone with the skill, time, and interest to do so. From a user point of view, this is the best possible option. (Perhaps have our developer-in-residence could work on it or the SC can hire someone). Also, the fix could be backported to give relief to current users who aren't aware of the downsides.

I'm marking this PR as do-not-merge because it is too catastrophic to go forward without broader discussion and collective buy in. This PR inflicts pain on 100% of the users of cached_property(). Even if fully reverted after four release cycles, it would continue to affect code that has to work across multiple versions of Python.

carljm · 2022-10-28T02:18:02Z

@rhettinger Yeah, makes sense. I would love to just rip out the locking functionality without deprecation (since my guess is that few if any users of cached_property actually depend on it), but if there are even a few depending on it, that violates the backward compatibility policy. I'm open to asking the SC for a policy exception, but not sure what the chances are of approval, and I won't go that route unless you are supportive.

In the meantime, closing this PR. Will try to find time to take a look at fixing the locking.

eendebakpt · 2023-02-09T09:56:15Z

@carljm This PR was closed because the deprecation would impact too many users. What about the following variation? Add a kwarg with values False, True and None.

Value True has the same behaviour as in this PR
Value False has the same behaviour as in this PR
Value None has the same behaviour as True, but detects whether the lock is actually used. Only if the lock is used, a deprecation warning is issued. (not sure whether this is technically possible)

The default value is None for several python releases, after which the default value is changed to False .

In this way most of the users (who do not use threading, so no use of the locking) will have the old behaviour for a couple of releases, and then switch to the desired implementation. Only users that make use of the locking will get a deprecation warning, but one that is easy to fix (e.g. update to lock=True).

carljm · 2023-02-09T15:34:50Z

@eendebakpt Interesting idea! I think the best place to keep an unfragmented discussion of options is over on the issue #87634 rather than on this closed PR, so I'll reply there.

pythongh-87634: deprecate cached_property locking, add lock kwarg

b578a4a

carljm requested a review from rhettinger as a code owner October 9, 2022 14:56

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

AlexWaygood reviewed Oct 9, 2022

View reviewed changes

carljm added 4 commits October 9, 2022 17:26

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 October 9, 2022 16:12

rhettinger removed their request for review October 9, 2022 18:44

carljm added 2 commits October 15, 2022 02:54

Review comments

ffdb471

rhettinger added the DO-NOT-MERGE label Oct 15, 2022

carljm closed this Oct 28, 2022

JelleZijlstra mentioned this pull request Dec 16, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Loading

AlexWaygood commented Oct 9, 2022 •

edited

Loading

AlexWaygood Oct 9, 2022

carljm Oct 9, 2022

AlexWaygood Oct 9, 2022 •

edited

Loading

AlexWaygood Oct 9, 2022

carljm Oct 14, 2022

rhettinger commented Oct 9, 2022

AlexWaygood commented Oct 9, 2022 •

edited

Loading

rhettinger commented Oct 9, 2022

carljm commented Oct 14, 2022 •

edited

Loading

rhettinger commented Oct 15, 2022 •

edited

Loading

carljm commented Oct 28, 2022

eendebakpt commented Feb 9, 2023

carljm commented Feb 9, 2023

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

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

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

Conversation

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

AlexWaygood commented Oct 9, 2022 • edited Loading

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 Loading

Choose a reason for hiding this comment

AlexWaygood Oct 9, 2022

Choose a reason for hiding this comment

carljm Oct 14, 2022

Choose a reason for hiding this comment

rhettinger commented Oct 9, 2022

AlexWaygood commented Oct 9, 2022 • edited Loading

rhettinger commented Oct 9, 2022

carljm commented Oct 14, 2022 • edited Loading

rhettinger commented Oct 15, 2022 • edited Loading

carljm commented Oct 28, 2022

eendebakpt commented Feb 9, 2023

carljm commented Feb 9, 2023

carljm commented Oct 9, 2022 •

edited by bedevere-bot

Loading

AlexWaygood commented Oct 9, 2022 •

edited

Loading

AlexWaygood Oct 9, 2022 •

edited

Loading

AlexWaygood commented Oct 9, 2022 •

edited

Loading

carljm commented Oct 14, 2022 •

edited

Loading

rhettinger commented Oct 15, 2022 •

edited

Loading