From b3474f3d65b9e7078ebbf6ca71abad8ee01d5fdd Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 14:23:40 -0600 Subject: [PATCH 01/12] Drop bootstate.interp and bootstate.runtime. --- Modules/_threadmodule.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 5d753b4a0ebc5e..fa809a2c633532 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1048,12 +1048,10 @@ _localdummy_destroyed(PyObject *localweakref, PyObject *dummyweakref) /* Module functions */ struct bootstate { - PyInterpreterState *interp; PyObject *func; PyObject *args; PyObject *kwargs; PyThreadState *tstate; - _PyRuntimeState *runtime; }; @@ -1122,7 +1120,6 @@ and False otherwise.\n"); static PyObject * thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) { - _PyRuntimeState *runtime = &_PyRuntime; PyObject *func, *args, *kwargs = NULL; if (!PyArg_UnpackTuple(fargs, "start_new_thread", 2, 3, @@ -1160,8 +1157,7 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) if (boot == NULL) { return PyErr_NoMemory(); } - boot->interp = _PyInterpreterState_GET(); - boot->tstate = _PyThreadState_New(boot->interp); + boot->tstate = _PyThreadState_New(interp); if (boot->tstate == NULL) { PyMem_Free(boot); if (!PyErr_Occurred()) { @@ -1169,7 +1165,6 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) } return NULL; } - boot->runtime = runtime; boot->func = Py_NewRef(func); boot->args = Py_NewRef(args); boot->kwargs = Py_XNewRef(kwargs); From ec9f53b15429d8183444144aaa580949c4abc328 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 15:13:34 -0600 Subject: [PATCH 02/12] Add module-owned threads to module state. --- Modules/_threadmodule.c | 160 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 156 insertions(+), 4 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index fa809a2c633532..4a69d5ee604535 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -22,7 +22,137 @@ static struct PyModuleDef thread_module; +/* threads owned by the modulo */ + +struct module_thread { + PyThreadState *tstate; + struct module_thread *prev; + struct module_thread *next; +}; + +struct module_threads { + // XXX This can replace interp->threads.count. + Py_ssize_t count; + PyThread_type_lock mutex; + struct module_thread *head; + struct module_thread *tail; +}; + +static int +module_threads_init(struct module_threads *threads) +{ + threads->count = 0; + threads->head = NULL; + threads->tail = NULL; + threads->mutex = PyThread_allocate_lock(); + if (threads->mutex == NULL) { + PyErr_NoMemory(); + return -1; + } + return 0; +} + +static void +module_threads_fini(struct module_threads *threads) +{ + PyThread_free_lock(threads->mutex); +} + +static void +module_threads_add(struct module_threads *threads, struct module_thread *mt) +{ + PyThread_acquire_lock(threads->mutex, WAIT_LOCK); + + // Add it to the end of the list. + if (threads->head == NULL) { + threads->head = mt; + } + else { + mt->prev = threads->tail; + threads->tail->next = mt; + } + threads->tail = mt; + threads->count++; + + PyThread_release_lock(threads->mutex); +} + +static void +module_threads_remove(struct module_threads *threads, struct module_thread *mt) +{ + PyThread_acquire_lock(threads->mutex, WAIT_LOCK); + + if (mt->prev == NULL) { + threads->head = mt->next; + } + else { + mt->prev->next = mt->next; + } + if (mt->next == NULL) { + threads->tail = mt->prev; + } + else { + mt->next->prev = mt->prev; + } + threads->count--; + + PyThread_release_lock(threads->mutex); +} + +static struct module_thread * +add_module_thread(struct module_threads *threads, PyThreadState *tstate) +{ + // Create the new list entry. + struct module_thread *mt = PyMem_RawMalloc(sizeof(struct module_thread)); + if (mt == NULL) { + if (!PyErr_Occurred()) { + PyErr_NoMemory(); + } + return NULL; + } + mt->tstate = tstate; + mt->prev = NULL; + mt->next = NULL; + + module_threads_add(threads, mt); + + return mt; +} + +static int +module_thread_starting(struct module_thread *mt) +{ + assert(mt->tstate == PyThreadState_Get()); + + mt->tstate->interp->threads.count++; + + return 0; +} + +static void +module_thread_finished(struct module_thread *mt) +{ + mt->tstate->interp->threads.count--; + + // XXX We should be notifying other threads here. +} + +static void +remove_module_thread(struct module_threads *threads, struct module_thread *mt) +{ + // Remove it from the list. + module_threads_remove(threads, mt); + + // Deallocate everything. + PyMem_RawFree(mt); +} + + +/* module state */ + typedef struct { + struct module_threads threads; + PyTypeObject *excepthook_type; PyTypeObject *lock_type; PyTypeObject *local_type; @@ -1052,6 +1182,8 @@ struct bootstate { PyObject *args; PyObject *kwargs; PyThreadState *tstate; + thread_module_state *module_state; + struct module_thread *module_thread; }; @@ -1069,12 +1201,14 @@ static void thread_run(void *boot_raw) { struct bootstate *boot = (struct bootstate *) boot_raw; - PyThreadState *tstate; + PyThreadState *tstate = boot->tstate; + thread_module_state *state = boot->module_state; + struct module_thread *mt = boot->module_thread; - tstate = boot->tstate; _PyThreadState_Bind(tstate); PyEval_AcquireThread(tstate); - tstate->interp->threads.count++; + + module_thread_starting(mt); PyObject *res = PyObject_Call(boot->func, boot->args, boot->kwargs); if (res == NULL) { @@ -1089,9 +1223,12 @@ thread_run(void *boot_raw) Py_DECREF(res); } + module_thread_finished(mt); + thread_bootstate_free(boot); - tstate->interp->threads.count--; PyThreadState_Clear(tstate); + // XXX This should be called last: + remove_module_thread(&state->threads, mt); _PyThreadState_DeleteCurrent(tstate); // bpo-44434: Don't call explicitly PyThread_exit_thread(). On Linux with @@ -1165,6 +1302,14 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) } return NULL; } + thread_module_state *state = get_thread_state(self); + boot->module_state = state; + boot->module_thread = add_module_thread(&state->threads, boot->tstate); + if (boot->module_thread == NULL) { + PyThreadState_Clear(boot->tstate); + PyMem_Free(boot); + return NULL; + } boot->func = Py_NewRef(func); boot->args = Py_NewRef(args); boot->kwargs = Py_XNewRef(kwargs); @@ -1604,6 +1749,11 @@ thread_module_exec(PyObject *module) // Initialize the C thread library PyThread_init_thread(); + // Initialize the list of threads owned by this module. + if (module_threads_init(&state->threads) < 0) { + return -1; + } + // Lock state->lock_type = (PyTypeObject *)PyType_FromSpec(&lock_type_spec); if (state->lock_type == NULL) { @@ -1694,6 +1844,8 @@ thread_module_clear(PyObject *module) static void thread_module_free(void *module) { + thread_module_state *state = get_thread_state(module); + module_threads_fini(&state->threads); thread_module_clear((PyObject *)module); } From 05aa205de5d944edfee1116bb8b7a6447c19a73b Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 15:18:11 -0600 Subject: [PATCH 03/12] Call on_delete() in remove_module_thread(). --- Modules/_threadmodule.c | 7 +++++++ Python/pystate.c | 4 ---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 4a69d5ee604535..0c66b1f97eddca 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -140,6 +140,13 @@ module_thread_finished(struct module_thread *mt) static void remove_module_thread(struct module_threads *threads, struct module_thread *mt) { + // Notify other threads that this one is done. + // XXX This should happen in module_thread_finished(). + // XXX These fields could be removed from PyThreadState. + if (mt->tstate->on_delete != NULL) { + mt->tstate->on_delete(mt->tstate->on_delete_data); + } + // Remove it from the list. module_threads_remove(threads, mt); diff --git a/Python/pystate.c b/Python/pystate.c index 25e655a2027918..a084e1db7fff26 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1490,10 +1490,6 @@ PyThreadState_Clear(PyThreadState *tstate) Py_CLEAR(tstate->context); - if (tstate->on_delete != NULL) { - tstate->on_delete(tstate->on_delete_data); - } - tstate->_status.cleared = 1; // XXX Call _PyThreadStateSwap(runtime, NULL) here if "current". From da85eeda70a3c051d3203b36bd9627eee66e4ebe Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 16:07:22 -0600 Subject: [PATCH 04/12] Drop interp.threads.count. --- Include/internal/pycore_interp.h | 2 -- Modules/_threadmodule.c | 22 ++++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index edc076fc04f6c3..98f620c59c6b24 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -57,8 +57,6 @@ struct _is { uint64_t next_unique_id; /* The linked list of threads, newest first. */ PyThreadState *head; - /* Used in Modules/_threadmodule.c. */ - long count; /* Support for runtime thread stack size tuning. A value of 0 means using the platform's default stack size or the size specified by the THREAD_STACK_SIZE macro. */ diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 0c66b1f97eddca..16c55855bc14a7 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -32,7 +32,8 @@ struct module_thread { struct module_threads { // XXX This can replace interp->threads.count. - Py_ssize_t count; + long num_total; + long num_running; PyThread_type_lock mutex; struct module_thread *head; struct module_thread *tail; @@ -41,7 +42,8 @@ struct module_threads { static int module_threads_init(struct module_threads *threads) { - threads->count = 0; + threads->num_total = 0; + threads->num_running = 0; threads->head = NULL; threads->tail = NULL; threads->mutex = PyThread_allocate_lock(); @@ -72,7 +74,7 @@ module_threads_add(struct module_threads *threads, struct module_thread *mt) threads->tail->next = mt; } threads->tail = mt; - threads->count++; + threads->num_total++; PyThread_release_lock(threads->mutex); } @@ -94,7 +96,7 @@ module_threads_remove(struct module_threads *threads, struct module_thread *mt) else { mt->next->prev = mt->prev; } - threads->count--; + threads->num_total--; PyThread_release_lock(threads->mutex); } @@ -120,19 +122,19 @@ add_module_thread(struct module_threads *threads, PyThreadState *tstate) } static int -module_thread_starting(struct module_thread *mt) +module_thread_starting(struct module_threads *threads, struct module_thread *mt) { assert(mt->tstate == PyThreadState_Get()); - mt->tstate->interp->threads.count++; + threads->num_running++; return 0; } static void -module_thread_finished(struct module_thread *mt) +module_thread_finished(struct module_threads *threads, struct module_thread *mt) { - mt->tstate->interp->threads.count--; + threads->num_running--; // XXX We should be notifying other threads here. } @@ -1215,7 +1217,7 @@ thread_run(void *boot_raw) _PyThreadState_Bind(tstate); PyEval_AcquireThread(tstate); - module_thread_starting(mt); + module_thread_starting(&state->threads, mt); PyObject *res = PyObject_Call(boot->func, boot->args, boot->kwargs); if (res == NULL) { @@ -1230,7 +1232,7 @@ thread_run(void *boot_raw) Py_DECREF(res); } - module_thread_finished(mt); + module_thread_finished(&state->threads, mt); thread_bootstate_free(boot); PyThreadState_Clear(tstate); From 004589be6d8489c2b75cac5c908e5fdd96cf09ef Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 16:09:02 -0600 Subject: [PATCH 05/12] Pass module_state to newlockobject(). --- Modules/_threadmodule.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 16c55855bc14a7..959773d87310da 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -736,10 +736,8 @@ static PyType_Spec rlock_type_spec = { }; static lockobject * -newlockobject(PyObject *module) +newlockobject(thread_module_state *state) { - thread_module_state *state = get_thread_state(module); - PyTypeObject *type = state->lock_type; lockobject *self = (lockobject *)type->tp_alloc(type, 0); if (self == NULL) { @@ -1384,12 +1382,13 @@ A subthread can use this function to interrupt the main thread.\n\ Note: the default signal handler for SIGINT raises ``KeyboardInterrupt``." ); -static lockobject *newlockobject(PyObject *module); +static lockobject *newlockobject(thread_module_state *); static PyObject * thread_PyThread_allocate_lock(PyObject *module, PyObject *Py_UNUSED(ignored)) { - return (PyObject *) newlockobject(module); + thread_module_state *state = get_thread_state(module); + return (PyObject *) newlockobject(state); } PyDoc_STRVAR(allocate_doc, @@ -1440,8 +1439,8 @@ particular thread within a system."); static PyObject * thread__count(PyObject *self, PyObject *Py_UNUSED(ignored)) { - PyInterpreterState *interp = _PyInterpreterState_GET(); - return PyLong_FromLong(interp->threads.count); + thread_module_state *state = get_thread_state(self); + return PyLong_FromLong(state->threads.num_running); } PyDoc_STRVAR(_count_doc, @@ -1482,6 +1481,7 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) { PyObject *wr; PyThreadState *tstate = _PyThreadState_GET(); + thread_module_state *state = get_thread_state(module); lockobject *lock; if (tstate->on_delete_data != NULL) { @@ -1493,7 +1493,7 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) tstate->on_delete_data = NULL; Py_DECREF(wr); } - lock = newlockobject(module); + lock = newlockobject(state); if (lock == NULL) return NULL; /* The lock is owned by whoever called _set_sentinel(), but the weakref From aa4d3bb0cef8a1cc7b9e6e9fac9fe3eae669c854 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 16:18:11 -0600 Subject: [PATCH 06/12] Create the thread lock in threading.py. --- Lib/threading.py | 3 ++- Modules/_threadmodule.c | 17 ++++++----------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/Lib/threading.py b/Lib/threading.py index df273870fa4273..21ba4fd5acd246 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -1024,7 +1024,8 @@ def _set_tstate_lock(self): Set a lock object which will be released by the interpreter when the underlying thread state (see pystate.h) gets deleted. """ - self._tstate_lock = _set_sentinel() + self._tstate_lock = _allocate_lock() + _set_sentinel(self._tstate_lock) self._tstate_lock.acquire() if not self.daemon: diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 959773d87310da..02343c3b4b659a 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1477,12 +1477,11 @@ release_sentinel(void *wr_raw) } static PyObject * -thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) +thread__set_sentinel(PyObject *module, PyObject *arg) { PyObject *wr; PyThreadState *tstate = _PyThreadState_GET(); - thread_module_state *state = get_thread_state(module); - lockobject *lock; + lockobject *lock = (lockobject *)arg; if (tstate->on_delete_data != NULL) { /* We must support the re-creation of the lock from a @@ -1493,25 +1492,21 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) tstate->on_delete_data = NULL; Py_DECREF(wr); } - lock = newlockobject(state); - if (lock == NULL) - return NULL; /* The lock is owned by whoever called _set_sentinel(), but the weakref hangs to the thread state. */ wr = PyWeakref_NewRef((PyObject *) lock, NULL); if (wr == NULL) { - Py_DECREF(lock); return NULL; } tstate->on_delete_data = (void *) wr; tstate->on_delete = &release_sentinel; - return (PyObject *) lock; + Py_RETURN_NONE; } PyDoc_STRVAR(_set_sentinel_doc, -"_set_sentinel() -> lock\n\ +"_set_sentinel(lock)\n\ \n\ -Set a sentinel lock that will be released when the current thread\n\ +Set the sentinel lock that will be released when the current thread\n\ state is finalized (after it is untied from the interpreter).\n\ \n\ This is a private API for the threading module."); @@ -1740,7 +1735,7 @@ static PyMethodDef thread_methods[] = { {"stack_size", (PyCFunction)thread_stack_size, METH_VARARGS, stack_size_doc}, {"_set_sentinel", thread__set_sentinel, - METH_NOARGS, _set_sentinel_doc}, + METH_O, _set_sentinel_doc}, {"_excepthook", thread_excepthook, METH_O, excepthook_doc}, {NULL, NULL} /* sentinel */ From 82ff2ba965113b5417c8fcd3af1a07e0e6f4eae1 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 17:53:51 -0600 Subject: [PATCH 07/12] Re-initialize the module threads lock upon fork. --- Lib/threading.py | 5 +++-- Modules/_threadmodule.c | 41 ++++++++++++++++++++++++++++++++++------- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/Lib/threading.py b/Lib/threading.py index 21ba4fd5acd246..acdd80fffa98e6 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -49,6 +49,7 @@ except AttributeError: _CRLock = None TIMEOUT_MAX = _thread.TIMEOUT_MAX +_internal_after_fork = _thread._after_fork del _thread @@ -1024,8 +1025,7 @@ def _set_tstate_lock(self): Set a lock object which will be released by the interpreter when the underlying thread state (see pystate.h) gets deleted. """ - self._tstate_lock = _allocate_lock() - _set_sentinel(self._tstate_lock) + self._tstate_lock = _set_sentinel() self._tstate_lock.acquire() if not self.daemon: @@ -1678,4 +1678,5 @@ def _after_fork(): if hasattr(_os, "register_at_fork"): + _os.register_at_fork(after_in_child=_internal_after_fork) _os.register_at_fork(after_in_child=_after_fork) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 02343c3b4b659a..16da0fb0a7d909 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -54,6 +54,16 @@ module_threads_init(struct module_threads *threads) return 0; } +static int +module_threads_reinit(struct module_threads *threads) +{ + if (_PyThread_at_fork_reinit(threads->mutex) < 0) { + PyErr_SetString(ThreadError, "failed to reinitialize lock at fork"); + return -1; + } + return 0; +} + static void module_threads_fini(struct module_threads *threads) { @@ -1477,11 +1487,12 @@ release_sentinel(void *wr_raw) } static PyObject * -thread__set_sentinel(PyObject *module, PyObject *arg) +thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) { PyObject *wr; PyThreadState *tstate = _PyThreadState_GET(); - lockobject *lock = (lockobject *)arg; + thread_module_state *state = get_thread_state(module); + lockobject *lock; if (tstate->on_delete_data != NULL) { /* We must support the re-creation of the lock from a @@ -1492,21 +1503,25 @@ thread__set_sentinel(PyObject *module, PyObject *arg) tstate->on_delete_data = NULL; Py_DECREF(wr); } + lock = newlockobject(state); + if (lock == NULL) + return NULL; /* The lock is owned by whoever called _set_sentinel(), but the weakref hangs to the thread state. */ wr = PyWeakref_NewRef((PyObject *) lock, NULL); if (wr == NULL) { + Py_DECREF(lock); return NULL; } tstate->on_delete_data = (void *) wr; tstate->on_delete = &release_sentinel; - Py_RETURN_NONE; + return (PyObject *) lock; } PyDoc_STRVAR(_set_sentinel_doc, -"_set_sentinel(lock)\n\ +"_set_sentinel() -> lock\n\ \n\ -Set the sentinel lock that will be released when the current thread\n\ +Set a sentinel lock that will be released when the current thread\n\ state is finalized (after it is untied from the interpreter).\n\ \n\ This is a private API for the threading module."); @@ -1707,6 +1722,16 @@ PyDoc_STRVAR(excepthook_doc, \n\ Handle uncaught Thread.run() exception."); +static PyObject * +thread__after_fork(PyObject *module, PyObject *Py_UNUSED(ignored)) +{ + thread_module_state *state = get_thread_state(module); + if (module_threads_reinit(&state->threads) < 0) { + return NULL; + } + Py_RETURN_NONE; +} + static PyMethodDef thread_methods[] = { {"start_new_thread", (PyCFunction)thread_PyThread_start_new_thread, METH_VARARGS, start_new_doc}, @@ -1735,9 +1760,11 @@ static PyMethodDef thread_methods[] = { {"stack_size", (PyCFunction)thread_stack_size, METH_VARARGS, stack_size_doc}, {"_set_sentinel", thread__set_sentinel, - METH_O, _set_sentinel_doc}, - {"_excepthook", thread_excepthook, + METH_NOARGS, _set_sentinel_doc}, + {"_excepthook", thread_excepthook, METH_O, excepthook_doc}, + {"_after_fork", thread__after_fork, + METH_NOARGS, NULL}, {NULL, NULL} /* sentinel */ }; From a19c68400c24bbe92a3e1bba5cb9fd3ab774512a Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 17:53:51 -0600 Subject: [PATCH 08/12] Add module_thread.running_lock. --- Modules/_threadmodule.c | 66 +++++++++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 16da0fb0a7d909..2551568f9d9f42 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -20,12 +20,16 @@ // Forward declarations static struct PyModuleDef thread_module; +struct module_state; +static struct lockobject *newlockobject(struct module_state *); /* threads owned by the modulo */ struct module_thread { PyThreadState *tstate; + /* This lock is released right after the Python code finishes. */ + PyObject *running_lock; struct module_thread *prev; struct module_thread *next; }; @@ -112,7 +116,25 @@ module_threads_remove(struct module_threads *threads, struct module_thread *mt) } static struct module_thread * -add_module_thread(struct module_threads *threads, PyThreadState *tstate) +module_threads_lookup(struct module_threads *threads, PyThreadState *tstate) +{ + PyThread_acquire_lock(threads->mutex, WAIT_LOCK); + + struct module_thread *mt = threads->head; + while (mt != NULL) { + if (mt->tstate == tstate) { + break; + } + mt = mt->next; + } + + PyThread_release_lock(threads->mutex); + return mt; +} + +static struct module_thread * +add_module_thread(struct module_state *state, struct module_threads *threads, + PyThreadState *tstate) { // Create the new list entry. struct module_thread *mt = PyMem_RawMalloc(sizeof(struct module_thread)); @@ -126,6 +148,13 @@ add_module_thread(struct module_threads *threads, PyThreadState *tstate) mt->prev = NULL; mt->next = NULL; + // Create the "running" lock. + mt->running_lock = (PyObject *) newlockobject(state); + if (mt->running_lock == NULL) { + PyErr_NoMemory(); + return NULL; + } + module_threads_add(threads, mt); return mt; @@ -146,6 +175,9 @@ module_thread_finished(struct module_threads *threads, struct module_thread *mt) { threads->num_running--; + // The threading module may keep this alive past here. + Py_CLEAR(mt->running_lock); + // XXX We should be notifying other threads here. } @@ -169,7 +201,7 @@ remove_module_thread(struct module_threads *threads, struct module_thread *mt) /* module state */ -typedef struct { +typedef struct module_state { struct module_threads threads; PyTypeObject *excepthook_type; @@ -189,7 +221,7 @@ get_thread_state(PyObject *module) /* Lock objects */ -typedef struct { +typedef struct lockobject { PyObject_HEAD PyThread_type_lock lock_lock; PyObject *in_weakreflist; @@ -1321,7 +1353,8 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) } thread_module_state *state = get_thread_state(self); boot->module_state = state; - boot->module_thread = add_module_thread(&state->threads, boot->tstate); + boot->module_thread = add_module_thread(state, &state->threads, + boot->tstate); if (boot->module_thread == NULL) { PyThreadState_Clear(boot->tstate); PyMem_Free(boot); @@ -1392,8 +1425,6 @@ A subthread can use this function to interrupt the main thread.\n\ Note: the default signal handler for SIGINT raises ``KeyboardInterrupt``." ); -static lockobject *newlockobject(thread_module_state *); - static PyObject * thread_PyThread_allocate_lock(PyObject *module, PyObject *Py_UNUSED(ignored)) { @@ -1492,7 +1523,20 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) PyObject *wr; PyThreadState *tstate = _PyThreadState_GET(); thread_module_state *state = get_thread_state(module); - lockobject *lock; + + PyObject *lock; + struct module_thread *mt = module_threads_lookup(&state->threads, tstate); + if (mt == NULL) { + /* It must be the "main" thread. */ + lock = (PyObject *) newlockobject(state); + if (lock == NULL) { + return NULL; + } + } + else { + assert(mt->running_lock != NULL); + lock = Py_NewRef(mt->running_lock); + } if (tstate->on_delete_data != NULL) { /* We must support the re-creation of the lock from a @@ -1503,19 +1547,17 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) tstate->on_delete_data = NULL; Py_DECREF(wr); } - lock = newlockobject(state); - if (lock == NULL) - return NULL; /* The lock is owned by whoever called _set_sentinel(), but the weakref hangs to the thread state. */ - wr = PyWeakref_NewRef((PyObject *) lock, NULL); + wr = PyWeakref_NewRef(lock, NULL); if (wr == NULL) { Py_DECREF(lock); return NULL; } tstate->on_delete_data = (void *) wr; tstate->on_delete = &release_sentinel; - return (PyObject *) lock; + + return lock; } PyDoc_STRVAR(_set_sentinel_doc, From de530b173522da45ecc6e0520915581d01eabe54 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 18:50:00 -0600 Subject: [PATCH 09/12] Be more specific in _wait_for_tstate_lock(). --- Lib/threading.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Lib/threading.py b/Lib/threading.py index acdd80fffa98e6..48e1550dbb85b0 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -1144,9 +1144,7 @@ def _wait_for_tstate_lock(self, block=True, timeout=-1): return try: - if lock.acquire(block, timeout): - lock.release() - self._stop() + locked = lock.acquire(block, timeout) except: if lock.locked(): # bpo-45274: lock.acquire() acquired the lock, but the function @@ -1156,6 +1154,9 @@ def _wait_for_tstate_lock(self, block=True, timeout=-1): lock.release() self._stop() raise + if locked: + lock.release() + self._stop() @property def name(self): From e2c7058abee4cd85c8d9e2773ec336edfe799267 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 19:12:37 -0600 Subject: [PATCH 10/12] Fix _set_sentinel(). --- Modules/_threadmodule.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 2551568f9d9f42..20afb7d2c4b3a9 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1551,7 +1551,9 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) hangs to the thread state. */ wr = PyWeakref_NewRef(lock, NULL); if (wr == NULL) { - Py_DECREF(lock); + if (mt == NULL) { + Py_DECREF(lock); + } return NULL; } tstate->on_delete_data = (void *) wr; From 0d30811396885f89deb94d8083bb4be980c19a6a Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 19:19:20 -0600 Subject: [PATCH 11/12] Track the "main" thread too. --- Modules/_threadmodule.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 20afb7d2c4b3a9..0b3904736d4799 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1528,15 +1528,15 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) struct module_thread *mt = module_threads_lookup(&state->threads, tstate); if (mt == NULL) { /* It must be the "main" thread. */ - lock = (PyObject *) newlockobject(state); - if (lock == NULL) { + mt = add_module_thread(state, &state->threads, tstate); + if (mt == NULL) { return NULL; } } else { assert(mt->running_lock != NULL); - lock = Py_NewRef(mt->running_lock); } + lock = Py_NewRef(mt->running_lock); if (tstate->on_delete_data != NULL) { /* We must support the re-creation of the lock from a @@ -1551,9 +1551,6 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) hangs to the thread state. */ wr = PyWeakref_NewRef(lock, NULL); if (wr == NULL) { - if (mt == NULL) { - Py_DECREF(lock); - } return NULL; } tstate->on_delete_data = (void *) wr; From 86e5d821b0911019b25b284d28e19efeeb19debe Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 15 May 2023 19:29:58 -0600 Subject: [PATCH 12/12] Drop tstate->on_delete. --- Include/cpython/pystate.h | 26 ---------------------- Modules/_threadmodule.c | 45 ++++++++++++++++++++++++++++----------- 2 files changed, 32 insertions(+), 39 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index f33c72d4cf4d2a..24d92f2b247284 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -188,32 +188,6 @@ struct _ts { struct _py_trashcan trash; - /* Called when a thread state is deleted normally, but not when it - * is destroyed after fork(). - * Pain: to prevent rare but fatal shutdown errors (issue 18808), - * Thread.join() must wait for the join'ed thread's tstate to be unlinked - * from the tstate chain. That happens at the end of a thread's life, - * in pystate.c. - * The obvious way doesn't quite work: create a lock which the tstate - * unlinking code releases, and have Thread.join() wait to acquire that - * lock. The problem is that we _are_ at the end of the thread's life: - * if the thread holds the last reference to the lock, decref'ing the - * lock will delete the lock, and that may trigger arbitrary Python code - * if there's a weakref, with a callback, to the lock. But by this time - * _PyRuntime.gilstate.tstate_current is already NULL, so only the simplest - * of C code can be allowed to run (in particular it must not be possible to - * release the GIL). - * So instead of holding the lock directly, the tstate holds a weakref to - * the lock: that's the value of on_delete_data below. Decref'ing a - * weakref is harmless. - * on_delete points to _threadmodule.c's static release_sentinel() function. - * After the tstate is unlinked, release_sentinel is called with the - * weakref-to-lock (on_delete_data) argument, and release_sentinel releases - * the indirectly held lock. - */ - void (*on_delete)(void *); - void *on_delete_data; - int coroutine_origin_tracking_depth; PyObject *async_gen_firstiter; diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 0b3904736d4799..172f49700123cd 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -30,6 +30,29 @@ struct module_thread { PyThreadState *tstate; /* This lock is released right after the Python code finishes. */ PyObject *running_lock; + /* Called when a thread state is deleted normally, but not when it + * is destroyed after fork(). + * Pain: to prevent rare but fatal shutdown errors (issue 18808), + * Thread.join() must wait for the join'ed thread's tstate to be unlinked + * from the tstate chain. That happens at the end of a thread's life, + * in pystate.c. + * The obvious way doesn't quite work: create a lock which the tstate + * unlinking code releases, and have Thread.join() wait to acquire that + * lock. The problem is that we _are_ at the end of the thread's life: + * if the thread holds the last reference to the lock, decref'ing the + * lock will delete the lock, and that may trigger arbitrary Python code + * if there's a weakref, with a callback, to the lock. But by this time + * _PyRuntime.gilstate.tstate_current is already NULL, so only the simplest + * of C code can be allowed to run (in particular it must not be possible to + * release the GIL). + * So instead of holding the lock directly, the tstate holds a weakref to + * the lock: that's the value of lock_weakref. Decref'ing a + * weakref is harmless. + * After the tstate is unlinked, release_sentinel is called with the + * weakref-to-lock (lock_weakref) argument, and release_sentinel releases + * the indirectly held lock. + */ + PyObject *running_lock_weakref; struct module_thread *prev; struct module_thread *next; }; @@ -154,6 +177,7 @@ add_module_thread(struct module_state *state, struct module_threads *threads, PyErr_NoMemory(); return NULL; } + mt->running_lock_weakref = NULL; module_threads_add(threads, mt); @@ -181,14 +205,15 @@ module_thread_finished(struct module_threads *threads, struct module_thread *mt) // XXX We should be notifying other threads here. } +static void release_sentinel(PyObject *wr); + static void remove_module_thread(struct module_threads *threads, struct module_thread *mt) { // Notify other threads that this one is done. // XXX This should happen in module_thread_finished(). - // XXX These fields could be removed from PyThreadState. - if (mt->tstate->on_delete != NULL) { - mt->tstate->on_delete(mt->tstate->on_delete_data); + if (mt->running_lock_weakref != NULL) { + release_sentinel(mt->running_lock_weakref); } // Remove it from the list. @@ -1497,9 +1522,8 @@ This function is meant for internal and specialized purposes only.\n\ In most applications `threading.enumerate()` should be used instead."); static void -release_sentinel(void *wr_raw) +release_sentinel(PyObject *wr) { - PyObject *wr = _PyObject_CAST(wr_raw); /* Tricky: this function is called when the current thread state is being deleted. Therefore, only simple C code can safely execute here. */ @@ -1538,14 +1562,10 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) } lock = Py_NewRef(mt->running_lock); - if (tstate->on_delete_data != NULL) { + if (mt->running_lock_weakref != NULL) { /* We must support the re-creation of the lock from a fork()ed child. */ - assert(tstate->on_delete == &release_sentinel); - wr = (PyObject *) tstate->on_delete_data; - tstate->on_delete = NULL; - tstate->on_delete_data = NULL; - Py_DECREF(wr); + Py_CLEAR(mt->running_lock_weakref); } /* The lock is owned by whoever called _set_sentinel(), but the weakref hangs to the thread state. */ @@ -1553,8 +1573,7 @@ thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) if (wr == NULL) { return NULL; } - tstate->on_delete_data = (void *) wr; - tstate->on_delete = &release_sentinel; + mt->running_lock_weakref = wr; return lock; }