gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF#134377

corona10 · 2025-05-20T18:26:27Z

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

colesbury · 2025-05-20T19:00:58Z

Objects/obmalloc.c

+_PyObject_XSetRefDelayed(PyObject **ptr, PyObject *value)
+{
+ PyObject *old = *ptr;
+ FT_ATOMIC_STORE_PTR_RELEASE(*ptr, Py_NewRef(value));


The default build implementation in pycore_pymem.h doesn't include the Py_NewRef().
My preference is to keep the Py_NewRef(value) in the callers, i.e.:
_PyObject_XSetRefDelayed(dictptr, Py_NewRef(value));
to match the semantics of Py_XSETREF.

corona10 · 2025-05-21T04:24:07Z

Interesting... CI passed in my local but not in the CI.. :(

corona10 · 2025-05-21T04:32:52Z

Ah okay..

test_aclose (test.test_asyncgen.TestUnawaitedWarnings.test_aclose) ... Assertion failed: (!_Py_IsImmortal(op)), function _Py_ExplicitMergeRefcount, file object.c, line 467.

markshannon · 2025-05-21T09:18:26Z

Why do we need _PyObject_XSetRefDelayed? It looks like all uses of it are wrapped in critical sections where the old Py_XSETREF would work.

Also, what is "delayed"? Either choose a clearer name, or add an explanatory comment in the header file.

…TREF

corona10 · 2025-06-07T13:40:56Z

Why do we need _PyObject_XSetRefDelayed? It looks like all uses of it are wrapped in critical sections where the old Py_XSETREF would work.

IIUC, the critical section is for the root object to prevent other threads from mutating the child object, and a delayed reference to prevent use-after-free from other threads that reference the child object.

Objects/obmalloc.c

Objects/genobject.c

corona10 · 2025-06-12T14:42:41Z

Gentle ping @kumaraditya303 @vstinner

Objects/typeobject.c

Objects/genobject.c

vstinner · 2025-06-16T14:24:43Z

Objects/genobject.c

 }
- Py_XSETREF(op->gi_name, Py_NewRef(value));
+ Py_BEGIN_CRITICAL_SECTION(self);
+ // To prevent use-after-free from other threads that reference the gi_name.


Maybe add gh-133931: prefix to these comments.

vstinner · 2025-06-16T14:26:36Z

Include/internal/pycore_pymem.h

+#ifdef Py_GIL_DISABLED
+// Same as `Py_XSETREF` but in free-threading, it stores the object atomically
+// and queues the old object to be decrefed at a safe point using QSBR.
+PyAPI_FUNC(void) _PyObject_XSetRefDelayed(PyObject **p_obj, PyObject *obj);


Why not adding this function to pycore_object.h header instead?

Because _PyObject_XDecRefDelayed is also declared here?

I think it's fine to place _PyObject_XDecRefDelayed and _PyObject_XSetRefDelayed in pycore_object.h instead if you prefer. (I agree that they should probably be in the same header as each other).

efimov-mikhail · 2025-06-16T15:02:01Z

Objects/typeobject.c

 }
 Py_BEGIN_CRITICAL_SECTION(obj);
- // To prevent use-after-free from other threads that reference the __dict__
+ // gh-133931: To prevent use-after-free from other threads that reference


Is it correct issue reference for __dict__ too?

colesbury

Looks good - a few comments below

Objects/typeobject.c

Objects/genobject.c

Objects/obmalloc.c

colesbury · 2025-06-16T15:37:12Z

Include/internal/pycore_pymem.h

+#ifdef Py_GIL_DISABLED
+// Same as `Py_XSETREF` but in free-threading, it stores the object atomically
+// and queues the old object to be decrefed at a safe point using QSBR.
+PyAPI_FUNC(void) _PyObject_XSetRefDelayed(PyObject **p_obj, PyObject *obj);


I think it's fine to place _PyObject_XDecRefDelayed and _PyObject_XSetRefDelayed in pycore_object.h instead if you prefer. (I agree that they should probably be in the same header as each other).

efimov-mikhail

IMO, it's better to use "gh-133980" comment for __dict__ and "gh-133931" comment for gi_name and gi_qualname.
Moreover, we've moved _PyObject_XSetRefDelayed and so we have to make little changes in includes.

Objects/genobject.c

efimov-mikhail

LGTM

Objects/obmalloc.c

vstinner

LGTM

…TREF (pythongh-134377)

bedevere-appbot mentioned this pull request May 20, 2025
Make generator methods gen_set_name and gen_set_qualname thread-safe in free-threaded builds #133931
Closed

corona10 added topic-free-threading skip news labels May 20, 2025

corona10 marked this pull request as ready for review May 20, 2025 18:28

corona10 requested review from ericsnowcurrently and markshannon as code owners May 20, 2025 18:28

bedevere-appbot added the awaiting core review label May 20, 2025

corona10 requested a review from colesbury May 20, 2025 18:28

corona10 marked this pull request as draft May 20, 2025 19:00

bedevere-appbot removed the awaiting core review label May 20, 2025

colesbury reviewed May 20, 2025
View reviewed changes

corona10 force-pushed the gh-133931 branch from bf0adb9 to 7156814Compare May 21, 2025 04:48

corona10 changed the title ~~gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF~~[WIP] gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREFMay 21, 2025

corona10 added 5 commits June 7, 2025 21:50

pythongh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSE…
09baa04
…TREF

fix
1ee0825

Address code review
753df7a

fix
a168de0

fix issue
e7f6d01

corona10 force-pushed the gh-133931 branch from 7156814 to e7f6d01Compare June 7, 2025 13:00

corona10 added 2 commits June 7, 2025 22:15

nit
6287322

Add comment
12a7016

corona10 changed the title ~~[WIP] gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF~~gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREFJun 7, 2025

corona10 marked this pull request as ready for review June 7, 2025 13:34

bedevere-appbot added the awaiting core review label Jun 7, 2025

corona10 requested a review from colesbury June 7, 2025 13:42

kumaraditya303 reviewed Jun 10, 2025
View reviewed changes

Objects/obmalloc.c Outdated Show resolvedHide resolved

kumaraditya303 reviewed Jun 11, 2025
View reviewed changes

Objects/genobject.cShow resolvedHide resolved

corona10 added 2 commits June 11, 2025 22:07

Use FT_ATOMIC_LOAD_PTR_ACQUIRE
e65eb1f

fix
4dbb0a7

corona10 requested a review from kumaraditya303 June 11, 2025 13:46

vstinner reviewed Jun 12, 2025
View reviewed changes

Objects/typeobject.cShow resolvedHide resolved
Objects/genobject.cShow resolvedHide resolved
Objects/genobject.cShow resolvedHide resolved

Address code review
d0ba9c9

corona10 requested a review from vstinner June 15, 2025 06:32

vstinner reviewed Jun 16, 2025
View reviewed changes

Address code review
ae5b6a4

efimov-mikhail reviewed Jun 16, 2025
View reviewed changes

colesbury reviewed Jun 16, 2025
View reviewed changes

Address code review
1651fa3

corona10 requested a review from colesbury June 17, 2025 02:48

efimov-mikhail reviewed Jun 17, 2025
View reviewed changes

Objects/genobject.c Outdated Show resolvedHide resolved
Objects/genobject.c Outdated Show resolvedHide resolved
Objects/genobject.c Outdated Show resolvedHide resolved
Objects/genobject.cShow resolvedHide resolved

Address code review
8d309dc

efimov-mikhail approved these changes Jun 17, 2025
View reviewed changes

kumaraditya303 approved these changes Jun 17, 2025
View reviewed changes

bedevere-appbot added awaiting merge and removed awaiting core review labels Jun 17, 2025

vstinner reviewed Jun 17, 2025
View reviewed changes

Objects/obmalloc.cShow resolvedHide resolved

corona10 requested a review from vstinner June 17, 2025 14:25

vstinner approved these changes Jun 17, 2025
View reviewed changes

corona10 merged commit 52be7f4 into python:mainJun 17, 2025
42 checks passed

bedevere-appbot removed the awaiting merge label Jun 17, 2025

lkollar pushed a commit to lkollar/cpython that referenced this pull request Jun 19, 2025
pythongh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSE…
7b9df0e
…TREF (pythongh-134377)

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
pythongh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSE…
900e5a9
…TREF (pythongh-134377)

taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
pythongh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSE…
1ab33b9
…TREF (pythongh-134377)

Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
pythongh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSE…
aeaf00d
…TREF (pythongh-134377)

Uh oh!

gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF#134377

gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF #134377

Uh oh!

Conversation

corona10 commented May 20, 2025• edited by bedevere-app botLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesburyMay 20, 2025

Choose a reason for hiding this comment

Uh oh!

corona10 commented May 21, 2025

Uh oh!

corona10 commented May 21, 2025

Uh oh!

markshannon commented May 21, 2025

Uh oh!

corona10 commented Jun 7, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corona10 commented Jun 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinnerJun 16, 2025

Choose a reason for hiding this comment

Uh oh!

vstinnerJun 16, 2025

Choose a reason for hiding this comment

Uh oh!

corona10Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

colesburyJun 16, 2025

Choose a reason for hiding this comment

Uh oh!

efimov-mikhailJun 16, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colesburyJun 16, 2025

Choose a reason for hiding this comment

Uh oh!

efimov-mikhail left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efimov-mikhail left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

corona10 commented May 20, 2025•
edited by bedevere-app bot
Loading

corona10 commented Jun 7, 2025•
edited
Loading

efimov-mikhailJun 16, 2025•
edited
Loading

efimov-mikhail left a comment •
edited
Loading

efimov-mikhail left a comment •
edited
Loading