diff --git a/Doc/c-api/contextvars.rst b/Doc/c-api/contextvars.rst index b7c6550ff34aac..94ee3df83b9876 100644 --- a/Doc/c-api/contextvars.rst +++ b/Doc/c-api/contextvars.rst @@ -108,6 +108,11 @@ Context object management functions: In case of error (e.g. no more watcher IDs available), return ``-1`` and set an exception. + This function may be called concurrently with other calls to + :c:func:`PyContext_AddWatcher` and :c:func:`PyContext_ClearWatcher`, and + while other threads are switching contexts. Each successful call returns a + distinct watcher ID. + .. versionadded:: 3.14 .. c:function:: int PyContext_ClearWatcher(int watcher_id) @@ -117,6 +122,14 @@ Context object management functions: Return ``0`` on success, or ``-1`` and set an exception on error (e.g. if the given *watcher_id* was never registered.) + After this returns, the callback is not selected for context switches that + begin later. It does not wait for callbacks already in progress: in the + :term:`free-threaded ` build a callback selected on another + thread just before the watcher was cleared may still be running, or may + still be about to run. State used by the callback must therefore remain + valid until the caller has established, by its own means, that no + invocation can still be in flight. + .. versionadded:: 3.14 .. c:type:: PyContextEvent @@ -135,6 +148,11 @@ Context object management functions: Context object watcher callback function. The object passed to the callback is event-specific; see :c:type:`PyContextEvent` for details. + The callback is invoked by the thread whose current context changed. In the + :term:`free-threaded ` build several threads may therefore + run the same callback at the same time, and the callback is responsible for + synchronizing any state it shares between those invocations. + If the callback returns with an exception set, it must return ``-1``; this exception will be printed as an unraisable exception using :c:func:`PyErr_FormatUnraisable`. Otherwise it should return ``0``. diff --git a/Lib/test/test_free_threading/test_context_watcher.py b/Lib/test/test_free_threading/test_context_watcher.py new file mode 100644 index 00000000000000..855575f0e440ef --- /dev/null +++ b/Lib/test/test_free_threading/test_context_watcher.py @@ -0,0 +1,69 @@ +import contextvars +import unittest + +from test.support import import_helper, threading_helper + +_testcapi = import_helper.import_module("_testcapi") + +ITERS = 1000 +NTHREADS = 8 + + +@threading_helper.requires_working_threading() +class TestContextWatcherThreadSafety(unittest.TestCase): + # gh-155619: the per-interpreter context watcher registry + # (PyInterpreterState.context_watchers plus the active_context_watchers + # bitmask) is read and written without synchronization. Every callback + # used here is a no-op, so any failure is CPython's registry rather than + # the test's own bookkeeping. + + def test_concurrent_add_clear_watchers(self): + """Race AddWatcher against ClearWatcher. + + Both scan/modify the callback array and do a read-modify-write on + the bitmask, so two callers can claim the same slot or lose a + bitmask update. + """ + results = [] + + def worker(): + for _ in range(ITERS): + try: + wid = _testcapi.add_noop_context_watcher() + except RuntimeError: + continue # all CONTEXT_MAX_WATCHERS slots taken + self.assertGreaterEqual(wid, 0) + results.append(wid) + _testcapi.clear_context_watcher(wid) + + threading_helper.run_concurrently(worker, NTHREADS) + self.assertGreater(len(results), 0) + + def test_clear_watcher_races_notification(self): + """Race ClearWatcher against notification. + + notify_context_watchers() snapshots the bitmask, then loads + context_watchers[i], which ClearWatcher may have set to NULL in + between. A debug build reaches assert(cb != NULL); a release build + may call a NULL or stale function pointer. + """ + def switcher(): + # Context().run() enters and exits a context, and each switch + # dispatches to every active watcher on this thread. + for _ in range(ITERS): + contextvars.Context().run(lambda: None) + + def churner(): + for _ in range(ITERS): + try: + wid = _testcapi.add_noop_context_watcher() + except RuntimeError: + continue + _testcapi.clear_context_watcher(wid) + + workers = [switcher, churner] * (NTHREADS // 2) + threading_helper.run_concurrently(workers) + + +if __name__ == "__main__": + unittest.main() diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-08-17-16-08-05.gh-issue-155619.Qw8Ftz.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-08-17-16-08-05.gh-issue-155619.Qw8Ftz.rst new file mode 100644 index 00000000000000..e11886b911197a --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-08-17-16-08-05.gh-issue-155619.Qw8Ftz.rst @@ -0,0 +1,9 @@ +Fix data races in the per-interpreter context watcher registry on +:term:`free-threaded ` builds. :c:func:`PyContext_AddWatcher` +now claims a slot with an atomic compare-and-swap, so concurrent callers can +no longer select the same slot or receive the same watcher ID, and +notification loads each callback with an acquire load and skips one that +:c:func:`PyContext_ClearWatcher` has retired concurrently. Previously a +context switch racing with :c:func:`PyContext_ClearWatcher` could reach an +assertion in a debug build, or call a ``NULL`` function pointer in a release +build. diff --git a/Modules/_testcapi/watchers.c b/Modules/_testcapi/watchers.c index 71cdc54009017a..6398abb9de50ed 100644 --- a/Modules/_testcapi/watchers.c +++ b/Modules/_testcapi/watchers.c @@ -733,6 +733,19 @@ clear_context_watcher(PyObject *self, PyObject *watcher_id) Py_RETURN_NONE; } +/* Register a watcher whose callback touches no shared state, so that a + concurrency test exercises only CPython's watcher registry and cannot + race on the test harness's own bookkeeping. */ +static PyObject * +add_noop_context_watcher(PyObject *Py_UNUSED(self), PyObject *Py_UNUSED(args)) +{ + int watcher_id = PyContext_AddWatcher(noop_context_event_handler); + if (watcher_id < 0) { + return NULL; + } + return PyLong_FromLong(watcher_id); +} + static PyObject * clear_context_stack(PyObject *Py_UNUSED(self), PyObject *Py_UNUSED(args)) { @@ -864,6 +877,7 @@ static PyMethodDef test_methods[] = { // Code object watchers. {"add_context_watcher", add_context_watcher, METH_O, NULL}, + {"add_noop_context_watcher", add_noop_context_watcher, METH_NOARGS, NULL}, {"clear_context_watcher", clear_context_watcher, METH_O, NULL}, {"clear_context_stack", clear_context_stack, METH_NOARGS, NULL}, {"get_context_switches", get_context_switches, METH_O, NULL}, diff --git a/Python/context.c b/Python/context.c index d48543c9e023a6..64f44dc7dfe319 100644 --- a/Python/context.c +++ b/Python/context.c @@ -163,14 +163,19 @@ notify_context_watchers(PyThreadState *ts, PyContextEvent event, PyObject *ctx) assert(Py_REFCNT(ctx) > 0); PyInterpreterState *interp = ts->interp; assert(interp->_initialized); - uint8_t bits = interp->active_context_watchers; + uint8_t bits = FT_ATOMIC_LOAD_UINT8_RELAXED(interp->active_context_watchers); int i = 0; while (bits) { assert(i < CONTEXT_MAX_WATCHERS); if (bits & 1) { - PyContext_WatchCallback cb = interp->context_watchers[i]; - assert(cb != NULL); - if (cb(event, ctx) < 0) { + // Pairs with the release store in PyContext_AddWatcher(), so that + // observing the active bit implies observing the callback pointer + // (and whatever the registering thread published before it). + PyContext_WatchCallback cb = + FT_ATOMIC_LOAD_PTR_ACQUIRE(interp->context_watchers[i]); + // PyContext_ClearWatcher() may have cleared this slot after the + // bitmask was read, so the callback can legitimately be NULL. + if (cb != NULL && cb(event, ctx) < 0) { PyErr_FormatUnraisable( "Exception ignored in %s watcher callback for %R", context_event_name(event), ctx); @@ -182,6 +187,30 @@ notify_context_watchers(PyThreadState *ts, PyContextEvent event, PyObject *ctx) } +static inline void +set_context_watcher_bit(PyInterpreterState *interp, int watcher_id) +{ + uint8_t bit = (uint8_t)(1 << watcher_id); +#ifdef Py_GIL_DISABLED + (void)_Py_atomic_or_uint8(&interp->active_context_watchers, bit); +#else + interp->active_context_watchers |= bit; +#endif +} + + +static inline void +clear_context_watcher_bit(PyInterpreterState *interp, int watcher_id) +{ + uint8_t bit = (uint8_t)(1 << watcher_id); +#ifdef Py_GIL_DISABLED + (void)_Py_atomic_and_uint8(&interp->active_context_watchers, (uint8_t)~bit); +#else + interp->active_context_watchers &= (uint8_t)~bit; +#endif +} + + int PyContext_AddWatcher(PyContext_WatchCallback callback) { @@ -189,9 +218,14 @@ PyContext_AddWatcher(PyContext_WatchCallback callback) assert(interp->_initialized); for (int i = 0; i < CONTEXT_MAX_WATCHERS; i++) { - if (!interp->context_watchers[i]) { - interp->context_watchers[i] = callback; - interp->active_context_watchers |= (1 << i); + // Claim the slot with a single atomic step: a plain test-then-store + // lets two threads select the same slot and return the same ID. + // Losing the race means the slot is taken, so just try the next one. + PyContext_WatchCallback expected = NULL; + if (_Py_atomic_compare_exchange_ptr(&interp->context_watchers[i], + &expected, callback)) { + // Publish the callback before advertising the slot as active. + set_context_watcher_bit(interp, i); return i; } } @@ -210,12 +244,16 @@ PyContext_ClearWatcher(int watcher_id) PyErr_Format(PyExc_ValueError, "Invalid context watcher ID %d", watcher_id); return -1; } - if (!interp->context_watchers[watcher_id]) { + if (FT_ATOMIC_LOAD_PTR_RELAXED(interp->context_watchers[watcher_id]) == NULL) { PyErr_Format(PyExc_ValueError, "No context watcher set for ID %d", watcher_id); return -1; } - interp->context_watchers[watcher_id] = NULL; - interp->active_context_watchers &= ~(1 << watcher_id); + // Stop notification selecting this slot before retiring the callback, so + // the window in which a notifier sees the bit but loads NULL is as short + // as possible. That window is benign: notify_context_watchers() skips a + // NULL callback. + clear_context_watcher_bit(interp, watcher_id); + FT_ATOMIC_STORE_PTR_RELEASE(interp->context_watchers[watcher_id], NULL); return 0; }