Use `dict.setdefault()` in functools caching for better thread-safety

[seberg]

Feature or enhancement

Proposal:

Currently, functools.cache family of functions uses _PyDict_SetItem_KnownHash. But I am wondering if it shouldn't use SetDefault style access at basically no-cost?

That way, when working with many threads, while multiple evaluations are still possible, all threads are guaranteed to end up with the same object.
There may be great reasons why that isn't wanted, but on first sight it seems like a simple and practically free way to remove a pit-fall for threaded code.
(Of course building a custom version that does this is easy, but functools.cache is great and used quite a lot.)

Admittedly, an optional once= would also be a very cool feature (or maybe as a dedicated pattern).

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Linked PRs

• gh-150708: Make functools.cache (unbound) and cached_property strictly idempotent #154069

[picnixz]

If you believe setdefault is better, run some benchmarks with pyperf to see whether it helps.

[seberg]

I don't see why the performance should be different, right now there is no _PyDict_SetDefault_KnownHash, but otherwise the work done by the two functions are practically identical (also dict insertion effectively checks what is in the dict right now).

My ask is to improve the thread-safety comfort, under the (to me very sure) assumption of no meaningful speed impact; especially since nothing changes on the hot path where the cache is filled.

EDIT: In other words, IMO it's a small but real annoyance if users have to replace the convenience of functools.cache with something custom when they start getting interested in threading. And in this instance unless there are BC concerns, I see little reason to not at least improve things. (But I admit, I may just not understand the reason, perf doesn't seem like one though.)

[picnixz]

What is wrong with the current implementation more concretely? do you have examples where setdefault() would solve a real issue? I do not understand clearly what you want to solve and in which situations.

[seberg]

@picnixz sorry, I guess I opened this issue for people familiar with threading issues. As a (silly) example, if you do:

@functools.cache
def get_resource(something):
    return create_resource_for_something(something)

then the implementation doesn't guarantee that all callers of get_resource(value) will get the identical resource, but the code sure looks like that is the case.
So if resource is supposed to be shared (as a silly thing, just a list that get's appended/popped from but should be shared by all callers even from multiple threads) then it is broken.

With the suggested change, create_resource_for_something() might be called more than once, but there would only ever be a single resource returned for all callers.

[picnixz]

In this case the problem is not the threading it is the base hypothesis. I do not believe it has been claimed to be thread safe and I think the docs already mention these limitations.

[seberg]

I do not believe it has been claimed to be thread safe and I think the docs already mention these limitations.

I am asking to make it more safe on grounds of it being a completely free improvement and removing one small but real surprise when running multi-threaded.

[picnixz]

Can you dig at the history of why it was decided not to make it thread-safe in the first place? or check if there doesn't exist such packages on PyPI? If you want to make it thread-safe, feel free to make a PR that shows the improvement and add tests.

[rhettinger]

In the mean time, I suggest we close this. The code is correct as-is and matches the documented behavior. It is thread-safe as-is. It just doesn't provide a call_once behavior. Instead, it favors its core task which is saving and reusing already computed values.

I am dubious about dict.setdefault being able to add this new behavior (the code for setdefault is already mimicked in the lru_cache code). In the future, I may propose a new call_once function with all the locking tricks that would be required to prevent a concurrent call.

[seberg]

It just doesn't provide a call_once behavior. Instead, it favors its core task which is saving and reusing already computed values.

I am dubious about dict.setdefault being able to add this new behavior

@rhettinger with the chance of repeating the obvious, but the behavior I am asking for is not call_once (I mentioned this, but it isn't).
What I am asking for is this:

@functools.cache
def myfunc(...):
   # ...


def do_work():
    assert myfunc() is myfunc()  # will always pass even in a threaded environment.

to be guaranteed in a thread-safe environment. I.e. I am asking for the cache to be idempotent (in the sense of returning the identical object). setdefault guarantees this and "the code for setdefault is already mimicked in the lru_cache code" is thus not true in this regard.

(More concretely, this allows the function to return a, possibly mutable object, that is guaranteed to be shared by all callers. While call_once would ensure that myfunc() isn't called more than once for the same key which is a much strong guarantee.)

And let me spell out the thread-safety difference:

def current(key):
    if key in cache:
        return cache[key]

    res = func(key)
    # At this point another thread may have reached cache[key] = res
    # so the following may *replace* it:
    cache[key] = res
    return res

def new(key):
    if key in cache:
        return cache[key]

    res = func(key)
    # At this point another thread may have set cache[key]
    # but `setdefault()` doesn't replace it in that case, rather
    # it returns the already set result instead.
    return cache.setdefault(key, res)

[seberg]

@lysnikolaou not sure if I should bother you with this, but I now realized that the guarantee that multiple d.setdefault(key, obj) calls are guaranteed to only result in one of them to update the dict seems important (this is stronger than what is currently documented).
It is a pattern I have needed to use many many times for code doing caches.