[Python-checkins] bpo-36402: Fix threading._shutdown() race condition (GH-13948) (GH-14050) (GH-14054)
Victor Stinner
webhook-mailer at python.org
Thu Jun 13 08:22:25 EDT 2019
https://github.com/python/cpython/commit/6eb2878e42152e9c45d7ee5e6f889532d753e67c
commit: 6eb2878e42152e9c45d7ee5e6f889532d753e67c
branch: 3.7
author: Miss Islington (bot) <31488909+miss-islington at users.noreply.github.com>
committer: Victor Stinner <vstinner at redhat.com>
date: 2019-06-13T14:22:20+02:00
summary:
bpo-36402: Fix threading._shutdown() race condition (GH-13948) (GH-14050) (GH-14054)
* 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.
(cherry picked from commit 468e5fec8a2f534f1685d59da3ca4fad425c38dd)
* bpo-36402: Fix threading.Thread._stop() (GH-14047)
Remove the _tstate_lock from _shutdown_locks, don't remove None.
(cherry picked from commit 6f75c873752a16a7ad8f35855b1e29f59d048e84)
(cherry picked from commit e40a97a721d46307dfdc2b0322028ccded6eb571)
Co-authored-by: Victor Stinner <vstinner at redhat.com>
files:
A Misc/NEWS.d/next/Library/2019-06-11-00-35-02.bpo-36402.b0IJVp.rst
M Lib/test/test_threading.py
M Lib/threading.py
diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py
index aa810bda1c2a..36d9fbb162e2 100644
--- a/Lib/test/test_threading.py
+++ b/Lib/test/test_threading.py
@@ -578,6 +578,41 @@ def __del__(self):
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()
@@ -698,6 +733,30 @@ def callback():
finally:
sys.settrace(old_trace)
+ @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)
+
class ThreadJoinOnShutdown(BaseTestCase):
@@ -875,15 +934,22 @@ def test_threads_join(self):
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)
@@ -900,22 +966,29 @@ def test_threads_join_2(self):
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 318b3301126a..0fb3bdd55cd7 100644
--- a/Lib/threading.py
+++ b/Lib/threading.py
@@ -733,6 +733,11 @@ def _newname(template="Thread-%d"):
_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
@@ -899,6 +904,10 @@ def _set_tstate_lock(self):
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()
@@ -987,6 +996,9 @@ def _stop(self):
assert not lock.locked()
self._is_stopped = True
self._tstate_lock = None
+ if not self.daemon:
+ with _shutdown_locks_lock:
+ _shutdown_locks.discard(lock)
def _delete(self):
"Remove current thread from the dict of currently running threads."
@@ -1261,6 +1273,9 @@ def enumerate():
_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
@@ -1269,6 +1284,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.
@@ -1276,16 +1293,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.
@@ -1311,12 +1336,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 000000000000..3bc537e40ff6
--- /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.
More information about the Python-checkins
mailing list