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

[ztane]

BPO	43468
Nosy	@tim-one, @rhettinger, @ncoghlan, @pitrou, @carljm, @jab, @serhiy-storchaka, @ztane, @graingert, @youtux
PRs	bpo-43468: Per instance locking for functools.cached_property  #27609

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-03-11.06:37:47.884>
labels = ['3.8', 'library', '3.9', '3.10', 'performance']
title = 'functools.cached_property incorrectly locks the entire descriptor on class instead of per-instance locking'
updated_at = <Date 2021-09-07.16:52:45.061>
user = 'https://github.com/ztane'

bugs.python.org fields:

activity = <Date 2021-09-07.16:52:45.061>
actor = 'pitrou'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2021-03-11.06:37:47.884>
creator = 'ztane'
dependencies = []
files = []
hgrepos = []
issue_num = 43468
keywords = ['patch']
message_count = 11.0
messages = ['388480', '388499', '388592', '398290', '398291', '398680', '398686', '398719', '398837', '398861', '401308']
nosy_count = 11.0
nosy_names = ['tim.peters', 'rhettinger', 'ncoghlan', 'pitrou', 'carljm', 'pydanny', 'jab', 'serhiy.storchaka', 'ztane', 'graingert', 'youtux']
pr_nums = ['27609']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue43468'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

Linked PRs

• gh-87634: remove locking from functools.cached_property #101890

[ztane]

The locking on functools.cached_property (

cpython/Lib/functools.py

Line 934 in 87f649a

class cached_property:

) as it was written is completely undesirable for I/O bound values, parallel processing. Instead of protecting the calculation of cached property to the same instance in two threads, it completely blocks parallel calculations of cached values to *distinct instances* of the same class.

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.

[ztane]

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

[ztane]

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