gh-87634: deprecate cached_property locking, add lock kwarg

[carljm]

• Issue: gh-87634

[AlexWaygood]

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

[rhettinger]

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]

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]

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]

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.

[rhettinger]

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]

@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]

@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]

@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.