gh-87634: remove locking from functools.cached_property #101890
Conversation
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.
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.
"(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.
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.
|
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) ...
In https://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 a third-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.