gh-128714: Fix function object races in FT build #129016
Conversation
|
This PR needs a NEWS entry. Can a triager please add the interpreter-core label. |
Labels such as these are usually added on the issues rather than the PR. |
|
@picnixz My bad. |
|
There seem to be similar races on all other settable attributes on function objects, so it probably makes sense to fix them all at once. |
Agree. I'll try to fix them. |
func.__annotations__ races in FT build2290b7e to
3d7d16b
Compare
|
I think it's fine to just fix getters and setters of function in this PR. Fixing it for interp will require more massive changes which can be done separately if needed. |
Objects/funcobject.c
Outdated
| static PyGetSetDef func_getsetlist[] = { | ||
| FUNCTION___CODE___GETSETDEF | ||
| {"__defaults__", func_get_defaults, func_set_defaults}, | ||
| {"__kwdefaults__", func_get_kwdefaults, func_set_kwdefaults}, |
There was a problem hiding this comment.
Please add critical sections to all other ones as well
|
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 |
0ebf27a to
28a3fbc
Compare
28a3fbc to
795d80e
Compare
|
There are public C APIs like |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @kumaraditya303: please review the changes made to this pull request. |
They should use also critical sections, not atomics. |
There was a problem hiding this comment.
Thanks again for taking this on @xuantengh.
We do not want to add locking for most of the PyFunctionObject attributes. In particular, we do not want locking for anything that will be accessed during function calls, like func_code, func_builtins, func_defaults, etc. both because it will affect single-threaded performance and because it will inhibit efficient multithreaded scaling.
Locking is probably fine for func_annotations, func_annotate, and func_typeparams. We should limit this PR to those fields.
We have a few general strategies for thread-safe accesses:
- Locking around both reading and writing the field.
- Locking around writers, and something like
_Py_TryXGetReffor readers (a non-locking fast-path) - Locking around writers with some sort of delayed deallocation (like QSBR). Readers do not use locks, but still need atomic loads.
- Stop-the-world around writers and no synchronization for readers
We will probably want to use either (3) or (4) for fields like func_code, but I want to be careful about making those changes and don't think we should pursue those as part of this PR.
2d574cd to
7ca47de
Compare
I've reverted the PR to change only |
7ca47de to
70fefd5
Compare
|
Thanks @xuantengh for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
|
Sorry, @xuantengh and @kumaraditya303, I could not cleanly backport this to |
…tations__` and `__type_params__` in free-threading build (python#129016) (cherry picked from commit 55f17b7)
|
GH-129729 is a backport of this pull request to the 3.13 branch. |
…tations__` and `__type_params__` in free-threading build (python#129016)
…tations__` and `__type_params__` in free-threading build (python#129016)
…otations__` and `__type_params__` in free-threading build (GH-129016) (#129729) * gh-128714: Fix function object races in `__annotate__`, `__annotations__` and `__type_params__` in free-threading build (#129016) (cherry picked from commit 55f17b7) --------- Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
This PR aims to fix the potential data races when getting and setting funcobject
__annotations__. And it also add the test to validate the consistency under concurrent reads and writes.