gh-135607: Remove null checking of weakref list in dealloc of extension modules and objects #135614

xuantengh · 2025-06-17T13:28:05Z

Issue: data race with extension modules checking for empty weaklist #135607

ZeroIntensity

Please add a test case and blurb entry.

Misc/NEWS.d/next/Core_and_Builtins/2025-06-17-22-34-58.gh-issue-135607.ucsLVu.rst

Lib/test/test_weakref.py

vstinner · 2025-06-18T15:55:15Z

Lib/test/test_weakref.py

+    @threading_helper.requires_working_threading()
+    @unittest.skipUnless(support.Py_GIL_DISABLED, 'only used under free-threaded build')
+    def test_module_weakref(self):
+        # gh-135607: Avoid potential races on module weaklist under free-threaded build


This is unrelated to module_dealloc(). I'm not convinced that this test is relevant.

colesbury

A few comments:

I don't think we need to add test cases for this kind of thing.
I think you should do this project wide. There's a bunch of places with this pattern and we don't need 20 separate one line PRs.

There's some project "cost" for adding code, including new tests both in maintenance, readability, and CI time. These sorts of tests are so specific that they are unlikely to catch any future bugs; I don't think they are worthwhile.

colesbury · 2025-06-18T17:44:00Z

Objects/moduleobject.c

@@ -826,8 +826,7 @@ module_dealloc(PyObject *self)
    if (verbose && m->md_name) {
        PySys_FormatStderr("# destroy %U\n", m->md_name);
    }
-    if (m->md_weaklist != NULL)
-        PyObject_ClearWeakRefs((PyObject *) m);
+    PyObject_ClearWeakRefs(self);


We can also write:

if (FT_ATOMIC_LOAD_PTR(m->md_weaklist != NULL)) PyObject_ClearWeakRefs((PyObject *) m);

Which is a less risky change in the sense that it doesn't change the default build at all.

I think we should remove the null check for both builds, it is redundant and many modules already doesn't check it like _asyncio and _thread

That's fine, just make sure it doesn't introduce a perf regression

xuantengh · 2025-06-19T01:03:15Z

So next we should:

remove the introduced test
find all patterns with null checks followed by PyObject_ClearWeakRefs, e.g.:

cpython/Modules/_threadmodule.c

Lines 1534 to 1542 in 725da50

    
           static void 
        
           local_dealloc(PyObject *op) 
        
           { 
        
               localobject *self = localobject_CAST(op); 
        
               /* Weakrefs must be invalidated right now, otherwise they can be used 
        
                  from code called below, which is very dangerous since Py_REFCNT(self) == 0 */ 
        
               if (self->weakreflist != NULL) { 
        
                   PyObject_ClearWeakRefs(op); 
        
               }

cpython/Modules/_threadmodule.c

Lines 1364 to 1370 in 725da50

    
           static void 
        
           localdummy_dealloc(PyObject *op) 
        
           { 
        
               localdummyobject *self = localdummyobject_CAST(op); 
        
               if (self->weakreflist != NULL) { 
        
                   PyObject_ClearWeakRefs(op); 
        
               }

rhettinger · 2025-06-19T03:10:27Z

Is this going to create an additional cost for deallocating every instance of every extension module type even if weakrefs are never used?

Does this need a macro with an #ifdef so as to not impact the performance of a no-gil build?

xuantengh · 2025-06-19T09:20:20Z

Is this going to create an additional cost for deallocating every instance of every extension module type even if weakrefs are never used?

For objects without weakref, the "additional cost" would be:

cpython/Objects/weakrefobject.c

Lines 1010 to 1016 in 140731f

    
           if (object == NULL 
        
               || !_PyType_SUPPORTS_WEAKREFS(Py_TYPE(object)) 
        
               || Py_REFCNT(object) != 0) 
        
           { 
        
               PyErr_BadInternalCall(); 
        
               return; 
        
           }

which IMHO is negligible?

ZeroIntensity · 2025-06-19T12:39:05Z

Is this going to create an additional cost for deallocating every instance of every extension module type even if weakrefs are never used?

PyObject_ClearWeakRefs does the same check, just with an atomic load under free-threading, and a non-atomic load under the default build. I don't think we'll see any performance difference.

There's some project "cost" for adding code, including new tests both in maintenance, readability, and CI time. These sorts of tests are so specific that they are unlikely to catch any future bugs; I don't think they are worthwhile.

Ok, for clarification, when do you want free-threading changes to include tests? I think that a lot of them could go under the "trivial" umbrella, but we've still added tests for them in the past. (It makes sense to me to omit tests now, because we're changing lots of different parts of the codebase, but I don't totally see why it wasn't a good idea for the original one-line change.)

rhettinger · 2025-06-19T16:10:22Z

I recommend using a macro that restores the NULL check for nogil builds. ISTM there is no reason to hurt the nogil builds, even slightly.

picnixz

Ok I haven't comment all occurrences, but avoid re-casting to PyObject* something that has been cast to the object type. Just re-use the original argument.

Modules/_testcapi/heaptype.c

Objects/classobject.c

Objects/funcobject.c

Objects/genericaliasobject.c

Objects/methodobject.c

vstinner

LGTM

Modules/_threadmodule.c

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

miss-islington-app · 2025-06-30T11:14:35Z

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

miss-islington-app · 2025-06-30T11:14:38Z

Sorry, @xuantengh and @kumaraditya303, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker b1056c2a446b43452e457d5fd5f1bde66afd3883 3.14

miss-islington-app · 2025-06-30T11:14:42Z

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 b1056c2a446b43452e457d5fd5f1bde66afd3883 3.13

…xtension modules and objects (python#135614) Co-authored-by: Kumar Aditya <kumaraditya@python.org> Co-authored-by: Victor Stinner <vstinner@python.org>

bedevere-app · 2025-06-30T13:23:08Z

GH-136119 is a backport of this pull request to the 3.14 branch.

…xtension modules and objects (python#135614) Co-authored-by: Kumar Aditya <kumaraditya@python.org> Co-authored-by: Victor Stinner <vstinner@python.org> (cherry picked from commit b1056c2)

…extension modules and objects (#135614) (#136119) gh-135607: remove null checking of weakref list in dealloc of extension modules and objects (#135614) (cherry picked from commit b1056c2) Co-authored-by: Xuanteng Huang <44627253+xuantengh@users.noreply.github.com> Co-authored-by: Kumar Aditya <kumaraditya@python.org>

bedevere-app · 2025-06-30T14:07:15Z

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

…xtension modules and objects (python#135614) Co-authored-by: Kumar Aditya <kumaraditya@python.org> Co-authored-by: Victor Stinner <vstinner@python.org> (cherry picked from commit b1056c2)

…extension modules and objects (#135614) (#136126) gh-135607: remove null checking of weakref list in dealloc of extension modules and objects (#135614) (cherry picked from commit b1056c2) Co-authored-by: Xuanteng Huang <44627253+xuantengh@users.noreply.github.com> Co-authored-by: Kumar Aditya <kumaraditya@python.org>

vstinner · 2025-07-01T09:27:58Z

Thanks @xuantengh. I backported your change manually to 3.13 and 3.14 to fix merge conflicts.

xuantengh · 2025-07-01T15:22:12Z

Thanks @xuantengh. I backported your change manually to 3.13 and 3.14 to fix merge conflicts.

Thanks for your efforts!

xuantengh requested a review from ericsnowcurrently as a code owner June 17, 2025 13:28

bedevere-app bot added the awaiting review label Jun 17, 2025

bedevere-app bot mentioned this pull request Jun 17, 2025

data race with extension modules checking for empty weaklist #135607

Closed

ZeroIntensity added topic-free-threading needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jun 17, 2025

ZeroIntensity reviewed Jun 17, 2025

View reviewed changes

xuantengh requested a review from kumaraditya303 as a code owner June 17, 2025 15:05

ZeroIntensity reviewed Jun 17, 2025

View reviewed changes

Misc/NEWS.d/next/Core_and_Builtins/2025-06-17-22-34-58.gh-issue-135607.ucsLVu.rst Outdated Show resolved Hide resolved

ZeroIntensity reviewed Jun 17, 2025

View reviewed changes

Misc/NEWS.d/next/Core_and_Builtins/2025-06-17-22-34-58.gh-issue-135607.ucsLVu.rst Outdated Show resolved Hide resolved

Lib/test/test_weakref.py Outdated Show resolved Hide resolved

xuantengh force-pushed the remove-weakref-null-checking branch 2 times, most recently from 9a6819c to 68528e6 Compare June 18, 2025 08:34

vstinner reviewed Jun 18, 2025

View reviewed changes

Lib/test/test_weakref.py Outdated Show resolved Hide resolved

vstinner reviewed Jun 18, 2025

View reviewed changes

colesbury reviewed Jun 18, 2025

View reviewed changes

xuantengh changed the title ~~gh-135607: Remove null checking of weakref list in module dealloc~~ gh-135607: Remove null checking of weakref list in dealloc of extension modules Jun 19, 2025

xuantengh force-pushed the remove-weakref-null-checking branch from bc28bd7 to 9024079 Compare June 19, 2025 02:23

xuantengh requested review from rhettinger, berkerpeksag and erlend-aasland as code owners June 19, 2025 02:23

xuantengh requested review from markshannon and JelleZijlstra as code owners June 19, 2025 09:05

xuantengh force-pushed the remove-weakref-null-checking branch from 22860cd to 54fa61e Compare June 19, 2025 09:30

picnixz reviewed Jun 19, 2025

View reviewed changes

xuantengh force-pushed the remove-weakref-null-checking branch from 62d1912 to b636d07 Compare June 20, 2025 02:46

xuantengh added 6 commits June 29, 2025 10:13

update news

c493605

avoid cast

5949264

use macros to clear weak refs

a3a789a

add refcnt check in FT_CLEAR_WEAKREFS

398e9f1

add header comment

190af33

add assert for gil build

fd1fa03

xuantengh force-pushed the remove-weakref-null-checking branch from a344210 to fd1fa03 Compare June 29, 2025 02:37

kumaraditya303 approved these changes Jun 29, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Jun 29, 2025

vstinner approved these changes Jun 30, 2025

View reviewed changes

Modules/_threadmodule.c Outdated Show resolved Hide resolved

Update Modules/_threadmodule.c

b74914d

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

kumaraditya303 enabled auto-merge (squash) June 30, 2025 09:45

kumaraditya303 merged commit b1056c2 into python:main Jun 30, 2025
71 of 73 checks passed

bedevere-app bot removed the awaiting merge label Jun 30, 2025

xuantengh deleted the remove-weakref-null-checking branch June 30, 2025 11:27

bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Jun 30, 2025

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 30, 2025

Uh oh!

gh-135607: Remove null checking of weakref list in dealloc of extension modules and objects #135614

gh-135607: Remove null checking of weakref list in dealloc of extension modules and objects #135614

Conversation

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

Uh oh!

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

colesbury Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

kumaraditya303 Jun 18, 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 Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

rhettinger commented Jun 19, 2025

Uh oh!

xuantengh commented Jun 19, 2025

Uh oh!

ZeroIntensity commented Jun 19, 2025

Uh oh!

rhettinger commented Jun 19, 2025

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miss-islington-app bot commented Jun 30, 2025

Uh oh!

miss-islington-app bot commented Jun 30, 2025

Uh oh!

miss-islington-app bot commented Jun 30, 2025

Uh oh!

bedevere-app bot commented Jun 30, 2025

Uh oh!

bedevere-app bot commented Jun 30, 2025

Uh oh!

vstinner commented Jul 1, 2025

Uh oh!

xuantengh commented Jul 1, 2025

Uh oh!

Uh oh!

xuantengh commented Jun 17, 2025 •

edited by bedevere-app bot

Loading

kumaraditya303 Jun 18, 2025 •

edited

Loading

xuantengh commented Jun 19, 2025 •

edited

Loading