gh-135607: Remove null checking of weakref list in module dealloc #135614
Conversation
ZeroIntensity
added
topic-free-threading
needs backport to 3.13
Misc/NEWS.d/next/Core_and_Builtins/2025-06-17-22-34-58.gh-issue-135607.ucsLVu.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Core_and_Builtins/2025-06-17-22-34-58.gh-issue-135607.ucsLVu.rst
Outdated
Show resolved
Hide resolved
xuantengh
force-pushed
the
remove-weakref-null-checking
branch
from
d375d34 to
9a6819c
Compare
xuantengh
force-pushed
the
remove-weakref-null-checking
branch
from
9a6819c to
68528e6
Compare
| @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 |
There was a problem hiding this comment.
This is unrelated to module_dealloc(). I'm not convinced that this test is relevant.
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
That's fine, just make sure it doesn't introduce a perf regression
Uh oh!
There was an error while loading. Please reload this page.