gh-135607: Remove null checking of weakref list in dealloc of extension modules

[xuantengh]

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

[ZeroIntensity]

Please add a test case and blurb entry.

[vstinner]

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

[colesbury]

A few comments:

1. I don't think we need to add test cases for this kind of thing.
2. 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]

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.

[kumaraditya303]

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

[colesbury]

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

[xuantengh]

So next we should:

1. remove the introduced test
2. 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]

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]

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]

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.)