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

Co-authored-by: Kumar Aditya <kumaraditya@python.org>

Include/internal/pycore_pymem.h

Co-authored-by: Kumar Aditya <kumaraditya@python.org>

Objects/obmalloc.c

Include/internal/pycore_pymem.h

Co-authored-by: Victor Stinner <vstinner@python.org>

efimov-mikhail · 2025-06-10T13:17:34Z

Could someone please explain to me, why _PyObject_XDecRefDelayed is called under critical section in this PR (and some others), but in Objects/dictobject.c decref_maybe_delay function is called after critical section, see

cpython/Objects/dictobject.c

Line 7381 in 598aa7c

decref_maybe_delay((PyObject *)dict, true);

Which one is more correct?

efimov-mikhail · 2025-06-10T20:22:26Z

@corona10
FYI, I've created some tests related to these changes.
It's located here:
efimov-mikhail@8677625
Those tests crashes the interpeter w/o such changes and works fine w/ them.
It will be great if you add those test to your PR (with some changes if needed, of course).

Objects/genobject.c

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

Gentle ping @kumaraditya303 @vstinner

vstinner · 2025-06-12T16:12:05Z

Objects/typeobject.c

@@ -3967,13 +3967,7 @@ _PyObject_SetDict(PyObject *obj, PyObject *value)
        return -1;
    }
    Py_BEGIN_CRITICAL_SECTION(obj);


Would you mind adding a comment explaining why XDecRefDelayed() is needed here?

vstinner · 2025-06-12T16:12:25Z

Objects/genobject.c

@@ -740,7 +745,9 @@ gen_set_qualname(PyObject *self, PyObject *value, void *Py_UNUSED(ignored))
                        "__qualname__ must be set to a string object");
        return -1;
    }
-    Py_XSETREF(op->gi_qualname, Py_NewRef(value));
+    Py_BEGIN_CRITICAL_SECTION(self);


Would you mind adding a comment explaining why XDecRefDelayed() is needed here? The qualname should be a simple Python str object, no?

Even with simple str objects there can be a crash w/o critical session. What exact way of fixing you're proposing?

I'm not proposing anything, I don't understand well the purpose of XDecRefDelayed(). That's why I'm asking for a comment :-)

IMO, it will be great to have some best practices for free-threading build in InternalDocs. This case seems like a standard one, and some standard solutions will be great.

For this exact case we just need some kind of locking for synchronization reading/writing. And there's one way for this.

vstinner · 2025-06-12T16:12:36Z

Objects/genobject.c

@@ -718,15 +720,18 @@ gen_set_name(PyObject *self, PyObject *value, void *Py_UNUSED(ignored))
                        "__name__ must be set to a string object");
        return -1;
    }
-    Py_XSETREF(op->gi_name, Py_NewRef(value));
+    Py_BEGIN_CRITICAL_SECTION(self);


Would you mind adding a comment explaining why XDecRefDelayed() is needed here? The name should be a simple Python str object, no?

bedevere-app bot mentioned this pull request May 20, 2025

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

Open

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-app bot 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-app bot 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 7156814 Compare 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_XSETREF May 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 e7f6d01 Compare 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_XSETREF Jun 7, 2025

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

bedevere-app bot 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 resolved Hide resolved

Update Objects/obmalloc.c

5f42224

Co-authored-by: Kumar Aditya <kumaraditya@python.org>

corona10 requested a review from kumaraditya303 June 10, 2025 11:18

kumaraditya303 reviewed Jun 10, 2025

View reviewed changes

Include/internal/pycore_pymem.h Outdated Show resolved Hide resolved

kumaraditya303 reviewed Jun 10, 2025

View reviewed changes

Include/internal/pycore_pymem.h Outdated Show resolved Hide resolved

Update Include/internal/pycore_pymem.h

388f0ca

Co-authored-by: Kumar Aditya <kumaraditya@python.org>

kumaraditya303 reviewed Jun 10, 2025

View reviewed changes

Objects/obmalloc.c Outdated Show resolved Hide resolved

vstinner reviewed Jun 10, 2025

View reviewed changes

Include/internal/pycore_pymem.h Outdated Show resolved Hide resolved

corona10 and others added 2 commits June 10, 2025 22:05

Update Include/internal/pycore_pymem.h

8220367

Co-authored-by: Victor Stinner <vstinner@python.org>

Address code review

c82e0fb

corona10 added 3 commits June 11, 2025 12:24

Add test

f99c8f1

fix

5dc24a3

fix

99d2718

corona10 requested review from kumaraditya303 and vstinner June 11, 2025 03:46

Merge remote-tracking branch 'upstream/main' into pythongh-133931

7dc6892

kumaraditya303 reviewed Jun 11, 2025

View reviewed changes

Objects/genobject.c Show resolved Hide 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

Uh oh!

gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF #134377

Are you sure you want to change the base?

gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF #134377

Conversation

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

Uh oh!

Uh oh!

colesbury May 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!

Uh oh!

Uh oh!

Uh oh!

efimov-mikhail commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efimov-mikhail commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

corona10 commented Jun 12, 2025

Uh oh!

vstinner Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

vstinner Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

efimov-mikhail Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

vstinner Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

efimov-mikhail Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

vstinner Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corona10 commented May 20, 2025 •

edited by bedevere-app bot

Loading

corona10 commented Jun 7, 2025 •

edited

Loading

efimov-mikhail commented Jun 10, 2025 •

edited

Loading

efimov-mikhail commented Jun 10, 2025 •

edited

Loading