The locking on functools.cached_property (

Here's the code of __get__ in cached_property:

    def __get__(self, instance, owner=None):
        if instance is None:
            return self
        if self.attrname is None:
            raise TypeError(
                "Cannot use cached_property instance without calling __set_name__ on it.")
        try:
            cache = instance.__dict__
        except AttributeError:  # not all objects have __dict__ (e.g. class defines slots)
            msg = (
                f"No '__dict__' attribute on {type(instance).__name__!r} "
                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
        return val

I noticed this because I was recommending that Pyramid web framework deprecate its much simpler reify decorator in favour of using cached_property, and then noticed why it won't do.

Here is the test case for cached_property:

from functools import cached_property
from threading import Thread
from random import randint
import time



class Spam:
    @cached_property
    def ham(self):
        print(f'Calculating amount of ham in {self}')
        time.sleep(10)
        return randint(0, 100)


def bacon():
    spam = Spam()
    print(f'The amount of ham in {spam} is {spam.ham}')


start = time.time()
threads = []
for _ in range(3):
    t = Thread(target=bacon)
    threads.append(t)
    t.start()

for t in threads:
    t.join()

print(f'Total running time was {time.time() - start}')

Calculating amount of ham in <main.Spam object at 0x7fa50bcaa220>
The amount of ham in <main.Spam object at 0x7fa50bcaa220> is 97
Calculating amount of ham in <main.Spam object at 0x7fa50bcaa4f0>
The amount of ham in <main.Spam object at 0x7fa50bcaa4f0> is 8
Calculating amount of ham in <main.Spam object at 0x7fa50bcaa7c0>
The amount of ham in <main.Spam object at 0x7fa50bcaa7c0> is 53
Total running time was 30.02147102355957

The runtime is 30 seconds; for pyramid.decorator.reify the runtime would be 10 seconds:

Calculating amount of ham in <main.Spam object at 0x7fc4d8272430>
Calculating amount of ham in <main.Spam object at 0x7fc4d82726d0>
Calculating amount of ham in <main.Spam object at 0x7fc4d8272970>
The amount of ham in <main.Spam object at 0x7fc4d82726d0> is 94
The amount of ham in <main.Spam object at 0x7fc4d8272970> is 29
The amount of ham in <main.Spam object at 0x7fc4d8272430> is 93
Total running time was 10.010624170303345

reify in Pyramid is used heavily to add properties to incoming HTTP request objects - using functools.cached_property instead would mean that each independent request thread blocks others because most of them would always get the value for the same lazy property using the the same descriptor instance and locking the same lock.

Django was going to replace their cached_property by the standard library one https://code.djangoproject.com/ticket/30949

I've been giving thought to implementing the locking on the instance or per instance instead, and there are bad and worse ideas like inserting per (instance, descriptor) into the instance __dict__, guarded by the per-descriptor lock; using a per-descriptor WeakKeyDictionary to map the instance to locks (which would of course not work - is there any way to map unhashable instances weakly?)

So far best ideas that I have heard from others or discovered myself are along the lines of "remove locking altogether" (breaks compatibility); "add thread_unsafe keyword argument" with documentation saying that this is what you want to use if you're actually running threads; "implement Java-style object monitors and synchronized methods in CPython and use those instead"; or "create yet another method".

how about deprecating the functools.cached_property?

using a per-descriptor WeakKeyDictionary to map the instance to locks (which would of course not work - is there any way to map unhashable instances weakly?)

there's an issue for that too: https://bugs.python.org/issue44140

Antti Haapala, I agree that this situation is catastrophic and that we need some way to avoid blocking parallel calculations of cached values for distinct instances of the same class.

Here's an idea that might possibly work. Perhaps, hold one lock only briefly to atomically test and set a variable to track which instances are actively being updated.

If another thread is performing the update, use a separate condition condition variable to wait for the update to complete.

If no other thread is doing the update, we don't need to hold a lock while performing the I/O bound underlying function. And when we're done updating this specific instance, atomically update the set of instances being actively updated and notify threads waiting on the condition variable to wake-up.

The key idea is to hold the lock only for variable updates (which are fast) rather than for the duration of the underlying function call (which is slow). Only when this specific instance is being updated do we use a separate lock (wrapped in a condition variable) to block until the slow function call is complete.

The logic is hairy, so I've added Serhiy and Tim to the nosy list to help think it through.

--- Untested sketch ---------------------------------

class cached_property:
    def __init__(self, func):
        self.update_lock = RLock()
        self.instances_other_thread_is_updating = {}
        self.cv = Condition(RLock())
        ...
        
    def __get__(self, instance, owner=None):
        if instance is None:
            return self
        if self.attrname is None:
            raise TypeError(
                "Cannot use cached_property instance without calling __set_name__ on it.")
        try:
            cache = instance.__dict__
        except AttributeError:  # not all objects have __dict__ (e.g. class defines slots)
            msg = (
                f"No '__dict__' attribute on {type(instance).__name__!r} "
                f"instance to cache {self.attrname!r} property."
            )
            raise TypeError(msg) from None
        
        val = cache.get(self.attrname, _NOT_FOUND)
        if val is not _NOT_FOUND:
            return val
        
        # Now we need to either call the function or wait for another thread to do it
        with self.update_lock:
            # Invariant: no more than one thread can report
            # that the instance is actively being updated
            other_thread_is_updating = instance in instance_being_updated
            if other_thread_is_updating:
                instance_being_updated.add(instance)

        # ONLY if this is the EXACT instance being updated
        # will we block and wait for the computed result.
        # Other instances won't have to wait
        if other_thread_is_updating:
            with self.cv:
                while instance in instance_being_updated:
                    self.cv.wait()
                return cache[self.attrname]

        # Let this thread do the update in this thread
        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
        with self.update_lock:
            instance_being_updated.discard(instance)
        self.cv.notify_all()
        return val   
        
        # XXX What to do about waiting threads when an exception occurs? 
        # We don't want them to hang.  Should they repeat the underlying
        # function call to get their own exception to propagate upwards?

I think it was a mistake to use lock in cached_property at first place. In most cases there is nothing wrong in evaluating the cached value more than once. lru_cache() does not use a lock for calling the wrapped function, and it works well. The only lock in the Python implementation is for updating internal structure, it is released when the wrapped function is called. The C implementation uses the GIL for this purpose.

It is hard to get rid of the execution lock once it was introduced, but I think we should do it. Maybe deprecate cached_property and add a new decorator without a lock. Perhaps add separate decorators for automatically locking method execution, different decorators for different type of locking (global, per-method, per-instance, per-instance-and-method).

Java has the synchronized keyword which allows to add implicit lock to a method. As it turned out, it was a bad idea, and it is no longer used in the modern code.

It is hard to get rid of the execution lock once
it was introduced, but I think we should do it.

It's likely some apps are relying on the locking feature.
So if we can fix it, we should do prefer that over more
disruptive changes.

Addenda to my code sketch: It should use "(id(instance), instance)" rather than just "instance" as the key.

It should use "(id(instance), instance)" rather than just "instance" as the key.

It should use a dict with id(instance) as a key and instance as value. An instance can be non-hashable.

Here's the latest effort:

---------------------------------------------------------------

    def __get__(self, instance, owner=None):
        if instance is None:
            return self
        if self.attrname is None:
            raise TypeError(
                "Cannot use cached_property instance without calling __set_name__ on it.")
        try:
            cache = instance.__dict__
        except AttributeError:  # not all objects have __dict__ (e.g. class defines slots)
            msg = (
                f"No '__dict__' attribute on {type(instance).__name__!r} "
                f"instance to cache {self.attrname!r} property."
            )
            raise TypeError(msg) from None

        # Quickly and atomically determine which thread is reponsible
        # for doing the update, so other threads can wait for that
        # update to complete.  If the update is already done, don't
        # wait.  If the updating thread is reentrant, don't wait.
        key = id(self)
        this_thread = get_ident()
        with self.updater_lock:
            val = cache.get(self.attrname, _NOT_FOUND)
            if val is not _NOT_FOUND:
                return val
            wait = self.updater.setdefault(key, this_thread) != this_thread

        # ONLY if this instance currently being updated, block and wait
        # for the computed result. Other instances won't have to wait.
        # If an exception occurred, stop waiting.
        if wait:
            with self.cv:
                while cache.get(self.attrname, _NOT_FOUND) is _NOT_FOUND:
                    self.cv.wait()
                val = cache[self.attrname]
                if val is not _EXCEPTION_RAISED:
                    return val

    # Call the underlying function to compute the value.
    try:
        val = self.func(instance)
    except Exception:
        val = _EXCEPTION_RAISED

    # Attempt to store the value
    try:
        cache[self.attrname] = val
    except TypeError:
        # Note: we have no way to communicate this exception to
        # threads waiting on the condition variable.  However, the
        # inability to store an attribute is a programming problem
        # rather than a runtime problem -- this exception would
        # likely occur early in testing rather than being runtime
        # event triggered by specific data.
        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

        # Now that the value is stored, threads waiting on the condition
        # variable can be awakened and the updater dictionary can be
        # cleaned up.
        with self.updater_lock:
            self.updater.pop(key, None)
            cache[self.attrname] = _EXCEPTION_RAISED
            self.cv.notify_all()

        if val is _EXCEPTION_RAISED:
            raise
        return val

In addition to _NOT_FOUND and _EXCEPTION_RAISED, you could have an additional sentinel value _CONCURRENTLY_COMPUTING. Then you don't need to maintain a separate self.updater mapping.

Sorry everyone for the bad implementation here. The initial PR (based on the widely used implementations in e.g. Django and Pyramid) didn't have locking; I added locking during review at the request of some commenters in the original issue (based on the implementation in some other less-used third-party package), but I think (even apart from this issue) it was a mistake to add it in the first place.

I agree with @serhiy-storchaka and https://mobile.twitter.com/kristjanvalur/status/1431027975298895883 that the locking is basically out-of-place here: Python data structures (e.g. dict) guarantee only (mostly via the GIL) that they won't get into invalid internal states and segfault the interpreter in the face of concurrency, they don't guarantee an absence of application-level data races. That's up to the application to determine and apply the locking that it needs at a higher level.

I think the best of bad options here is to add either cached_property_no_lock (name to-be-bikeshedded) or a nolock=True keyword argument, and eventually deprecate and remove the locking behavior. I can put up a PR for this. Any preferences about new name vs kwarg?

fast_cached_property, cursed_property

I think the best of bad options here is to add either cached_property_no_lock (name to-be-bikeshedded) or a nolock=True keyword argument, and eventually deprecate and remove the locking behavior. I can put up a PR for this. Any preferences about new name vs kwarg?

Is there any progress in this direction?

Any preferences about new name vs kwarg?

Personally I'd vote for the kwarg approach.

I think cached_property.lock is causing a PicklingError when a cached_property overrides another cached_property:

from functools import cached_property
from joblib import Parallel, delayed

class AAA:
    @cached_property
    def num(self): return 111

class BBB(AAA):
    @cached_property
    def num(self): return 222

def fn(arg): return arg

Parallel(n_jobs=2)(delayed(fn)(bbb) for bbb in [BBB(), BBB()])

joblib.externals.loky.process_executor._RemoteTraceback:
"""
Traceback (most recent call last):
  File "$PYTHON_HOME/site-packages/joblib/externals/loky/backend/queues.py", line 153, in _feed
    obj_ = dumps(obj, reducers=reducers)
  File "$PYTHON_HOME/site-packages/joblib/externals/loky/backend/reduction.py", line 271, in dumps
    dump(obj, buf, reducers=reducers, protocol=protocol)
  File "$PYTHON_HOME/site-packages/joblib/externals/loky/backend/reduction.py", line 264, in dump
    _LokyPickler(file, reducers=reducers, protocol=protocol).dump(obj)
  File "$PYTHON_HOME/site-packages/joblib/externals/cloudpickle/cloudpickle_fast.py", line 563, in dump
    return Pickler.dump(self, obj)
TypeError: cannot pickle '_thread.RLock' object
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "$PWD/foo.py", line 14, in <module>
    Parallel(n_jobs=2)(delayed(fn)(bbb) for bbb in [BBB(), BBB()])
  File "$PYTHON_HOME/site_packages/joblib/parallel.py", line 1054, in __call__
    self.retrieve()
  File "$PYTHON_HOME/site_packages/joblib/parallel.py", line 933, in retrieve
    self._output.extend(job.get(timeout=self.timeout))
  File "$PYTHON_HOME/site_packages/joblib/_parallel_backends.py", line 542, in wrap_future_result
    return future.result(timeout=timeout)
  File "$PYTHON_HOME/concurrent/futures/_base.py", line 445, in result
    return self.__get_result()
  File "$PYTHON_HOME/concurrent/futures/_base.py", line 390, in __get_result
    raise self._exception
_pickle.PicklingError: Could not pickle the task to send it to the workers.

Notes:

• If both AAA.num and BBB.num are changed to @property, the error goes away.
  • If one is a @property and the other is a @cached_property, the error remains.
• I tried working around it by overriding __getstate__ to omit any attributes that are cached_propertys on the class instance, but it didn't help.

class cached_property:
    """Based on code from David Beazley's "Python Cookbook" / https://stackoverflow.com/q/62160411/544236."""
    def __init__(self, func):
        self.__doc__ = getattr(func, '__doc__')
        self.func = func

    def __get__(self, instance, cls):
        if instance is None:
            return self
        else:
            value = instance.__dict__[self.func.__name__] = self.func(instance)
            return value

PR #98123 was closed because the proposed deprecation approach is too painful for users. Unfortunately that still leaves us without a good solution.

Here are some alternatives:

1. Just rip out the locking code without a deprecation in code. This would fix the bug and improve performance for most users, but it would break users who rely on the current per-class locking. Do any such users exist? I'm not convinced they do, but our general policy is to assume yes. Perhaps we can justify a BC-breaking change here if we clearly document the behavior change and call it out widely in documentation. Concretely, we could do a docs-only deprecation in 3.12 and 3.13 and then remove the locking code in 3.14.
2. Create a new property that is like cached_property but without locking. But this has most of the same problems as gh-87634: deprecate cached_property locking, add lock kwarg #98123 had, and it's hard to think of a good name.
3. "Fix" the locking code to be per-instance instead of per-class. But as @carljm calls out in gh-87634: deprecate cached_property locking, add lock kwarg #98123 (comment), this isn't without problems either. The locking code would be complex and pickling may be affected. I would add that this change may itself cause a BC break: hypothetical code that relies on the current behavior might well be protecting some global state instead of per-instance state.

I feel (1) is the best solution, though it's sad to have to wait until 3.14.

Why not:

4. Add an optional lock argument, but without deprecating the current form.

1. Do any such users exist? I'm not convinced they do, but our general policy is to assume yes.

I'm sure they do. Moreover, this will break code in mysterious and random ways that will leave users puzzled. So I'm strongly -1 on this.

I suppose the advantage of adding a new class without the locking behaviour is that users will potentially only have to update one line of code in each file to opt out of the locking:

-from functools import cached_property
+from functools import cached_property_no_lock as cached_property

Adding the new class could maybe be combined with @pitrou's suggestion of adding the optional lock argument to the existing class without deprecating the current form. That would mean the new class could just be a very simple wrapper:

class cached_property_no_lock(cached_property):
    def __init__(self, func):
        super().__init__(func, lock=False)

Thanks @JelleZijlstra for re-starting the conversation here.

I'm quite tempted by option (1), because it's clearly what we would do given a time machine, and I haven't yet seen anyone, in any forum, mention a real use case in which they are using cached_property and depending on the locking. Whereas I have seen many "significant" users (e.g. the Django project) avoid the stdlib cached_property entirely because of this bug, and instead continue maintaining their own external lockless version. That said, I think @pitrou is right that there's a reasonable chance that someone, somewhere, even if unaware, is depending on it, and the behavior change when it goes away could be quite hard to debug, so it's probably not a good option :/

I think the "change only one line to opt out" advantage of cached_property_no_lock is significant, and I'm not sure there is any need to provide both cached_property_no_lock and cached_property(lock=False). The latter is slightly more verbose, more punctuation, and slightly harder to alias to a shorter name (can't be done in the import.) Also there is inherent hard-to-read complexity in the implementation of decorators with optional arguments. So I think "new name" is better than "optional argument."

I can submit a new PR with cached_property_no_lock and no deprecation.

I think the "change only one line to opt out" advantage of cached_property_no_lock is significant

It also seems to create an odd library situation down the road, when new users have to choose between the more version compatible and intuitively named cached_property which comes at the cost of an undocumented global thread lock, or a newer cached_property_no_lock that should be the default choice.

Seems counterintuitive with the idea of no lock being what most users want, and more importantly, expect.

The concept of the lock, (and as implemented by third party libraries), has always seemed to be intended to be based on instances and not be global, if we can't rationalize a valid use case for this global lock, why should cached_property still stay in the stdlib after a theoretical cached_property_no_lock?

We will either have new users who don't need a lock accidentally using cached_propery when they should be using cached_propery_no_lock, or users needing an instance lock accidentally using cached_property when they really need an instance-based thread lock, which the stdlib wouldn't offer.

Choices that benefit seemingly nobody to keep a undocumented implementation detail broken for a theoretical user that likely does not exist would be detrimental to functools's coherence as a library and to CPython.

Imo, the current optimistic path is removing the lock from cached_property and documenting this. I wouldn't think it affects backwards compatibility if the lock was an undocumented implementation detail.

and the behavior change when it goes away could be quite hard to debug

I would argue that the vast majority of users are also experiencing hard to debug thread lock issues due to this undocumented global class thread lock. Already this issue is referenced dozens of times across downstream libraries as a caution against switching to functools.cached_property and instead to rely on a third party library.

A change in undocumented implementation behavior should aim to fix bugs for the majority of users. Under the idea that any affected behavior is breaking BC there would theoretically be no bug fix possible for any feature.

Either this issue should be changed under the assumption that the current behavior is not incorrect, or we should fix it.

@eendebakpt made another suggestion over on my closed PR #98123 :

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

I think the main problem here is what it means to "detect if the lock is used." The only reasonable definition I can think of would be "is the lock ever held by another thread when we try to access it." I think this is detectable by using a non-blocking call to acquire; if it fails, we issue the deprecation warning followed by a blocking call. It is, however, a bit strange to issue a deprecation warning based on a behavior of the system that may be non-deterministic.

I haven't submitted the new PR for cached_property_no_lock yet, mostly because I'm not very happy with that solution either, for the reasons @ionite34 describes, and I still wonder if "just remove the locking, with several versions' doc warning" isn't practically the best option for the vast majority of Python users.

It seems like we have strong differences of opinion here among core developers; there are tradeoffs with any option we choose, and it seems like a largely subjective matter of weighing the tradeoffs. Would a Discourse thread/poll be a reasonable next step to gather more perspectives? Or even bringing the question to the SC?

I started a thread at https://discuss.python.org/t/finding-a-path-forward-for-functools-cached-property/23757

Following extensive discussion in https://discuss.python.org/t/finding-a-path-forward-for-functools-cached-property/23757 that did not reach full consensus of core devs, I asked for a Steering Council ruling on how to handle this issue: python/steering-council#172

The Steering Council's decision was that the locking should be removed in Python 3.12, with a note in What's New, as done in #101890

Thanks everyone for your contributions to this issue!