Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-87634: remove locking from functools.cached_property#101890
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Doc/whatsnew/3.12.rst Outdated
| is removed, because it locked across all instances of the class, leading to high | ||
| lock contention. This means that a cached property getter function could now | ||
| occasionally run more than once for a single instance, if two threads race. For | ||
| most simple cached properties (e.g. those that are idempotent and simply |
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.
The word "occasionally" is not necessary -- we already have the "could" and "if".
Doc/library/functools.rst Outdated
| The *cached_property* does not prevent a possible race condition in | ||
| multi-threaded usage. The getter function could run more than once on the | ||
| same instance, with the latest run setting the cached value. If the cached | ||
| property is idempotent or otherwise not harmful to (in rare cases) run more |
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.
"(in rare cases)" seems to mean that it's rare for a multiply executed getter to be okay -- I would expect the opposite to be true. If you agree, just remove the parenthetical.
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.
The intent was to say that the "running more than once on an instance" is a rare case. But the wording could be confusing, and doesn't seem necessary, so I'll remove it.
bedevere-bot commentedFeb 21, 2023
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
* main: (76 commits) Fix syntax error in struct doc example (python#102160)pythongh-99108: Import MD5 and SHA1 from HACL* (python#102089)pythonGH-101777: `queue.rst`: use 2 spaces after a period to be consistent. (python#102143) Few coverage nitpicks for the cmath module (python#102067)pythonGH-100982: Restrict `FOR_ITER_RANGE` to a single instruction to allow instrumentation. (pythonGH-101985)pythongh-102135: Update turtle docs to rename wikipedia demo to rosette (python#102137)pythongh-99942: python.pc on android/cygwin should link to libpython per configure.ac (pythonGH-100356)pythongh-95672 fix typo SkitTest to SkipTest (pythongh-102119)pythongh-101936: Update the default value of fp from io.StringIO to io.BytesIO (pythongh-102100)pythongh-102008: simplify test_except_star by using sys.exception() instead of sys.exc_info() (python#102009)pythongh-101903: Remove obsolete undefs for previously removed macros Py_EnterRecursiveCall and Py_LeaveRecursiveCall (python#101923)pythongh-100556: Improve clarity of `or` docs (python#100589)pythongh-101777: Make `PriorityQueue` docs slightly clearer (python#102026)pythongh-101965: Fix usage of Py_EnterRecursiveCall return value in _bisectmodule.c (pythonGH-101966)pythongh-101578: Amend exception docs (python#102057)pythongh-101961 fileinput.hookcompressed should not set the encoding value for the binary mode (pythongh-102068)pythongh-102056: Fix a few bugs in error handling of exception printing code (python#102078)pythongh-102011: use sys.exception() instead of sys.exc_info() in docs where possible (python#102012)pythongh-101566: Sync with zipp 3.14. (pythonGH-102018)pythonGH-99818: improve the documentation for zipfile.Path and Traversable (pythonGH-101589) ...
| ################################################################################ | ||
| ### cached_property() - computed once per instance, cached as attribute |
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.
This line should also be changed.@carljm
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.
It's not an absolute guarantee in the presence of multi-threading race conditions, but this is still a generally accurate description of the usual behavior of acached_property. What alternative wording would you suggest for this one-line comment that is both clear and concise? Note the public-facing documentation already contains a full and clear description of the behavior. (To be clear this is a genuine question, I'm open to changing this comment if you have a good suggestion, I'm just not sure what a clear and concise wording would be, and I think it might be OK to leave it as is.)
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.
How about
### cached_property() - property result cached as instance attributeThere 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.
Works for me! I've rolled that change into#106380 where I'm already touchingcached_property.
…GH-101890)Remove the undocumented locking capabilities of functools.cached_property.
Uh oh!
There was an error while loading.Please reload this page.
Inhttps://discuss.python.org/t/finding-a-path-forward-for-functools-cached-property/23757/26, by far the most favored option for dealing with#87634 is to simply remove the locking behavior from
cached_property. For the sake of moving things forward, here's a PR for discussion which does that.Even if we do this, there are several options to consider here:
cached_propertyavailable sooner, and I'm not sure how much benefit the extra time gives anyone: there isn't a runtime deprecation warning that this gives people more time to see, and this isn't a difficult change to adapt to: the current locking version ofcached_propertyis <50 lines of self-contained pure Python code; any project that wants it can easily just take it. And there's already athird-party library that includes it. At least one core developer in the discourse thread suggested that this is changing an undocumented behavior detail, and can just be done without a lengthy deprecation.locking_cached_property), in hopes that in the future it will be fixed to use a more fine-grained locking strategy. That makes it even simpler to adapt to the change.