From cf6491cf149e041adf457be0ab0df87793981b2b Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 7 Feb 2024 19:36:26 -0800 Subject: [PATCH 01/20] Make `_thread.ThreadHandle` thread-safe in free-threaded builds We protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`. Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` block until it is their turn to execute or an earlier operation succeeds. Once an operation has been applied successfully all future operations complete immediately. The `join()` method is now idempotent. It may be called multiple times but the underlying OS thread will only be joined once. After `join()` succeeds, any future calls to `join()` will succeed immediately. The `detach()` method is also idempotent. It may be called multiple times but the underlying OS thread will only be detached once. After `detach()` succeeds, any future calls to `detach()` will succeed immediately. If the handle is being joined, `detach()` blocks until the join completes. --- Lib/test/test_thread.py | 20 +++-- Lib/threading.py | 21 ++---- Modules/_threadmodule.c | 162 +++++++++++++++++++++++++++++++--------- 3 files changed, 145 insertions(+), 58 deletions(-) diff --git a/Lib/test/test_thread.py b/Lib/test/test_thread.py index 931cb4b797e0b2..3db36d77c86078 100644 --- a/Lib/test/test_thread.py +++ b/Lib/test/test_thread.py @@ -189,8 +189,8 @@ def task(): with threading_helper.wait_threads_exit(): handle = thread.start_joinable_thread(task) handle.join() - with self.assertRaisesRegex(ValueError, "not joinable"): - handle.join() + # Subsequent join() calls should succeed + handle.join() def test_joinable_not_joined(self): handle_destroyed = thread.allocate_lock() @@ -255,7 +255,7 @@ def task(): handles.append(handle) start_joinable_thread_returned.release() thread_detached.acquire() - with self.assertRaisesRegex(ValueError, "not joinable"): + with self.assertRaisesRegex(ValueError, "detached and thus cannot be joined"): handle.join() assert len(errors) == 0 @@ -272,7 +272,7 @@ def task(): # detach() returns even though the thread is blocked on lock handle.detach() # join() then cannot be called anymore - with self.assertRaisesRegex(ValueError, "not joinable"): + with self.assertRaisesRegex(ValueError, "detached and thus cannot be joined"): handle.join() lock.release() @@ -283,9 +283,19 @@ def task(): with threading_helper.wait_threads_exit(): handle = thread.start_joinable_thread(task) handle.join() - with self.assertRaisesRegex(ValueError, "not joinable"): + with self.assertRaisesRegex(ValueError, "joined and thus cannot be detached"): handle.detach() + def test_detach_then_detach(self): + def task(): + pass + + with threading_helper.wait_threads_exit(): + handle = thread.start_joinable_thread(task) + handle.detach() + # Subsequent calls to detach() should succeed + handle.detach() + class Barrier: def __init__(self, num_threads): diff --git a/Lib/threading.py b/Lib/threading.py index b6ff00acadd58f..364c45d840cf2b 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -956,14 +956,11 @@ def _after_fork(self, new_ident=None): if self._tstate_lock is not None: self._tstate_lock._at_fork_reinit() self._tstate_lock.acquire() - if self._join_lock is not None: - self._join_lock._at_fork_reinit() else: # This thread isn't alive after fork: it doesn't have a tstate # anymore. self._is_stopped = True self._tstate_lock = None - self._join_lock = None self._handle = None def __repr__(self): @@ -996,8 +993,6 @@ def start(self): if self._started.is_set(): raise RuntimeError("threads can only be started once") - self._join_lock = _allocate_lock() - with _active_limbo_lock: _limbo[self] = self try: @@ -1167,17 +1162,7 @@ def join(self, timeout=None): self._join_os_thread() def _join_os_thread(self): - join_lock = self._join_lock - if join_lock is None: - return - with join_lock: - # Calling join() multiple times would raise an exception - # in one of the callers. - if self._handle is not None: - self._handle.join() - self._handle = None - # No need to keep this around - self._join_lock = None + self._handle.join() def _wait_for_tstate_lock(self, block=True, timeout=-1): # Issue #18808: wait for the thread state to be gone. @@ -1478,6 +1463,10 @@ def __init__(self): with _active_limbo_lock: _active[self._ident] = self + def _join_os_thread(self): + # No ThreadHandle for main thread + pass + # Helper thread-local instance to detect when a _DummyThread # is collected. Not a part of the public API. diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index df02b023012fbd..5e3696f49df1ff 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1,9 +1,9 @@ - /* Thread module */ /* Interface to Sjoerd's portable C thread library */ #include "Python.h" #include "pycore_interp.h" // _PyInterpreterState.threads.count +#include "pycore_lock.h" #include "pycore_moduleobject.h" // _PyModule_GetState() #include "pycore_modsupport.h" // _PyArg_NoKeywords() #include "pycore_pylifecycle.h" @@ -42,12 +42,32 @@ get_thread_state(PyObject *module) // _ThreadHandle type +typedef enum { + THREAD_HANDLE_INVALID, + THREAD_HANDLE_JOINED, + THREAD_HANDLE_DETACHED, +} ThreadHandleState; + +// A handle to join or detach an OS thread. +// +// Joining or detaching the handle is idempotent; the underlying OS thread is +// joined or detached only once. Concurrent operations block until it is their +// turn to execute or an operation completes successfully. Once an operation +// has completed successfully all future operations complete immediately. typedef struct { PyObject_HEAD struct llist_node node; // linked list node (see _pythread_runtime_state) + + // The `ident` and `handle` fields are immutable once the object is visible + // to threads other than its creator, thus they do not need to be accessed + // atomically. PyThread_ident_t ident; PyThread_handle_t handle; - char joinable; + + // State is set once by the first successful `join` or `detach` operation + // (or if the handle is invalidated). + ThreadHandleState state; + _PyOnceFlag once; } ThreadHandleObject; static ThreadHandleObject* @@ -59,7 +79,7 @@ new_thread_handle(thread_module_state* state) } self->ident = 0; self->handle = 0; - self->joinable = 0; + self->once = (_PyOnceFlag){0}; HEAD_LOCK(&_PyRuntime); llist_insert_tail(&_PyRuntime.threads.handles, &self->node); @@ -68,6 +88,18 @@ new_thread_handle(thread_module_state* state) return self; } +static int +detach_thread(ThreadHandleObject *handle) +{ + // This is typically short so no need to release the GIL + if (PyThread_detach_thread(handle->handle)) { + PyErr_SetString(ThreadError, "Failed detaching thread"); + return -1; + } + handle->state = THREAD_HANDLE_DETACHED; + return 0; +} + static void ThreadHandle_dealloc(ThreadHandleObject *self) { @@ -80,17 +112,32 @@ ThreadHandle_dealloc(ThreadHandleObject *self) } HEAD_UNLOCK(&_PyRuntime); - if (self->joinable) { - int ret = PyThread_detach_thread(self->handle); - if (ret) { - PyErr_SetString(ThreadError, "Failed detaching thread"); - PyErr_WriteUnraisable(tp); - } + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)detach_thread, + self) == -1) { + PyErr_WriteUnraisable(tp); } PyObject_Free(self); Py_DECREF(tp); } +static int +do_invalidate_thread_handle(ThreadHandleObject *handle) +{ + handle->state = THREAD_HANDLE_INVALID; + return 0; +} + +static void +invalidate_thread_handle(ThreadHandleObject *handle) +{ + if (_PyOnceFlag_CallOnce(&handle->once, + (_Py_once_fn_t *)do_invalidate_thread_handle, + handle) == -1) { + Py_FatalError("failed invalidating thread handle"); + Py_UNREACHABLE(); + } +} + void _PyThread_AfterFork(struct _pythread_runtime_state *state) { @@ -107,8 +154,11 @@ _PyThread_AfterFork(struct _pythread_runtime_state *state) continue; } - // Disallow calls to detach() and join() as they could crash. - hobj->joinable = 0; + // Disallow calls to detach() and join() on handles who were not + // previously joined or detached as they could crash. Calls to detach() + // or join() on handles that were successfully joined or detached are + // allowed as they do not perform any unsafe operations. + invalidate_thread_handle(hobj); llist_remove(node); } } @@ -126,49 +176,87 @@ ThreadHandle_get_ident(ThreadHandleObject *self, void *ignored) return PyLong_FromUnsignedLongLong(self->ident); } +static PyObject * +invalid_handle_error(void) +{ + PyErr_SetString(PyExc_ValueError, + "the handle is invalid and thus cannot be detached"); + return NULL; +} static PyObject * ThreadHandle_detach(ThreadHandleObject *self, void* ignored) { - if (!self->joinable) { - PyErr_SetString(PyExc_ValueError, - "the thread is not joinable and thus cannot be detached"); + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)detach_thread, + self) == -1) { return NULL; } - self->joinable = 0; - // This is typically short so no need to release the GIL - int ret = PyThread_detach_thread(self->handle); - if (ret) { - PyErr_SetString(ThreadError, "Failed detaching thread"); - return NULL; + + switch (self->state) { + case THREAD_HANDLE_DETACHED: { + Py_RETURN_NONE; + } + case THREAD_HANDLE_INVALID: { + return invalid_handle_error(); + } + case THREAD_HANDLE_JOINED: { + PyErr_SetString( + PyExc_ValueError, + "the thread has been joined and thus cannot be detached"); + return NULL; + } + default: { + Py_UNREACHABLE(); + } } - Py_RETURN_NONE; } -static PyObject * -ThreadHandle_join(ThreadHandleObject *self, void* ignored) +static int +join_thread(ThreadHandleObject *handle) { - if (!self->joinable) { - PyErr_SetString(PyExc_ValueError, "the thread is not joinable"); - return NULL; - } - if (self->ident == PyThread_get_thread_ident_ex()) { + if (handle->ident == PyThread_get_thread_ident_ex()) { // PyThread_join_thread() would deadlock or error out. PyErr_SetString(ThreadError, "Cannot join current thread"); - return NULL; + return -1; } - // Before actually joining, we must first mark the thread as non-joinable, - // as joining several times simultaneously or sequentially is undefined behavior. - self->joinable = 0; - int ret; + + int err; Py_BEGIN_ALLOW_THREADS - ret = PyThread_join_thread(self->handle); + err = PyThread_join_thread(handle->handle); Py_END_ALLOW_THREADS - if (ret) { + if (err) { PyErr_SetString(ThreadError, "Failed joining thread"); + return -1; + } + handle->state = THREAD_HANDLE_JOINED; + return 0; +} + +static PyObject * +ThreadHandle_join(ThreadHandleObject *self, void* ignored) +{ + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)join_thread, + self) == -1) { return NULL; } - Py_RETURN_NONE; + + switch (self->state) { + case THREAD_HANDLE_DETACHED: { + PyErr_SetString( + PyExc_ValueError, + "the thread is detached and thus cannot be joined"); + return NULL; + } + case THREAD_HANDLE_INVALID: { + return invalid_handle_error(); + } + case THREAD_HANDLE_JOINED: { + Py_RETURN_NONE; + } + default: { + Py_UNREACHABLE(); + } + } } static PyGetSetDef ThreadHandle_getsetlist[] = { @@ -1424,12 +1512,12 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *func) } if (do_start_new_thread(state, func, args, /*kwargs=*/ NULL, /*joinable=*/ 1, &hobj->ident, &hobj->handle)) { + invalidate_thread_handle(hobj); Py_DECREF(args); Py_DECREF(hobj); return NULL; } Py_DECREF(args); - hobj->joinable = 1; return (PyObject*) hobj; } From e86dde8c722a8067b3294ed1aae37fbaeb1db106 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 9 Feb 2024 11:50:47 -0800 Subject: [PATCH 02/20] Don't use PyOnceFlag post fork --- Modules/_threadmodule.c | 50 +++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 5e3696f49df1ff..735281903aaeaf 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -43,7 +43,6 @@ get_thread_state(PyObject *module) // _ThreadHandle type typedef enum { - THREAD_HANDLE_INVALID, THREAD_HANDLE_JOINED, THREAD_HANDLE_DETACHED, } ThreadHandleState; @@ -64,6 +63,10 @@ typedef struct { PyThread_ident_t ident; PyThread_handle_t handle; + // Set before a handle is accessible to any threads other than its creator + // and cleared post-fork. Does not need to be accessed atomically. + bool is_valid; + // State is set once by the first successful `join` or `detach` operation // (or if the handle is invalidated). ThreadHandleState state; @@ -79,6 +82,7 @@ new_thread_handle(thread_module_state* state) } self->ident = 0; self->handle = 0; + self->is_valid = false; self->once = (_PyOnceFlag){0}; HEAD_LOCK(&_PyRuntime); @@ -112,7 +116,8 @@ ThreadHandle_dealloc(ThreadHandleObject *self) } HEAD_UNLOCK(&_PyRuntime); - if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)detach_thread, + if (self->is_valid && + _PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)detach_thread, self) == -1) { PyErr_WriteUnraisable(tp); } @@ -120,24 +125,6 @@ ThreadHandle_dealloc(ThreadHandleObject *self) Py_DECREF(tp); } -static int -do_invalidate_thread_handle(ThreadHandleObject *handle) -{ - handle->state = THREAD_HANDLE_INVALID; - return 0; -} - -static void -invalidate_thread_handle(ThreadHandleObject *handle) -{ - if (_PyOnceFlag_CallOnce(&handle->once, - (_Py_once_fn_t *)do_invalidate_thread_handle, - handle) == -1) { - Py_FatalError("failed invalidating thread handle"); - Py_UNREACHABLE(); - } -} - void _PyThread_AfterFork(struct _pythread_runtime_state *state) { @@ -154,11 +141,8 @@ _PyThread_AfterFork(struct _pythread_runtime_state *state) continue; } - // Disallow calls to detach() and join() on handles who were not - // previously joined or detached as they could crash. Calls to detach() - // or join() on handles that were successfully joined or detached are - // allowed as they do not perform any unsafe operations. - invalidate_thread_handle(hobj); + // Disallow calls to detach() and join() as they could crash. + hobj->is_valid = false; llist_remove(node); } } @@ -187,6 +171,10 @@ invalid_handle_error(void) static PyObject * ThreadHandle_detach(ThreadHandleObject *self, void* ignored) { + if (!self->is_valid) { + return invalid_handle_error(); + } + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)detach_thread, self) == -1) { return NULL; @@ -196,9 +184,6 @@ ThreadHandle_detach(ThreadHandleObject *self, void* ignored) case THREAD_HANDLE_DETACHED: { Py_RETURN_NONE; } - case THREAD_HANDLE_INVALID: { - return invalid_handle_error(); - } case THREAD_HANDLE_JOINED: { PyErr_SetString( PyExc_ValueError, @@ -235,6 +220,10 @@ join_thread(ThreadHandleObject *handle) static PyObject * ThreadHandle_join(ThreadHandleObject *self, void* ignored) { + if (!self->is_valid) { + return invalid_handle_error(); + } + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)join_thread, self) == -1) { return NULL; @@ -247,9 +236,6 @@ ThreadHandle_join(ThreadHandleObject *self, void* ignored) "the thread is detached and thus cannot be joined"); return NULL; } - case THREAD_HANDLE_INVALID: { - return invalid_handle_error(); - } case THREAD_HANDLE_JOINED: { Py_RETURN_NONE; } @@ -1512,12 +1498,12 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *func) } if (do_start_new_thread(state, func, args, /*kwargs=*/ NULL, /*joinable=*/ 1, &hobj->ident, &hobj->handle)) { - invalidate_thread_handle(hobj); Py_DECREF(args); Py_DECREF(hobj); return NULL; } Py_DECREF(args); + hobj->is_valid = true; return (PyObject*) hobj; } From 9a61e2007351718eea85f4ca73a824442804aec7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 9 Feb 2024 13:07:23 -0800 Subject: [PATCH 03/20] Check for self-join outside of the once flag --- Include/internal/pycore_lock.h | 33 ++++++++++++++++++ Lib/test/test_thread.py | 55 ++++++++++++++++++++++++++++++ Modules/_threadmodule.c | 61 +++++++++++++++++++++++++++++----- Python/lock.c | 7 ++++ 4 files changed, 147 insertions(+), 9 deletions(-) diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index 18a8896d97a548..e76813becee64d 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -135,6 +135,10 @@ typedef struct { uint8_t v; } PyEvent; +// Check if the event is set without blocking. Returns 1 if the event is set or +// 0 otherwise. +PyAPI_FUNC(int) _PyEvent_IsSet(PyEvent *evt); + // Set the event and notify any waiting threads. // Export for '_testinternalcapi' shared extension PyAPI_FUNC(void) _PyEvent_Notify(PyEvent *evt); @@ -148,6 +152,35 @@ PyAPI_FUNC(void) PyEvent_Wait(PyEvent *evt); // and 0 if the timeout expired or thread was interrupted. PyAPI_FUNC(int) PyEvent_WaitTimed(PyEvent *evt, _PyTime_t timeout_ns); +// A one-time event notification with reference counting. +typedef struct _PyEventRc { + PyEvent event; + Py_ssize_t refcount; +} _PyEventRc; + +static inline _PyEventRc * +_PyEventRc_New(void) +{ + _PyEventRc *erc = (_PyEventRc *)PyMem_RawCalloc(1, sizeof(_PyEventRc)); + if (erc != NULL) { + erc->refcount = 1; + } + return erc; +} + +static inline void +_PyEventRc_Incref(_PyEventRc *erc) +{ + _Py_atomic_add_ssize(&erc->refcount, 1); +} + +static inline void +_PyEventRc_Decref(_PyEventRc *erc) +{ + if (_Py_atomic_add_ssize(&erc->refcount, -1) == 1) { + PyMem_RawFree(erc); + } +} // _PyRawMutex implements a word-sized mutex that that does not depend on the // parking lot API, and therefore can be used in the parking lot diff --git a/Lib/test/test_thread.py b/Lib/test/test_thread.py index 3db36d77c86078..26126538fe351c 100644 --- a/Lib/test/test_thread.py +++ b/Lib/test/test_thread.py @@ -296,6 +296,61 @@ def task(): # Subsequent calls to detach() should succeed handle.detach() + def test_join_then_self_join(self): + # make sure we can't deadlock in the following scenario with + # threads t0 and t1: + # + # - t0 joins t1 + # - t1 self joins + def make_lock(): + lock = thread.allocate_lock() + lock.acquire() + return lock + + error = None + self_joiner_handle = None + self_joiner_started = make_lock() + self_joiner_barrier = make_lock() + def self_joiner(): + nonlocal error + + self_joiner_started.release() + self_joiner_barrier.acquire() + + try: + self_joiner_handle.join() + except Exception as e: + error = e + + joiner_started = make_lock() + def joiner(): + joiner_started.release() + self_joiner_handle.join() + + with threading_helper.wait_threads_exit(): + self_joiner_handle = thread.start_joinable_thread(self_joiner) + # Wait for the self-joining thread to start + self_joiner_started.acquire() + + # Start the thread that joins the self-joiner + joiner_handle = thread.start_joinable_thread(joiner) + + # Wait for the joiner to start + joiner_started.acquire() + + # Not great, but I don't think there's a deterministic way to do + # make sure that the self-joining thread has been joined. + time.sleep(0.1) + + # Unblock the self-joiner + self_joiner_barrier.release() + + self_joiner_handle.join() + joiner_handle.join() + + with self.assertRaisesRegex(RuntimeError, "Cannot join current thread"): + raise error + class Barrier: def __init__(self, num_threads): diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 735281903aaeaf..092cdd94e29cb9 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -67,6 +67,10 @@ typedef struct { // and cleared post-fork. Does not need to be accessed atomically. bool is_valid; + // Set immediately before `thread_run` returns to indicate that the OS + // thread is about to exit. + _PyEventRc *thread_is_exiting; + // State is set once by the first successful `join` or `detach` operation // (or if the handle is invalidated). ThreadHandleState state; @@ -76,13 +80,22 @@ typedef struct { static ThreadHandleObject* new_thread_handle(thread_module_state* state) { + _PyEventRc *event = _PyEventRc_New(); + if (event == NULL) { + PyErr_NoMemory(); + return NULL; + } + ThreadHandleObject* self = PyObject_New(ThreadHandleObject, state->thread_handle_type); if (self == NULL) { + _PyEventRc_Decref(event); return NULL; } + self->ident = 0; self->handle = 0; self->is_valid = false; + self->thread_is_exiting = event; self->once = (_PyOnceFlag){0}; HEAD_LOCK(&_PyRuntime); @@ -121,6 +134,7 @@ ThreadHandle_dealloc(ThreadHandleObject *self) self) == -1) { PyErr_WriteUnraisable(tp); } + _PyEventRc_Decref(self->thread_is_exiting); PyObject_Free(self); Py_DECREF(tp); } @@ -199,12 +213,6 @@ ThreadHandle_detach(ThreadHandleObject *self, void* ignored) static int join_thread(ThreadHandleObject *handle) { - if (handle->ident == PyThread_get_thread_ident_ex()) { - // PyThread_join_thread() would deadlock or error out. - PyErr_SetString(ThreadError, "Cannot join current thread"); - return -1; - } - int err; Py_BEGIN_ALLOW_THREADS err = PyThread_join_thread(handle->handle); @@ -224,6 +232,23 @@ ThreadHandle_join(ThreadHandleObject *self, void* ignored) return invalid_handle_error(); } + // We want to perform this check outside of the `_PyOnceFlag` to prevent + // deadlock in the scenario where another thread joins us and we then + // attempt to join ourselves. However, it's not safe to check thread + // identity once the handle's os thread has finished. We may end up with + // the identity stored in the handle and erroneously think we are + // attempting to join ourselves. + // + // To work around this, we set `thread_is_exiting` immediately before + // `thread_run` returns. We can be sure that we are not attempting to join + // ourselves if the handle's thread is about to exit. + if (!_PyEvent_IsSet(&self->thread_is_exiting->event) && + self->ident == PyThread_get_thread_ident_ex()) { + // PyThread_join_thread() would deadlock or error out. + PyErr_SetString(ThreadError, "Cannot join current thread"); + return NULL; + } + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)join_thread, self) == -1) { return NULL; @@ -1276,6 +1301,7 @@ struct bootstate { PyObject *func; PyObject *args; PyObject *kwargs; + _PyEventRc *thread_is_exiting; }; @@ -1287,6 +1313,9 @@ thread_bootstate_free(struct bootstate *boot, int decref) Py_DECREF(boot->args); Py_XDECREF(boot->kwargs); } + if (boot->thread_is_exiting != NULL) { + _PyEventRc_Decref(boot->thread_is_exiting); + } PyMem_RawFree(boot); } @@ -1297,6 +1326,10 @@ thread_run(void *boot_raw) struct bootstate *boot = (struct bootstate *) boot_raw; PyThreadState *tstate = boot->tstate; + // `thread_is_exiting` needs to be set after bootstate has been freed + _PyEventRc *thread_is_exiting = boot->thread_is_exiting; + boot->thread_is_exiting = NULL; + // gh-108987: If _thread.start_new_thread() is called before or while // Python is being finalized, thread_run() can called *after*. // _PyRuntimeState_SetFinalizing() is called. At this point, all Python @@ -1341,6 +1374,11 @@ thread_run(void *boot_raw) _PyThreadState_DeleteCurrent(tstate); exit: + if (thread_is_exiting != NULL) { + _PyEvent_Notify(&thread_is_exiting->event); + _PyEventRc_Decref(thread_is_exiting); + } + // bpo-44434: Don't call explicitly PyThread_exit_thread(). On Linux with // the glibc, pthread_exit() can abort the whole process if dlopen() fails // to open the libgcc_s.so library (ex: EMFILE error). @@ -1369,7 +1407,8 @@ static int do_start_new_thread(thread_module_state* state, PyObject *func, PyObject* args, PyObject* kwargs, int joinable, - PyThread_ident_t* ident, PyThread_handle_t* handle) + PyThread_ident_t* ident, PyThread_handle_t* handle, + _PyEventRc *thread_is_exiting) { PyInterpreterState *interp = _PyInterpreterState_GET(); if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_THREADS)) { @@ -1402,6 +1441,10 @@ do_start_new_thread(thread_module_state* state, boot->func = Py_NewRef(func); boot->args = Py_NewRef(args); boot->kwargs = Py_XNewRef(kwargs); + boot->thread_is_exiting = thread_is_exiting; + if (thread_is_exiting != NULL) { + _PyEventRc_Incref(thread_is_exiting); + } int err; if (joinable) { @@ -1453,7 +1496,7 @@ thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs) PyThread_ident_t ident = 0; PyThread_handle_t handle; if (do_start_new_thread(state, func, args, kwargs, /*joinable=*/ 0, - &ident, &handle)) { + &ident, &handle, NULL)) { return NULL; } return PyLong_FromUnsignedLongLong(ident); @@ -1497,7 +1540,7 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *func) return NULL; } if (do_start_new_thread(state, func, args, /*kwargs=*/ NULL, /*joinable=*/ 1, - &hobj->ident, &hobj->handle)) { + &hobj->ident, &hobj->handle, hobj->thread_is_exiting)) { Py_DECREF(args); Py_DECREF(hobj); return NULL; diff --git a/Python/lock.c b/Python/lock.c index f0ff1176941da8..65f5c5789f4cd4 100644 --- a/Python/lock.c +++ b/Python/lock.c @@ -248,6 +248,13 @@ _PyRawMutex_UnlockSlow(_PyRawMutex *m) } } +int +_PyEvent_IsSet(PyEvent *evt) +{ + uint8_t v = _Py_atomic_load_uint8(&evt->v); + return v == _Py_LOCKED; +} + void _PyEvent_Notify(PyEvent *evt) { From bcf66cef63dd09abe3876465a16095ab2ee50353 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 9 Feb 2024 13:32:32 -0800 Subject: [PATCH 04/20] Fix grammar and formatting --- Lib/test/test_thread.py | 4 ++-- Modules/_threadmodule.c | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_thread.py b/Lib/test/test_thread.py index 26126538fe351c..fbe879961a6d2c 100644 --- a/Lib/test/test_thread.py +++ b/Lib/test/test_thread.py @@ -338,8 +338,8 @@ def joiner(): # Wait for the joiner to start joiner_started.acquire() - # Not great, but I don't think there's a deterministic way to do - # make sure that the self-joining thread has been joined. + # Not great, but I don't think there's a deterministic way to make + # sure that the self-joining thread has been joined. time.sleep(0.1) # Unblock the self-joiner diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 092cdd94e29cb9..4225568a784e9b 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -85,13 +85,11 @@ new_thread_handle(thread_module_state* state) PyErr_NoMemory(); return NULL; } - ThreadHandleObject* self = PyObject_New(ThreadHandleObject, state->thread_handle_type); if (self == NULL) { _PyEventRc_Decref(event); return NULL; } - self->ident = 0; self->handle = 0; self->is_valid = false; From 7f9720d7529b0e12fe428cfd86a66295f9d0ca0d Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 9 Feb 2024 13:44:27 -0800 Subject: [PATCH 05/20] Update comment. --- Modules/_threadmodule.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 4225568a784e9b..ec6690f4337c08 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -71,8 +71,7 @@ typedef struct { // thread is about to exit. _PyEventRc *thread_is_exiting; - // State is set once by the first successful `join` or `detach` operation - // (or if the handle is invalidated). + // State is set once by the first successful `join` or `detach` operation. ThreadHandleState state; _PyOnceFlag once; } ThreadHandleObject; From 092c2ebd3f0fb03d408b81e47cc61ede8fc6322e Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 9 Feb 2024 21:30:55 -0800 Subject: [PATCH 06/20] Account for handles being cleared after fork --- Lib/threading.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/threading.py b/Lib/threading.py index 364c45d840cf2b..3a447884dd10bc 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -1162,7 +1162,9 @@ def join(self, timeout=None): self._join_os_thread() def _join_os_thread(self): - self._handle.join() + # self._handle may be cleared post-fork + if self._handle is not None: + self._handle.join() def _wait_for_tstate_lock(self, block=True, timeout=-1): # Issue #18808: wait for the thread state to be gone. From 77da31d49718845190f435b56f5239e04281dac5 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 12 Feb 2024 10:20:00 -0800 Subject: [PATCH 07/20] Move implementations of `_PyEventRc` functions out of header --- Include/internal/pycore_lock.h | 26 +++----------------------- Python/lock.c | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index e76813becee64d..f8d66c1cd9f3aa 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -158,29 +158,9 @@ typedef struct _PyEventRc { Py_ssize_t refcount; } _PyEventRc; -static inline _PyEventRc * -_PyEventRc_New(void) -{ - _PyEventRc *erc = (_PyEventRc *)PyMem_RawCalloc(1, sizeof(_PyEventRc)); - if (erc != NULL) { - erc->refcount = 1; - } - return erc; -} - -static inline void -_PyEventRc_Incref(_PyEventRc *erc) -{ - _Py_atomic_add_ssize(&erc->refcount, 1); -} - -static inline void -_PyEventRc_Decref(_PyEventRc *erc) -{ - if (_Py_atomic_add_ssize(&erc->refcount, -1) == 1) { - PyMem_RawFree(erc); - } -} +_PyEventRc *_PyEventRc_New(void); +void _PyEventRc_Incref(_PyEventRc *erc); +void _PyEventRc_Decref(_PyEventRc *erc); // _PyRawMutex implements a word-sized mutex that that does not depend on the // parking lot API, and therefore can be used in the parking lot diff --git a/Python/lock.c b/Python/lock.c index 65f5c5789f4cd4..e6a5f64912da13 100644 --- a/Python/lock.c +++ b/Python/lock.c @@ -303,6 +303,30 @@ PyEvent_WaitTimed(PyEvent *evt, _PyTime_t timeout_ns) } } +_PyEventRc * +_PyEventRc_New(void) +{ + _PyEventRc *erc = (_PyEventRc *)PyMem_RawCalloc(1, sizeof(_PyEventRc)); + if (erc != NULL) { + erc->refcount = 1; + } + return erc; +} + +void +_PyEventRc_Incref(_PyEventRc *erc) +{ + _Py_atomic_add_ssize(&erc->refcount, 1); +} + +void +_PyEventRc_Decref(_PyEventRc *erc) +{ + if (_Py_atomic_add_ssize(&erc->refcount, -1) == 1) { + PyMem_RawFree(erc); + } +} + static int unlock_once(_PyOnceFlag *o, int res) { From 321a4e089fde56448f9e62483c832a8155ac4ca8 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 12 Feb 2024 14:29:36 -0800 Subject: [PATCH 08/20] Track all states of the handle in the enum --- Modules/_threadmodule.c | 69 +++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 23 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index ec6690f4337c08..b0e2939d657009 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -42,9 +42,13 @@ get_thread_state(PyObject *module) // _ThreadHandle type +// Handles transition from RUNNING to one of JOINED, DETACHED, or INVALID (post +// fork). typedef enum { + THREAD_HANDLE_RUNNING, THREAD_HANDLE_JOINED, THREAD_HANDLE_DETACHED, + THREAD_HANDLE_INVALID, } ThreadHandleState; // A handle to join or detach an OS thread. @@ -63,19 +67,29 @@ typedef struct { PyThread_ident_t ident; PyThread_handle_t handle; - // Set before a handle is accessible to any threads other than its creator - // and cleared post-fork. Does not need to be accessed atomically. - bool is_valid; + // Holds a value from the `ThreadHandleState` enum. + int state; // Set immediately before `thread_run` returns to indicate that the OS // thread is about to exit. _PyEventRc *thread_is_exiting; - // State is set once by the first successful `join` or `detach` operation. - ThreadHandleState state; + // Serializes calls to `join` and `detach`. _PyOnceFlag once; } ThreadHandleObject; +static inline int +get_thread_handle_state(ThreadHandleObject *handle) +{ + return _Py_atomic_load_int_relaxed(&handle->state); +} + +static inline void +set_thread_handle_state(ThreadHandleObject *handle, ThreadHandleState state) +{ + _Py_atomic_store_int_relaxed(&handle->state, state); +} + static ThreadHandleObject* new_thread_handle(thread_module_state* state) { @@ -91,10 +105,11 @@ new_thread_handle(thread_module_state* state) } self->ident = 0; self->handle = 0; - self->is_valid = false; self->thread_is_exiting = event; self->once = (_PyOnceFlag){0}; + set_thread_handle_state(self, THREAD_HANDLE_INVALID); + HEAD_LOCK(&_PyRuntime); llist_insert_tail(&_PyRuntime.threads.handles, &self->node); HEAD_UNLOCK(&_PyRuntime); @@ -105,12 +120,14 @@ new_thread_handle(thread_module_state* state) static int detach_thread(ThreadHandleObject *handle) { + assert(get_thread_handle_state(handle) == THREAD_HANDLE_RUNNING); + // This is typically short so no need to release the GIL if (PyThread_detach_thread(handle->handle)) { PyErr_SetString(ThreadError, "Failed detaching thread"); return -1; } - handle->state = THREAD_HANDLE_DETACHED; + set_thread_handle_state(handle, THREAD_HANDLE_DETACHED); return 0; } @@ -126,7 +143,7 @@ ThreadHandle_dealloc(ThreadHandleObject *self) } HEAD_UNLOCK(&_PyRuntime); - if (self->is_valid && + if (get_thread_handle_state(self) != THREAD_HANDLE_INVALID && _PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)detach_thread, self) == -1) { PyErr_WriteUnraisable(tp); @@ -152,8 +169,9 @@ _PyThread_AfterFork(struct _pythread_runtime_state *state) continue; } - // Disallow calls to detach() and join() as they could crash. - hobj->is_valid = false; + // Disallow calls to detach() and join() as they could crash. We are + // the only thread; its safe to do this without an atomic. + hobj->state = THREAD_HANDLE_INVALID; llist_remove(node); } } @@ -171,19 +189,22 @@ ThreadHandle_get_ident(ThreadHandleObject *self, void *ignored) return PyLong_FromUnsignedLongLong(self->ident); } -static PyObject * -invalid_handle_error(void) +static bool +check_handle_valid(ThreadHandleObject *handle) { - PyErr_SetString(PyExc_ValueError, - "the handle is invalid and thus cannot be detached"); - return NULL; + if (get_thread_handle_state(handle) == THREAD_HANDLE_INVALID) { + PyErr_SetString(PyExc_ValueError, + "the handle is invalid and thus cannot be detached"); + return false; + } + return true; } static PyObject * ThreadHandle_detach(ThreadHandleObject *self, void* ignored) { - if (!self->is_valid) { - return invalid_handle_error(); + if (!check_handle_valid(self)) { + return NULL; } if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)detach_thread, @@ -191,7 +212,7 @@ ThreadHandle_detach(ThreadHandleObject *self, void* ignored) return NULL; } - switch (self->state) { + switch (get_thread_handle_state(self)) { case THREAD_HANDLE_DETACHED: { Py_RETURN_NONE; } @@ -210,6 +231,8 @@ ThreadHandle_detach(ThreadHandleObject *self, void* ignored) static int join_thread(ThreadHandleObject *handle) { + assert(get_thread_handle_state(handle) == THREAD_HANDLE_RUNNING); + int err; Py_BEGIN_ALLOW_THREADS err = PyThread_join_thread(handle->handle); @@ -218,15 +241,15 @@ join_thread(ThreadHandleObject *handle) PyErr_SetString(ThreadError, "Failed joining thread"); return -1; } - handle->state = THREAD_HANDLE_JOINED; + set_thread_handle_state(handle, THREAD_HANDLE_JOINED); return 0; } static PyObject * ThreadHandle_join(ThreadHandleObject *self, void* ignored) { - if (!self->is_valid) { - return invalid_handle_error(); + if (!check_handle_valid(self)) { + return NULL; } // We want to perform this check outside of the `_PyOnceFlag` to prevent @@ -251,7 +274,7 @@ ThreadHandle_join(ThreadHandleObject *self, void* ignored) return NULL; } - switch (self->state) { + switch (get_thread_handle_state(self)) { case THREAD_HANDLE_DETACHED: { PyErr_SetString( PyExc_ValueError, @@ -1543,7 +1566,7 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *func) return NULL; } Py_DECREF(args); - hobj->is_valid = true; + set_thread_handle_state(hobj, THREAD_HANDLE_RUNNING); return (PyObject*) hobj; } From cd0372c8d95a350211d71ed0162700b99bc547f7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 12 Feb 2024 14:50:22 -0800 Subject: [PATCH 09/20] Remove `detach()` method --- Lib/test/test_thread.py | 63 --------------------------------------- Modules/_threadmodule.c | 66 ++++++++--------------------------------- 2 files changed, 13 insertions(+), 116 deletions(-) diff --git a/Lib/test/test_thread.py b/Lib/test/test_thread.py index fbe879961a6d2c..fd10ac2d1e9e01 100644 --- a/Lib/test/test_thread.py +++ b/Lib/test/test_thread.py @@ -233,69 +233,6 @@ def task(): with self.assertRaisesRegex(RuntimeError, "Cannot join current thread"): raise errors[0] - def test_detach_from_self(self): - errors = [] - handles = [] - start_joinable_thread_returned = thread.allocate_lock() - start_joinable_thread_returned.acquire() - thread_detached = thread.allocate_lock() - thread_detached.acquire() - - def task(): - start_joinable_thread_returned.acquire() - try: - handles[0].detach() - except Exception as e: - errors.append(e) - finally: - thread_detached.release() - - with threading_helper.wait_threads_exit(): - handle = thread.start_joinable_thread(task) - handles.append(handle) - start_joinable_thread_returned.release() - thread_detached.acquire() - with self.assertRaisesRegex(ValueError, "detached and thus cannot be joined"): - handle.join() - - assert len(errors) == 0 - - def test_detach_then_join(self): - lock = thread.allocate_lock() - lock.acquire() - - def task(): - lock.acquire() - - with threading_helper.wait_threads_exit(): - handle = thread.start_joinable_thread(task) - # detach() returns even though the thread is blocked on lock - handle.detach() - # join() then cannot be called anymore - with self.assertRaisesRegex(ValueError, "detached and thus cannot be joined"): - handle.join() - lock.release() - - def test_join_then_detach(self): - def task(): - pass - - with threading_helper.wait_threads_exit(): - handle = thread.start_joinable_thread(task) - handle.join() - with self.assertRaisesRegex(ValueError, "joined and thus cannot be detached"): - handle.detach() - - def test_detach_then_detach(self): - def task(): - pass - - with threading_helper.wait_threads_exit(): - handle = thread.start_joinable_thread(task) - handle.detach() - # Subsequent calls to detach() should succeed - handle.detach() - def test_join_then_self_join(self): # make sure we can't deadlock in the following scenario with # threads t0 and t1: diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index b0e2939d657009..b53259e85ba833 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -51,12 +51,14 @@ typedef enum { THREAD_HANDLE_INVALID, } ThreadHandleState; -// A handle to join or detach an OS thread. +// A handle around an OS thread. // -// Joining or detaching the handle is idempotent; the underlying OS thread is -// joined or detached only once. Concurrent operations block until it is their -// turn to execute or an operation completes successfully. Once an operation -// has completed successfully all future operations complete immediately. +// The OS thread is either joined or detached after the handle is destroyed. +// +// Joining the handle is idempotent; the underlying OS thread is joined or +// detached only once. Concurrent join operations are serialized until it is +// their turn to execute or an earlier operation completes successfully. Once a +// join has completed successfully all future joins complete immediately. typedef struct { PyObject_HEAD struct llist_node node; // linked list node (see _pythread_runtime_state) @@ -74,7 +76,7 @@ typedef struct { // thread is about to exit. _PyEventRc *thread_is_exiting; - // Serializes calls to `join` and `detach`. + // Serializes calls to `join`. _PyOnceFlag once; } ThreadHandleObject; @@ -169,8 +171,8 @@ _PyThread_AfterFork(struct _pythread_runtime_state *state) continue; } - // Disallow calls to detach() and join() as they could crash. We are - // the only thread; its safe to do this without an atomic. + // Disallow calls to join() as they could crash. We are the only + // thread; it's safe to set this without an atomic. hobj->state = THREAD_HANDLE_INVALID; llist_remove(node); } @@ -200,34 +202,6 @@ check_handle_valid(ThreadHandleObject *handle) return true; } -static PyObject * -ThreadHandle_detach(ThreadHandleObject *self, void* ignored) -{ - if (!check_handle_valid(self)) { - return NULL; - } - - if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)detach_thread, - self) == -1) { - return NULL; - } - - switch (get_thread_handle_state(self)) { - case THREAD_HANDLE_DETACHED: { - Py_RETURN_NONE; - } - case THREAD_HANDLE_JOINED: { - PyErr_SetString( - PyExc_ValueError, - "the thread has been joined and thus cannot be detached"); - return NULL; - } - default: { - Py_UNREACHABLE(); - } - } -} - static int join_thread(ThreadHandleObject *handle) { @@ -255,7 +229,7 @@ ThreadHandle_join(ThreadHandleObject *self, void* ignored) // We want to perform this check outside of the `_PyOnceFlag` to prevent // deadlock in the scenario where another thread joins us and we then // attempt to join ourselves. However, it's not safe to check thread - // identity once the handle's os thread has finished. We may end up with + // identity once the handle's os thread has finished. We may end up reusing // the identity stored in the handle and erroneously think we are // attempting to join ourselves. // @@ -273,21 +247,8 @@ ThreadHandle_join(ThreadHandleObject *self, void* ignored) self) == -1) { return NULL; } - - switch (get_thread_handle_state(self)) { - case THREAD_HANDLE_DETACHED: { - PyErr_SetString( - PyExc_ValueError, - "the thread is detached and thus cannot be joined"); - return NULL; - } - case THREAD_HANDLE_JOINED: { - Py_RETURN_NONE; - } - default: { - Py_UNREACHABLE(); - } - } + assert(get_thread_handle_state(self) == THREAD_HANDLE_JOINED); + Py_RETURN_NONE; } static PyGetSetDef ThreadHandle_getsetlist[] = { @@ -297,7 +258,6 @@ static PyGetSetDef ThreadHandle_getsetlist[] = { static PyMethodDef ThreadHandle_methods[] = { - {"detach", (PyCFunction)ThreadHandle_detach, METH_NOARGS}, {"join", (PyCFunction)ThreadHandle_join, METH_NOARGS}, {0, 0} }; From 7badb2d0c19b08039ec7f089c51d8776e2d7860b Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 12 Feb 2024 18:06:55 -0800 Subject: [PATCH 10/20] Don't need to atomically initialize state in ThreadHandle ctor --- Modules/_threadmodule.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index b53259e85ba833..9ed454cb2c1975 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -109,8 +109,7 @@ new_thread_handle(thread_module_state* state) self->handle = 0; self->thread_is_exiting = event; self->once = (_PyOnceFlag){0}; - - set_thread_handle_state(self, THREAD_HANDLE_INVALID); + self->state = THREAD_HANDLE_INVALID; HEAD_LOCK(&_PyRuntime); llist_insert_tail(&_PyRuntime.threads.handles, &self->node); From 9d35990d3f526e278a6d023fc45adbfb55ddae9e Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 12 Feb 2024 18:17:28 -0800 Subject: [PATCH 11/20] Remove unneeded function --- Modules/_threadmodule.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 9ed454cb2c1975..4a21f21933413d 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -190,17 +190,6 @@ ThreadHandle_get_ident(ThreadHandleObject *self, void *ignored) return PyLong_FromUnsignedLongLong(self->ident); } -static bool -check_handle_valid(ThreadHandleObject *handle) -{ - if (get_thread_handle_state(handle) == THREAD_HANDLE_INVALID) { - PyErr_SetString(PyExc_ValueError, - "the handle is invalid and thus cannot be detached"); - return false; - } - return true; -} - static int join_thread(ThreadHandleObject *handle) { @@ -221,7 +210,9 @@ join_thread(ThreadHandleObject *handle) static PyObject * ThreadHandle_join(ThreadHandleObject *self, void* ignored) { - if (!check_handle_valid(self)) { + if (get_thread_handle_state(self) == THREAD_HANDLE_INVALID) { + PyErr_SetString(PyExc_ValueError, + "the handle is invalid and thus cannot be joined"); return NULL; } From e554c5efb080b32f88b1c2c3b35c7fdc2cef2d53 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 12 Feb 2024 19:10:15 -0800 Subject: [PATCH 12/20] Don't need atomics in dtor --- Modules/_threadmodule.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 4a21f21933413d..2f4e6b4c12a566 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -118,20 +118,6 @@ new_thread_handle(thread_module_state* state) return self; } -static int -detach_thread(ThreadHandleObject *handle) -{ - assert(get_thread_handle_state(handle) == THREAD_HANDLE_RUNNING); - - // This is typically short so no need to release the GIL - if (PyThread_detach_thread(handle->handle)) { - PyErr_SetString(ThreadError, "Failed detaching thread"); - return -1; - } - set_thread_handle_state(handle, THREAD_HANDLE_DETACHED); - return 0; -} - static void ThreadHandle_dealloc(ThreadHandleObject *self) { @@ -144,10 +130,15 @@ ThreadHandle_dealloc(ThreadHandleObject *self) } HEAD_UNLOCK(&_PyRuntime); - if (get_thread_handle_state(self) != THREAD_HANDLE_INVALID && - _PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)detach_thread, - self) == -1) { - PyErr_WriteUnraisable(tp); + if (self->state == THREAD_HANDLE_RUNNING) { + // This is typically short so no need to release the GIL + if (PyThread_detach_thread(self->handle)) { + PyErr_SetString(ThreadError, "Failed detaching thread"); + PyErr_WriteUnraisable(tp); + } + else { + self->state = THREAD_HANDLE_DETACHED; + } } _PyEventRc_Decref(self->thread_is_exiting); PyObject_Free(self); From 147a1923b7b515dda44efb4d5e2683d8b3158ce5 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 13 Feb 2024 09:58:41 -0800 Subject: [PATCH 13/20] Elaborate on use of `thread_is_exiting` in `ThreadHandleObject` definition --- Modules/_threadmodule.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 2f4e6b4c12a566..fa28a52f75efa3 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -73,7 +73,9 @@ typedef struct { int state; // Set immediately before `thread_run` returns to indicate that the OS - // thread is about to exit. + // thread is about to exit. This is used to avoid false positives when + // detecting self-join attempts. See the comment in `ThreadHandle_join()` + // for a more detailed explanation. _PyEventRc *thread_is_exiting; // Serializes calls to `join`. From 1fe27ce40ecedfc35e9132e3cbee054dff14f2f7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 13 Feb 2024 16:20:01 -0800 Subject: [PATCH 14/20] Use sequential consistency for operations on state --- Modules/_threadmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index fa28a52f75efa3..6a539e7cd64524 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -85,13 +85,13 @@ typedef struct { static inline int get_thread_handle_state(ThreadHandleObject *handle) { - return _Py_atomic_load_int_relaxed(&handle->state); + return _Py_atomic_load_int(&handle->state); } static inline void set_thread_handle_state(ThreadHandleObject *handle, ThreadHandleState state) { - _Py_atomic_store_int_relaxed(&handle->state, state); + _Py_atomic_store_int(&handle->state, state); } static ThreadHandleObject* From a46b727545fa0b4e617921ea35ba36c004860f29 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 26 Feb 2024 15:18:55 -0800 Subject: [PATCH 15/20] Remove `_join_lock` attribute --- Lib/threading.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/threading.py b/Lib/threading.py index 3a447884dd10bc..ec89550d6b022e 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -931,7 +931,6 @@ class is implemented. if _HAVE_THREAD_NATIVE_ID: self._native_id = None self._tstate_lock = None - self._join_lock = None self._handle = None self._started = Event() self._is_stopped = False From 61d9fa95295f611c52d1ba25b2adddca5c34ab46 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 26 Feb 2024 15:26:25 -0800 Subject: [PATCH 16/20] Add reference to more details for self-join test --- Lib/test/test_thread.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_thread.py b/Lib/test/test_thread.py index fd10ac2d1e9e01..83235230d5c112 100644 --- a/Lib/test/test_thread.py +++ b/Lib/test/test_thread.py @@ -235,7 +235,8 @@ def task(): def test_join_then_self_join(self): # make sure we can't deadlock in the following scenario with - # threads t0 and t1: + # threads t0 and t1 (see comment in `ThreadHandle_join()` for more + # details): # # - t0 joins t1 # - t1 self joins From f6bfa087ef69a6f2dafca7fb971aee308991a082 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 26 Feb 2024 16:13:46 -0800 Subject: [PATCH 17/20] Assign explicit values for ThreadHandle states --- Modules/_threadmodule.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index c8f7cd7c25fbc9..534b594ba287ec 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -45,10 +45,10 @@ get_thread_state(PyObject *module) // Handles transition from RUNNING to one of JOINED, DETACHED, or INVALID (post // fork). typedef enum { - THREAD_HANDLE_RUNNING, - THREAD_HANDLE_JOINED, - THREAD_HANDLE_DETACHED, - THREAD_HANDLE_INVALID, + THREAD_HANDLE_RUNNING = 1, + THREAD_HANDLE_JOINED = 2, + THREAD_HANDLE_DETACHED = 3, + THREAD_HANDLE_INVALID = 4, } ThreadHandleState; // A handle around an OS thread. From 1cc0f2ea456959065f37cc03e91fe7afdaa9f4ee Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 26 Feb 2024 16:17:33 -0800 Subject: [PATCH 18/20] Detail why non-atomic state access is safe in the destructor --- Modules/_threadmodule.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 534b594ba287ec..14a682c6848b10 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -132,6 +132,10 @@ ThreadHandle_dealloc(ThreadHandleObject *self) } HEAD_UNLOCK(&_PyRuntime); + // It's safe to access state non-atomically: + // 1. This is the destructor; nothing else holds a reference. + // 2. The refcount going to zero is a "synchronizes-with" event; + // all changes from other threads are visible. if (self->state == THREAD_HANDLE_RUNNING) { // This is typically short so no need to release the GIL if (PyThread_detach_thread(self->handle)) { From 3fbf11900bbdb22bcb37ddbb3942969c5483f6df Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 26 Feb 2024 16:21:16 -0800 Subject: [PATCH 19/20] Document the purpose of bootstate --- Modules/_threadmodule.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 14a682c6848b10..9ac2d3890138c7 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1263,6 +1263,9 @@ _localdummy_destroyed(PyObject *localweakref, PyObject *dummyweakref) /* Module functions */ +// bootstate is used to "bootstrap" new threads. Any arguments needed by +// `thread_run()`, which can only take a single argument due to platform +// limitations, are contained in bootstate. struct bootstate { PyThreadState *tstate; PyObject *func; From 9d214444839a965f6d2b4c053643b7d65ebb66a6 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 26 Feb 2024 16:22:23 -0800 Subject: [PATCH 20/20] Mark handles running before decrefing args --- Modules/_threadmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 9ac2d3890138c7..c173d328a945cb 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1515,8 +1515,8 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *func) Py_DECREF(hobj); return NULL; } - Py_DECREF(args); set_thread_handle_state(hobj, THREAD_HANDLE_RUNNING); + Py_DECREF(args); return (PyObject*) hobj; }