Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PyType_AddWatcher and friends are not thread safe in free-threading #131544

Open
kumaraditya303 opened this issue Mar 21, 2025 · 4 comments
Open
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-free-threading type-bug An unexpected behavior, bug, or error

Comments

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Mar 21, 2025

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

PyType_AddWatcher:

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:

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.

@colesbury
Copy link
Contributor

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

@colesbury
Copy link
Contributor

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

@picnixz picnixz added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Mar 22, 2025
@corona10
Copy link
Member

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-free-threading type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants