Use a thread-safe implementation of cached_property #1316
Conversation
|
This pull request introduces 2 alerts when merging 8948c86 into c7068ef - view on LGTM.com new alerts:
|
thedrow
force-pushed
the
thread-safe-cached-property
branch
from
8948c86
to
923369f
Compare
|
It turns out that our cached_property implementation needs to support a setter as well. |
|
This pull request introduces 2 alerts when merging 923369f into c7068ef - view on LGTM.com new alerts:
|
| try: | ||
| from functools import _NOT_FOUND, cached_property as _cached_property | ||
| except ImportError: | ||
| # TODO: Remove this fallback once we drop support for Python < 3.8 | ||
| from cached_property import threaded_cached_property as _cached_property | ||
|
|
||
| Caches the return value of the get method on first call. | ||
| _NOT_FOUND = object() |
There was a problem hiding this comment.
[suggestion]: Create local NOT_FOUND sentinel class instead of using a non-public class from functools.
Looks like _NOT_FOUND is a private class used in the functools module. We should not be using it. Instead we can define our own sentinel class:
class MISSING: pass
# ...
# Use it in __delete__
if self.__del and value is not MISSING:
self.__del(instance, value)There was a problem hiding this comment.
But if we inherit from the stdlib cached_property we can't use our own sentinel, right?
It's already using _NOT_FOUND.
| def __set__(self, instance, value): | ||
| if instance is None: | ||
| return self | ||
| if self.__set is not None: | ||
| value = self.__set(obj, value) | ||
| obj.__dict__[self.__name__] = value | ||
|
|
||
| def __delete__(self, obj, _sentinel=object()): | ||
| if obj is None: | ||
| with self.lock: | ||
| if self.__set is not None: | ||
| value = self.__set(instance, value) | ||
|
|
||
| cache = instance.__dict__ | ||
| cache[self.attrname] = value |
There was a problem hiding this comment.
[question]: Why do we have to override set? functools.cached_property and cached_property both implement only get to calculate the value only once.
Do we really need to re-define the value on cached properties?
There was a problem hiding this comment.
Because that's how it's implemented and used in Celery.
There was a problem hiding this comment.
I am not sure whether this PR is fixing the issue. The problem of cached_property in celery is that the cache is shared between the threads. Current PR is adding just locks but it lacks storing the cache in thread local storage.
>>> from kombu.utils import cached_property
>>> from threading import Thread
>>>
>>> class Foo:
... @cached_property
... def foo(self):
... print('foo')
... return 5
...
>>> foo = Foo()
>>>
>>> def call_foo():
... foo.foo
...
>>>
>>> Thread(target=call_foo).start()
foo
>>> foo.foo # Here we should see `foo` printed out since the cache should not be hit since it is called from different thread than previous call.
5
|
Wait, we need thread-local storage instead of using locks? |
|
To be honest it is difficult to say but in Celery there is a lot of usage of Rationale is following:
|
|
I was thinking more about So I am aproving this PR. @thedrow the only change I am suggesting is adding docstring to the |
|
We can add a version of cached_property which is thread-local later on. |
Fixes #1303.