From 42ba6e79162046f3cb45e23dd833fd25d9a9fe93 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Thu, 26 Jun 2025 15:44:06 +0200 Subject: [PATCH 1/6] gh-87135: threading.Lock: Raise rather than hang on Python finalization After Python finalization gets to the point where no other thread can attach thread state, attempting to acquire a Python lock must hang. Raise PythonFinalizationError instead of hanging. --- Include/internal/pycore_lock.h | 5 ++++ Lib/test/test_threading.py | 45 ++++++++++++++++++++++++++++++++++ Modules/_threadmodule.c | 12 +++++++-- Python/lock.c | 12 +++++++++ 4 files changed, 72 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index 32b60cc33a21f1..3e8e1856052598 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -51,6 +51,11 @@ typedef enum _PyLockFlags { // Fail if interrupted by a signal while waiting on the lock. _PY_FAIL_IF_INTERRUPTED = 4, + + // Locking & unlocking this lock requires attached thread state. + // If locking returns PY_LOCK_FAILURE, a Python exception *may* be raised. + // Implies _PY_LOCK_HANDLE_SIGNALS and _PY_LOCK_DETACH. + _PY_LOCK_PYTHONLOCK = 8 | 2 | 1, } _PyLockFlags; // Lock a mutex with an optional timeout and additional options. See diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 13b55d0f0a2e73..d117a017ffe7e8 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1247,6 +1247,51 @@ def __del__(self): self.assertEqual(err, b"") self.assertIn(b"all clear", out) + def test_acquire_daemon_thread_lock_in_finalization(self): + # gh-123940: Py_Finalize() prevents other threads from running Python + # code (and so, releasing locks), so acquiring a locked lock can not + # succeed. + # We raise an exception rather than hang. + for timeout in (None, 10): + with self.subTest(timeout=timeout): + code = textwrap.dedent(f""" + import threading + import time + + thread_started_event = threading.Event() + + lock = threading.Lock() + def loop(): + with lock: + thread_started_event.set() + while True: + time.sleep(1) + + class Cycle: + def __init__(self): + self.self_ref = self + self.thr = threading.Thread( + target=loop, daemon=True) + self.thr.start() + thread_started_event.wait() + + def __del__(self): + assert self.thr.is_alive() + try: + lock.acquire() + except PythonFinalizationError: + assert self.thr.is_alive() + print('got the correct exception!') + + # Cycle holds a reference to itself, which ensures it is + # cleaned up during the GC that runs after daemon threads + # have been forced to exit during finalization. + Cycle() + """) + rc, out, err = assert_python_ok("-c", code) + self.assertEqual(err, b"") + self.assertIn(b"got the correct exception", out) + def test_start_new_thread_failed(self): # gh-109746: if Python fails to start newly created thread # due to failure of underlying PyThread_start_new_thread() call, diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 150a266b521736..174598d00e6168 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -835,8 +835,12 @@ lock_PyThread_acquire_lock(PyObject *op, PyObject *args, PyObject *kwds) } PyLockStatus r = _PyMutex_LockTimed(&self->lock, timeout, - _PY_LOCK_HANDLE_SIGNALS | _PY_LOCK_DETACH); + _PY_LOCK_PYTHONLOCK); if (r == PY_LOCK_INTR) { + assert(PyErr_Occurred()); + return NULL; + } + if (r == PY_LOCK_FAILURE && PyErr_Occurred()) { return NULL; } @@ -1055,8 +1059,12 @@ rlock_acquire(PyObject *op, PyObject *args, PyObject *kwds) } PyLockStatus r = _PyRecursiveMutex_LockTimed(&self->lock, timeout, - _PY_LOCK_HANDLE_SIGNALS | _PY_LOCK_DETACH); + _PY_LOCK_PYTHONLOCK); if (r == PY_LOCK_INTR) { + assert(PyErr_Occurred()); + return NULL; + } + if (r == PY_LOCK_FAILURE && PyErr_Occurred()) { return NULL; } diff --git a/Python/lock.c b/Python/lock.c index ea6ac00bfeccb4..eb09019e0a236d 100644 --- a/Python/lock.c +++ b/Python/lock.c @@ -95,6 +95,18 @@ _PyMutex_LockTimed(PyMutex *m, PyTime_t timeout, _PyLockFlags flags) if (timeout == 0) { return PY_LOCK_FAILURE; } + if ((flags & _PY_LOCK_PYTHONLOCK) && Py_IsFinalizing()) { + // At this phase of runtime shutdown, only the finalization thread + // can have attached thread state; others hang if they try + // attaching. And since operations on this lock requires attached + // thread state (_PY_LOCK_PYTHONLOCK), the finalization thread is + // running this code, and no other thread can unlock. + // Raise rather than hang. (_PY_LOCK_PYTHONLOCK allows raising + // exceptons.) + PyErr_SetString(PyExc_PythonFinalizationError, + "cannot acquire lock at interpreter finalization"); + return PY_LOCK_FAILURE; + } uint8_t newv = v; if (!(v & _Py_HAS_PARKED)) { From b330172f1d03824d4d31a41b112442e0cd63b2cc Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 27 Jun 2025 09:17:41 +0200 Subject: [PATCH 2/6] Use single bit in the enum value --- Include/internal/pycore_lock.h | 4 ++-- Modules/_threadmodule.c | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index 3e8e1856052598..bd6011b60ac09a 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -54,8 +54,8 @@ typedef enum _PyLockFlags { // Locking & unlocking this lock requires attached thread state. // If locking returns PY_LOCK_FAILURE, a Python exception *may* be raised. - // Implies _PY_LOCK_HANDLE_SIGNALS and _PY_LOCK_DETACH. - _PY_LOCK_PYTHONLOCK = 8 | 2 | 1, + // (Intended for use with _PY_LOCK_HANDLE_SIGNALS and _PY_LOCK_DETACH.) + _PY_LOCK_PYTHONLOCK = 8, } _PyLockFlags; // Lock a mutex with an optional timeout and additional options. See diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 174598d00e6168..a13a4784fde015 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -834,8 +834,9 @@ lock_PyThread_acquire_lock(PyObject *op, PyObject *args, PyObject *kwds) return NULL; } - PyLockStatus r = _PyMutex_LockTimed(&self->lock, timeout, - _PY_LOCK_PYTHONLOCK); + PyLockStatus r = _PyMutex_LockTimed( + &self->lock, timeout, + _PY_LOCK_PYTHONLOCK | _PY_LOCK_HANDLE_SIGNALS | _PY_LOCK_DETACH); if (r == PY_LOCK_INTR) { assert(PyErr_Occurred()); return NULL; @@ -1058,8 +1059,9 @@ rlock_acquire(PyObject *op, PyObject *args, PyObject *kwds) return NULL; } - PyLockStatus r = _PyRecursiveMutex_LockTimed(&self->lock, timeout, - _PY_LOCK_PYTHONLOCK); + PyLockStatus r = _PyRecursiveMutex_LockTimed( + &self->lock, timeout, + _PY_LOCK_PYTHONLOCK | _PY_LOCK_HANDLE_SIGNALS | _PY_LOCK_DETACH); if (r == PY_LOCK_INTR) { assert(PyErr_Occurred()); return NULL; From 657c68dfd76e98a25e7dce2ec07f2d80e6a91f72 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 27 Jun 2025 09:26:10 +0200 Subject: [PATCH 3/6] Add docs --- Doc/library/exceptions.rst | 9 ++++++++- .../2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst | 3 +++ 2 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Library/2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst diff --git a/Doc/library/exceptions.rst b/Doc/library/exceptions.rst index bb72032891ea98..d39603cb386fb3 100644 --- a/Doc/library/exceptions.rst +++ b/Doc/library/exceptions.rst @@ -429,7 +429,9 @@ The following exceptions are the exceptions that are usually raised. * Creating a new Python thread. * :meth:`Joining ` a running daemon thread. - * :func:`os.fork`. + * :func:`os.fork`, + * acquiring a lock such as :cls:`threading.Lock`, when it is known that + the operation would otherwise deadlock. See also the :func:`sys.is_finalizing` function. @@ -440,6 +442,11 @@ The following exceptions are the exceptions that are usually raised. :meth:`threading.Thread.join` can now raise this exception. + .. versionchanged:: next + + This exception may be raised when acquiring :meth:`threading.Lock` + or :meth:`threading.RLock`. + .. exception:: RecursionError This exception is derived from :exc:`RuntimeError`. It is raised when the diff --git a/Misc/NEWS.d/next/Library/2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst b/Misc/NEWS.d/next/Library/2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst new file mode 100644 index 00000000000000..277b6eefc05209 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst @@ -0,0 +1,3 @@ +Acquiring a :cls:`threading.Lock` or :cls:`threading.RLock` at interpreter +shutdown will raise :exc:`PythonFinalizationError` if Python can determine +that it would otherwise deadlock. From 5bfdbc7a728969bbb542f7b0df9aa703a3857753 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 27 Jun 2025 09:36:27 +0200 Subject: [PATCH 4/6] Improve test --- Lib/test/test_threading.py | 76 ++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index d117a017ffe7e8..eb641ad4fd9c11 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1247,50 +1247,56 @@ def __del__(self): self.assertEqual(err, b"") self.assertIn(b"all clear", out) - def test_acquire_daemon_thread_lock_in_finalization(self): + @support.subTests('lock_class_name', ['Lock', 'RLock']) + def test_acquire_daemon_thread_lock_in_finalization(self, lock_class_name): # gh-123940: Py_Finalize() prevents other threads from running Python # code (and so, releasing locks), so acquiring a locked lock can not # succeed. # We raise an exception rather than hang. - for timeout in (None, 10): - with self.subTest(timeout=timeout): - code = textwrap.dedent(f""" - import threading - import time + code = textwrap.dedent(f""" + import threading + import time - thread_started_event = threading.Event() + thread_started_event = threading.Event() - lock = threading.Lock() - def loop(): - with lock: - thread_started_event.set() - while True: - time.sleep(1) + lock = threading.{lock_class_name}() + def loop(): + with lock: + thread_started_event.set() + while True: + time.sleep(1) - class Cycle: - def __init__(self): - self.self_ref = self - self.thr = threading.Thread( - target=loop, daemon=True) - self.thr.start() - thread_started_event.wait() + uncontested_lock = threading.{lock_class_name}() - def __del__(self): - assert self.thr.is_alive() - try: - lock.acquire() - except PythonFinalizationError: - assert self.thr.is_alive() - print('got the correct exception!') + class Cycle: + def __init__(self): + self.self_ref = self + self.thr = threading.Thread( + target=loop, daemon=True) + self.thr.start() + thread_started_event.wait() - # Cycle holds a reference to itself, which ensures it is - # cleaned up during the GC that runs after daemon threads - # have been forced to exit during finalization. - Cycle() - """) - rc, out, err = assert_python_ok("-c", code) - self.assertEqual(err, b"") - self.assertIn(b"got the correct exception", out) + def __del__(self): + assert self.thr.is_alive() + + # We *can* acquire an unlocked lock + uncontested_lock.acquire() + + # Acquiring a locked one fails + try: + lock.acquire() + except PythonFinalizationError: + assert self.thr.is_alive() + print('got the correct exception!') + + # Cycle holds a reference to itself, which ensures it is + # cleaned up during the GC that runs after daemon threads + # have been forced to exit during finalization. + Cycle() + """) + rc, out, err = assert_python_ok("-c", code) + self.assertEqual(err, b"") + self.assertIn(b"got the correct exception", out) def test_start_new_thread_failed(self): # gh-109746: if Python fails to start newly created thread From 59d07fa0688d51a7c6c7b0e1d30815126f5f617e Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 27 Jun 2025 09:44:03 +0200 Subject: [PATCH 5/6] Test locking RLock recursively --- Lib/test/test_threading.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index eb641ad4fd9c11..00a3037c3e1e01 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1261,6 +1261,8 @@ def test_acquire_daemon_thread_lock_in_finalization(self, lock_class_name): lock = threading.{lock_class_name}() def loop(): + if {lock_class_name!r} == 'RLock': + lock.acquire() with lock: thread_started_event.set() while True: @@ -1281,6 +1283,8 @@ def __del__(self): # We *can* acquire an unlocked lock uncontested_lock.acquire() + if {lock_class_name!r} == 'RLock': + uncontested_lock.acquire() # Acquiring a locked one fails try: From 8f93dd24ff5ed64bddbaf226016626584d78fcf6 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Fri, 27 Jun 2025 09:59:40 +0200 Subject: [PATCH 6/6] Fix ReST role --- Doc/library/exceptions.rst | 2 +- .../next/Library/2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Doc/library/exceptions.rst b/Doc/library/exceptions.rst index d39603cb386fb3..e35a06749ea686 100644 --- a/Doc/library/exceptions.rst +++ b/Doc/library/exceptions.rst @@ -430,7 +430,7 @@ The following exceptions are the exceptions that are usually raised. * Creating a new Python thread. * :meth:`Joining ` a running daemon thread. * :func:`os.fork`, - * acquiring a lock such as :cls:`threading.Lock`, when it is known that + * acquiring a lock such as :class:`threading.Lock`, when it is known that the operation would otherwise deadlock. See also the :func:`sys.is_finalizing` function. diff --git a/Misc/NEWS.d/next/Library/2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst b/Misc/NEWS.d/next/Library/2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst index 277b6eefc05209..4b6bc74cad87a4 100644 --- a/Misc/NEWS.d/next/Library/2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst +++ b/Misc/NEWS.d/next/Library/2025-06-27-09-26-04.gh-issue-87135.33z0UW.rst @@ -1,3 +1,3 @@ -Acquiring a :cls:`threading.Lock` or :cls:`threading.RLock` at interpreter +Acquiring a :class:`threading.Lock` or :class:`threading.RLock` at interpreter shutdown will raise :exc:`PythonFinalizationError` if Python can determine that it would otherwise deadlock.