Skip to content

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

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

Merged
merged 8 commits into from
Feb 6, 2025

Conversation

xuantengh
Copy link
Contributor

@xuantengh xuantengh commented Jan 19, 2025

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.

@StanFromIreland
Copy link
Contributor

StanFromIreland commented Jan 19, 2025

This PR needs a NEWS entry.

Can a triager please add the interpreter-core label.

@picnixz
Copy link
Member

picnixz commented Jan 19, 2025

Can a triager please add the interpreter-core label.

Labels such as these are usually added on the issues rather than the PR.

@StanFromIreland
Copy link
Contributor

@picnixz My bad.

@JelleZijlstra
Copy link
Member

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.

@xuantengh
Copy link
Contributor Author

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.

@xuantengh 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
@kumaraditya303 kumaraditya303 self-assigned this Jan 21, 2025
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Jan 27, 2025

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add critical sections to all other ones as well

@bedevere-app
Copy link

bedevere-app bot commented Jan 27, 2025

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.

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@xuantengh
Copy link
Contributor Author

xuantengh commented Jan 31, 2025

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?

@xuantengh
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Jan 31, 2025

Thanks for making the requested changes!

@kumaraditya303: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from kumaraditya303 January 31, 2025 11:53
@kumaraditya303
Copy link
Contributor

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.

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Locking around both reading and writing the field.
  2. Locking around writers, and something like _Py_TryXGetRef for readers (a non-locking fast-path)
  3. Locking around writers with some sort of delayed deallocation (like QSBR). Readers do not use locks, but still need atomic loads.
  4. 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.

@xuantengh
Copy link
Contributor Author

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.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kumaraditya303 kumaraditya303 merged commit 55f17b7 into python:main Feb 6, 2025
43 of 44 checks passed
@miss-islington-app
Copy link

Thanks @xuantengh for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

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

@xuantengh xuantengh deleted the ft-annotations branch February 6, 2025 14:50
xuantengh added a commit to xuantengh/cpython that referenced this pull request Feb 6, 2025
…tations__` and `__type_params__` in free-threading build (python#129016)

(cherry picked from commit 55f17b7)
@bedevere-app
Copy link

bedevere-app bot commented Feb 6, 2025

GH-129729 is a backport of this pull request to the 3.13 branch.

@bedevere-app 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
…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
…tations__` and `__type_params__` in free-threading build (python#129016)
terryjreedy pushed a commit that referenced this pull request Feb 24, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants