-
Couldn't load subscription status.
- Fork 1.9k
make cached_classproperty more robust
#6114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The clear_cache method’s branch for owner+name currently pops the entire class cache; it should instead delete only the specific property entry under that owner.
- The keys_to_remove logic assumes _cache keys are (owner, name) tuples, but they’re actually owner types—simplify owner-only removal to cls._cache.pop(owner, None).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The clear_cache method’s branch for owner+name currently pops the entire class cache; it should instead delete only the specific property entry under that owner.
- The keys_to_remove logic assumes _cache keys are (owner, name) tuples, but they’re actually owner types—simplify owner-only removal to cls._cache.pop(owner, None).
## Individual Comments
### Comment 1
<location> `beets/util/__init__.py:1066-1068` </location>
<code_context>
- # "Callable[[Album], ...]"; expected "Callable[[type[Album]], ...]"
- #
- # Therefore, we just use `Any` here, which is not ideal, but works.
+ _cache: ClassVar[
+ weakref.WeakKeyDictionary[type[object], dict[str, object]]
+ ] = weakref.WeakKeyDictionary()
+ _lock: ClassVar[threading.RLock] = threading.RLock()
+ name: str | None = None
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider thread-safety of WeakKeyDictionary usage with RLock.
Ensure that all access to the WeakKeyDictionary, including reads, is performed within the RLock to prevent race conditions.
</issue_to_address>
### Comment 2
<location> `beets/util/__init__.py:1113-1114` </location>
<code_context>
+ class_cache[self.name] = value
+ return value
+
+ @classmethod
+ def clear_cache(
+ cls, owner: type[object] | None = None, name: str | None = None
+ ) -> None:
</code_context>
<issue_to_address>
**issue (bug_risk):** clear_cache implementation does not match new cache structure.
Update clear_cache to use owner as the key and property names as values to ensure correct cache clearing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
09766af to
f1fdb40
Compare
|
I want to understand what issue does this solve as this balloons its implementation complexity. |
|
@snejus feel free to revert changes you think are unnecessary
The changes mitigate possible race conditions by ensuring thread-safety and aim to prevent misuse by marking the cache as private and introducing |
d62c7a2 to
567a425
Compare
|
I think, we'd rather solve this when we have an issue. For now, we use it in very limited contexts (simply as a dynamic class property) and I doubt this logic is required (at least for now). |
|
if it's ever needed, just revert the commit above |
Description
continuation of #6110
Documentation