gh-128714: Fix function object races in FT build by xuantengh · Pull Request #129016 · python/cpython · GitHub

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.

Issue: Race in func_get_annotations under free-threading #128714

This PR needs a NEWS entry.

Can a triager please add the interpreter-core label.

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.

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.

I suspect some of these fields (e.g. func_code) are also accessed by the eval loop or other functions without the Python-level getter, and therefore without the lock. Can we look into finding those cases?

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.

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Please reduce the diff, the getters and setters doesn't need to be moved, you should add @critical_section where it was already defined.

There are public C APIs like PyFunction_GetCode and PyFunction_GetGlobals which access only one field in function object. Should we use atomic load in these APIs?

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.

There are public C APIs like PyFunction_GetCode and PyFunction_GetGlobals which access only one field in function object. Should we use atomic load in these APIs?

They should use also critical sections, not atomics.

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_TryXGetRef for 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.

Locking is probably fine for func_annotations, func_annotate, and func_typeparams. We should limit this PR to those fields.

I've reverted the PR to change only __annotate__, __annotations__ and __type_params. And now the PR only changes attributes which are not performance critical.

lgtm

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 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 55f17b77c305be877ac856d6426b13591cbc7fc8 3.13

…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)

…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>

bedevere-app Bot mentioned this pull request Jan 19, 2025

Race in func_get_annotations under free-threading #128714

Closed

bedevere-app Bot added the awaiting review label Jan 19, 2025

kumaraditya303 reviewed Jan 19, 2025

View reviewed changes

Comment thread Objects/funcobject.c

ZeroIntensity added topic-free-threading needs backport to 3.13 bugs and security fixes labels Jan 19, 2025

xuantengh changed the title gh-128714: Fix func.__annotations__ races in FT build gh-128714: Fix function object races in FT build Jan 21, 2025

xuantengh force-pushed the ft-annotations branch from 2290b7e to 3d7d16b Compare January 21, 2025 06:16

kumaraditya303 self-assigned this Jan 21, 2025

ZeroIntensity reviewed Jan 21, 2025

View reviewed changes

kumaraditya303 requested changes Jan 27, 2025

View reviewed changes

bedevere-app Bot removed the awaiting review label Jan 27, 2025

bedevere-app Bot added the awaiting changes label Jan 27, 2025

xuantengh force-pushed the ft-annotations branch from 0ebf27a to 28a3fbc Compare January 30, 2025 16:17

kumaraditya303 requested changes Jan 30, 2025

View reviewed changes

xuantengh force-pushed the ft-annotations branch from 28a3fbc to 795d80e Compare January 31, 2025 05:29

bedevere-app Bot added awaiting change review and removed awaiting changes labels Jan 31, 2025

bedevere-app Bot requested a review from kumaraditya303 January 31, 2025 11:53

kumaraditya303 reviewed Jan 31, 2025

View reviewed changes

Comment thread Objects/funcobject.c

ZeroIntensity added the DO-NOT-MERGE label Feb 2, 2025

colesbury reviewed Feb 3, 2025

View reviewed changes

xuantengh force-pushed the ft-annotations branch from 2d574cd to 7ca47de Compare February 4, 2025 12:30

corona10 approved these changes Feb 4, 2025

View reviewed changes

colesbury reviewed Feb 5, 2025

View reviewed changes

Comment thread Objects/funcobject.c Outdated

Comment thread Objects/funcobject.c Outdated

xuantengh and others added 7 commits February 6, 2025 09:51

add ft support for func annotation read

736e5dc

add concurrent write tests

594e0f0

📜🤖 Added by blurb_it.

4026608

init return value

6d8a5d5

clinic getset methods which is not performance critical

00a538e

update news entry

5913786

remove unnecessary critical sections

70fefd5

xuantengh force-pushed the ft-annotations branch from 7ca47de to 70fefd5 Compare February 6, 2025 01:51

kumaraditya303 approved these changes Feb 6, 2025

View reviewed changes

Merge branch 'main' into ft-annotations

d793ba3

kumaraditya303 removed the DO-NOT-MERGE label Feb 6, 2025

kumaraditya303 merged commit 55f17b7 into python:main Feb 6, 2025

bedevere-app Bot removed the awaiting merge label Feb 6, 2025

xuantengh deleted the ft-annotations branch February 6, 2025 14:50

xuantengh mentioned this pull request Feb 6, 2025

[3.13] gh-128714: Fix function object races in __annotate__, __annotations__ and __type_params__ in free-threading build (GH-129016) #129729

Merged

xuantengh added a commit to xuantengh/cpython that referenced this pull request Feb 6, 2025

pythongh-128714 : Fix function object races in __annotate__, `__anno…

0e09c6f

…tations__` and `__type_params__` in free-threading build (python#129016) (cherry picked from commit 55f17b7)

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Feb 6, 2025

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025

pythongh-128714 : Fix function object races in __annotate__, `__anno…

31f6cd1

…tations__` and `__type_params__` in free-threading build (python#129016)

cmaloney pushed a commit to cmaloney/cpython that referenced this pull request Feb 8, 2025

pythongh-128714 : Fix function object races in __annotate__, `__anno…

0e02a24

…tations__` and `__type_params__` in free-threading build (python#129016)

xuantengh mentioned this pull request May 17, 2025

Make generator methods gen_set_name and gen_set_qualname thread-safe in free-threaded builds #133931

Closed

+              static PyGetSetDef func_getsetlist[] = {
+                  FUNCTION___CODE___GETSETDEF
+                  {"__defaults__", func_get_defaults, func_set_defaults},
+                  {"__kwdefaults__", func_get_kwdefaults, func_set_kwdefaults},

gh-128714: Fix function object races in FT build by xuantengh · Pull Request #129016 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-128714: Fix function object races in FT build - #129016

gh-128714: Fix function object races in FT build#129016kumaraditya303 merged 8 commits intopython:mainpython/cpython:mainfrom xuantengh:ft-annotationsxuantengh/cpython:ft-annotationsCopy head branch name to clipboard

Conversation

xuantengh commented Jan 19, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StanFromIreland commented Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz commented Jan 19, 2025

Uh oh!

StanFromIreland commented Jan 19, 2025

Uh oh!

Uh oh!

JelleZijlstra commented Jan 19, 2025

Uh oh!

xuantengh commented Jan 20, 2025

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

kumaraditya303 commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumaraditya303 Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Jan 27, 2025

Uh oh!

kumaraditya303 left a comment

Choose a reason for hiding this comment

Uh oh!

xuantengh commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuantengh commented Jan 31, 2025

Uh oh!

bedevere-app Bot commented Jan 31, 2025

Uh oh!

kumaraditya303 commented Jan 31, 2025

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

xuantengh commented Feb 4, 2025

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miss-islington-app Bot commented Feb 6, 2025

Uh oh!

miss-islington-app Bot commented Feb 6, 2025

Uh oh!

bedevere-app Bot commented Feb 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gh-128714: Fix function object races in FT build#129016
kumaraditya303 merged 8 commits into
python:mainfrom
xuantengh:ft-annotations

xuantengh commented Jan 19, 2025 •
edited by bedevere-app Bot
Loading

StanFromIreland commented Jan 19, 2025 •
edited
Loading

kumaraditya303 commented Jan 27, 2025 •
edited
Loading

xuantengh commented Jan 31, 2025 •
edited
Loading