`PyType_AddWatcher` and friends are not thread safe in free-threading

[kumaraditya303]

PyType_AddWatcher and PyType_ClearWatcher are not thread safe as it modifies the interp's type watchers non-atomically.

PyType_AddWatcher:

cpython/Objects/typeobject.c

Lines 949 to 963 in d3f6063

	PyType_AddWatcher(PyType_WatchCallback callback)
	{
	PyInterpreterState *interp = _PyInterpreterState_GET();
	// start at 1, 0 is reserved for cpython optimizer
	for (int i = 1; i < TYPE_MAX_WATCHERS; i++) {
	if (!interp->type_watchers[i]) {
	interp->type_watchers[i] = callback;
	return i;
	}
	}
	PyErr_SetString(PyExc_RuntimeError, "no more type watcher IDs available");
	return -1;
	}

PyType_ClearWatcher:

cpython/Objects/typeobject.c

Lines 980 to 989 in d3f6063

	PyType_ClearWatcher(int watcher_id)
	{
	PyInterpreterState *interp = _PyInterpreterState_GET();
	if (validate_watcher_id(interp, watcher_id) < 0) {
	return -1;
	}
	interp->type_watchers[watcher_id] = NULL;
	return 0;
	}

I think adding and removing of type watchers is a rare event so maybe instead of adding atomics or locks it would be better to change them to use stop-the-world pause event.

Linked PRs

• gh-131544: Update docs for PyType_AddWatcher #132015
• [3.13] gh-131544: Update docs for PyType_AddWatcher (gh-132015) #132019

[colesbury]

I think it may be fine to just document that PyType_AddWatcher() isn't thread-safe and should be called at start up.

[colesbury]

I'm pretty sure that Cinder does this already (@mpage ?) and I'm not sure there are any other current users of the API:

https://grep.app/search?q=PyType_AddWatcher

[corona10]

so maybe instead of adding atomics or locks it would be better to change them to use stop-the-world pause event.

Hmm, I think that we can use CAS operation at PyType_AddWatcher and use atomic store operation for PyType_ClearWatcher will be fine if we consider that PyType_AddWatcher only 8.

Is there any concerns to add extra atomic operations for those APIs at free-threading?

[colesbury]

If you rely on atomics, you have to use them for every access, so also every read of interp->type_watchers. This adds a bunch of complexity and I don't think there's a good reason to do it.