From fb13cf6dfa4b5f0420f283823581bcc6e2c9839d Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 26 Jan 2024 14:54:51 -0800 Subject: [PATCH 01/22] Fix race in `Thread.join()` There is a race between when `Thread._tstate_lock` is released[^1] in `Thread._wait_for_tstate_lock()` and when `Thread._stop()` asserts[^2] that it is unlocked. Consider the following execution involving threads A, B, and C: 1. A starts. 2. B joins A, blocking on its `_tstate_lock`. 3. C joins A, blocking on its `_tstate_lock`. 4. A finishes and releases its `_tstate_lock`. 5. B acquires A's `_tstate_lock` in `_wait_for_tstate_lock()`, releases it, but is swapped out before calling `_stop()`. 6. C is scheduled, acquires A's `_tstate_lock` in `_wait_for_tstate_lock()` but is swapped out before releasing it. 7. B is scheduled, calls `_stop()`, which asserts that A's `_tstate_lock` is not held. However, C holds it, so the assertion fails. The race can be reproduced[^3] by inserting sleeps at the appropriate points in the threading code. To do so, run the `repro_join_race.py` from the linked repo. There are two main parts to this PR: 1. `_tstate_lock` is replaced with an event that is attached to `PyThreadState`. The event is set by the runtime prior to the thread being cleared (in the same place that `_tstate_lock` was released). `Thread.join()` blocks waiting for the event to be set. 2. `_PyInterpreterState_WaitForThreads()` provides the ability to wait for all non-daemon threads to exit. To do so, an `is_daemon` predicate was added to `PyThreadState`. This field is set each time a thread is created. `threading._shutdown()` now calls into `_PyInterpreterState_WaitForThreads()` instead of waiting on `_tstate_lock`s. [^1]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1201 [^2]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1115 [^3]: https://github.com/mpage/cpython/commit/81946532792f938cd6f6ab4c4ff92a4edf61314f --- Include/cpython/pystate.h | 32 +- Include/internal/pycore_interp.h | 1 + Include/internal/pycore_lock.h | 33 ++ Include/internal/pycore_pystate.h | 3 + Lib/test/test_audit.py | 4 +- .../test_process_pool.py | 4 +- Lib/test/test_threading.py | 53 +-- Lib/threading.py | 188 ++--------- Modules/_threadmodule.c | 305 +++++++++++++----- Python/lock.c | 7 + Python/pystate.c | 117 +++++-- 11 files changed, 414 insertions(+), 333 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 1dbf97660f382fd..35941f1ab37393c 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -61,6 +61,8 @@ struct _py_trashcan { PyObject *delete_later; }; +typedef struct _PyEventRc _PyEventRc; + struct _ts { /* See Python/ceval.c for comments explaining most fields */ @@ -156,31 +158,13 @@ struct _ts { */ uintptr_t critical_section; - /* 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. + /* Boolean storing whether or not this is a daemon thread. All non-daemon + * threads are joined prior to interpreter exit. */ - void (*on_delete)(void *); - void *on_delete_data; + int is_daemon; + + /* Set when the thread has finished execution and is about to be freed. */ + _PyEventRc *done_event; int coroutine_origin_tracking_depth; diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 04e75940dcb5734..666a0a2a6868be4 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -297,6 +297,7 @@ PyAPI_FUNC(PyInterpreterState *) _PyInterpreterState_LookUpID(int64_t); PyAPI_FUNC(int) _PyInterpreterState_IDInitref(PyInterpreterState *); PyAPI_FUNC(int) _PyInterpreterState_IDIncref(PyInterpreterState *); PyAPI_FUNC(void) _PyInterpreterState_IDDecref(PyInterpreterState *); +PyAPI_FUNC(void) _PyInterpreterState_WaitForThreads(PyInterpreterState *); extern const PyConfig* _PyInterpreterState_GetConfig(PyInterpreterState *interp); diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index 18a8896d97a5481..e96f45b6cf94ed1 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -139,6 +139,10 @@ typedef struct { // Export for '_testinternalcapi' shared extension PyAPI_FUNC(void) _PyEvent_Notify(PyEvent *evt); +// 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); + // Wait for the event to be set. If the event is already set, then this returns // immediately. PyAPI_FUNC(void) PyEvent_Wait(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/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 289ef28f0dd9a9c..0105812368389a5 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -214,6 +214,9 @@ static inline PyInterpreterState* _PyInterpreterState_GET(void) { extern PyThreadState * _PyThreadState_New( PyInterpreterState *interp, int whence); +extern PyThreadState * +_PyThreadState_NewWithEvent(PyInterpreterState *interp, int whence, + _PyEventRc *done_event); extern void _PyThreadState_Bind(PyThreadState *tstate); extern void _PyThreadState_DeleteExcept(PyThreadState *tstate); extern void _PyThreadState_ClearMimallocHeaps(PyThreadState *tstate); diff --git a/Lib/test/test_audit.py b/Lib/test/test_audit.py index cd0a4e2264865d3..77d516ebd6e1ab2 100644 --- a/Lib/test/test_audit.py +++ b/Lib/test/test_audit.py @@ -207,9 +207,9 @@ def test_threading(self): print(*events, sep='\n') actual = [(ev[0], ev[2]) for ev in events] expected = [ - ("_thread.start_new_thread", "(, (), None)"), + ("_thread.start_new_thread", "(, (), None, None, 0)"), ("test.test_func", "()"), - ("_thread.start_joinable_thread", "(,)"), + ("_thread.start_joinable_thread", "(, None, 0)"), ("test.test_func", "()"), ] diff --git a/Lib/test/test_concurrent_futures/test_process_pool.py b/Lib/test/test_concurrent_futures/test_process_pool.py index 3e61b0c9387c6fa..e20d85d750b9f88 100644 --- a/Lib/test/test_concurrent_futures/test_process_pool.py +++ b/Lib/test/test_concurrent_futures/test_process_pool.py @@ -200,13 +200,13 @@ def test_python_finalization_error(self): # QueueFeederThread. orig_start_new_thread = threading._start_joinable_thread nthread = 0 - def mock_start_new_thread(func, *args): + def mock_start_new_thread(func, *args, **kwargs): nonlocal nthread if nthread >= 1: raise RuntimeError("can't create new thread at " "interpreter shutdown") nthread += 1 - return orig_start_new_thread(func, *args) + return orig_start_new_thread(func, *args, **kwargs) with support.swap_attr(threading, '_start_joinable_thread', mock_start_new_thread): diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 1ab223b81e939e5..dd6df592da6705d 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -406,7 +406,7 @@ def run(self): def test_limbo_cleanup(self): # Issue 7481: Failure to start thread should cleanup the limbo map. - def fail_new_thread(*args): + def fail_new_thread(*args, **kwargs): raise threading.ThreadError() _start_joinable_thread = threading._start_joinable_thread threading._start_joinable_thread = fail_new_thread @@ -901,7 +901,8 @@ def f(): rc, out, err = assert_python_ok("-c", code) self.assertEqual(err, b"") - def test_tstate_lock(self): + @cpython_only + def test_done_event(self): # Test an implementation detail of Thread objects. started = _thread.allocate_lock() finish = _thread.allocate_lock() @@ -911,29 +912,17 @@ def f(): started.release() finish.acquire() time.sleep(0.01) - # The tstate lock is None until the thread is started + # The done event is not set if the thread hasn't started t = threading.Thread(target=f) - self.assertIs(t._tstate_lock, None) + self.assertFalse(t._done_event.is_set()) t.start() started.acquire() self.assertTrue(t.is_alive()) - # The tstate lock can't be acquired when the thread is running - # (or suspended). - tstate_lock = t._tstate_lock - self.assertFalse(tstate_lock.acquire(timeout=0), False) + # The done event is not set while the thread is alive + self.assertFalse(t._done_event.wait(0), False) finish.release() - # When the thread ends, the state_lock can be successfully - # acquired. - self.assertTrue(tstate_lock.acquire(timeout=support.SHORT_TIMEOUT), False) - # But is_alive() is still True: we hold _tstate_lock now, which - # prevents is_alive() from knowing the thread's end-of-life C code - # is done. - self.assertTrue(t.is_alive()) - # Let is_alive() find out the C code is done. - tstate_lock.release() - self.assertFalse(t.is_alive()) - # And verify the thread disposed of _tstate_lock. - self.assertIsNone(t._tstate_lock) + # When the thread ends, the done event is set. + self.assertTrue(t._done_event.wait(support.SHORT_TIMEOUT), False) t.join() def test_repr_stopped(self): @@ -1101,30 +1090,6 @@ def checker(): self.assertEqual(threading.getprofile(), old_profile) self.assertEqual(sys.getprofile(), old_profile) - @cpython_only - def test_shutdown_locks(self): - for daemon in (False, True): - with self.subTest(daemon=daemon): - event = threading.Event() - thread = threading.Thread(target=event.wait, daemon=daemon) - - # Thread.start() must add lock to _shutdown_locks, - # but only for non-daemon thread - thread.start() - tstate_lock = thread._tstate_lock - if not daemon: - self.assertIn(tstate_lock, threading._shutdown_locks) - else: - self.assertNotIn(tstate_lock, threading._shutdown_locks) - - # unblock the thread and join it - event.set() - thread.join() - - # Thread._stop() must remove tstate_lock from _shutdown_locks. - # Daemon threads must never add it to _shutdown_locks. - self.assertNotIn(tstate_lock, threading._shutdown_locks) - def test_locals_at_exit(self): # bpo-19466: thread locals must not be deleted before destructors # are called diff --git a/Lib/threading.py b/Lib/threading.py index 75a08e5aac97d60..6637f0bf3478df6 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -36,7 +36,9 @@ _daemon_threads_allowed = _thread.daemon_threads_allowed _allocate_lock = _thread.allocate_lock _LockType = _thread.LockType -_set_sentinel = _thread._set_sentinel +_Event = _thread.Event +_get_done_event = _thread._get_done_event +_thread_shutdown = _thread._shutdown get_ident = _thread.get_ident _is_main_interpreter = _thread._is_main_interpreter try: @@ -847,25 +849,6 @@ def _newname(name_template): _limbo = {} _dangling = WeakSet() -# Set of Thread._tstate_lock locks of non-daemon threads used by _shutdown() -# to wait until all Python thread states get deleted: -# see Thread._set_tstate_lock(). -_shutdown_locks_lock = _allocate_lock() -_shutdown_locks = set() - -def _maintain_shutdown_locks(): - """ - Drop any shutdown locks that don't correspond to running threads anymore. - - Calling this from time to time avoids an ever-growing _shutdown_locks - set when Thread objects are not joined explicitly. See bpo-37788. - - This must be called with _shutdown_locks_lock acquired. - """ - # If a lock was released, the corresponding thread has exited - to_remove = [lock for lock in _shutdown_locks if not lock.locked()] - _shutdown_locks.difference_update(to_remove) - # Main class for threads @@ -930,11 +913,10 @@ class is implemented. self._ident = None 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 + self._done_event = _Event() self._initialized = True # Copy of sys.stderr used by self._invoke_excepthook() self._stderr = _sys.stderr @@ -951,19 +933,12 @@ def _after_fork(self, new_ident=None): if self._handle is not None: self._handle.after_fork_alive() assert self._handle.ident == new_ident - # bpo-42350: If the fork happens when the thread is already stopped - # (ex: after threading._shutdown() has been called), _tstate_lock - # is None. Do nothing in this case. - 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._done_event.set() self._join_lock = None if self._handle is not None: self._handle.after_fork_dead() @@ -974,8 +949,7 @@ def __repr__(self): status = "initial" if self._started.is_set(): status = "started" - self.is_alive() # easy way to get ._is_stopped set when appropriate - if self._is_stopped: + if self._done_event.is_set(): status = "stopped" if self._daemonic: status += " daemon" @@ -1005,7 +979,7 @@ def start(self): _limbo[self] = self try: # Start joinable thread - self._handle = _start_joinable_thread(self._bootstrap) + self._handle = _start_joinable_thread(self._bootstrap, done_event=self._done_event, daemon=self.daemon) except Exception: with _active_limbo_lock: del _limbo[self] @@ -1056,23 +1030,9 @@ def _set_ident(self): def _set_native_id(self): self._native_id = get_native_id() - 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.acquire() - - if not self.daemon: - with _shutdown_locks_lock: - _maintain_shutdown_locks() - _shutdown_locks.add(self._tstate_lock) - def _bootstrap_inner(self): try: self._set_ident() - self._set_tstate_lock() if _HAVE_THREAD_NATIVE_ID: self._set_native_id() self._started.set() @@ -1092,33 +1052,6 @@ def _bootstrap_inner(self): finally: self._delete() - def _stop(self): - # After calling ._stop(), .is_alive() returns False and .join() returns - # immediately. ._tstate_lock must be released before calling ._stop(). - # - # Normal case: C code at the end of the thread's life - # (release_sentinel in _threadmodule.c) releases ._tstate_lock, and - # that's detected by our ._wait_for_tstate_lock(), called by .join() - # and .is_alive(). Any number of threads _may_ call ._stop() - # simultaneously (for example, if multiple threads are blocked in - # .join() calls), and they're not serialized. That's harmless - - # they'll just make redundant rebindings of ._is_stopped and - # ._tstate_lock. Obscure: we rebind ._tstate_lock last so that the - # "assert self._is_stopped" in ._wait_for_tstate_lock() always works - # (the assert is executed only if ._tstate_lock is None). - # - # Special case: _main_thread releases ._tstate_lock via this - # module's _shutdown() function. - lock = self._tstate_lock - if lock is not None: - assert not lock.locked() - self._is_stopped = True - self._tstate_lock = None - if not self.daemon: - with _shutdown_locks_lock: - # Remove our lock and other released locks from _shutdown_locks - _maintain_shutdown_locks() - def _delete(self): "Remove current thread from the dict of currently running threads." with _active_limbo_lock: @@ -1159,14 +1092,13 @@ def join(self, timeout=None): if self is current_thread(): raise RuntimeError("cannot join current thread") - if timeout is None: - self._wait_for_tstate_lock() - else: - # the behavior of a negative timeout isn't documented, but - # historically .join(timeout=x) for x<0 has acted as if timeout=0 - self._wait_for_tstate_lock(timeout=max(timeout, 0)) + # the behavior of a negative timeout isn't documented, but + # historically .join(timeout=x) for x<0 has acted as if timeout=0 + if timeout is not None: + timeout = max(timeout, 0) + self._done_event.wait(timeout) - if self._is_stopped: + if self._done_event.is_set(): self._join_os_thread() def _join_os_thread(self): @@ -1182,33 +1114,6 @@ def _join_os_thread(self): # No need to keep this around self._join_lock = None - def _wait_for_tstate_lock(self, block=True, timeout=-1): - # Issue #18808: wait for the thread state to be gone. - # At the end of the thread's life, after all knowledge of the thread - # is removed from C data structures, C code releases our _tstate_lock. - # This method passes its arguments to _tstate_lock.acquire(). - # If the lock is acquired, the C code is done, and self._stop() is - # called. That sets ._is_stopped to True, and ._tstate_lock to None. - lock = self._tstate_lock - if lock is None: - # already determined that the C code is done - assert self._is_stopped - return - - try: - if lock.acquire(block, timeout): - lock.release() - self._stop() - except: - if lock.locked(): - # bpo-45274: lock.acquire() acquired the lock, but the function - # was interrupted with an exception before reaching the - # lock.release(). It can happen if a signal handler raises an - # exception, like CTRL+C which raises KeyboardInterrupt. - lock.release() - self._stop() - raise - @property def name(self): """A string used for identification purposes only. @@ -1258,13 +1163,7 @@ def is_alive(self): """ assert self._initialized, "Thread.__init__() not called" - if self._is_stopped or not self._started.is_set(): - return False - self._wait_for_tstate_lock(False) - if not self._is_stopped: - return True - self._join_os_thread() - return False + return self._started.is_set() and not self._done_event.is_set() @property def daemon(self): @@ -1473,7 +1372,7 @@ class _MainThread(Thread): def __init__(self): Thread.__init__(self, name="MainThread", daemon=False) - self._set_tstate_lock() + self._done_event = _get_done_event() self._started.set() self._set_ident() if _HAVE_THREAD_NATIVE_ID: @@ -1521,6 +1420,7 @@ class _DummyThread(Thread): def __init__(self): Thread.__init__(self, name=_newname("Dummy-%d"), daemon=_daemon_threads_allowed()) + self._done_event = _get_done_event() self._started.set() self._set_ident() if _HAVE_THREAD_NATIVE_ID: @@ -1529,11 +1429,8 @@ def __init__(self): _active[self._ident] = self _DeleteDummyThreadOnDel(self) - def _stop(self): - pass - def is_alive(self): - if not self._is_stopped and self._started.is_set(): + if self._started.is_set() and not self._done_event.is_set(): return True raise RuntimeError("thread is not alive") @@ -1545,7 +1442,6 @@ def _after_fork(self, new_ident=None): self.__class__ = _MainThread self._name = 'MainThread' self._daemonic = False - self._set_tstate_lock() Thread._after_fork(self, new_ident=new_ident) @@ -1644,16 +1540,15 @@ def _shutdown(): """ Wait until the Python thread state of all non-daemon threads get deleted. """ + global _SHUTTING_DOWN # Obscure: other threads may be waiting to join _main_thread. That's - # dubious, but some code does it. We can't wait for C code to release - # the main thread's tstate_lock - that won't happen until the interpreter - # is nearly dead. So we release it here. Note that just calling _stop() - # isn't enough: other threads may already be waiting on _tstate_lock. - if _main_thread._is_stopped and _is_main_interpreter(): + # dubious, but some code does it. We can't wait for C code to set + # the main thread's done_event - that won't happen until the interpreter + # is nearly dead. So we set it here. + if _main_thread._done_event.is_set() and _is_main_interpreter() and _SHUTTING_DOWN: # _shutdown() was already called return - global _SHUTTING_DOWN _SHUTTING_DOWN = True # Call registered threading atexit functions before threads are joined. @@ -1661,18 +1556,11 @@ def _shutdown(): for atexit_call in reversed(_threading_atexits): atexit_call() - # Main thread if _main_thread.ident == get_ident(): - tlock = _main_thread._tstate_lock - # The main thread isn't finished yet, so its thread state lock can't - # have been released. - assert tlock is not None - if tlock.locked(): - # It should have been released already by - # _PyInterpreterState_SetNotRunningMain(), but there may be - # embedders that aren't calling that yet. - tlock.release() - _main_thread._stop() + # It should have been set already by + # _PyInterpreterState_SetNotRunningMain(), but there may be embedders + # that aren't calling that yet. + _main_thread._done_event.set() else: # bpo-1596321: _shutdown() must be called in the main thread. # If the threading module was not imported by the main thread, @@ -1681,22 +1569,8 @@ def _shutdown(): # spawned by C libraries or using _thread.start_new_thread(). pass - # Join all non-deamon threads - while True: - with _shutdown_locks_lock: - locks = list(_shutdown_locks) - _shutdown_locks.clear() - - if not locks: - break - - for lock in locks: - # mimic Thread.join() - lock.acquire() - lock.release() - - # new threads can be spawned while we were waiting for the other - # threads to complete + # Wait for all non-daemon threads to exit. + _thread_shutdown() def main_thread(): @@ -1716,7 +1590,6 @@ def _after_fork(): # Reset _active_limbo_lock, in case we forked while the lock was held # by another (non-forked) thread. http://bugs.python.org/issue874900 global _active_limbo_lock, _main_thread - global _shutdown_locks_lock, _shutdown_locks _active_limbo_lock = RLock() # fork() only copied the current thread; clear references to others. @@ -1732,10 +1605,6 @@ def _after_fork(): _main_thread = current - # reset _shutdown() locks: threads re-register their _tstate_lock below - _shutdown_locks_lock = _allocate_lock() - _shutdown_locks = set() - with _active_limbo_lock: # Dangling thread instances must still have their locks reset, # because someone may join() them. @@ -1752,7 +1621,6 @@ def _after_fork(): else: # All the others are already stopped. thread._after_fork() - thread._stop() _limbo.clear() _active.clear() diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 5cceb84658deb78..3c6c3a92c8e906e 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -4,6 +4,7 @@ #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" @@ -30,6 +31,7 @@ typedef struct { PyTypeObject *local_type; PyTypeObject *local_dummy_type; PyTypeObject *thread_handle_type; + PyTypeObject *event_type; } thread_module_state; static inline thread_module_state* @@ -40,6 +42,124 @@ get_thread_state(PyObject *module) return (thread_module_state *)state; } +// Event type + +typedef struct { + PyObject_HEAD + _PyEventRc *event; +} PyEventObject; + +static void +event_dealloc(PyEventObject *self) +{ + if (self->event) { + _PyEventRc_Decref(self->event); + self->event = NULL; + } + Py_TYPE(self)->tp_free(self); +} + +static PyObject * +wrap_event(PyTypeObject *type, _PyEventRc *event) +{ + PyEventObject *self; + self = (PyEventObject *)type->tp_alloc(type, 0); + if (self != NULL) { + self->event = event; + _PyEventRc_Incref(event); + } + return (PyObject *)self; +} + +static PyObject * +event_new(PyTypeObject *type, PyObject *args, PyObject *kwds) +{ + _PyEventRc *event = _PyEventRc_New(); + if (event == NULL) { + PyErr_SetString(ThreadError, "can't allocate event"); + return NULL; + } + PyObject *result = wrap_event(type, event); + _PyEventRc_Decref(event); + return result; +} + +static PyObject * +event_repr(PyEventObject *self) +{ + int is_set = _PyEvent_IsSet(&self->event->event); + return PyUnicode_FromFormat("<_thread.Event object is_set=%d at %p>", + is_set, self); +} + +static PyObject * +event_is_set(PyEventObject *self, PyObject *Py_UNUSED(ignored)) +{ + if (_PyEvent_IsSet(&self->event->event)) { + Py_RETURN_TRUE; + } + else { + Py_RETURN_FALSE; + } +} + +static PyObject * +event_set(PyEventObject *self, PyObject *Py_UNUSED(ignored)) +{ + _PyEvent_Notify(&self->event->event); + Py_RETURN_NONE; +} + +static PyObject * +event_wait(PyEventObject *self, PyObject *args) +{ + PyObject *timeout_obj = NULL; + if (!PyArg_ParseTuple(args, "|O:wait", &timeout_obj)) { + return NULL; + } + + _PyTime_t timeout_ns = -1; + if (timeout_obj && timeout_obj != Py_None) { + int err = _PyTime_FromSecondsObject(&timeout_ns, timeout_obj, + _PyTime_ROUND_TIMEOUT); + + if (err < 0) { + return NULL; + } + } + + int ok = PyEvent_WaitTimed(&self->event->event, timeout_ns); + if (ok) { + Py_RETURN_TRUE; + } + else { + Py_RETURN_FALSE; + } +} + +static PyMethodDef event_methods[] = { + {"is_set", (PyCFunction)event_is_set, METH_NOARGS, NULL}, + {"set", (PyCFunction)event_set, METH_NOARGS, NULL}, + {"wait", (PyCFunction)event_wait, METH_VARARGS, NULL}, + {NULL, NULL} /* sentinel */ +}; + +static PyType_Slot event_type_slots[] = { + {Py_tp_repr, (reprfunc)event_repr}, + {Py_tp_methods, event_methods}, + {Py_tp_alloc, PyType_GenericAlloc}, + {Py_tp_dealloc, (destructor)event_dealloc}, + {Py_tp_new, event_new}, + {0, 0}, +}; + +static PyType_Spec event_type_spec = { + .name = "_thread.Event", + .basicsize = sizeof(PyEventObject), + .flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_IMMUTABLETYPE), + .slots = event_type_slots, +}; + // _ThreadHandle type typedef struct { @@ -1272,11 +1392,24 @@ PyDoc_STRVAR(daemon_threads_allowed_doc, Return True if daemon threads are allowed in the current interpreter,\n\ and False otherwise.\n"); +static _PyEventRc * +unpack_or_create_event(PyObject *op) +{ + if (op) { + _PyEventRc *event = ((PyEventObject *)op)->event; + _PyEventRc_Incref(event); + return event; + } + else { + return _PyEventRc_New(); + } +} + 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) +do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args, + PyObject *kwargs, int joinable, PyThread_ident_t *ident, + PyThread_handle_t *handle, PyObject *done_event, + int daemon) { PyInterpreterState *interp = _PyInterpreterState_GET(); if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_THREADS)) { @@ -1298,7 +1431,15 @@ do_start_new_thread(thread_module_state* state, PyErr_NoMemory(); return -1; } - boot->tstate = _PyThreadState_New(interp, _PyThreadState_WHENCE_THREADING); + _PyEventRc *event = unpack_or_create_event(done_event); + if (event == NULL) { + PyMem_RawFree(boot); + PyErr_NoMemory(); + return -1; + } + boot->tstate = _PyThreadState_NewWithEvent( + interp, _PyThreadState_WHENCE_THREADING, event); + _PyEventRc_Decref(event); if (boot->tstate == NULL) { PyMem_RawFree(boot); if (!PyErr_Occurred()) { @@ -1306,6 +1447,7 @@ do_start_new_thread(thread_module_state* state, } return -1; } + boot->tstate->is_daemon = daemon; boot->func = Py_NewRef(func); boot->args = Py_NewRef(args); boot->kwargs = Py_XNewRef(kwargs); @@ -1328,14 +1470,20 @@ do_start_new_thread(thread_module_state* state, } static PyObject * -thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs) +thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs, + PyObject *fkwargs) { + PyObject *done_event = NULL; + static char *keywords[] = {"function", "args", "kwargs", + "done_event", "daemon", NULL}; PyObject *func, *args, *kwargs = NULL; + int daemon = 0; thread_module_state *state = get_thread_state(module); - - if (!PyArg_UnpackTuple(fargs, "start_new_thread", 2, 3, - &func, &args, &kwargs)) + if (!PyArg_ParseTupleAndKeywords( + fargs, fkwargs, "OO|OO!p:start_new_thread", keywords, &func, &args, + &kwargs, state->event_type, &done_event)) { return NULL; + } if (!PyCallable_Check(func)) { PyErr_SetString(PyExc_TypeError, "first arg must be callable"); @@ -1352,22 +1500,23 @@ thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs) return NULL; } - if (PySys_Audit("_thread.start_new_thread", "OOO", - func, args, kwargs ? kwargs : Py_None) < 0) { + if (PySys_Audit("_thread.start_new_thread", "OOOOi", func, args, + kwargs ? kwargs : Py_None, + done_event ? done_event : Py_None, daemon) < 0) { return NULL; } PyThread_ident_t ident = 0; PyThread_handle_t handle; if (do_start_new_thread(state, func, args, kwargs, /*joinable=*/ 0, - &ident, &handle)) { + &ident, &handle, done_event, daemon)) { return NULL; } return PyLong_FromUnsignedLongLong(ident); } PyDoc_STRVAR(start_new_doc, -"start_new_thread(function, args[, kwargs])\n\ +"start_new_thread(function, args[, kwargs[, done_event[, daemon]]])\n\ (start_new() is an obsolete synonym)\n\ \n\ Start a new thread and return its identifier.\n\ @@ -1377,12 +1526,24 @@ tuple args and keyword arguments taken from the optional dictionary\n\ kwargs. The thread exits when the function returns; the return value\n\ is ignored. The thread will also exit when the function raises an\n\ unhandled exception; a stack trace will be printed unless the exception\n\ -is SystemExit.\n"); +is SystemExit. The optional done_event will be set when the thread is\n\ +finished. The runtime will not wait for the thread to exit when shutting\n\ +down if daemon is truthy.\n"); static PyObject * -thread_PyThread_start_joinable_thread(PyObject *module, PyObject *func) +thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, + PyObject *fkwargs) { + PyObject *done_event = NULL; + static char *keywords[] = {"function", "done_event", "daemon", NULL}; + PyObject *func = NULL; + int daemon = 0; thread_module_state *state = get_thread_state(module); + if (!PyArg_ParseTupleAndKeywords( + fargs, fkwargs, "O|O!p:start_joinable_thread", keywords, &func, + state->event_type, &done_event, &daemon)) { + return NULL; + } if (!PyCallable_Check(func)) { PyErr_SetString(PyExc_TypeError, @@ -1390,7 +1551,8 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *func) return NULL; } - if (PySys_Audit("_thread.start_joinable_thread", "O", func) < 0) { + if (PySys_Audit("_thread.start_joinable_thread", "OOi", func, + done_event ? done_event : Py_None, daemon) < 0) { return NULL; } @@ -1404,7 +1566,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, done_event, daemon)) { Py_DECREF(args); Py_DECREF(hobj); return NULL; @@ -1415,7 +1577,7 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *func) } PyDoc_STRVAR(start_joinable_doc, -"start_joinable_thread(function)\n\ +"start_joinable_thread(function[, done_event[, daemon]])\n\ \n\ *For internal use only*: start a new thread.\n\ \n\ @@ -1423,7 +1585,9 @@ Like start_new_thread(), this starts a new thread calling the given function.\n\ Unlike start_new_thread(), this returns a handle object with methods to join\n\ or detach the given thread.\n\ This function is not for third-party code, please use the\n\ -`threading` module instead.\n"); +`threading` module instead. The optional done_event will be set when the thread\n\ +is finished. During finalization the runtime will not wait for the thread\n\ +to exit if daemon is truthy.\n"); static PyObject * thread_PyThread_exit_thread(PyObject *self, PyObject *Py_UNUSED(ignored)) @@ -1535,64 +1699,18 @@ yet finished.\n\ 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 *weakref_raw) -{ - PyObject *weakref = _PyObject_CAST(weakref_raw); - - /* Tricky: this function is called when the current thread state - is being deleted. Therefore, only simple C code can safely - execute here. */ - lockobject *lock = (lockobject *)_PyWeakref_GET_REF(weakref); - if (lock != NULL) { - if (lock->locked) { - PyThread_release_lock(lock->lock_lock); - lock->locked = 0; - } - Py_DECREF(lock); - } - - /* Deallocating a weakref with a NULL callback only calls - PyObject_GC_Del(), which can't call any Python code. */ - Py_DECREF(weakref); -} - static PyObject * -thread__set_sentinel(PyObject *module, PyObject *Py_UNUSED(ignored)) +thread__get_done_event(PyObject *module, PyObject *Py_UNUSED(ignore)) { - PyObject *wr; - PyThreadState *tstate = _PyThreadState_GET(); - lockobject *lock; - - if (tstate->on_delete_data != 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); - } - lock = newlockobject(module); - 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; + PyThreadState *tstate = PyThreadState_Get(); + thread_module_state *state = get_thread_state(module); + return wrap_event(state->event_type, tstate->done_event); } -PyDoc_STRVAR(_set_sentinel_doc, -"_set_sentinel() -> lock\n\ +PyDoc_STRVAR(_get_done_event_doc, +"_get_done_event() -> _thread.Event\n\ \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\ +Return the done event for the current thread.\n\ \n\ This is a private API for the threading module."); @@ -1802,13 +1920,26 @@ PyDoc_STRVAR(thread__is_main_interpreter_doc, \n\ Return True if the current interpreter is the main Python interpreter."); +static PyObject * +thread_shutdown(PyObject *self, PyObject *args) +{ + PyInterpreterState *interp = PyInterpreterState_Get(); + _PyInterpreterState_WaitForThreads(interp); + Py_RETURN_NONE; +} + +PyDoc_STRVAR(shutdown_doc, +"_shutdown()\n\ +\n\ +Waits for all non-daemon threads (other than the calling thread) to stop."); + static PyMethodDef thread_methods[] = { - {"start_new_thread", (PyCFunction)thread_PyThread_start_new_thread, - METH_VARARGS, start_new_doc}, - {"start_new", (PyCFunction)thread_PyThread_start_new_thread, - METH_VARARGS, start_new_doc}, - {"start_joinable_thread", (PyCFunction)thread_PyThread_start_joinable_thread, - METH_O, start_joinable_doc}, + {"start_new_thread", _PyCFunction_CAST(thread_PyThread_start_new_thread), + METH_VARARGS | METH_KEYWORDS, start_new_doc}, + {"start_new", _PyCFunction_CAST(thread_PyThread_start_new_thread), + METH_VARARGS | METH_KEYWORDS, start_new_doc}, + {"start_joinable_thread", _PyCFunction_CAST(thread_PyThread_start_joinable_thread), + METH_VARARGS | METH_KEYWORDS, start_joinable_doc}, {"daemon_threads_allowed", (PyCFunction)thread_daemon_threads_allowed, METH_NOARGS, daemon_threads_allowed_doc}, {"allocate_lock", thread_PyThread_allocate_lock, @@ -1821,6 +1952,8 @@ static PyMethodDef thread_methods[] = { METH_NOARGS, exit_doc}, {"interrupt_main", (PyCFunction)thread_PyThread_interrupt_main, METH_VARARGS, interrupt_doc}, + {"_get_done_event", thread__get_done_event, + METH_NOARGS, _get_done_event_doc}, {"get_ident", thread_get_ident, METH_NOARGS, get_ident_doc}, #ifdef PY_HAVE_THREAD_NATIVE_ID @@ -1831,12 +1964,12 @@ static PyMethodDef thread_methods[] = { METH_NOARGS, _count_doc}, {"stack_size", (PyCFunction)thread_stack_size, METH_VARARGS, stack_size_doc}, - {"_set_sentinel", thread__set_sentinel, - METH_NOARGS, _set_sentinel_doc}, {"_excepthook", thread_excepthook, METH_O, excepthook_doc}, {"_is_main_interpreter", thread__is_main_interpreter, METH_NOARGS, thread__is_main_interpreter_doc}, + {"_shutdown", thread_shutdown, + METH_NOARGS, shutdown_doc}, {NULL, NULL} /* sentinel */ }; @@ -1926,6 +2059,16 @@ thread_module_exec(PyObject *module) return -1; } + // Event + state->event_type = (PyTypeObject *)PyType_FromModuleAndSpec( + module, &event_type_spec, NULL); + if (state->event_type == NULL) { + return -1; + } + if (PyModule_AddType(module, state->event_type) < 0) { + return -1; + } + return 0; } @@ -1939,6 +2082,7 @@ thread_module_traverse(PyObject *module, visitproc visit, void *arg) Py_VISIT(state->local_type); Py_VISIT(state->local_dummy_type); Py_VISIT(state->thread_handle_type); + Py_VISIT(state->event_type); return 0; } @@ -1951,6 +2095,7 @@ thread_module_clear(PyObject *module) Py_CLEAR(state->local_type); Py_CLEAR(state->local_dummy_type); Py_CLEAR(state->thread_handle_type); + Py_CLEAR(state->event_type); return 0; } diff --git a/Python/lock.c b/Python/lock.c index f0ff1176941da8f..dd6c9103b825c92 100644 --- a/Python/lock.c +++ b/Python/lock.c @@ -266,6 +266,13 @@ _PyEvent_Notify(PyEvent *evt) } } +int +_PyEvent_IsSet(PyEvent *evt) +{ + uint8_t v = _Py_atomic_load_uint8(&evt->v); + return v == _Py_LOCKED; +} + void PyEvent_Wait(PyEvent *evt) { diff --git a/Python/pystate.c b/Python/pystate.c index 430121a6a35d7f9..c0e2a8f7bddd85e 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1026,15 +1026,13 @@ _PyInterpreterState_SetNotRunningMain(PyInterpreterState *interp) PyThreadState *tstate = interp->threads.main; assert(tstate == current_fast_get()); - if (tstate->on_delete != NULL) { + if (tstate->done_event != NULL) { // The threading module was imported for the first time in this // thread, so it was set as threading._main_thread. (See gh-75698.) // The thread has finished running the Python program so we mark // the thread object as finished. assert(tstate->_whence != _PyThreadState_WHENCE_THREADING); - tstate->on_delete(tstate->on_delete_data); - tstate->on_delete = NULL; - tstate->on_delete_data = NULL; + _PyEvent_Notify(&tstate->done_event->event); } interp->threads.main = NULL; @@ -1128,6 +1126,53 @@ _PyInterpreterState_IDDecref(PyInterpreterState *interp) } } +void +_PyInterpreterState_WaitForThreads(PyInterpreterState *interp) +{ + _PyRuntimeState *runtime = &_PyRuntime; + PyThreadState *tstate = _PyThreadState_GET(); + + for (;;) { + _PyEventRc *done_event = NULL; + + // Find a thread that's not yet finished. + HEAD_LOCK(runtime); + for (PyThreadState *p = interp->threads.head; p != NULL; p = p->next) { + if (p == tstate) { + continue; + } + if (p->done_event && !p->is_daemon) { + done_event = p->done_event; + _PyEventRc_Incref(done_event); + break; + } + } + HEAD_UNLOCK(runtime); + + if (!done_event) { + // No more non-daemon threads to wait on! + break; + } + + // Wait for the other thread to finish. If we're interrupted, such + // as by a ctrl-c we print the error and exit early. + for (;;) { + if (PyEvent_WaitTimed(&done_event->event, -1)) { + break; + } + + // interrupted + if (Py_MakePendingCalls() < 0) { + PyErr_WriteUnraisable(NULL); + _PyEventRc_Decref(done_event); + return; + } + } + + _PyEventRc_Decref(done_event); + } +} + int _PyInterpreterState_RequiresIDRef(PyInterpreterState *interp) { @@ -1294,8 +1339,8 @@ free_threadstate(_PyThreadStateImpl *tstate) */ static void -init_threadstate(_PyThreadStateImpl *_tstate, - PyInterpreterState *interp, uint64_t id, int whence) +init_threadstate(_PyThreadStateImpl *_tstate, PyInterpreterState *interp, + uint64_t id, int whence, _PyEventRc *done_event) { PyThreadState *tstate = (PyThreadState *)_tstate; if (tstate->_status.initialized) { @@ -1339,6 +1384,10 @@ init_threadstate(_PyThreadStateImpl *_tstate, tstate->state = _Py_THREAD_SUSPENDED; } + _PyEventRc_Incref(done_event); + tstate->is_daemon = (id > 1); + tstate->done_event = done_event; + tstate->_status.initialized = 1; } @@ -1357,7 +1406,7 @@ add_threadstate(PyInterpreterState *interp, PyThreadState *tstate, } static PyThreadState * -new_threadstate(PyInterpreterState *interp, int whence) +new_threadstate(PyInterpreterState *interp, int whence, _PyEventRc *done_event) { _PyThreadStateImpl *tstate; _PyRuntimeState *runtime = interp->runtime; @@ -1396,7 +1445,7 @@ new_threadstate(PyInterpreterState *interp, int whence) sizeof(*tstate)); } - init_threadstate(tstate, interp, id, whence); + init_threadstate(tstate, interp, id, whence, done_event); add_threadstate(interp, (PyThreadState *)tstate, old_head); HEAD_UNLOCK(runtime); @@ -1410,8 +1459,8 @@ new_threadstate(PyInterpreterState *interp, int whence) PyThreadState * PyThreadState_New(PyInterpreterState *interp) { - PyThreadState *tstate = new_threadstate(interp, - _PyThreadState_WHENCE_UNKNOWN); + PyThreadState *tstate = + _PyThreadState_New(interp, _PyThreadState_WHENCE_UNKNOWN); if (tstate) { bind_tstate(tstate); // This makes sure there's a gilstate tstate bound @@ -1427,7 +1476,24 @@ PyThreadState_New(PyInterpreterState *interp) PyThreadState * _PyThreadState_New(PyInterpreterState *interp, int whence) { - return new_threadstate(interp, whence); + _PyEventRc *done_event = _PyEventRc_New(); + if (done_event == NULL) { + return NULL; + } + PyThreadState *tstate = new_threadstate(interp, whence, done_event); + + // tstate has a reference + _PyEventRc_Decref(done_event); + + return tstate; +} + +// This must be followed by a call to _PyThreadState_Bind(); +PyThreadState * +_PyThreadState_NewWithEvent(PyInterpreterState *interp, int whence, + _PyEventRc *done_event) +{ + return new_threadstate(interp, whence, done_event); } // We keep this for stable ABI compabibility. @@ -1542,15 +1608,24 @@ PyThreadState_Clear(PyThreadState *tstate) Py_CLEAR(tstate->context); - if (tstate->on_delete != NULL) { - // For the "main" thread of each interpreter, this is meant - // to be done in _PyInterpreterState_SetNotRunningMain(). - // That leaves threads created by the threading module, - // and any threads killed by forking. - // However, we also accommodate "main" threads that still - // don't call _PyInterpreterState_SetNotRunningMain() yet. - tstate->on_delete(tstate->on_delete_data); + _PyEventRc *done_event = tstate->done_event; + tstate->done_event = NULL; + + // Notify threads waiting on Thread.join(). This should happen after the + // thread state is unlinked, but must happen before parking lot is + // deinitialized. + // + // For the "main" thread of each interpreter, this is meant + // to be done in _PyInterpreterState_SetNotRunningMain(). + // That leaves threads created by the threading module, + // and any threads killed by forking. + // However, we also accommodate "main" threads that still + // don't call _PyInterpreterState_SetNotRunningMain() yet. + if (done_event) { + _PyEvent_Notify(&done_event->event); + _PyEventRc_Decref(done_event); } + #ifdef Py_GIL_DISABLED // Each thread should clear own freelists in free-threading builds. _PyFreeListState *freelist_state = &((_PyThreadStateImpl*)tstate)->freelist_state; @@ -2509,8 +2584,8 @@ PyGILState_Ensure(void) if (tcur == NULL) { /* Create a new Python thread state for this thread */ // XXX Use PyInterpreterState_EnsureThreadState()? - tcur = new_threadstate(runtime->gilstate.autoInterpreterState, - _PyThreadState_WHENCE_GILSTATE); + tcur = _PyThreadState_New(runtime->gilstate.autoInterpreterState, + _PyThreadState_WHENCE_GILSTATE); if (tcur == NULL) { Py_FatalError("Couldn't create thread-state for new thread"); } From b093e5db2c9e4abf8f550f1d7b2af636d8f87baf Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Thu, 1 Feb 2024 03:09:39 +0000 Subject: [PATCH 02/22] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst diff --git a/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst b/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst new file mode 100644 index 000000000000000..8d0a834e9173038 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst @@ -0,0 +1 @@ +Fix a race in `threading.Thread.join()`. From 1039208652c0254bccd70e26bd43327e3508ff5e Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 31 Jan 2024 19:15:25 -0800 Subject: [PATCH 03/22] Fix NEWS entry --- .../next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst b/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst index 8d0a834e9173038..6e41da819c52482 100644 --- a/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst +++ b/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst @@ -1 +1 @@ -Fix a race in `threading.Thread.join()`. +Fix a race in ``threading.Thread.join()``. From 196081d6346b994780667812aebea898c142c86d Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 5 Feb 2024 13:26:05 -0800 Subject: [PATCH 04/22] Work around c-analyzer limitation When we have `typedef struct _PyEventRc _PyEventRc` in `pstate.h` the C analyzer can't decide between that and the definition in `pycore_lock.h`. --- Include/cpython/pystate.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 35941f1ab37393c..8a8d9bf7e0745f4 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -61,8 +61,6 @@ struct _py_trashcan { PyObject *delete_later; }; -typedef struct _PyEventRc _PyEventRc; - struct _ts { /* See Python/ceval.c for comments explaining most fields */ @@ -164,7 +162,7 @@ struct _ts { int is_daemon; /* Set when the thread has finished execution and is about to be freed. */ - _PyEventRc *done_event; + struct _PyEventRc *done_event; int coroutine_origin_tracking_depth; From 1c82786c20e2b4e9ba7c2f25bfb3f7d2d5954dbc Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 1 Mar 2024 15:57:54 -0800 Subject: [PATCH 05/22] Fix two compilation errors post merge --- Modules/_threadmodule.c | 2 +- Python/lock.c | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 133b313df1e5b15..61f4b535ba84019 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -119,7 +119,7 @@ event_wait(PyEventObject *self, PyObject *args) return NULL; } - _PyTime_t timeout_ns = -1; + PyTime_t timeout_ns = -1; if (timeout_obj && timeout_obj != Py_None) { int err = _PyTime_FromSecondsObject(&timeout_ns, timeout_obj, _PyTime_ROUND_TIMEOUT); diff --git a/Python/lock.c b/Python/lock.c index 1ab8ee5116fedf6..de25adce3851050 100644 --- a/Python/lock.c +++ b/Python/lock.c @@ -274,13 +274,6 @@ _PyEvent_Notify(PyEvent *evt) } } -int -_PyEvent_IsSet(PyEvent *evt) -{ - uint8_t v = _Py_atomic_load_uint8(&evt->v); - return v == _Py_LOCKED; -} - void PyEvent_Wait(PyEvent *evt) { From 0e86cf97670d54391cf9f3a2fa444009e8b0a97c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 1 Mar 2024 16:52:21 -0800 Subject: [PATCH 06/22] Use ThreadHandle as the single abstraction for thread joining --- Include/cpython/pystate.h | 12 +- Include/internal/pycore_pystate.h | 3 - Lib/test/test_audit.py | 4 +- Lib/test/test_thread.py | 15 + Lib/test/test_threading.py | 24 -- Lib/threading.py | 52 +-- Modules/_threadmodule.c | 523 +++++++++++++----------------- Python/pystate.c | 95 ++---- 8 files changed, 310 insertions(+), 418 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 05f21e0ea762b49..1000e26821dcaf0 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -161,12 +161,16 @@ struct _ts { */ uintptr_t critical_section; - /* Boolean storing whether or not this is a daemon thread. All non-daemon - * threads are joined prior to interpreter exit. - */ + /* Boolean storing whether or not this is a daemon thread. */ int is_daemon; - /* Set when the thread has finished execution and is about to be freed. */ + /* Set when the tstate has been cleared and unlinked from the list of + * active tstates. + * + * This is used by _PyInterpreterState_WaitForThreads to wait for all + * non-daemon threads to finish. It cannot be a PyObject because its + * lifetime exceeds the tstate to which it is bound. + */ struct _PyEventRc *done_event; int coroutine_origin_tracking_depth; diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 7517e5e7fb2f510..6f9e6a332a7830b 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -214,9 +214,6 @@ static inline PyInterpreterState* _PyInterpreterState_GET(void) { extern PyThreadState * _PyThreadState_New( PyInterpreterState *interp, int whence); -extern PyThreadState * -_PyThreadState_NewWithEvent(PyInterpreterState *interp, int whence, - _PyEventRc *done_event); extern void _PyThreadState_Bind(PyThreadState *tstate); extern void _PyThreadState_DeleteExcept(PyThreadState *tstate); extern void _PyThreadState_ClearMimallocHeaps(PyThreadState *tstate); diff --git a/Lib/test/test_audit.py b/Lib/test/test_audit.py index 77d516ebd6e1ab2..11ad56958ecc493 100644 --- a/Lib/test/test_audit.py +++ b/Lib/test/test_audit.py @@ -207,9 +207,9 @@ def test_threading(self): print(*events, sep='\n') actual = [(ev[0], ev[2]) for ev in events] expected = [ - ("_thread.start_new_thread", "(, (), None, None, 0)"), + ("_thread.start_new_thread", "(, (), None)"), ("test.test_func", "()"), - ("_thread.start_joinable_thread", "(, None, 0)"), + ("_thread.start_joinable_thread", "(, 0)"), ("test.test_func", "()"), ] diff --git a/Lib/test/test_thread.py b/Lib/test/test_thread.py index 83235230d5c1120..e3231c71896d351 100644 --- a/Lib/test/test_thread.py +++ b/Lib/test/test_thread.py @@ -289,6 +289,21 @@ def joiner(): with self.assertRaisesRegex(RuntimeError, "Cannot join current thread"): raise error + def test_join_with_timeout(self): + lock = thread.allocate_lock() + lock.acquire() + + def thr(): + lock.acquire() + + with threading_helper.wait_threads_exit(): + handle = thread.start_joinable_thread(thr) + handle.join(0.1) + self.assertFalse(handle.is_done()) + lock.release() + handle.join() + self.assertTrue(handle.is_done()) + class Barrier: def __init__(self, num_threads): diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 7b6f40b1d4ed08d..0dc8f43daa41a86 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -901,30 +901,6 @@ def f(): rc, out, err = assert_python_ok("-c", code) self.assertEqual(err, b"") - @cpython_only - def test_done_event(self): - # Test an implementation detail of Thread objects. - started = _thread.allocate_lock() - finish = _thread.allocate_lock() - started.acquire() - finish.acquire() - def f(): - started.release() - finish.acquire() - time.sleep(0.01) - # The done event is not set if the thread hasn't started - t = threading.Thread(target=f) - self.assertFalse(t._done_event.is_set()) - t.start() - started.acquire() - self.assertTrue(t.is_alive()) - # The done event is not set while the thread is alive - self.assertFalse(t._done_event.wait(0), False) - finish.release() - # When the thread ends, the done event is set. - self.assertTrue(t._done_event.wait(support.SHORT_TIMEOUT), False) - t.join() - def test_repr_stopped(self): # Verify that "stopped" shows up in repr(Thread) appropriately. started = _thread.allocate_lock() diff --git a/Lib/threading.py b/Lib/threading.py index a57c36db8f8818a..e623cb6d2a3cc1b 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -36,9 +36,8 @@ _daemon_threads_allowed = _thread.daemon_threads_allowed _allocate_lock = _thread.allocate_lock _LockType = _thread.LockType -_Event = _thread.Event -_get_done_event = _thread._get_done_event _thread_shutdown = _thread._shutdown +_get_thread_handle = _thread._get_thread_handle get_ident = _thread.get_ident _is_main_interpreter = _thread._is_main_interpreter try: @@ -915,7 +914,6 @@ class is implemented. self._native_id = None self._handle = None self._started = Event() - self._done_event = _Event() self._initialized = True # Copy of sys.stderr used by self._invoke_excepthook() self._stderr = _sys.stderr @@ -932,17 +930,16 @@ def _after_fork(self, new_ident=None): if self._handle is not None: assert self._handle.ident == new_ident else: - # This thread isn't alive after fork: it doesn't have a tstate - # anymore. - self._done_event.set() - self._handle = None + # Otherwise, the thread is dead, Jim. If we had a handle + # _PyThread_AfterFork() already marked it done. + pass def __repr__(self): assert self._initialized, "Thread.__init__() was not called" status = "initial" if self._started.is_set(): status = "started" - if self._done_event.is_set(): + if self._handle and self._handle.is_done(): status = "stopped" if self._daemonic: status += " daemon" @@ -970,7 +967,7 @@ def start(self): _limbo[self] = self try: # Start joinable thread - self._handle = _start_joinable_thread(self._bootstrap, done_event=self._done_event, daemon=self.daemon) + self._handle = _start_joinable_thread(self._bootstrap, daemon=self.daemon) except Exception: with _active_limbo_lock: del _limbo[self] @@ -1087,15 +1084,8 @@ def join(self, timeout=None): # historically .join(timeout=x) for x<0 has acted as if timeout=0 if timeout is not None: timeout = max(timeout, 0) - self._done_event.wait(timeout) - - if self._done_event.is_set(): - self._join_os_thread() - def _join_os_thread(self): - # self._handle may be cleared post-fork - if self._handle is not None: - self._handle.join() + self._handle.join(timeout) @property def name(self): @@ -1146,7 +1136,7 @@ def is_alive(self): """ assert self._initialized, "Thread.__init__() not called" - return self._started.is_set() and not self._done_event.is_set() + return self._started.is_set() and not self._handle.is_done() @property def daemon(self): @@ -1355,18 +1345,14 @@ class _MainThread(Thread): def __init__(self): Thread.__init__(self, name="MainThread", daemon=False) - self._done_event = _get_done_event() self._started.set() self._set_ident() + self._handle = _get_thread_handle() if _HAVE_THREAD_NATIVE_ID: self._set_native_id() 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. @@ -1407,9 +1393,9 @@ class _DummyThread(Thread): def __init__(self): Thread.__init__(self, name=_newname("Dummy-%d"), daemon=_daemon_threads_allowed()) - self._done_event = _get_done_event() self._started.set() self._set_ident() + self._handle = _get_thread_handle() if _HAVE_THREAD_NATIVE_ID: self._set_native_id() with _active_limbo_lock: @@ -1417,7 +1403,7 @@ def __init__(self): _DeleteDummyThreadOnDel(self) def is_alive(self): - if self._started.is_set() and not self._done_event.is_set(): + if self._started.is_set() and not self._handle.is_done(): return True raise RuntimeError("thread is not alive") @@ -1528,11 +1514,11 @@ def _shutdown(): Wait until the Python thread state of all non-daemon threads get deleted. """ global _SHUTTING_DOWN - # Obscure: other threads may be waiting to join _main_thread. That's - # dubious, but some code does it. We can't wait for C code to set - # the main thread's done_event - that won't happen until the interpreter - # is nearly dead. So we set it here. - if _main_thread._done_event.is_set() and _is_main_interpreter() and _SHUTTING_DOWN: + # Obscure: other threads may be waiting to join _main_thread. That's + # dubious, but some code does it. We can't wait for it to be marked as done + # normally - that won't happen until the interpreter is nearly dead. So + # mark it done here. + if _is_main_interpreter() and _SHUTTING_DOWN: # _shutdown() was already called return @@ -1543,11 +1529,9 @@ def _shutdown(): for atexit_call in reversed(_threading_atexits): atexit_call() + # Main thread if _main_thread.ident == get_ident(): - # It should have been set already by - # _PyInterpreterState_SetNotRunningMain(), but there may be embedders - # that aren't calling that yet. - _main_thread._done_event.set() + _main_thread._handle._set_done() else: # bpo-1596321: _shutdown() must be called in the main thread. # If the threading module was not imported by the main thread, diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 61f4b535ba84019..963a1bc40bfc574 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -12,7 +12,6 @@ #include "pycore_time.h" // _PyTime_FromSeconds() #include "pycore_weakref.h" // _PyWeakref_GET_REF() -#include #include // offsetof() #ifdef HAVE_SIGNAL_H # include // SIGINT @@ -32,7 +31,6 @@ typedef struct { PyTypeObject *local_type; PyTypeObject *local_dummy_type; PyTypeObject *thread_handle_type; - PyTypeObject *event_type; } thread_module_state; static inline thread_module_state* @@ -43,160 +41,45 @@ get_thread_state(PyObject *module) return (thread_module_state *)state; } -// Event type - -typedef struct { - PyObject_HEAD - _PyEventRc *event; -} PyEventObject; - -static void -event_dealloc(PyEventObject *self) -{ - if (self->event) { - _PyEventRc_Decref(self->event); - self->event = NULL; - } - Py_TYPE(self)->tp_free(self); -} - -static PyObject * -wrap_event(PyTypeObject *type, _PyEventRc *event) -{ - PyEventObject *self; - self = (PyEventObject *)type->tp_alloc(type, 0); - if (self != NULL) { - self->event = event; - _PyEventRc_Incref(event); - } - return (PyObject *)self; -} - -static PyObject * -event_new(PyTypeObject *type, PyObject *args, PyObject *kwds) -{ - _PyEventRc *event = _PyEventRc_New(); - if (event == NULL) { - PyErr_SetString(ThreadError, "can't allocate event"); - return NULL; - } - PyObject *result = wrap_event(type, event); - _PyEventRc_Decref(event); - return result; -} - -static PyObject * -event_repr(PyEventObject *self) -{ - int is_set = _PyEvent_IsSet(&self->event->event); - return PyUnicode_FromFormat("<_thread.Event object is_set=%d at %p>", - is_set, self); -} - -static PyObject * -event_is_set(PyEventObject *self, PyObject *Py_UNUSED(ignored)) -{ - if (_PyEvent_IsSet(&self->event->event)) { - Py_RETURN_TRUE; - } - else { - Py_RETURN_FALSE; - } -} - -static PyObject * -event_set(PyEventObject *self, PyObject *Py_UNUSED(ignored)) -{ - _PyEvent_Notify(&self->event->event); - Py_RETURN_NONE; -} - -static PyObject * -event_wait(PyEventObject *self, PyObject *args) -{ - PyObject *timeout_obj = NULL; - if (!PyArg_ParseTuple(args, "|O:wait", &timeout_obj)) { - return NULL; - } - - PyTime_t timeout_ns = -1; - if (timeout_obj && timeout_obj != Py_None) { - int err = _PyTime_FromSecondsObject(&timeout_ns, timeout_obj, - _PyTime_ROUND_TIMEOUT); - - if (err < 0) { - return NULL; - } - } - - int ok = PyEvent_WaitTimed(&self->event->event, timeout_ns); - if (ok) { - Py_RETURN_TRUE; - } - else { - Py_RETURN_FALSE; - } -} - -static PyMethodDef event_methods[] = { - {"is_set", (PyCFunction)event_is_set, METH_NOARGS, NULL}, - {"set", (PyCFunction)event_set, METH_NOARGS, NULL}, - {"wait", (PyCFunction)event_wait, METH_VARARGS, NULL}, - {NULL, NULL} /* sentinel */ -}; - -static PyType_Slot event_type_slots[] = { - {Py_tp_repr, (reprfunc)event_repr}, - {Py_tp_methods, event_methods}, - {Py_tp_alloc, PyType_GenericAlloc}, - {Py_tp_dealloc, (destructor)event_dealloc}, - {Py_tp_new, event_new}, - {0, 0}, -}; - -static PyType_Spec event_type_spec = { - .name = "_thread.Event", - .basicsize = sizeof(PyEventObject), - .flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_IMMUTABLETYPE), - .slots = event_type_slots, -}; - // _ThreadHandle type -// Handles transition from RUNNING to one of JOINED, DETACHED, or INVALID (post -// fork). +// Handles transition from INVALID to RUNNING to DONE. typedef enum { THREAD_HANDLE_RUNNING = 1, - THREAD_HANDLE_JOINED = 2, - THREAD_HANDLE_DETACHED = 3, - THREAD_HANDLE_INVALID = 4, + THREAD_HANDLE_DONE = 2, + THREAD_HANDLE_INVALID = 3, } ThreadHandleState; -// A handle around an OS thread. +// A handle to wait for thread completion. // -// The OS thread is either joined or detached after the handle is destroyed. +// This may be used to wait for threads that were spawned by the threading +// module as well as for threads that were spawned by other means. In the +// former case an OS thread, identified by the `os_handle` field, will be +// associated with the handle. The handle "owns" this thread and ensures that +// the 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. +// Joining the handle is idempotent; the underlying OS thread, if any, 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) - // 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. + // The `ident`, `os_handle`, and `has_os_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; + PyThread_handle_t os_handle; + int has_os_handle; // 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. This is used to avoid false positives when - // detecting self-join attempts. See the comment in `ThreadHandle_join()` - // for a more detailed explanation. + // Set immediately before the 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`. @@ -216,20 +99,16 @@ set_thread_handle_state(ThreadHandleObject *handle, ThreadHandleState state) } static ThreadHandleObject* -new_thread_handle(thread_module_state* state) +new_thread_handle(thread_module_state* state, _PyEventRc *event) { - _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->os_handle = 0; + self->has_os_handle = 1; + _PyEventRc_Incref(event); self->thread_is_exiting = event; self->once = (_PyOnceFlag){0}; self->state = THREAD_HANDLE_INVALID; @@ -241,6 +120,20 @@ new_thread_handle(thread_module_state* state) return self; } +static int +detach_thread(ThreadHandleObject *self) +{ + if (!self->has_os_handle) { + return 0; + } + // This is typically short so no need to release the GIL + if (PyThread_detach_thread(self->os_handle)) { + PyErr_SetString(ThreadError, "Failed detaching thread"); + return -1; + } + return 0; +} + static void ThreadHandle_dealloc(ThreadHandleObject *self) { @@ -255,16 +148,14 @@ ThreadHandle_dealloc(ThreadHandleObject *self) // 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. + // 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)) { - PyErr_SetString(ThreadError, "Failed detaching thread"); + if (detach_thread(self) < 0) { PyErr_WriteUnraisable(tp); } else { - self->state = THREAD_HANDLE_DETACHED; + self->state = THREAD_HANDLE_DONE; } } _PyEventRc_Decref(self->thread_is_exiting); @@ -288,9 +179,12 @@ _PyThread_AfterFork(struct _pythread_runtime_state *state) continue; } - // 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; + // Mark all threads as done. Any attempts to join or detach the + // underlying OS thread (if any) could crash. We are the only thread; + // it's safe to set this non-atomically. + hobj->state = THREAD_HANDLE_DONE; + hobj->once = (_PyOnceFlag){_Py_ONCE_INITIALIZED}; + _PyEvent_Notify(&hobj->thread_is_exiting->event); llist_remove(node); } } @@ -312,21 +206,22 @@ 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); - Py_END_ALLOW_THREADS - if (err) { - PyErr_SetString(ThreadError, "Failed joining thread"); - return -1; + if (handle->has_os_handle) { + int err = 0; + Py_BEGIN_ALLOW_THREADS + err = PyThread_join_thread(handle->os_handle); + Py_END_ALLOW_THREADS + if (err) { + PyErr_SetString(ThreadError, "Failed joining thread"); + return -1; + } } - set_thread_handle_state(handle, THREAD_HANDLE_JOINED); + set_thread_handle_state(handle, THREAD_HANDLE_DONE); return 0; } static PyObject * -ThreadHandle_join(ThreadHandleObject *self, void* ignored) +ThreadHandle_join(ThreadHandleObject *self, PyObject *args) { if (get_thread_handle_state(self) == THREAD_HANDLE_INVALID) { PyErr_SetString(PyExc_ValueError, @@ -334,6 +229,19 @@ ThreadHandle_join(ThreadHandleObject *self, void* ignored) return NULL; } + PyObject *timeout_obj = NULL; + if (!PyArg_ParseTuple(args, "|O:join", &timeout_obj)) { + return NULL; + } + + PyTime_t timeout_ns = -1; + if (timeout_obj != NULL && timeout_obj != Py_None) { + if (_PyTime_FromSecondsObject(&timeout_ns, timeout_obj, + _PyTime_ROUND_TIMEOUT) < 0) { + return NULL; + } + } + // 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 @@ -351,11 +259,83 @@ ThreadHandle_join(ThreadHandleObject *self, void* ignored) return NULL; } + // Wait until the deadline for the thread to exit. + PyTime_t deadline = timeout_ns != -1 ? _PyDeadline_Init(timeout_ns) : 0; + while (!PyEvent_WaitTimed(&self->thread_is_exiting->event, timeout_ns)) { + if (deadline) { + // _PyDeadline_Get will return a negative value if the deadline has + // been exceeded. + timeout_ns = Py_MAX(_PyDeadline_Get(deadline), 0); + } + + if (timeout_ns) { + // Interrupted + if (Py_MakePendingCalls() < 0) { + return NULL; + } + } + else { + // Timed out + Py_RETURN_NONE; + } + } + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)join_thread, self) == -1) { return NULL; } - assert(get_thread_handle_state(self) == THREAD_HANDLE_JOINED); + assert(get_thread_handle_state(self) == THREAD_HANDLE_DONE); + Py_RETURN_NONE; +} + +static int +check_handle_valid(ThreadHandleObject *handle) +{ + if (get_thread_handle_state(handle) == THREAD_HANDLE_INVALID) { + PyErr_SetString(PyExc_ValueError, "the handle is invalid"); + return -1; + } + return 0; +} + +static PyObject * +ThreadHandle_is_done(ThreadHandleObject *self, PyObject *Py_UNUSED(ignored)) +{ + if (check_handle_valid(self) < 0) { + return NULL; + } + if (_PyEvent_IsSet(&self->thread_is_exiting->event)) { + Py_RETURN_TRUE; + } + else { + Py_RETURN_FALSE; + } +} + +static int +set_done(ThreadHandleObject *handle) +{ + assert(get_thread_handle_state(handle) == THREAD_HANDLE_RUNNING); + if (detach_thread(handle) < 0) { + return -1; + } + _PyEvent_Notify(&handle->thread_is_exiting->event); + set_thread_handle_state(handle, THREAD_HANDLE_DONE); + return 0; +} + +// Only to be used to mark the main thread as done +static PyObject * +ThreadHandle_set_done(ThreadHandleObject *self, PyObject *Py_UNUSED(ignored)) +{ + if (check_handle_valid(self) < 0) { + return NULL; + } + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)set_done, self) == + -1) { + return NULL; + } + assert(get_thread_handle_state(self) == THREAD_HANDLE_DONE); Py_RETURN_NONE; } @@ -366,7 +346,9 @@ static PyGetSetDef ThreadHandle_getsetlist[] = { static PyMethodDef ThreadHandle_methods[] = { - {"join", (PyCFunction)ThreadHandle_join, METH_NOARGS}, + {"join", (PyCFunction)ThreadHandle_join, METH_VARARGS, NULL}, + {"_set_done", (PyCFunction)ThreadHandle_set_done, METH_NOARGS, NULL}, + {"is_done", (PyCFunction)ThreadHandle_is_done, METH_NOARGS, NULL}, {0, 0} }; @@ -1415,9 +1397,7 @@ 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); - } + _PyEventRc_Decref(boot->thread_is_exiting); PyMem_RawFree(boot); } @@ -1430,7 +1410,7 @@ thread_run(void *boot_raw) // `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; + _PyEventRc_Incref(thread_is_exiting); // gh-108987: If _thread.start_new_thread() is called before or while // Python is being finalized, thread_run() can called *after*. @@ -1476,10 +1456,8 @@ 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); - } + _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 @@ -1505,35 +1483,20 @@ PyDoc_STRVAR(daemon_threads_allowed_doc, Return True if daemon threads are allowed in the current interpreter,\n\ and False otherwise.\n"); -static _PyEventRc * -unpack_or_create_event(PyObject *op) -{ - if (op) { - _PyEventRc *event = ((PyEventObject *)op)->event; - _PyEventRc_Incref(event); - return event; - } - else { - return _PyEventRc_New(); - } -} - -static int +static PyObject * do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args, - PyObject *kwargs, int joinable, PyThread_ident_t *ident, - PyThread_handle_t *handle, _PyEventRc *thread_is_exiting, - PyObject *done_event, int daemon) + PyObject *kwargs, int daemon) { PyInterpreterState *interp = _PyInterpreterState_GET(); if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_THREADS)) { PyErr_SetString(PyExc_RuntimeError, "thread is not supported for isolated subinterpreters"); - return -1; + return NULL; } if (interp->finalizing) { PyErr_SetString(PyExc_PythonFinalizationError, "can't create new thread at interpreter shutdown"); - return -1; + return NULL; } // gh-109795: Use PyMem_RawMalloc() instead of PyMem_Malloc(), @@ -1542,65 +1505,62 @@ do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args, struct bootstate *boot = PyMem_RawMalloc(sizeof(struct bootstate)); if (boot == NULL) { PyErr_NoMemory(); - return -1; + return NULL; } - _PyEventRc *event = unpack_or_create_event(done_event); - if (event == NULL) { + boot->thread_is_exiting = _PyEventRc_New(); + if (boot->thread_is_exiting == NULL) { PyMem_RawFree(boot); PyErr_NoMemory(); - return -1; + return NULL; } - boot->tstate = _PyThreadState_NewWithEvent( - interp, _PyThreadState_WHENCE_THREADING, event); - _PyEventRc_Decref(event); + boot->tstate = _PyThreadState_New(interp, _PyThreadState_WHENCE_THREADING); if (boot->tstate == NULL) { - PyMem_RawFree(boot); + thread_bootstate_free(boot, 0); if (!PyErr_Occurred()) { PyErr_NoMemory(); } - return -1; + return NULL; + } + ThreadHandleObject *handle = + new_thread_handle(state, boot->thread_is_exiting); + if (handle == NULL) { + PyThreadState_Delete(boot->tstate); + thread_bootstate_free(boot, 0); + return NULL; } boot->tstate->is_daemon = daemon; 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) { - err = PyThread_start_joinable_thread(thread_run, (void*) boot, ident, handle); - } else { - *handle = 0; - *ident = PyThread_start_new_thread(thread_run, (void*) boot); - err = (*ident == PYTHREAD_INVALID_THREAD_ID); - } + int err = PyThread_start_joinable_thread( + thread_run, (void *)boot, &handle->ident, &handle->os_handle); if (err) { PyErr_SetString(ThreadError, "can't start new thread"); PyThreadState_Clear(boot->tstate); + // Normally this happens when the thread state is deleted. Perform it + // manually here to ensure that interpreter shutdown doesn't wait + // indefinitely for this thread state. + _PyEvent_Notify(&boot->tstate->done_event->event); thread_bootstate_free(boot, 1); - return -1; + Py_DECREF(handle); + return NULL; } - return 0; + + set_thread_handle_state(handle, THREAD_HANDLE_RUNNING); + + return (PyObject *)handle; } static PyObject * -thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs, - PyObject *fkwargs) +thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs) { - PyObject *done_event = NULL; - static char *keywords[] = {"function", "args", "kwargs", - "done_event", "daemon", NULL}; PyObject *func, *args, *kwargs = NULL; - int daemon = 0; thread_module_state *state = get_thread_state(module); - if (!PyArg_ParseTupleAndKeywords( - fargs, fkwargs, "OO|OO!p:start_new_thread", keywords, &func, &args, - &kwargs, state->event_type, &done_event)) { + + if (!PyArg_UnpackTuple(fargs, "start_new_thread", 2, 3, + &func, &args, &kwargs)) return NULL; - } if (!PyCallable_Check(func)) { PyErr_SetString(PyExc_TypeError, "first arg must be callable"); @@ -1617,23 +1577,23 @@ thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs, return NULL; } - if (PySys_Audit("_thread.start_new_thread", "OOOOi", func, args, - kwargs ? kwargs : Py_None, - done_event ? done_event : Py_None, daemon) < 0) { + if (PySys_Audit("_thread.start_new_thread", "OOO", + func, args, kwargs ? kwargs : Py_None) < 0) { return NULL; } - PyThread_ident_t ident = 0; - PyThread_handle_t handle; - if (do_start_new_thread(state, func, args, kwargs, /*joinable=*/ 0, - &ident, &handle, NULL, done_event, daemon)) { + PyObject *handle = + do_start_new_thread(state, func, args, kwargs, /*daemon=*/1); + if (handle == NULL) { return NULL; } + PyThread_ident_t ident = ((ThreadHandleObject *)handle)->ident; + Py_DECREF(handle); return PyLong_FromUnsignedLongLong(ident); } PyDoc_STRVAR(start_new_doc, -"start_new_thread(function, args[, kwargs[, done_event[, daemon]]])\n\ +"start_new_thread(function, args[, kwargs])\n\ (start_new() is an obsolete synonym)\n\ \n\ Start a new thread and return its identifier.\n\ @@ -1643,22 +1603,19 @@ tuple args and keyword arguments taken from the optional dictionary\n\ kwargs. The thread exits when the function returns; the return value\n\ is ignored. The thread will also exit when the function raises an\n\ unhandled exception; a stack trace will be printed unless the exception\n\ -is SystemExit. The optional done_event will be set when the thread is\n\ -finished. The runtime will not wait for the thread to exit when shutting\n\ -down if daemon is truthy.\n"); +is SystemExit.\n"); static PyObject * thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, PyObject *fkwargs) { - PyObject *done_event = NULL; - static char *keywords[] = {"function", "done_event", "daemon", NULL}; + static char *keywords[] = {"function", "daemon", NULL}; PyObject *func = NULL; int daemon = 0; thread_module_state *state = get_thread_state(module); - if (!PyArg_ParseTupleAndKeywords( - fargs, fkwargs, "O|O!p:start_joinable_thread", keywords, &func, - state->event_type, &done_event, &daemon)) { + if (!PyArg_ParseTupleAndKeywords(fargs, fkwargs, + "O|p:start_joinable_thread", keywords, + &func, &daemon)) { return NULL; } @@ -1668,8 +1625,7 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, return NULL; } - if (PySys_Audit("_thread.start_joinable_thread", "OOi", func, - done_event ? done_event : Py_None, daemon) < 0) { + if (PySys_Audit("_thread.start_joinable_thread", "Oi", func, daemon) < 0) { return NULL; } @@ -1677,24 +1633,14 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, if (args == NULL) { return NULL; } - ThreadHandleObject* hobj = new_thread_handle(state); - if (hobj == NULL) { - Py_DECREF(args); - return NULL; - } - if (do_start_new_thread(state, func, args, /*kwargs=*/ NULL, /*joinable=*/ 1, - &hobj->ident, &hobj->handle, hobj->thread_is_exiting, done_event, daemon)) { - Py_DECREF(args); - Py_DECREF(hobj); - return NULL; - } - set_thread_handle_state(hobj, THREAD_HANDLE_RUNNING); + PyObject *handle = do_start_new_thread(state, func, args, + /*kwargs=*/ NULL, daemon); Py_DECREF(args); - return (PyObject*) hobj; + return handle; } PyDoc_STRVAR(start_joinable_doc, -"start_joinable_thread(function[, done_event[, daemon]])\n\ +"start_joinable_thread(function[, daemon]])\n\ \n\ *For internal use only*: start a new thread.\n\ \n\ @@ -1702,9 +1648,8 @@ Like start_new_thread(), this starts a new thread calling the given function.\n\ Unlike start_new_thread(), this returns a handle object with methods to join\n\ or detach the given thread.\n\ This function is not for third-party code, please use the\n\ -`threading` module instead. The optional done_event will be set when the thread\n\ -is finished. During finalization the runtime will not wait for the thread\n\ -to exit if daemon is truthy.\n"); +`threading` module instead. During finalization the runtime will not wait for\n\ +the thread to exit if daemon is truthy.\n"); static PyObject * thread_PyThread_exit_thread(PyObject *self, PyObject *Py_UNUSED(ignored)) @@ -1816,21 +1761,6 @@ yet finished.\n\ This function is meant for internal and specialized purposes only.\n\ In most applications `threading.enumerate()` should be used instead."); -static PyObject * -thread__get_done_event(PyObject *module, PyObject *Py_UNUSED(ignore)) -{ - PyThreadState *tstate = PyThreadState_Get(); - thread_module_state *state = get_thread_state(module); - return wrap_event(state->event_type, tstate->done_event); -} - -PyDoc_STRVAR(_get_done_event_doc, -"_get_done_event() -> _thread.Event\n\ -\n\ -Return the done event for the current thread.\n\ -\n\ -This is a private API for the threading module."); - static PyObject * thread_stack_size(PyObject *self, PyObject *args) { @@ -2050,6 +1980,27 @@ PyDoc_STRVAR(shutdown_doc, \n\ Waits for all non-daemon threads (other than the calling thread) to stop."); +static PyObject * +thread__get_thread_handle(PyObject *module, PyObject *Py_UNUSED(ignored)) +{ + thread_module_state *state = get_thread_state(module); + PyThreadState *tstate = _PyThreadState_GET(); + ThreadHandleObject *handle = new_thread_handle(state, tstate->done_event); + if (handle == NULL) { + return NULL; + } + handle->has_os_handle = 0; + handle->ident = PyThread_get_thread_ident_ex(); + set_thread_handle_state(handle, THREAD_HANDLE_RUNNING); + return (PyObject*) handle; +} + +PyDoc_STRVAR(thread__get_thread_handle_doc, +"_get_thread_handle()\n\ +\n\ +Internal only. Return a thread handle that can be used to wait for the current\n\ +thread to finish."); + static PyMethodDef thread_methods[] = { {"start_new_thread", _PyCFunction_CAST(thread_PyThread_start_new_thread), METH_VARARGS | METH_KEYWORDS, start_new_doc}, @@ -2069,8 +2020,6 @@ static PyMethodDef thread_methods[] = { METH_NOARGS, exit_doc}, {"interrupt_main", (PyCFunction)thread_PyThread_interrupt_main, METH_VARARGS, interrupt_doc}, - {"_get_done_event", thread__get_done_event, - METH_NOARGS, _get_done_event_doc}, {"get_ident", thread_get_ident, METH_NOARGS, get_ident_doc}, #ifdef PY_HAVE_THREAD_NATIVE_ID @@ -2087,6 +2036,8 @@ static PyMethodDef thread_methods[] = { METH_NOARGS, thread__is_main_interpreter_doc}, {"_shutdown", thread_shutdown, METH_NOARGS, shutdown_doc}, + {"_get_thread_handle", thread__get_thread_handle, + METH_NOARGS, thread__get_thread_handle_doc}, {NULL, NULL} /* sentinel */ }; @@ -2176,16 +2127,6 @@ thread_module_exec(PyObject *module) return -1; } - // Event - state->event_type = (PyTypeObject *)PyType_FromModuleAndSpec( - module, &event_type_spec, NULL); - if (state->event_type == NULL) { - return -1; - } - if (PyModule_AddType(module, state->event_type) < 0) { - return -1; - } - return 0; } @@ -2199,7 +2140,6 @@ thread_module_traverse(PyObject *module, visitproc visit, void *arg) Py_VISIT(state->local_type); Py_VISIT(state->local_dummy_type); Py_VISIT(state->thread_handle_type); - Py_VISIT(state->event_type); return 0; } @@ -2212,7 +2152,6 @@ thread_module_clear(PyObject *module) Py_CLEAR(state->local_type); Py_CLEAR(state->local_dummy_type); Py_CLEAR(state->thread_handle_type); - Py_CLEAR(state->event_type); return 0; } diff --git a/Python/pystate.c b/Python/pystate.c index ee99ed64f3253c7..595f771581a345f 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1032,14 +1032,10 @@ _PyInterpreterState_SetNotRunningMain(PyInterpreterState *interp) PyThreadState *tstate = interp->threads.main; assert(tstate == current_fast_get()); - if (tstate->done_event != NULL) { - // The threading module was imported for the first time in this - // thread, so it was set as threading._main_thread. (See gh-75698.) - // The thread has finished running the Python program so we mark - // the thread object as finished. - assert(tstate->_whence != _PyThreadState_WHENCE_THREADING); - _PyEvent_Notify(&tstate->done_event->event); - } + // The thread has finished running the Python program so we mark + // the thread object as finished. + assert(tstate->_whence != _PyThreadState_WHENCE_THREADING); + _PyEvent_Notify(&tstate->done_event->event); interp->threads.main = NULL; } @@ -1150,12 +1146,13 @@ _PyInterpreterState_WaitForThreads(PyInterpreterState *interp) // Find a thread that's not yet finished. HEAD_LOCK(runtime); - for (PyThreadState *p = interp->threads.head; p != NULL; p = p->next) { - if (p == tstate) { + for (PyThreadState *ts = interp->threads.head; ts != NULL; + ts = ts->next) { + if (ts == tstate) { continue; } - if (p->done_event && !p->is_daemon) { - done_event = p->done_event; + if (!ts->is_daemon && !_PyEvent_IsSet(&ts->done_event->event)) { + done_event = ts->done_event; _PyEventRc_Incref(done_event); break; } @@ -1169,11 +1166,7 @@ _PyInterpreterState_WaitForThreads(PyInterpreterState *interp) // Wait for the other thread to finish. If we're interrupted, such // as by a ctrl-c we print the error and exit early. - for (;;) { - if (PyEvent_WaitTimed(&done_event->event, -1)) { - break; - } - + while (!PyEvent_WaitTimed(&done_event->event, -1)) { // interrupted if (Py_MakePendingCalls() < 0) { PyErr_WriteUnraisable(NULL); @@ -1406,7 +1399,6 @@ init_threadstate(_PyThreadStateImpl *_tstate, PyInterpreterState *interp, tstate->state = _Py_THREAD_SUSPENDED; } - _PyEventRc_Incref(done_event); tstate->is_daemon = (id > 1); tstate->done_event = done_event; @@ -1428,10 +1420,14 @@ add_threadstate(PyInterpreterState *interp, PyThreadState *tstate, } static PyThreadState * -new_threadstate(PyInterpreterState *interp, int whence, _PyEventRc *done_event) +new_threadstate(PyInterpreterState *interp, int whence) { _PyThreadStateImpl *tstate; _PyRuntimeState *runtime = interp->runtime; + _PyEventRc *done_event = _PyEventRc_New(); + if (done_event == NULL) { + return NULL; + } // We don't need to allocate a thread state for the main interpreter // (the common case), but doing it later for the other case revealed a // reentrancy problem (deadlock). So for now we always allocate before @@ -1439,11 +1435,13 @@ new_threadstate(PyInterpreterState *interp, int whence, _PyEventRc *done_event) _PyThreadStateImpl *new_tstate = alloc_threadstate(); int used_newtstate; if (new_tstate == NULL) { + _PyEventRc_Decref(done_event); return NULL; } #ifdef Py_GIL_DISABLED Py_ssize_t qsbr_idx = _Py_qsbr_reserve(interp); if (qsbr_idx < 0) { + _PyEventRc_Decref(done_event); PyMem_RawFree(new_tstate); return NULL; } @@ -1495,8 +1493,8 @@ new_threadstate(PyInterpreterState *interp, int whence, _PyEventRc *done_event) PyThreadState * PyThreadState_New(PyInterpreterState *interp) { - PyThreadState *tstate = - _PyThreadState_New(interp, _PyThreadState_WHENCE_UNKNOWN); + PyThreadState *tstate = new_threadstate(interp, + _PyThreadState_WHENCE_UNKNOWN); if (tstate) { bind_tstate(tstate); // This makes sure there's a gilstate tstate bound @@ -1512,24 +1510,7 @@ PyThreadState_New(PyInterpreterState *interp) PyThreadState * _PyThreadState_New(PyInterpreterState *interp, int whence) { - _PyEventRc *done_event = _PyEventRc_New(); - if (done_event == NULL) { - return NULL; - } - PyThreadState *tstate = new_threadstate(interp, whence, done_event); - - // tstate has a reference - _PyEventRc_Decref(done_event); - - return tstate; -} - -// This must be followed by a call to _PyThreadState_Bind(); -PyThreadState * -_PyThreadState_NewWithEvent(PyInterpreterState *interp, int whence, - _PyEventRc *done_event) -{ - return new_threadstate(interp, whence, done_event); + return new_threadstate(interp, whence); } // We keep this for stable ABI compabibility. @@ -1633,24 +1614,6 @@ PyThreadState_Clear(PyThreadState *tstate) Py_CLEAR(tstate->context); - _PyEventRc *done_event = tstate->done_event; - tstate->done_event = NULL; - - // Notify threads waiting on Thread.join(). This should happen after the - // thread state is unlinked, but must happen before parking lot is - // deinitialized. - // - // For the "main" thread of each interpreter, this is meant - // to be done in _PyInterpreterState_SetNotRunningMain(). - // That leaves threads created by the threading module, - // and any threads killed by forking. - // However, we also accommodate "main" threads that still - // don't call _PyInterpreterState_SetNotRunningMain() yet. - if (done_event) { - _PyEvent_Notify(&done_event->event); - _PyEventRc_Decref(done_event); - } - #ifdef Py_GIL_DISABLED // Each thread should clear own freelists in free-threading builds. struct _Py_object_freelists *freelists = _Py_object_freelists_GET(); @@ -1709,6 +1672,20 @@ tstate_delete_common(PyThreadState *tstate) } HEAD_UNLOCK(runtime); + _PyEventRc *done_event = tstate->done_event; + tstate->done_event = NULL; + + // Notify threads waiting on Thread.join(). This should happen after the + // thread state is unlinked, but must happen before the parking lot is + // deinitialized. + // + // For the "main" thread of each interpreter, this is meant to be done in + // _PyInterpreterState_SetNotRunningMain(). However, we also accommodate + // "main" threads that still don't call + // _PyInterpreterState_SetNotRunningMain() yet. + _PyEvent_Notify(&done_event->event); + _PyEventRc_Decref(done_event); + #ifdef Py_GIL_DISABLED _Py_qsbr_unregister((_PyThreadStateImpl *)tstate); #endif @@ -2627,8 +2604,8 @@ PyGILState_Ensure(void) if (tcur == NULL) { /* Create a new Python thread state for this thread */ // XXX Use PyInterpreterState_EnsureThreadState()? - tcur = _PyThreadState_New(runtime->gilstate.autoInterpreterState, - _PyThreadState_WHENCE_GILSTATE); + tcur = new_threadstate(runtime->gilstate.autoInterpreterState, + _PyThreadState_WHENCE_GILSTATE); if (tcur == NULL) { Py_FatalError("Couldn't create thread-state for new thread"); } From 76bde036984431df51f2c432b4cd078e6a9ccc58 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 4 Mar 2024 11:24:52 -0800 Subject: [PATCH 07/22] Isolate all logic in _threadmodule --- Include/cpython/pystate.h | 12 - Include/internal/pycore_interp.h | 1 - Include/internal/pycore_pythread.h | 2 +- Lib/threading.py | 19 +- ...-02-01-03-09-38.gh-issue-114271.raCkt5.rst | 6 + Modules/_threadmodule.c | 446 +++++++++++------- Python/lock.c | 24 - Python/pystate.c | 79 +--- 8 files changed, 302 insertions(+), 287 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 1000e26821dcaf0..38d0897ea131611 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -161,18 +161,6 @@ struct _ts { */ uintptr_t critical_section; - /* Boolean storing whether or not this is a daemon thread. */ - int is_daemon; - - /* Set when the tstate has been cleared and unlinked from the list of - * active tstates. - * - * This is used by _PyInterpreterState_WaitForThreads to wait for all - * non-daemon threads to finish. It cannot be a PyObject because its - * lifetime exceeds the tstate to which it is bound. - */ - struct _PyEventRc *done_event; - int coroutine_origin_tracking_depth; PyObject *async_gen_firstiter; diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 18d6b54715b2d91..6a00aafea73779a 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -302,7 +302,6 @@ PyAPI_FUNC(PyInterpreterState *) _PyInterpreterState_LookUpID(int64_t); PyAPI_FUNC(int) _PyInterpreterState_IDInitref(PyInterpreterState *); PyAPI_FUNC(int) _PyInterpreterState_IDIncref(PyInterpreterState *); PyAPI_FUNC(void) _PyInterpreterState_IDDecref(PyInterpreterState *); -PyAPI_FUNC(void) _PyInterpreterState_WaitForThreads(PyInterpreterState *); extern const PyConfig* _PyInterpreterState_GetConfig(PyInterpreterState *interp); diff --git a/Include/internal/pycore_pythread.h b/Include/internal/pycore_pythread.h index d2e7cc2a206ced9..f032cb973886572 100644 --- a/Include/internal/pycore_pythread.h +++ b/Include/internal/pycore_pythread.h @@ -78,7 +78,7 @@ struct _pythread_runtime_state { } stubs; #endif - // Linked list of ThreadHandleObjects + // Linked list of ThreadHandles struct llist_node handles; }; diff --git a/Lib/threading.py b/Lib/threading.py index e623cb6d2a3cc1b..352285f4e4d7c64 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -37,8 +37,9 @@ _allocate_lock = _thread.allocate_lock _LockType = _thread.LockType _thread_shutdown = _thread._shutdown -_get_thread_handle = _thread._get_thread_handle +_make_thread_handle = _thread._make_thread_handle get_ident = _thread.get_ident +_get_main_thread_ident = _thread._get_main_thread_ident _is_main_interpreter = _thread._is_main_interpreter try: get_native_id = _thread.get_native_id @@ -1346,8 +1347,8 @@ class _MainThread(Thread): def __init__(self): Thread.__init__(self, name="MainThread", daemon=False) self._started.set() - self._set_ident() - self._handle = _get_thread_handle() + self._ident = _get_main_thread_ident() + self._handle = _make_thread_handle(self._ident) if _HAVE_THREAD_NATIVE_ID: self._set_native_id() with _active_limbo_lock: @@ -1395,7 +1396,7 @@ def __init__(self): daemon=_daemon_threads_allowed()) self._started.set() self._set_ident() - self._handle = _get_thread_handle() + self._handle = _make_thread_handle(self._ident) if _HAVE_THREAD_NATIVE_ID: self._set_native_id() with _active_limbo_lock: @@ -1529,16 +1530,8 @@ def _shutdown(): for atexit_call in reversed(_threading_atexits): atexit_call() - # Main thread - if _main_thread.ident == get_ident(): + if _is_main_interpreter(): _main_thread._handle._set_done() - else: - # bpo-1596321: _shutdown() must be called in the main thread. - # If the threading module was not imported by the main thread, - # _main_thread is the thread which imported the threading module. - # In this case, ignore _main_thread, similar behavior than for threads - # spawned by C libraries or using _thread.start_new_thread(). - pass # Wait for all non-daemon threads to exit. _thread_shutdown() diff --git a/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst b/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst index 6e41da819c52482..2cd35eeda830b93 100644 --- a/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst +++ b/Misc/NEWS.d/next/Library/2024-02-01-03-09-38.gh-issue-114271.raCkt5.rst @@ -1 +1,7 @@ Fix a race in ``threading.Thread.join()``. + +``threading._MainThread`` now always represents the main thread of the main +interpreter. + +``PyThreadState.on_delete`` and ``PyThreadState.on_delete_data`` have been +removed. diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 963a1bc40bfc574..8ed0270418484f4 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -31,6 +31,10 @@ typedef struct { PyTypeObject *local_type; PyTypeObject *local_dummy_type; PyTypeObject *thread_handle_type; + + // Linked list of handles to all non-daemon threads created by the + // threading module. We wait for these to finish at shutdown. + struct llist_node shutdown_handles; } thread_module_state; static inline thread_module_state* @@ -43,17 +47,16 @@ get_thread_state(PyObject *module) // _ThreadHandle type -// Handles transition from INVALID to RUNNING to DONE. +// Handles transition from RUNNING to DONE. typedef enum { THREAD_HANDLE_RUNNING = 1, THREAD_HANDLE_DONE = 2, - THREAD_HANDLE_INVALID = 3, } ThreadHandleState; // A handle to wait for thread completion. // // This may be used to wait for threads that were spawned by the threading -// module as well as for threads that were spawned by other means. In the +// module as well as for the "main" thread of the threading module. In the // former case an OS thread, identified by the `os_handle` field, will be // associated with the handle. The handle "owns" this thread and ensures that // the thread is either joined or detached after the handle is destroyed. @@ -63,10 +66,15 @@ typedef enum { // until it is their turn to execute or an earlier operation completes // successfully. Once a join has completed successfully all future joins // complete immediately. +// +// This must be separately reference counted because it may be destroyed +// in `thread_run()` after the PyThreadState has been destroyed. typedef struct { - PyObject_HEAD struct llist_node node; // linked list node (see _pythread_runtime_state) + // linked list node (see thread_module_state) + struct llist_node shutdown_node; + // The `ident`, `os_handle`, and `has_os_handle` fields are immutable once // the object is visible to threads other than its creator, thus they do // not need to be accessed atomically. @@ -77,41 +85,64 @@ typedef struct { // Holds a value from the `ThreadHandleState` enum. int state; - // Set immediately before the 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; + // Set immediately before `thread_run` returns to indicate that the OS + // 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. + PyEvent thread_is_exiting; // Serializes calls to `join`. _PyOnceFlag once; -} ThreadHandleObject; + + Py_ssize_t refcount; +} ThreadHandle; static inline int -get_thread_handle_state(ThreadHandleObject *handle) +get_thread_handle_state(ThreadHandle *handle) { return _Py_atomic_load_int(&handle->state); } static inline void -set_thread_handle_state(ThreadHandleObject *handle, ThreadHandleState state) +set_thread_handle_state(ThreadHandle *handle, ThreadHandleState state) { _Py_atomic_store_int(&handle->state, state); } -static ThreadHandleObject* -new_thread_handle(thread_module_state* state, _PyEventRc *event) +static void +add_to_shutdown_handles(thread_module_state *state, ThreadHandle *handle) { - ThreadHandleObject* self = PyObject_New(ThreadHandleObject, state->thread_handle_type); + HEAD_LOCK(&_PyRuntime); + llist_insert_tail(&state->shutdown_handles, &handle->shutdown_node); + HEAD_UNLOCK(&_PyRuntime); +} + +static void +remove_from_shutdown_handles(ThreadHandle *handle) +{ + HEAD_LOCK(&_PyRuntime); + if (handle->shutdown_node.next != NULL) { + llist_remove(&handle->shutdown_node); + } + HEAD_UNLOCK(&_PyRuntime); +} + +static ThreadHandle * +ThreadHandle_new(void) +{ + ThreadHandle *self = + (ThreadHandle *)PyMem_RawCalloc(1, sizeof(ThreadHandle)); if (self == NULL) { + PyErr_NoMemory(); return NULL; } self->ident = 0; self->os_handle = 0; self->has_os_handle = 1; - _PyEventRc_Incref(event); - self->thread_is_exiting = event; + self->thread_is_exiting = (PyEvent){0}; self->once = (_PyOnceFlag){0}; - self->state = THREAD_HANDLE_INVALID; + self->state = THREAD_HANDLE_RUNNING; + self->refcount = 1; HEAD_LOCK(&_PyRuntime); llist_insert_tail(&_PyRuntime.threads.handles, &self->node); @@ -120,24 +151,35 @@ new_thread_handle(thread_module_state* state, _PyEventRc *event) return self; } +static void +ThreadHandle_incref(ThreadHandle *self) +{ + _Py_atomic_add_ssize(&self->refcount, 1); +} + static int -detach_thread(ThreadHandleObject *self) +detach_thread(ThreadHandle *self) { if (!self->has_os_handle) { return 0; } // This is typically short so no need to release the GIL if (PyThread_detach_thread(self->os_handle)) { - PyErr_SetString(ThreadError, "Failed detaching thread"); + fprintf(stderr, "detach_thread: failed detaching thread\n"); return -1; } return 0; } +// NB: This may be called after the PyThreadState in `thread_run` has been +// deleted; it cannot call anything that relies on a valid PyThreadState +// existing. static void -ThreadHandle_dealloc(ThreadHandleObject *self) +ThreadHandle_decref(ThreadHandle *self) { - PyObject *tp = (PyObject *) Py_TYPE(self); + if (_Py_atomic_add_ssize(&self->refcount, -1) > 1) { + return; + } // Remove ourself from the global list of handles HEAD_LOCK(&_PyRuntime); @@ -146,21 +188,17 @@ ThreadHandle_dealloc(ThreadHandleObject *self) } HEAD_UNLOCK(&_PyRuntime); + assert(self->shutdown_node.next == NULL); + // 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) { - if (detach_thread(self) < 0) { - PyErr_WriteUnraisable(tp); - } - else { - self->state = THREAD_HANDLE_DONE; - } + if (self->state == THREAD_HANDLE_RUNNING && !detach_thread(self)) { + self->state = THREAD_HANDLE_DONE; } - _PyEventRc_Decref(self->thread_is_exiting); - PyObject_Free(self); - Py_DECREF(tp); + + PyMem_RawFree(self); } void @@ -174,36 +212,24 @@ _PyThread_AfterFork(struct _pythread_runtime_state *state) struct llist_node *node; llist_for_each_safe(node, &state->handles) { - ThreadHandleObject *hobj = llist_data(node, ThreadHandleObject, node); - if (hobj->ident == current) { + ThreadHandle *handle = llist_data(node, ThreadHandle, node); + if (handle->ident == current) { continue; } // Mark all threads as done. Any attempts to join or detach the // underlying OS thread (if any) could crash. We are the only thread; // it's safe to set this non-atomically. - hobj->state = THREAD_HANDLE_DONE; - hobj->once = (_PyOnceFlag){_Py_ONCE_INITIALIZED}; - _PyEvent_Notify(&hobj->thread_is_exiting->event); + handle->state = THREAD_HANDLE_DONE; + handle->once = (_PyOnceFlag){_Py_ONCE_INITIALIZED}; + _PyEvent_Notify(&handle->thread_is_exiting); llist_remove(node); + remove_from_shutdown_handles(handle); } } -static PyObject * -ThreadHandle_repr(ThreadHandleObject *self) -{ - return PyUnicode_FromFormat("<%s object: ident=%" PY_FORMAT_THREAD_IDENT_T ">", - Py_TYPE(self)->tp_name, self->ident); -} - -static PyObject * -ThreadHandle_get_ident(ThreadHandleObject *self, void *ignored) -{ - return PyLong_FromUnsignedLongLong(self->ident); -} - static int -join_thread(ThreadHandleObject *handle) +join_thread(ThreadHandle *handle) { assert(get_thread_handle_state(handle) == THREAD_HANDLE_RUNNING); if (handle->has_os_handle) { @@ -220,28 +246,9 @@ join_thread(ThreadHandleObject *handle) return 0; } -static PyObject * -ThreadHandle_join(ThreadHandleObject *self, PyObject *args) +static int +ThreadHandle_join(ThreadHandle *self, PyTime_t timeout_ns) { - if (get_thread_handle_state(self) == THREAD_HANDLE_INVALID) { - PyErr_SetString(PyExc_ValueError, - "the handle is invalid and thus cannot be joined"); - return NULL; - } - - PyObject *timeout_obj = NULL; - if (!PyArg_ParseTuple(args, "|O:join", &timeout_obj)) { - return NULL; - } - - PyTime_t timeout_ns = -1; - if (timeout_obj != NULL && timeout_obj != Py_None) { - if (_PyTime_FromSecondsObject(&timeout_ns, timeout_obj, - _PyTime_ROUND_TIMEOUT) < 0) { - return NULL; - } - } - // 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 @@ -252,16 +259,16 @@ ThreadHandle_join(ThreadHandleObject *self, PyObject *args) // 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) && + if (!_PyEvent_IsSet(&self->thread_is_exiting) && self->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; } // Wait until the deadline for the thread to exit. PyTime_t deadline = timeout_ns != -1 ? _PyDeadline_Init(timeout_ns) : 0; - while (!PyEvent_WaitTimed(&self->thread_is_exiting->event, timeout_ns)) { + while (!PyEvent_WaitTimed(&self->thread_is_exiting, timeout_ns)) { if (deadline) { // _PyDeadline_Get will return a negative value if the deadline has // been exceeded. @@ -271,90 +278,157 @@ ThreadHandle_join(ThreadHandleObject *self, PyObject *args) if (timeout_ns) { // Interrupted if (Py_MakePendingCalls() < 0) { - return NULL; + return -1; } } else { // Timed out - Py_RETURN_NONE; + return 0; } } if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)join_thread, self) == -1) { - return NULL; + return -1; } assert(get_thread_handle_state(self) == THREAD_HANDLE_DONE); - Py_RETURN_NONE; + return 0; } static int -check_handle_valid(ThreadHandleObject *handle) +set_done(ThreadHandle *handle) { - if (get_thread_handle_state(handle) == THREAD_HANDLE_INVALID) { - PyErr_SetString(PyExc_ValueError, "the handle is invalid"); + assert(get_thread_handle_state(handle) == THREAD_HANDLE_RUNNING); + if (detach_thread(handle) < 0) { + PyErr_SetString(ThreadError, "failed detaching handle"); return -1; } + _PyEvent_Notify(&handle->thread_is_exiting); + set_thread_handle_state(handle, THREAD_HANDLE_DONE); return 0; } +static int +ThreadHandle_set_done(ThreadHandle *self) +{ + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)set_done, self) == + -1) { + return -1; + } + assert(get_thread_handle_state(self) == THREAD_HANDLE_DONE); + return 0; +} + +// A wrapper around a ThreadHandle. +typedef struct { + PyObject_HEAD + + ThreadHandle *handle; +} ThreadHandleObject; + +static ThreadHandleObject * +ThreadHandleObject_new(thread_module_state *state) +{ + ThreadHandle *handle = ThreadHandle_new(); + if (handle == NULL) { + return NULL; + } + + ThreadHandleObject *self = + PyObject_New(ThreadHandleObject, state->thread_handle_type); + if (self == NULL) { + ThreadHandle_decref(handle); + return NULL; + } + + self->handle = handle; + + return self; +} + +static void +ThreadHandleObject_dealloc(ThreadHandleObject *self) +{ + PyObject *tp = (PyObject *) Py_TYPE(self); + ThreadHandle_decref(self->handle); + PyObject_Free(self); + Py_DECREF(tp); +} + +static PyObject * +ThreadHandleObject_repr(ThreadHandleObject *self) +{ + return PyUnicode_FromFormat("<%s object: ident=%" PY_FORMAT_THREAD_IDENT_T ">", + Py_TYPE(self)->tp_name, self->handle->ident); +} + +static PyObject * +ThreadHandleObject_get_ident(ThreadHandleObject *self, + PyObject *Py_UNUSED(ignored)) +{ + return PyLong_FromUnsignedLongLong(self->handle->ident); +} + static PyObject * -ThreadHandle_is_done(ThreadHandleObject *self, PyObject *Py_UNUSED(ignored)) +ThreadHandleObject_join(ThreadHandleObject *self, PyObject *args) { - if (check_handle_valid(self) < 0) { + PyObject *timeout_obj = NULL; + if (!PyArg_ParseTuple(args, "|O:join", &timeout_obj)) { return NULL; } - if (_PyEvent_IsSet(&self->thread_is_exiting->event)) { - Py_RETURN_TRUE; + + PyTime_t timeout_ns = -1; + if (timeout_obj != NULL && timeout_obj != Py_None) { + if (_PyTime_FromSecondsObject(&timeout_ns, timeout_obj, + _PyTime_ROUND_TIMEOUT) < 0) { + return NULL; + } } - else { - Py_RETURN_FALSE; + + if (ThreadHandle_join(self->handle, timeout_ns) < 0) { + return NULL; } + Py_RETURN_NONE; } -static int -set_done(ThreadHandleObject *handle) +static PyObject * +ThreadHandleObject_is_done(ThreadHandleObject *self, + PyObject *Py_UNUSED(ignored)) { - assert(get_thread_handle_state(handle) == THREAD_HANDLE_RUNNING); - if (detach_thread(handle) < 0) { - return -1; + if (_PyEvent_IsSet(&self->handle->thread_is_exiting)) { + Py_RETURN_TRUE; + } + else { + Py_RETURN_FALSE; } - _PyEvent_Notify(&handle->thread_is_exiting->event); - set_thread_handle_state(handle, THREAD_HANDLE_DONE); - return 0; } -// Only to be used to mark the main thread as done static PyObject * -ThreadHandle_set_done(ThreadHandleObject *self, PyObject *Py_UNUSED(ignored)) +ThreadHandleObject_set_done(ThreadHandleObject *self, + PyObject *Py_UNUSED(ignored)) { - if (check_handle_valid(self) < 0) { - return NULL; - } - if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)set_done, self) == - -1) { + if (ThreadHandle_set_done(self->handle) < 0) { return NULL; } - assert(get_thread_handle_state(self) == THREAD_HANDLE_DONE); Py_RETURN_NONE; } static PyGetSetDef ThreadHandle_getsetlist[] = { - {"ident", (getter)ThreadHandle_get_ident, NULL, NULL}, + {"ident", (getter)ThreadHandleObject_get_ident, NULL, NULL}, {0}, }; static PyMethodDef ThreadHandle_methods[] = { - {"join", (PyCFunction)ThreadHandle_join, METH_VARARGS, NULL}, - {"_set_done", (PyCFunction)ThreadHandle_set_done, METH_NOARGS, NULL}, - {"is_done", (PyCFunction)ThreadHandle_is_done, METH_NOARGS, NULL}, + {"join", (PyCFunction)ThreadHandleObject_join, METH_VARARGS, NULL}, + {"_set_done", (PyCFunction)ThreadHandleObject_set_done, METH_NOARGS, NULL}, + {"is_done", (PyCFunction)ThreadHandleObject_is_done, METH_NOARGS, NULL}, {0, 0} }; static PyType_Slot ThreadHandle_Type_slots[] = { - {Py_tp_dealloc, (destructor)ThreadHandle_dealloc}, - {Py_tp_repr, (reprfunc)ThreadHandle_repr}, + {Py_tp_dealloc, (destructor)ThreadHandleObject_dealloc}, + {Py_tp_repr, (reprfunc)ThreadHandleObject_repr}, {Py_tp_getset, ThreadHandle_getsetlist}, {Py_tp_methods, ThreadHandle_methods}, {0, 0} @@ -1385,7 +1459,7 @@ struct bootstate { PyObject *func; PyObject *args; PyObject *kwargs; - _PyEventRc *thread_is_exiting; + ThreadHandle *handle; }; @@ -1397,7 +1471,7 @@ thread_bootstate_free(struct bootstate *boot, int decref) Py_DECREF(boot->args); Py_XDECREF(boot->kwargs); } - _PyEventRc_Decref(boot->thread_is_exiting); + ThreadHandle_decref(boot->handle); PyMem_RawFree(boot); } @@ -1408,9 +1482,9 @@ 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; - _PyEventRc_Incref(thread_is_exiting); + // `handle` needs to be manipulated after bootstate has been freed + ThreadHandle *handle = boot->handle; + ThreadHandle_incref(handle); // gh-108987: If _thread.start_new_thread() is called before or while // Python is being finalized, thread_run() can called *after*. @@ -1456,8 +1530,11 @@ thread_run(void *boot_raw) _PyThreadState_DeleteCurrent(tstate); exit: - _PyEvent_Notify(&thread_is_exiting->event); - _PyEventRc_Decref(thread_is_exiting); + // Don't need to wait for this thread anymore + remove_from_shutdown_handles(handle); + + _PyEvent_Notify(&handle->thread_is_exiting); + ThreadHandle_decref(handle); // bpo-44434: Don't call explicitly PyThread_exit_thread(). On Linux with // the glibc, pthread_exit() can abort the whole process if dlopen() fails @@ -1507,49 +1584,42 @@ do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args, PyErr_NoMemory(); return NULL; } - boot->thread_is_exiting = _PyEventRc_New(); - if (boot->thread_is_exiting == NULL) { - PyMem_RawFree(boot); - PyErr_NoMemory(); - return NULL; - } boot->tstate = _PyThreadState_New(interp, _PyThreadState_WHENCE_THREADING); if (boot->tstate == NULL) { - thread_bootstate_free(boot, 0); + PyMem_RawFree(boot); if (!PyErr_Occurred()) { PyErr_NoMemory(); } return NULL; } - ThreadHandleObject *handle = - new_thread_handle(state, boot->thread_is_exiting); - if (handle == NULL) { + ThreadHandleObject *hobj = ThreadHandleObject_new(state); + if (hobj == NULL) { PyThreadState_Delete(boot->tstate); - thread_bootstate_free(boot, 0); + PyMem_RawFree(boot); return NULL; } - boot->tstate->is_daemon = daemon; + ThreadHandle *handle = hobj->handle; boot->func = Py_NewRef(func); boot->args = Py_NewRef(args); boot->kwargs = Py_XNewRef(kwargs); + boot->handle = handle; + ThreadHandle_incref(handle); int err = PyThread_start_joinable_thread( thread_run, (void *)boot, &handle->ident, &handle->os_handle); if (err) { PyErr_SetString(ThreadError, "can't start new thread"); PyThreadState_Clear(boot->tstate); - // Normally this happens when the thread state is deleted. Perform it - // manually here to ensure that interpreter shutdown doesn't wait - // indefinitely for this thread state. - _PyEvent_Notify(&boot->tstate->done_event->event); thread_bootstate_free(boot, 1); - Py_DECREF(handle); + Py_DECREF(hobj); return NULL; } - set_thread_handle_state(handle, THREAD_HANDLE_RUNNING); + if (!daemon) { + add_to_shutdown_handles(state, handle); + } - return (PyObject *)handle; + return (PyObject *)hobj; } static PyObject * @@ -1582,13 +1652,13 @@ thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs) return NULL; } - PyObject *handle = + PyObject *hobj = do_start_new_thread(state, func, args, kwargs, /*daemon=*/1); - if (handle == NULL) { + if (hobj == NULL) { return NULL; } - PyThread_ident_t ident = ((ThreadHandleObject *)handle)->ident; - Py_DECREF(handle); + PyThread_ident_t ident = ((ThreadHandleObject *)hobj)->handle->ident; + Py_DECREF(hobj); return PyLong_FromUnsignedLongLong(ident); } @@ -1633,10 +1703,10 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, if (args == NULL) { return NULL; } - PyObject *handle = do_start_new_thread(state, func, args, - /*kwargs=*/ NULL, daemon); + PyObject *hobj = do_start_new_thread(state, func, args, + /*kwargs=*/ NULL, daemon); Py_DECREF(args); - return handle; + return hobj; } PyDoc_STRVAR(start_joinable_doc, @@ -1970,8 +2040,41 @@ Return True if the current interpreter is the main Python interpreter."); static PyObject * thread_shutdown(PyObject *self, PyObject *args) { - PyInterpreterState *interp = PyInterpreterState_Get(); - _PyInterpreterState_WaitForThreads(interp); + PyThread_ident_t ident = PyThread_get_thread_ident_ex(); + thread_module_state *state = get_thread_state(self); + + for (;;) { + ThreadHandle *handle = NULL; + + // Find a thread that's not yet finished. + HEAD_LOCK(&_PyRuntime); + struct llist_node *node; + llist_for_each_safe(node, &state->shutdown_handles) { + ThreadHandle *cur = llist_data(node, ThreadHandle, shutdown_node); + if (cur->ident != ident) { + ThreadHandle_incref(cur); + handle = cur; + break; + } + } + HEAD_UNLOCK(&_PyRuntime); + + if (!handle) { + // No more threads to wait on! + break; + } + + // Wait for the thread to finish. If we're interrupted, such + // as by a ctrl-c we print the error and exit early. + if (ThreadHandle_join(handle, -1) < 0) { + PyErr_WriteUnraisable(NULL); + ThreadHandle_decref(handle); + Py_RETURN_NONE; + } + + ThreadHandle_decref(handle); + } + Py_RETURN_NONE; } @@ -1981,30 +2084,49 @@ PyDoc_STRVAR(shutdown_doc, Waits for all non-daemon threads (other than the calling thread) to stop."); static PyObject * -thread__get_thread_handle(PyObject *module, PyObject *Py_UNUSED(ignored)) +thread__make_thread_handle(PyObject *module, PyObject *ident) { thread_module_state *state = get_thread_state(module); - PyThreadState *tstate = _PyThreadState_GET(); - ThreadHandleObject *handle = new_thread_handle(state, tstate->done_event); - if (handle == NULL) { + ThreadHandleObject *hobj = ThreadHandleObject_new(state); + if (hobj == NULL) { return NULL; } - handle->has_os_handle = 0; - handle->ident = PyThread_get_thread_ident_ex(); - set_thread_handle_state(handle, THREAD_HANDLE_RUNNING); - return (PyObject*) handle; + if (!PyLong_Check(ident)) { + PyErr_SetString(PyExc_TypeError, "ident must be an integer"); + Py_DECREF(hobj); + return NULL; + } + hobj->handle->ident = PyLong_AsUnsignedLongLong(ident); + if (PyErr_Occurred()) { + Py_DECREF(hobj); + return NULL; + } + hobj->handle->has_os_handle = 0; + return (PyObject*) hobj; } -PyDoc_STRVAR(thread__get_thread_handle_doc, -"_get_thread_handle()\n\ +PyDoc_STRVAR(thread__make_thread_handle_doc, +"_make_thread_handle(ident)\n\ \n\ -Internal only. Return a thread handle that can be used to wait for the current\n\ -thread to finish."); +Internal only. Make a thread handle for threads not spawned by the threading\n\ +module."); + +static PyObject * +thread__get_main_thread_ident(PyObject *module, PyObject *Py_UNUSED(ignored)) +{ + return PyLong_FromUnsignedLongLong(_PyRuntime.main_thread); +} + +PyDoc_STRVAR(thread__get_main_thread_ident_doc, +"_get_main_thread_ident()\n\ +\n\ +Internal only. Return a non-zero integer that uniquely identifies the main thread\n\ +of the main interpreter."); static PyMethodDef thread_methods[] = { - {"start_new_thread", _PyCFunction_CAST(thread_PyThread_start_new_thread), + {"start_new_thread", (PyCFunction)thread_PyThread_start_new_thread, METH_VARARGS | METH_KEYWORDS, start_new_doc}, - {"start_new", _PyCFunction_CAST(thread_PyThread_start_new_thread), + {"start_new", (PyCFunction)thread_PyThread_start_new_thread, METH_VARARGS | METH_KEYWORDS, start_new_doc}, {"start_joinable_thread", _PyCFunction_CAST(thread_PyThread_start_joinable_thread), METH_VARARGS | METH_KEYWORDS, start_joinable_doc}, @@ -2036,8 +2158,10 @@ static PyMethodDef thread_methods[] = { METH_NOARGS, thread__is_main_interpreter_doc}, {"_shutdown", thread_shutdown, METH_NOARGS, shutdown_doc}, - {"_get_thread_handle", thread__get_thread_handle, - METH_NOARGS, thread__get_thread_handle_doc}, + {"_make_thread_handle", thread__make_thread_handle, + METH_O, thread__make_thread_handle_doc}, + {"_get_main_thread_ident", thread__get_main_thread_ident, + METH_NOARGS, thread__get_main_thread_ident_doc}, {NULL, NULL} /* sentinel */ }; @@ -2127,6 +2251,8 @@ thread_module_exec(PyObject *module) return -1; } + llist_init(&state->shutdown_handles); + return 0; } diff --git a/Python/lock.c b/Python/lock.c index de25adce3851050..7d1ead585dee6ce 100644 --- a/Python/lock.c +++ b/Python/lock.c @@ -304,30 +304,6 @@ 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) { diff --git a/Python/pystate.c b/Python/pystate.c index 595f771581a345f..61dbb3ef6d6be37 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1031,12 +1031,6 @@ _PyInterpreterState_SetNotRunningMain(PyInterpreterState *interp) { PyThreadState *tstate = interp->threads.main; assert(tstate == current_fast_get()); - - // The thread has finished running the Python program so we mark - // the thread object as finished. - assert(tstate->_whence != _PyThreadState_WHENCE_THREADING); - _PyEvent_Notify(&tstate->done_event->event); - interp->threads.main = NULL; } @@ -1135,50 +1129,6 @@ _PyInterpreterState_IDDecref(PyInterpreterState *interp) } } -void -_PyInterpreterState_WaitForThreads(PyInterpreterState *interp) -{ - _PyRuntimeState *runtime = &_PyRuntime; - PyThreadState *tstate = _PyThreadState_GET(); - - for (;;) { - _PyEventRc *done_event = NULL; - - // Find a thread that's not yet finished. - HEAD_LOCK(runtime); - for (PyThreadState *ts = interp->threads.head; ts != NULL; - ts = ts->next) { - if (ts == tstate) { - continue; - } - if (!ts->is_daemon && !_PyEvent_IsSet(&ts->done_event->event)) { - done_event = ts->done_event; - _PyEventRc_Incref(done_event); - break; - } - } - HEAD_UNLOCK(runtime); - - if (!done_event) { - // No more non-daemon threads to wait on! - break; - } - - // Wait for the other thread to finish. If we're interrupted, such - // as by a ctrl-c we print the error and exit early. - while (!PyEvent_WaitTimed(&done_event->event, -1)) { - // interrupted - if (Py_MakePendingCalls() < 0) { - PyErr_WriteUnraisable(NULL); - _PyEventRc_Decref(done_event); - return; - } - } - - _PyEventRc_Decref(done_event); - } -} - int _PyInterpreterState_RequiresIDRef(PyInterpreterState *interp) { @@ -1345,8 +1295,8 @@ free_threadstate(_PyThreadStateImpl *tstate) */ static void -init_threadstate(_PyThreadStateImpl *_tstate, PyInterpreterState *interp, - uint64_t id, int whence, _PyEventRc *done_event) +init_threadstate(_PyThreadStateImpl *_tstate, + PyInterpreterState *interp, uint64_t id, int whence) { PyThreadState *tstate = (PyThreadState *)_tstate; if (tstate->_status.initialized) { @@ -1399,9 +1349,6 @@ init_threadstate(_PyThreadStateImpl *_tstate, PyInterpreterState *interp, tstate->state = _Py_THREAD_SUSPENDED; } - tstate->is_daemon = (id > 1); - tstate->done_event = done_event; - tstate->_status.initialized = 1; } @@ -1424,10 +1371,6 @@ new_threadstate(PyInterpreterState *interp, int whence) { _PyThreadStateImpl *tstate; _PyRuntimeState *runtime = interp->runtime; - _PyEventRc *done_event = _PyEventRc_New(); - if (done_event == NULL) { - return NULL; - } // We don't need to allocate a thread state for the main interpreter // (the common case), but doing it later for the other case revealed a // reentrancy problem (deadlock). So for now we always allocate before @@ -1435,13 +1378,11 @@ new_threadstate(PyInterpreterState *interp, int whence) _PyThreadStateImpl *new_tstate = alloc_threadstate(); int used_newtstate; if (new_tstate == NULL) { - _PyEventRc_Decref(done_event); return NULL; } #ifdef Py_GIL_DISABLED Py_ssize_t qsbr_idx = _Py_qsbr_reserve(interp); if (qsbr_idx < 0) { - _PyEventRc_Decref(done_event); PyMem_RawFree(new_tstate); return NULL; } @@ -1473,7 +1414,7 @@ new_threadstate(PyInterpreterState *interp, int whence) sizeof(*tstate)); } - init_threadstate(tstate, interp, id, whence, done_event); + init_threadstate(tstate, interp, id, whence); add_threadstate(interp, (PyThreadState *)tstate, old_head); HEAD_UNLOCK(runtime); @@ -1672,20 +1613,6 @@ tstate_delete_common(PyThreadState *tstate) } HEAD_UNLOCK(runtime); - _PyEventRc *done_event = tstate->done_event; - tstate->done_event = NULL; - - // Notify threads waiting on Thread.join(). This should happen after the - // thread state is unlinked, but must happen before the parking lot is - // deinitialized. - // - // For the "main" thread of each interpreter, this is meant to be done in - // _PyInterpreterState_SetNotRunningMain(). However, we also accommodate - // "main" threads that still don't call - // _PyInterpreterState_SetNotRunningMain() yet. - _PyEvent_Notify(&done_event->event); - _PyEventRc_Decref(done_event); - #ifdef Py_GIL_DISABLED _Py_qsbr_unregister((_PyThreadStateImpl *)tstate); #endif From 02123b84818079be7cb870fd2f14bd69fbcccee7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 5 Mar 2024 10:49:05 -0800 Subject: [PATCH 08/22] Fix flag --- Modules/_threadmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 8ed0270418484f4..64eb323ce734f6e 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -2125,9 +2125,9 @@ of the main interpreter."); static PyMethodDef thread_methods[] = { {"start_new_thread", (PyCFunction)thread_PyThread_start_new_thread, - METH_VARARGS | METH_KEYWORDS, start_new_doc}, + METH_VARARGS, start_new_doc}, {"start_new", (PyCFunction)thread_PyThread_start_new_thread, - METH_VARARGS | METH_KEYWORDS, start_new_doc}, + METH_VARARGS, start_new_doc}, {"start_joinable_thread", _PyCFunction_CAST(thread_PyThread_start_joinable_thread), METH_VARARGS | METH_KEYWORDS, start_joinable_doc}, {"daemon_threads_allowed", (PyCFunction)thread_daemon_threads_allowed, From 24c1d476f19260914ace0cbb3449bed6015da9c7 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 6 Mar 2024 12:03:24 -0800 Subject: [PATCH 09/22] Note that the once flag serializes both join and set_done --- Modules/_threadmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 64eb323ce734f6e..cd8382a3456dce0 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -91,7 +91,7 @@ typedef struct { // for a more detailed explanation. PyEvent thread_is_exiting; - // Serializes calls to `join`. + // Serializes calls to `join` and `set_done`. _PyOnceFlag once; Py_ssize_t refcount; From 1a1bfde979cba5059b134ffa278428252952d926 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 6 Mar 2024 12:06:22 -0800 Subject: [PATCH 10/22] Fix unused variable warning --- Python/pystate.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 61dbb3ef6d6be37..176bf604ecac4a2 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1029,8 +1029,7 @@ _PyInterpreterState_SetRunningMain(PyInterpreterState *interp) void _PyInterpreterState_SetNotRunningMain(PyInterpreterState *interp) { - PyThreadState *tstate = interp->threads.main; - assert(tstate == current_fast_get()); + assert(interp->threads.main == current_fast_get()); interp->threads.main = NULL; } From b3c2c4513b25435b8ad097029ed4dd901dfe024a Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 6 Mar 2024 15:18:33 -0800 Subject: [PATCH 11/22] Rename ThreadHandleObject to PyThreadHandleObject --- Modules/_threadmodule.c | 51 ++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index cd8382a3456dce0..5c77b1da518a50a 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -324,18 +324,18 @@ typedef struct { PyObject_HEAD ThreadHandle *handle; -} ThreadHandleObject; +} PyThreadHandleObject; -static ThreadHandleObject * -ThreadHandleObject_new(thread_module_state *state) +static PyThreadHandleObject * +PyThreadHandleObject_new(thread_module_state *state) { ThreadHandle *handle = ThreadHandle_new(); if (handle == NULL) { return NULL; } - ThreadHandleObject *self = - PyObject_New(ThreadHandleObject, state->thread_handle_type); + PyThreadHandleObject *self = + PyObject_New(PyThreadHandleObject, state->thread_handle_type); if (self == NULL) { ThreadHandle_decref(handle); return NULL; @@ -347,7 +347,7 @@ ThreadHandleObject_new(thread_module_state *state) } static void -ThreadHandleObject_dealloc(ThreadHandleObject *self) +PyThreadHandleObject_dealloc(PyThreadHandleObject *self) { PyObject *tp = (PyObject *) Py_TYPE(self); ThreadHandle_decref(self->handle); @@ -356,21 +356,21 @@ ThreadHandleObject_dealloc(ThreadHandleObject *self) } static PyObject * -ThreadHandleObject_repr(ThreadHandleObject *self) +PyThreadHandleObject_repr(PyThreadHandleObject *self) { return PyUnicode_FromFormat("<%s object: ident=%" PY_FORMAT_THREAD_IDENT_T ">", Py_TYPE(self)->tp_name, self->handle->ident); } static PyObject * -ThreadHandleObject_get_ident(ThreadHandleObject *self, - PyObject *Py_UNUSED(ignored)) +PyThreadHandleObject_get_ident(PyThreadHandleObject *self, + PyObject *Py_UNUSED(ignored)) { return PyLong_FromUnsignedLongLong(self->handle->ident); } static PyObject * -ThreadHandleObject_join(ThreadHandleObject *self, PyObject *args) +PyThreadHandleObject_join(PyThreadHandleObject *self, PyObject *args) { PyObject *timeout_obj = NULL; if (!PyArg_ParseTuple(args, "|O:join", &timeout_obj)) { @@ -392,8 +392,8 @@ ThreadHandleObject_join(ThreadHandleObject *self, PyObject *args) } static PyObject * -ThreadHandleObject_is_done(ThreadHandleObject *self, - PyObject *Py_UNUSED(ignored)) +PyThreadHandleObject_is_done(PyThreadHandleObject *self, + PyObject *Py_UNUSED(ignored)) { if (_PyEvent_IsSet(&self->handle->thread_is_exiting)) { Py_RETURN_TRUE; @@ -404,8 +404,8 @@ ThreadHandleObject_is_done(ThreadHandleObject *self, } static PyObject * -ThreadHandleObject_set_done(ThreadHandleObject *self, - PyObject *Py_UNUSED(ignored)) +PyThreadHandleObject_set_done(PyThreadHandleObject *self, + PyObject *Py_UNUSED(ignored)) { if (ThreadHandle_set_done(self->handle) < 0) { return NULL; @@ -414,21 +414,20 @@ ThreadHandleObject_set_done(ThreadHandleObject *self, } static PyGetSetDef ThreadHandle_getsetlist[] = { - {"ident", (getter)ThreadHandleObject_get_ident, NULL, NULL}, + {"ident", (getter)PyThreadHandleObject_get_ident, NULL, NULL}, {0}, }; -static PyMethodDef ThreadHandle_methods[] = -{ - {"join", (PyCFunction)ThreadHandleObject_join, METH_VARARGS, NULL}, - {"_set_done", (PyCFunction)ThreadHandleObject_set_done, METH_NOARGS, NULL}, - {"is_done", (PyCFunction)ThreadHandleObject_is_done, METH_NOARGS, NULL}, +static PyMethodDef ThreadHandle_methods[] = { + {"join", (PyCFunction)PyThreadHandleObject_join, METH_VARARGS, NULL}, + {"_set_done", (PyCFunction)PyThreadHandleObject_set_done, METH_NOARGS, NULL}, + {"is_done", (PyCFunction)PyThreadHandleObject_is_done, METH_NOARGS, NULL}, {0, 0} }; static PyType_Slot ThreadHandle_Type_slots[] = { - {Py_tp_dealloc, (destructor)ThreadHandleObject_dealloc}, - {Py_tp_repr, (reprfunc)ThreadHandleObject_repr}, + {Py_tp_dealloc, (destructor)PyThreadHandleObject_dealloc}, + {Py_tp_repr, (reprfunc)PyThreadHandleObject_repr}, {Py_tp_getset, ThreadHandle_getsetlist}, {Py_tp_methods, ThreadHandle_methods}, {0, 0} @@ -436,7 +435,7 @@ static PyType_Slot ThreadHandle_Type_slots[] = { static PyType_Spec ThreadHandle_Type_spec = { "_thread._ThreadHandle", - sizeof(ThreadHandleObject), + sizeof(PyThreadHandleObject), 0, Py_TPFLAGS_DEFAULT | Py_TPFLAGS_DISALLOW_INSTANTIATION, ThreadHandle_Type_slots, @@ -1592,7 +1591,7 @@ do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args, } return NULL; } - ThreadHandleObject *hobj = ThreadHandleObject_new(state); + PyThreadHandleObject *hobj = PyThreadHandleObject_new(state); if (hobj == NULL) { PyThreadState_Delete(boot->tstate); PyMem_RawFree(boot); @@ -1657,7 +1656,7 @@ thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs) if (hobj == NULL) { return NULL; } - PyThread_ident_t ident = ((ThreadHandleObject *)hobj)->handle->ident; + PyThread_ident_t ident = ((PyThreadHandleObject *)hobj)->handle->ident; Py_DECREF(hobj); return PyLong_FromUnsignedLongLong(ident); } @@ -2087,7 +2086,7 @@ static PyObject * thread__make_thread_handle(PyObject *module, PyObject *ident) { thread_module_state *state = get_thread_state(module); - ThreadHandleObject *hobj = ThreadHandleObject_new(state); + PyThreadHandleObject *hobj = PyThreadHandleObject_new(state); if (hobj == NULL) { return NULL; } From 7b9d0074b6581ac9fffd908caa669d9987abc069 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 6 Mar 2024 15:24:20 -0800 Subject: [PATCH 12/22] Be consistent with documentation of true values --- Modules/_threadmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 5c77b1da518a50a..11d60fc08197b92 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1718,7 +1718,7 @@ Unlike start_new_thread(), this returns a handle object with methods to join\n\ or detach the given thread.\n\ This function is not for third-party code, please use the\n\ `threading` module instead. During finalization the runtime will not wait for\n\ -the thread to exit if daemon is truthy.\n"); +the thread to exit if daemon is True.\n"); static PyObject * thread_PyThread_exit_thread(PyObject *self, PyObject *Py_UNUSED(ignored)) From 5d0dc7a77d53cf184d8311347203722e2082c72a Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 6 Mar 2024 15:32:51 -0800 Subject: [PATCH 13/22] Threads started using `start_joinable_thread` should be daemon threads by default --- Lib/test/test_audit.py | 2 +- Modules/_threadmodule.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_audit.py b/Lib/test/test_audit.py index 11ad56958ecc493..8304bb2411617b9 100644 --- a/Lib/test/test_audit.py +++ b/Lib/test/test_audit.py @@ -209,7 +209,7 @@ def test_threading(self): expected = [ ("_thread.start_new_thread", "(, (), None)"), ("test.test_func", "()"), - ("_thread.start_joinable_thread", "(, 0)"), + ("_thread.start_joinable_thread", "(, 1)"), ("test.test_func", "()"), ] diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 11d60fc08197b92..cf653c260abfa73 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1680,7 +1680,7 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, { static char *keywords[] = {"function", "daemon", NULL}; PyObject *func = NULL; - int daemon = 0; + int daemon = 1; thread_module_state *state = get_thread_state(module); if (!PyArg_ParseTupleAndKeywords(fargs, fkwargs, "O|p:start_joinable_thread", keywords, @@ -1709,7 +1709,7 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, } PyDoc_STRVAR(start_joinable_doc, -"start_joinable_thread(function[, daemon]])\n\ +"start_joinable_thread(function[, daemon=True]])\n\ \n\ *For internal use only*: start a new thread.\n\ \n\ From 11bb8267daaf987c923bab2197c5fc60bc7ec034 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 6 Mar 2024 17:04:16 -0800 Subject: [PATCH 14/22] Remove any remaining handles once the module is cleared --- Modules/_threadmodule.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index cf653c260abfa73..39c0510c1f333ee 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -117,6 +117,17 @@ add_to_shutdown_handles(thread_module_state *state, ThreadHandle *handle) HEAD_UNLOCK(&_PyRuntime); } +static void +clear_shutdown_handles(thread_module_state *state) +{ + HEAD_LOCK(&_PyRuntime); + struct llist_node *node; + llist_for_each_safe(node, &state->shutdown_handles) { + llist_remove(node); + } + HEAD_UNLOCK(&_PyRuntime); +} + static void remove_from_shutdown_handles(ThreadHandle *handle) { @@ -2277,6 +2288,10 @@ thread_module_clear(PyObject *module) Py_CLEAR(state->local_type); Py_CLEAR(state->local_dummy_type); Py_CLEAR(state->thread_handle_type); + // Remove any remaining handles (e.g. if shutdown exited early due to + // interrupt) so that attempts to unlink the handle after our module state + // is destroyed do not crash. + clear_shutdown_handles(state); return 0; } From 06f67879992e7aeb315834b0656f8a64ef45f227 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 7 Mar 2024 17:18:30 -0800 Subject: [PATCH 15/22] Always have a _ThreadHandle in Thread --- Lib/test/test_audit.py | 2 +- Lib/test/test_thread.py | 33 ++++ Lib/threading.py | 15 +- Modules/_threadmodule.c | 347 +++++++++++++++++++++++++++++----------- 4 files changed, 292 insertions(+), 105 deletions(-) diff --git a/Lib/test/test_audit.py b/Lib/test/test_audit.py index 8304bb2411617b9..c24c8213924196f 100644 --- a/Lib/test/test_audit.py +++ b/Lib/test/test_audit.py @@ -209,7 +209,7 @@ def test_threading(self): expected = [ ("_thread.start_new_thread", "(, (), None)"), ("test.test_func", "()"), - ("_thread.start_joinable_thread", "(, 1)"), + ("_thread.start_joinable_thread", "(, 1, None)"), ("test.test_func", "()"), ] diff --git a/Lib/test/test_thread.py b/Lib/test/test_thread.py index e3231c71896d351..d94e04250c93076 100644 --- a/Lib/test/test_thread.py +++ b/Lib/test/test_thread.py @@ -304,6 +304,39 @@ def thr(): handle.join() self.assertTrue(handle.is_done()) + def test_join_unstarted(self): + handle = thread._ThreadHandle() + with self.assertRaisesRegex(RuntimeError, "thread not started"): + handle.join() + + def test_set_done_unstarted(self): + handle = thread._ThreadHandle() + with self.assertRaisesRegex(RuntimeError, "thread not started"): + handle._set_done() + + def test_start_duplicate_handle(self): + lock = thread.allocate_lock() + lock.acquire() + + def func(): + lock.acquire() + + handle = thread._ThreadHandle() + with threading_helper.wait_threads_exit(): + thread.start_joinable_thread(func, handle=handle) + with self.assertRaisesRegex(RuntimeError, "thread already started"): + thread.start_joinable_thread(func, handle=handle) + lock.release() + handle.join() + + def test_start_with_none_handle(self): + def func(): + pass + + with threading_helper.wait_threads_exit(): + handle = thread.start_joinable_thread(func, handle=None) + handle.join() + class Barrier: def __init__(self, num_threads): diff --git a/Lib/threading.py b/Lib/threading.py index 352285f4e4d7c64..9fdc24c037bd38d 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -38,6 +38,7 @@ _LockType = _thread.LockType _thread_shutdown = _thread._shutdown _make_thread_handle = _thread._make_thread_handle +_ThreadHandle = _thread._ThreadHandle get_ident = _thread.get_ident _get_main_thread_ident = _thread._get_main_thread_ident _is_main_interpreter = _thread._is_main_interpreter @@ -913,7 +914,7 @@ class is implemented. self._ident = None if _HAVE_THREAD_NATIVE_ID: self._native_id = None - self._handle = None + self._handle = _ThreadHandle() self._started = Event() self._initialized = True # Copy of sys.stderr used by self._invoke_excepthook() @@ -928,11 +929,10 @@ def _after_fork(self, new_ident=None): if new_ident is not None: # This thread is alive. self._ident = new_ident - if self._handle is not None: - assert self._handle.ident == new_ident + assert self._handle.ident == new_ident else: - # Otherwise, the thread is dead, Jim. If we had a handle - # _PyThread_AfterFork() already marked it done. + # Otherwise, the thread is dead, Jim. _PyThread_AfterFork() + # already marked our handle done. pass def __repr__(self): @@ -940,7 +940,7 @@ def __repr__(self): status = "initial" if self._started.is_set(): status = "started" - if self._handle and self._handle.is_done(): + if self._handle.is_done(): status = "stopped" if self._daemonic: status += " daemon" @@ -968,7 +968,8 @@ def start(self): _limbo[self] = self try: # Start joinable thread - self._handle = _start_joinable_thread(self._bootstrap, daemon=self.daemon) + _start_joinable_thread(self._bootstrap, handle=self._handle, + daemon=self.daemon) except Exception: with _active_limbo_lock: del _limbo[self] diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 39c0510c1f333ee..4bf67106d4fa71c 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -20,10 +20,15 @@ // ThreadError is just an alias to PyExc_RuntimeError #define ThreadError PyExc_RuntimeError - // Forward declarations static struct PyModuleDef thread_module; +static void +thread_run(void *boot_raw); +struct bootstate; +static void +thread_bootstate_free(struct bootstate *boot, int decref); + // Module state typedef struct { PyTypeObject *excepthook_type; @@ -47,10 +52,17 @@ get_thread_state(PyObject *module) // _ThreadHandle type -// Handles transition from RUNNING to DONE. +// Handles state transitions according to the following diagram: +// +// NOT_STARTED -> STARTING -> RUNNING -> DONE +// | ^ +// | | +// +----- error --------+ typedef enum { - THREAD_HANDLE_RUNNING = 1, - THREAD_HANDLE_DONE = 2, + THREAD_HANDLE_NOT_STARTED = 1, + THREAD_HANDLE_STARTING = 2, + THREAD_HANDLE_RUNNING = 3, + THREAD_HANDLE_DONE = 4, } ThreadHandleState; // A handle to wait for thread completion. @@ -75,9 +87,8 @@ typedef struct { // linked list node (see thread_module_state) struct llist_node shutdown_node; - // The `ident`, `os_handle`, and `has_os_handle` fields are immutable once - // the object is visible to threads other than its creator, thus they do - // not need to be accessed atomically. + // The `ident`, `os_handle`, `has_os_handle`, and `state` fields are + // protected by `mutex`. PyThread_ident_t ident; PyThread_handle_t os_handle; int has_os_handle; @@ -85,6 +96,8 @@ typedef struct { // Holds a value from the `ThreadHandleState` enum. int state; + PyMutex mutex; + // Set immediately before `thread_run` returns to indicate that the OS // thread is about to exit. This is used to avoid false positives when // detecting self-join attempts. See the comment in `ThreadHandle_join()` @@ -97,16 +110,54 @@ typedef struct { Py_ssize_t refcount; } ThreadHandle; +// 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; + PyObject *args; + PyObject *kwargs; + ThreadHandle *handle; + PyEvent handle_ready; +}; + static inline int get_thread_handle_state(ThreadHandle *handle) { - return _Py_atomic_load_int(&handle->state); + PyMutex_Lock(&handle->mutex); + int state = handle->state; + PyMutex_Unlock(&handle->mutex); + return state; } static inline void set_thread_handle_state(ThreadHandle *handle, ThreadHandleState state) { - _Py_atomic_store_int(&handle->state, state); + PyMutex_Lock(&handle->mutex); + handle->state = state; + PyMutex_Unlock(&handle->mutex); +} + +static PyThread_ident_t +ThreadHandle_ident(ThreadHandle *handle) +{ + PyMutex_Lock(&handle->mutex); + PyThread_ident_t ident = handle->ident; + PyMutex_Unlock(&handle->mutex); + return ident; +} + +static int +ThreadHandle_get_os_handle(ThreadHandle *handle, PyThread_handle_t *os_handle) +{ + PyMutex_Lock(&handle->mutex); + int has_os_handle = handle->has_os_handle; + if (has_os_handle) { + *os_handle = handle->os_handle; + } + PyMutex_Unlock(&handle->mutex); + return has_os_handle; } static void @@ -149,10 +200,11 @@ ThreadHandle_new(void) } self->ident = 0; self->os_handle = 0; - self->has_os_handle = 1; + self->has_os_handle = 0; self->thread_is_exiting = (PyEvent){0}; + self->mutex = (PyMutex){_Py_UNLOCKED}; self->once = (_PyOnceFlag){0}; - self->state = THREAD_HANDLE_RUNNING; + self->state = THREAD_HANDLE_NOT_STARTED; self->refcount = 1; HEAD_LOCK(&_PyRuntime); @@ -233,20 +285,105 @@ _PyThread_AfterFork(struct _pythread_runtime_state *state) // it's safe to set this non-atomically. handle->state = THREAD_HANDLE_DONE; handle->once = (_PyOnceFlag){_Py_ONCE_INITIALIZED}; + handle->mutex = (PyMutex){_Py_UNLOCKED}; _PyEvent_Notify(&handle->thread_is_exiting); llist_remove(node); remove_from_shutdown_handles(handle); } } +static int +force_done(ThreadHandle *handle) +{ + assert(get_thread_handle_state(handle) == THREAD_HANDLE_STARTING); + _PyEvent_Notify(&handle->thread_is_exiting); + set_thread_handle_state(handle, THREAD_HANDLE_DONE); + return 0; +} + +static void +start_failed(ThreadHandle *handle) +{ + _PyOnceFlag_CallOnce(&handle->once, (_Py_once_fn_t *)force_done, handle); +} + +static int +ThreadHandle_start(ThreadHandle *self, PyObject *func, PyObject *args, + PyObject *kwargs) +{ + // Mark the handle as starting to prevent any other threads from doing so + PyMutex_Lock(&self->mutex); + if (self->state != THREAD_HANDLE_NOT_STARTED) { + PyMutex_Unlock(&self->mutex); + PyErr_SetString(ThreadError, "thread already started"); + return -1; + } + self->state = THREAD_HANDLE_STARTING; + PyMutex_Unlock(&self->mutex); + + // Do all the heavy lifting outside of the mutex. All other operations on + // the handle should fail since the handle is in the starting state. + + // gh-109795: Use PyMem_RawMalloc() instead of PyMem_Malloc(), + // because it should be possible to call thread_bootstate_free() + // without holding the GIL. + struct bootstate *boot = PyMem_RawMalloc(sizeof(struct bootstate)); + if (boot == NULL) { + PyErr_NoMemory(); + start_failed(self); + return -1; + } + PyInterpreterState *interp = _PyInterpreterState_GET(); + boot->tstate = _PyThreadState_New(interp, _PyThreadState_WHENCE_THREADING); + if (boot->tstate == NULL) { + PyMem_RawFree(boot); + if (!PyErr_Occurred()) { + PyErr_NoMemory(); + } + start_failed(self); + return -1; + } + boot->func = Py_NewRef(func); + boot->args = Py_NewRef(args); + boot->kwargs = Py_XNewRef(kwargs); + boot->handle = self; + ThreadHandle_incref(self); + boot->handle_ready = (PyEvent){0}; + + PyThread_ident_t ident; + PyThread_handle_t os_handle; + if (PyThread_start_joinable_thread(thread_run, boot, &ident, &os_handle)) { + PyThreadState_Clear(boot->tstate); + thread_bootstate_free(boot, 1); + start_failed(self); + PyErr_SetString(ThreadError, "can't start new thread"); + return -1; + } + + // Mark the handle running + PyMutex_Lock(&self->mutex); + assert(self->state == THREAD_HANDLE_STARTING); + self->ident = ident; + self->has_os_handle = 1; + self->os_handle = os_handle; + self->state = THREAD_HANDLE_RUNNING; + PyMutex_Unlock(&self->mutex); + + // Unblock the thread + _PyEvent_Notify(&boot->handle_ready); + + return 0; +} + static int join_thread(ThreadHandle *handle) { assert(get_thread_handle_state(handle) == THREAD_HANDLE_RUNNING); - if (handle->has_os_handle) { + PyThread_handle_t os_handle; + if (ThreadHandle_get_os_handle(handle, &os_handle)) { int err = 0; Py_BEGIN_ALLOW_THREADS - err = PyThread_join_thread(handle->os_handle); + err = PyThread_join_thread(os_handle); Py_END_ALLOW_THREADS if (err) { PyErr_SetString(ThreadError, "Failed joining thread"); @@ -257,9 +394,24 @@ join_thread(ThreadHandle *handle) return 0; } +static int +check_started(ThreadHandle *self) +{ + ThreadHandleState state = get_thread_handle_state(self); + if (state < THREAD_HANDLE_RUNNING) { + PyErr_SetString(ThreadError, "thread not started"); + return -1; + } + return 0; +} + static int ThreadHandle_join(ThreadHandle *self, PyTime_t timeout_ns) { + if (check_started(self) < 0) { + return -1; + } + // 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 @@ -271,7 +423,7 @@ ThreadHandle_join(ThreadHandle *self, PyTime_t timeout_ns) // `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) && - self->ident == PyThread_get_thread_ident_ex()) { + ThreadHandle_ident(self) == PyThread_get_thread_ident_ex()) { // PyThread_join_thread() would deadlock or error out. PyErr_SetString(ThreadError, "Cannot join current thread"); return -1; @@ -322,6 +474,10 @@ set_done(ThreadHandle *handle) static int ThreadHandle_set_done(ThreadHandle *self) { + if (check_started(self) < 0) { + return -1; + } + if (_PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)set_done, self) == -1) { return -1; @@ -338,7 +494,7 @@ typedef struct { } PyThreadHandleObject; static PyThreadHandleObject * -PyThreadHandleObject_new(thread_module_state *state) +PyThreadHandleObject_new(PyTypeObject *type) { ThreadHandle *handle = ThreadHandle_new(); if (handle == NULL) { @@ -346,7 +502,7 @@ PyThreadHandleObject_new(thread_module_state *state) } PyThreadHandleObject *self = - PyObject_New(PyThreadHandleObject, state->thread_handle_type); + (PyThreadHandleObject *)type->tp_alloc(type, 0); if (self == NULL) { ThreadHandle_decref(handle); return NULL; @@ -357,6 +513,12 @@ PyThreadHandleObject_new(thread_module_state *state) return self; } +static PyObject * +PyThreadHandleObject_tp_new(PyTypeObject *type, PyObject *args, PyObject *kwds) +{ + return (PyObject *)PyThreadHandleObject_new(type); +} + static void PyThreadHandleObject_dealloc(PyThreadHandleObject *self) { @@ -369,15 +531,16 @@ PyThreadHandleObject_dealloc(PyThreadHandleObject *self) static PyObject * PyThreadHandleObject_repr(PyThreadHandleObject *self) { + PyThread_ident_t ident = ThreadHandle_ident(self->handle); return PyUnicode_FromFormat("<%s object: ident=%" PY_FORMAT_THREAD_IDENT_T ">", - Py_TYPE(self)->tp_name, self->handle->ident); + Py_TYPE(self)->tp_name, ident); } static PyObject * PyThreadHandleObject_get_ident(PyThreadHandleObject *self, PyObject *Py_UNUSED(ignored)) { - return PyLong_FromUnsignedLongLong(self->handle->ident); + return PyLong_FromUnsignedLongLong(ThreadHandle_ident(self->handle)); } static PyObject * @@ -441,6 +604,7 @@ static PyType_Slot ThreadHandle_Type_slots[] = { {Py_tp_repr, (reprfunc)PyThreadHandleObject_repr}, {Py_tp_getset, ThreadHandle_getsetlist}, {Py_tp_methods, ThreadHandle_methods}, + {Py_tp_new, PyThreadHandleObject_tp_new}, {0, 0} }; @@ -448,7 +612,7 @@ static PyType_Spec ThreadHandle_Type_spec = { "_thread._ThreadHandle", sizeof(PyThreadHandleObject), 0, - Py_TPFLAGS_DEFAULT | Py_TPFLAGS_DISALLOW_INSTANTIATION, + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_IMMUTABLETYPE, ThreadHandle_Type_slots, }; @@ -1461,18 +1625,6 @@ _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; - PyObject *args; - PyObject *kwargs; - ThreadHandle *handle; -}; - - static void thread_bootstate_free(struct bootstate *boot, int decref) { @@ -1492,6 +1644,9 @@ thread_run(void *boot_raw) struct bootstate *boot = (struct bootstate *) boot_raw; PyThreadState *tstate = boot->tstate; + // Wait until the handle is marked as running + PyEvent_Wait(&boot->handle_ready); + // `handle` needs to be manipulated after bootstate has been freed ThreadHandle *handle = boot->handle; ThreadHandle_incref(handle); @@ -1570,66 +1725,31 @@ PyDoc_STRVAR(daemon_threads_allowed_doc, Return True if daemon threads are allowed in the current interpreter,\n\ and False otherwise.\n"); -static PyObject * +static int do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args, - PyObject *kwargs, int daemon) + PyObject *kwargs, ThreadHandle *handle, int daemon) { PyInterpreterState *interp = _PyInterpreterState_GET(); if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_THREADS)) { PyErr_SetString(PyExc_RuntimeError, "thread is not supported for isolated subinterpreters"); - return NULL; + return -1; } if (interp->finalizing) { PyErr_SetString(PyExc_PythonFinalizationError, "can't create new thread at interpreter shutdown"); - return NULL; - } - - // gh-109795: Use PyMem_RawMalloc() instead of PyMem_Malloc(), - // because it should be possible to call thread_bootstate_free() - // without holding the GIL. - struct bootstate *boot = PyMem_RawMalloc(sizeof(struct bootstate)); - if (boot == NULL) { - PyErr_NoMemory(); - return NULL; - } - boot->tstate = _PyThreadState_New(interp, _PyThreadState_WHENCE_THREADING); - if (boot->tstate == NULL) { - PyMem_RawFree(boot); - if (!PyErr_Occurred()) { - PyErr_NoMemory(); - } - return NULL; - } - PyThreadHandleObject *hobj = PyThreadHandleObject_new(state); - if (hobj == NULL) { - PyThreadState_Delete(boot->tstate); - PyMem_RawFree(boot); - return NULL; + return -1; } - ThreadHandle *handle = hobj->handle; - boot->func = Py_NewRef(func); - boot->args = Py_NewRef(args); - boot->kwargs = Py_XNewRef(kwargs); - boot->handle = handle; - ThreadHandle_incref(handle); - int err = PyThread_start_joinable_thread( - thread_run, (void *)boot, &handle->ident, &handle->os_handle); - if (err) { - PyErr_SetString(ThreadError, "can't start new thread"); - PyThreadState_Clear(boot->tstate); - thread_bootstate_free(boot, 1); - Py_DECREF(hobj); - return NULL; + if (ThreadHandle_start(handle, func, args, kwargs) < 0) { + return -1; } if (!daemon) { add_to_shutdown_handles(state, handle); } - return (PyObject *)hobj; + return 0; } static PyObject * @@ -1662,13 +1782,19 @@ thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs) return NULL; } - PyObject *hobj = - do_start_new_thread(state, func, args, kwargs, /*daemon=*/1); - if (hobj == NULL) { + ThreadHandle *handle = ThreadHandle_new(); + if (handle == NULL) { return NULL; } - PyThread_ident_t ident = ((PyThreadHandleObject *)hobj)->handle->ident; - Py_DECREF(hobj); + + int st = + do_start_new_thread(state, func, args, kwargs, handle, /*daemon=*/1); + if (st < 0) { + ThreadHandle_decref(handle); + return NULL; + } + PyThread_ident_t ident = ThreadHandle_ident(handle); + ThreadHandle_decref(handle); return PyLong_FromUnsignedLongLong(ident); } @@ -1689,13 +1815,14 @@ static PyObject * thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, PyObject *fkwargs) { - static char *keywords[] = {"function", "daemon", NULL}; + static char *keywords[] = {"function", "handle", "daemon", NULL}; PyObject *func = NULL; int daemon = 1; thread_module_state *state = get_thread_state(module); + PyObject *hobj = NULL; if (!PyArg_ParseTupleAndKeywords(fargs, fkwargs, - "O|p:start_joinable_thread", keywords, - &func, &daemon)) { + "O|Op:start_joinable_thread", keywords, + &func, &hobj, &daemon)) { return NULL; } @@ -1705,22 +1832,45 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, return NULL; } - if (PySys_Audit("_thread.start_joinable_thread", "Oi", func, daemon) < 0) { + if (hobj == NULL) { + hobj = Py_None; + } + else if (hobj != Py_None && !Py_IS_TYPE(hobj, state->thread_handle_type)) { + PyErr_SetString(PyExc_TypeError, "'handle' must be a _ThreadHandle"); + return NULL; + } + + if (PySys_Audit("_thread.start_joinable_thread", "OiO", func, daemon, + hobj) < 0) { return NULL; } + if (hobj == Py_None) { + hobj = (PyObject *)PyThreadHandleObject_new(state->thread_handle_type); + if (hobj == NULL) { + return NULL; + } + } + else { + Py_INCREF(hobj); + } + PyObject* args = PyTuple_New(0); if (args == NULL) { return NULL; } - PyObject *hobj = do_start_new_thread(state, func, args, - /*kwargs=*/ NULL, daemon); + int st = do_start_new_thread(state, func, args, + /*kwargs=*/ NULL, ((PyThreadHandleObject*)hobj)->handle, daemon); Py_DECREF(args); - return hobj; + if (st < 0) { + Py_DECREF(hobj); + return NULL; + } + return (PyObject *) hobj; } PyDoc_STRVAR(start_joinable_doc, -"start_joinable_thread(function[, daemon=True]])\n\ +"start_joinable_thread(function[, daemon=True[, handle=None]])\n\ \n\ *For internal use only*: start a new thread.\n\ \n\ @@ -1729,7 +1879,8 @@ Unlike start_new_thread(), this returns a handle object with methods to join\n\ or detach the given thread.\n\ This function is not for third-party code, please use the\n\ `threading` module instead. During finalization the runtime will not wait for\n\ -the thread to exit if daemon is True.\n"); +the thread to exit if daemon is True. If handle is provided it must be a\n\ +newly created thread._ThreadHandle instance."); static PyObject * thread_PyThread_exit_thread(PyObject *self, PyObject *Py_UNUSED(ignored)) @@ -2094,24 +2245,26 @@ PyDoc_STRVAR(shutdown_doc, Waits for all non-daemon threads (other than the calling thread) to stop."); static PyObject * -thread__make_thread_handle(PyObject *module, PyObject *ident) +thread__make_thread_handle(PyObject *module, PyObject *identobj) { thread_module_state *state = get_thread_state(module); - PyThreadHandleObject *hobj = PyThreadHandleObject_new(state); - if (hobj == NULL) { - return NULL; - } - if (!PyLong_Check(ident)) { + if (!PyLong_Check(identobj)) { PyErr_SetString(PyExc_TypeError, "ident must be an integer"); - Py_DECREF(hobj); return NULL; } - hobj->handle->ident = PyLong_AsUnsignedLongLong(ident); + PyThread_ident_t ident = PyLong_AsUnsignedLongLong(identobj); if (PyErr_Occurred()) { - Py_DECREF(hobj); return NULL; } - hobj->handle->has_os_handle = 0; + PyThreadHandleObject *hobj = + PyThreadHandleObject_new(state->thread_handle_type); + if (hobj == NULL) { + return NULL; + } + PyMutex_Lock(&hobj->handle->mutex); + hobj->handle->ident = ident; + hobj->handle->state = THREAD_HANDLE_RUNNING; + PyMutex_Unlock(&hobj->handle->mutex); return (PyObject*) hobj; } From d56f89226bf3e68a60f7ec5b06d442ac5c05f0f9 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 8 Mar 2024 09:52:55 -0800 Subject: [PATCH 16/22] Check main thread handle during shutdown This leads to a smaller diff --- Lib/threading.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/threading.py b/Lib/threading.py index 9fdc24c037bd38d..a615cee347e3529 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -1515,15 +1515,15 @@ def _shutdown(): """ Wait until the Python thread state of all non-daemon threads get deleted. """ - global _SHUTTING_DOWN # Obscure: other threads may be waiting to join _main_thread. That's # dubious, but some code does it. We can't wait for it to be marked as done # normally - that won't happen until the interpreter is nearly dead. So # mark it done here. - if _is_main_interpreter() and _SHUTTING_DOWN: + if _main_thread._handle.is_done() and _is_main_interpreter(): # _shutdown() was already called return + global _SHUTTING_DOWN _SHUTTING_DOWN = True # Call registered threading atexit functions before threads are joined. From 2218c0a4b6406c303b9e6dcc1383685d3403fdf6 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 8 Mar 2024 15:49:28 -0800 Subject: [PATCH 17/22] Remove vestigial _PyEventRc declarations --- Include/internal/pycore_lock.h | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index 241ea8a55e877c9..7e1c0d853bf491e 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -157,16 +157,6 @@ 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; - -_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 // implementation. From f9d329078e8423e731c3406ed4e585b4023c790d Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 8 Mar 2024 15:51:36 -0800 Subject: [PATCH 18/22] Remove duplicate declaration --- Include/internal/pycore_lock.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index 7e1c0d853bf491e..f993c95ecbf75ae 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -144,10 +144,6 @@ PyAPI_FUNC(int) _PyEvent_IsSet(PyEvent *evt); // Export for '_testinternalcapi' shared extension PyAPI_FUNC(void) _PyEvent_Notify(PyEvent *evt); -// 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); - // Wait for the event to be set. If the event is already set, then this returns // immediately. PyAPI_FUNC(void) PyEvent_Wait(PyEvent *evt); From a7095e47b60465c9a96fd1b2e7c822d7ae040f40 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 8 Mar 2024 15:54:23 -0800 Subject: [PATCH 19/22] Revert order change in `_DummyThread.is_alive` --- Lib/threading.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/threading.py b/Lib/threading.py index a615cee347e3529..31ab77c92b1c205 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -1405,7 +1405,7 @@ def __init__(self): _DeleteDummyThreadOnDel(self) def is_alive(self): - if self._started.is_set() and not self._handle.is_done(): + if not self._handle.is_done() and self._started.is_set(): return True raise RuntimeError("thread is not alive") From 180300c70fe8058c0aefa3ed97495ccfcc26008f Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 11 Mar 2024 15:47:09 -0700 Subject: [PATCH 20/22] Add handles to shutdown list before starting the thread --- Modules/_threadmodule.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 9f837d50cfa6562..faf9b3804d1b32b 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1737,14 +1737,20 @@ do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args, return -1; } - if (ThreadHandle_start(handle, func, args, kwargs) < 0) { - return -1; - } - if (!daemon) { + // Add the handle before starting the thread to avoid adding a handle + // to a thread that has already finished (i.e. if the thread finishes + // before the call to `ThreadHandle_start()` below returns). add_to_shutdown_handles(state, handle); } + if (ThreadHandle_start(handle, func, args, kwargs) < 0) { + if (!daemon) { + remove_from_shutdown_handles(handle); + } + return -1; + } + return 0; } From c486503616cb91932b247e6d35b57dc0ac8a8844 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 11 Mar 2024 21:11:19 -0700 Subject: [PATCH 21/22] Move some code around to remove need for forward decls --- Modules/_threadmodule.c | 193 +++++++++++++++++++--------------------- 1 file changed, 93 insertions(+), 100 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index faf9b3804d1b32b..09b09ee09ea6376 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -23,12 +23,6 @@ // Forward declarations static struct PyModuleDef thread_module; -static void -thread_run(void *boot_raw); -struct bootstate; -static void -thread_bootstate_free(struct bootstate *boot, int decref); - // Module state typedef struct { PyTypeObject *excepthook_type; @@ -110,18 +104,6 @@ typedef struct { Py_ssize_t refcount; } ThreadHandle; -// 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; - PyObject *args; - PyObject *kwargs; - ThreadHandle *handle; - PyEvent handle_ready; -}; - static inline int get_thread_handle_state(ThreadHandle *handle) { @@ -292,6 +274,99 @@ _PyThread_AfterFork(struct _pythread_runtime_state *state) } } +// 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; + PyObject *args; + PyObject *kwargs; + ThreadHandle *handle; + PyEvent handle_ready; +}; + +static void +thread_bootstate_free(struct bootstate *boot, int decref) +{ + if (decref) { + Py_DECREF(boot->func); + Py_DECREF(boot->args); + Py_XDECREF(boot->kwargs); + } + ThreadHandle_decref(boot->handle); + PyMem_RawFree(boot); +} + +static void +thread_run(void *boot_raw) +{ + struct bootstate *boot = (struct bootstate *) boot_raw; + PyThreadState *tstate = boot->tstate; + + // Wait until the handle is marked as running + PyEvent_Wait(&boot->handle_ready); + + // `handle` needs to be manipulated after bootstate has been freed + ThreadHandle *handle = boot->handle; + ThreadHandle_incref(handle); + + // 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 + // threads must exit, except of the thread calling Py_Finalize() whch holds + // the GIL and must not exit. + // + // At this stage, tstate can be a dangling pointer (point to freed memory), + // it's ok to call _PyThreadState_MustExit() with a dangling pointer. + if (_PyThreadState_MustExit(tstate)) { + // Don't call PyThreadState_Clear() nor _PyThreadState_DeleteCurrent(). + // These functions are called on tstate indirectly by Py_Finalize() + // which calls _PyInterpreterState_Clear(). + // + // Py_DECREF() cannot be called because the GIL is not held: leak + // references on purpose. Python is being finalized anyway. + thread_bootstate_free(boot, 0); + goto exit; + } + + _PyThreadState_Bind(tstate); + PyEval_AcquireThread(tstate); + _Py_atomic_add_ssize(&tstate->interp->threads.count, 1); + + PyObject *res = PyObject_Call(boot->func, boot->args, boot->kwargs); + if (res == NULL) { + if (PyErr_ExceptionMatches(PyExc_SystemExit)) + /* SystemExit is ignored silently */ + PyErr_Clear(); + else { + PyErr_FormatUnraisable( + "Exception ignored in thread started by %R", boot->func); + } + } + else { + Py_DECREF(res); + } + + thread_bootstate_free(boot, 1); + + _Py_atomic_add_ssize(&tstate->interp->threads.count, -1); + PyThreadState_Clear(tstate); + _PyThreadState_DeleteCurrent(tstate); + +exit: + // Don't need to wait for this thread anymore + remove_from_shutdown_handles(handle); + + _PyEvent_Notify(&handle->thread_is_exiting); + ThreadHandle_decref(handle); + + // 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). + return; +} + static int force_done(ThreadHandle *handle) { @@ -1621,88 +1696,6 @@ _localdummy_destroyed(PyObject *localweakref, PyObject *dummyweakref) /* Module functions */ -static void -thread_bootstate_free(struct bootstate *boot, int decref) -{ - if (decref) { - Py_DECREF(boot->func); - Py_DECREF(boot->args); - Py_XDECREF(boot->kwargs); - } - ThreadHandle_decref(boot->handle); - PyMem_RawFree(boot); -} - - -static void -thread_run(void *boot_raw) -{ - struct bootstate *boot = (struct bootstate *) boot_raw; - PyThreadState *tstate = boot->tstate; - - // Wait until the handle is marked as running - PyEvent_Wait(&boot->handle_ready); - - // `handle` needs to be manipulated after bootstate has been freed - ThreadHandle *handle = boot->handle; - ThreadHandle_incref(handle); - - // 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 - // threads must exit, except of the thread calling Py_Finalize() whch holds - // the GIL and must not exit. - // - // At this stage, tstate can be a dangling pointer (point to freed memory), - // it's ok to call _PyThreadState_MustExit() with a dangling pointer. - if (_PyThreadState_MustExit(tstate)) { - // Don't call PyThreadState_Clear() nor _PyThreadState_DeleteCurrent(). - // These functions are called on tstate indirectly by Py_Finalize() - // which calls _PyInterpreterState_Clear(). - // - // Py_DECREF() cannot be called because the GIL is not held: leak - // references on purpose. Python is being finalized anyway. - thread_bootstate_free(boot, 0); - goto exit; - } - - _PyThreadState_Bind(tstate); - PyEval_AcquireThread(tstate); - _Py_atomic_add_ssize(&tstate->interp->threads.count, 1); - - PyObject *res = PyObject_Call(boot->func, boot->args, boot->kwargs); - if (res == NULL) { - if (PyErr_ExceptionMatches(PyExc_SystemExit)) - /* SystemExit is ignored silently */ - PyErr_Clear(); - else { - PyErr_FormatUnraisable( - "Exception ignored in thread started by %R", boot->func); - } - } - else { - Py_DECREF(res); - } - - thread_bootstate_free(boot, 1); - - _Py_atomic_add_ssize(&tstate->interp->threads.count, -1); - PyThreadState_Clear(tstate); - _PyThreadState_DeleteCurrent(tstate); - -exit: - // Don't need to wait for this thread anymore - remove_from_shutdown_handles(handle); - - _PyEvent_Notify(&handle->thread_is_exiting); - ThreadHandle_decref(handle); - - // 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). - return; -} - static PyObject * thread_daemon_threads_allowed(PyObject *module, PyObject *Py_UNUSED(ignored)) { From 31216236c8bbe1a73b1b7f781a116cabbd49ee45 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 11 Mar 2024 21:15:09 -0700 Subject: [PATCH 22/22] Simplify start failure path --- Modules/_threadmodule.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 09b09ee09ea6376..438bbc4ca37c55d 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -376,12 +376,6 @@ force_done(ThreadHandle *handle) return 0; } -static void -start_failed(ThreadHandle *handle) -{ - _PyOnceFlag_CallOnce(&handle->once, (_Py_once_fn_t *)force_done, handle); -} - static int ThreadHandle_start(ThreadHandle *self, PyObject *func, PyObject *args, PyObject *kwargs) @@ -405,8 +399,7 @@ ThreadHandle_start(ThreadHandle *self, PyObject *func, PyObject *args, struct bootstate *boot = PyMem_RawMalloc(sizeof(struct bootstate)); if (boot == NULL) { PyErr_NoMemory(); - start_failed(self); - return -1; + goto start_failed; } PyInterpreterState *interp = _PyInterpreterState_GET(); boot->tstate = _PyThreadState_New(interp, _PyThreadState_WHENCE_THREADING); @@ -415,8 +408,7 @@ ThreadHandle_start(ThreadHandle *self, PyObject *func, PyObject *args, if (!PyErr_Occurred()) { PyErr_NoMemory(); } - start_failed(self); - return -1; + goto start_failed; } boot->func = Py_NewRef(func); boot->args = Py_NewRef(args); @@ -430,9 +422,8 @@ ThreadHandle_start(ThreadHandle *self, PyObject *func, PyObject *args, if (PyThread_start_joinable_thread(thread_run, boot, &ident, &os_handle)) { PyThreadState_Clear(boot->tstate); thread_bootstate_free(boot, 1); - start_failed(self); PyErr_SetString(ThreadError, "can't start new thread"); - return -1; + goto start_failed; } // Mark the handle running @@ -448,6 +439,10 @@ ThreadHandle_start(ThreadHandle *self, PyObject *func, PyObject *args, _PyEvent_Notify(&boot->handle_ready); return 0; + +start_failed: + _PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)force_done, self); + return -1; } static int