From 468e5fec8a2f534f1685d59da3ca4fad425c38dd Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 13 Jun 2019 01:30:17 +0200 Subject: [PATCH] bpo-36402: Fix threading._shutdown() race condition (GH-13948) Fix a race condition at Python shutdown when waiting for threads. Wait until the Python thread state of all non-daemon threads get deleted (join all non-daemon threads), rather than just wait until Python threads complete. * Add threading._shutdown_locks: 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(). * Add also threading._shutdown_locks_lock to protect access to threading._shutdown_locks. * Add test_finalization_shutdown() test. --- Lib/test/test_threading.py | 55 ++++++++++++++++++- Lib/threading.py | 49 ++++++++++++++--- .../2019-06-11-00-35-02.bpo-36402.b0IJVp.rst | 4 ++ 3 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2019-06-11-00-35-02.bpo-36402.b0IJVp.rst diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 6ac4ea9623..ad90010b8a 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -583,6 +583,41 @@ class ThreadTests(BaseTestCase): self.assertEqual(data.splitlines(), ["GC: True True True"] * 2) + def test_finalization_shutdown(self): + # bpo-36402: Py_Finalize() calls threading._shutdown() which must wait + # until Python thread states of all non-daemon threads get deleted. + # + # Test similar to SubinterpThreadingTests.test_threads_join_2(), but + # test the finalization of the main interpreter. + code = """if 1: + import os + import threading + import time + import random + + def random_sleep(): + seconds = random.random() * 0.010 + time.sleep(seconds) + + class Sleeper: + def __del__(self): + random_sleep() + + tls = threading.local() + + def f(): + # Sleep a bit so that the thread is still running when + # Py_Finalize() is called. + random_sleep() + tls.x = Sleeper() + random_sleep() + + threading.Thread(target=f).start() + random_sleep() + """ + rc, out, err = assert_python_ok("-c", code) + self.assertEqual(err, b"") + def test_tstate_lock(self): # Test an implementation detail of Thread objects. started = _thread.allocate_lock() @@ -878,15 +913,22 @@ class SubinterpThreadingTests(BaseTestCase): self.addCleanup(os.close, w) code = r"""if 1: import os + import random import threading import time + def random_sleep(): + seconds = random.random() * 0.010 + time.sleep(seconds) + def f(): # Sleep a bit so that the thread is still running when # Py_EndInterpreter is called. - time.sleep(0.05) + random_sleep() os.write(%d, b"x") + threading.Thread(target=f).start() + random_sleep() """ % (w,) ret = test.support.run_in_subinterp(code) self.assertEqual(ret, 0) @@ -903,22 +945,29 @@ class SubinterpThreadingTests(BaseTestCase): self.addCleanup(os.close, w) code = r"""if 1: import os + import random import threading import time + def random_sleep(): + seconds = random.random() * 0.010 + time.sleep(seconds) + class Sleeper: def __del__(self): - time.sleep(0.05) + random_sleep() tls = threading.local() def f(): # Sleep a bit so that the thread is still running when # Py_EndInterpreter is called. - time.sleep(0.05) + random_sleep() tls.x = Sleeper() os.write(%d, b"x") + threading.Thread(target=f).start() + random_sleep() """ % (w,) ret = test.support.run_in_subinterp(code) self.assertEqual(ret, 0) diff --git a/Lib/threading.py b/Lib/threading.py index 3d197eed6a..6792640377 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -739,6 +739,11 @@ _active_limbo_lock = _allocate_lock() _active = {} # maps thread id to Thread object _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() # Main class for threads @@ -903,6 +908,10 @@ class Thread: self._tstate_lock = _set_sentinel() self._tstate_lock.acquire() + if not self.daemon: + with _shutdown_locks_lock: + _shutdown_locks.add(self._tstate_lock) + def _bootstrap_inner(self): try: self._set_ident() @@ -954,6 +963,9 @@ class Thread: assert not lock.locked() self._is_stopped = True self._tstate_lock = None + if not self.daemon: + with _shutdown_locks_lock: + _shutdown_locks.discard(self._tstate_lock) def _delete(self): "Remove current thread from the dict of currently running threads." @@ -1342,6 +1354,9 @@ from _thread import stack_size _main_thread = _MainThread() def _shutdown(): + """ + Wait until the Python thread state of all non-daemon threads get deleted. + """ # 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 @@ -1350,6 +1365,8 @@ def _shutdown(): if _main_thread._is_stopped: # _shutdown() was already called return + + # Main thread tlock = _main_thread._tstate_lock # The main thread isn't finished yet, so its thread state lock can't have # been released. @@ -1357,16 +1374,24 @@ def _shutdown(): assert tlock.locked() tlock.release() _main_thread._stop() - t = _pickSomeNonDaemonThread() - while t: - t.join() - t = _pickSomeNonDaemonThread() -def _pickSomeNonDaemonThread(): - for t in enumerate(): - if not t.daemon and t.is_alive(): - return t - return None + # 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: + # mimick Thread.join() + lock.acquire() + lock.release() + + # new threads can be spawned while we were waiting for the other + # threads to complete + def main_thread(): """Return the main thread object. @@ -1392,12 +1417,18 @@ 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 = _allocate_lock() # fork() only copied the current thread; clear references to others. new_active = {} current = current_thread() _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. diff --git a/Misc/NEWS.d/next/Library/2019-06-11-00-35-02.bpo-36402.b0IJVp.rst b/Misc/NEWS.d/next/Library/2019-06-11-00-35-02.bpo-36402.b0IJVp.rst new file mode 100644 index 0000000000..3bc537e40f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-06-11-00-35-02.bpo-36402.b0IJVp.rst @@ -0,0 +1,4 @@ +Fix a race condition at Python shutdown when waiting for threads. Wait until +the Python thread state of all non-daemon threads get deleted (join all +non-daemon threads), rather than just wait until non-daemon Python threads +complete. -- 2.40.0