From cbc2e33459689618f1c89bd90451146bcbd08422 Mon Sep 17 00:00:00 2001 From: Jeremy Maitin-Shepard Date: Wed, 22 Sep 2021 14:09:32 -0700 Subject: [PATCH 1/8] bpo-42969: Hang non-main threads that attempt to acquire the GIL during finalization --- Doc/c-api/init.rst | 186 +++++++++++++++--- Doc/data/stable_abi.dat | 4 + Doc/tools/extensions/pyspecific.py | 6 +- Include/internal/pycore_runtime.h | 13 ++ Include/pystate.h | 47 +++++ Include/pythread.h | 32 ++- Lib/test/test_stable_abi_ctypes.py | 4 + Lib/test/test_threading.py | 119 +++++++++++ ...2-08-05-19-41-20.gh-issue-28525.SCNBYj.rst | 17 ++ .../2021-09-22-14-19-02.bpo-42969.8Z2mth.rst | 3 + Misc/stable_abi.toml | 8 + Modules/_testcapimodule.c | 45 +++++ PC/python3dll.c | 4 + Python/ceval_gil.c | 48 +---- Python/condvar.h | 40 ++++ Python/pylifecycle.c | 35 ++++ Python/pystate.c | 49 +++++ Python/thread_nt.h | 8 + Python/thread_pthread.h | 10 + 19 files changed, 611 insertions(+), 67 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-28525.SCNBYj.rst create mode 100644 Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 038498f325ceb3..18d94a296fee74 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -409,7 +409,11 @@ Initializing and finalizing the interpreter freed. Some memory allocated by extension modules may not be freed. Some extensions may not work properly if their initialization routine is called more than once; this can happen if an application calls :c:func:`Py_Initialize` and - :c:func:`Py_FinalizeEx` more than once. + :c:func:`Py_FinalizeEx` more than once. :c:func:`Py_FinalizeEx` must not be + called recursively from within itself. Therefore, it must not be called by any + code that may be run as part of the interpreter shutdown process, such as + :py:mod:`atexit` handlers, object finalizers, or any code that may be run while + flushing the stdout and stderr files. .. audit-event:: cpython._PySys_ClearAuditHooks "" c.Py_FinalizeEx @@ -1000,6 +1004,78 @@ thread, where the CPython global runtime was originally initialized. The only exception is if :c:func:`exec` will be called immediately after. +.. _cautions-regarding-runtime-finalization: + +Cautions regarding runtime finalization +--------------------------------------- + +In the late stage of :term:`interpreter shutdown`, after attempting to wait for +non-daemon threads to exit (though this can be interrupted by +:class:`KeyboardInterrupt`) and running the :mod:`atexit` functions, the runtime +is marked as *finalizing*: :c:func:`_Py_IsFinalizing` and +:func:`sys.is_finalizing` return true. At this point, only the *finalization +thread* that initiated finalization (typically the main thread) is allowed to +acquire the :term:`GIL`. + +If any thread, other than the finalization thread, attempts to acquire the GIL +during finalization, either explicitly via :c:func:`PyGILState_Ensure`, +:c:macro:`Py_END_ALLOW_THREADS`, :c:func:`PyEval_AcquireThread`, or +:c:func:`PyEval_AcquireLock`, or implicitly when the interpreter attempts to +reacquire it after having yielded it, the thread enters a permanently blocked +state where it remains until the program exits. In most cases this is harmless, +but this can result in deadlock if a later stage of finalization attempts to +acquire a lock owned by the blocked thread, or otherwise waits on the blocked +thread. + +To avoid non-Python threads becoming blocked, or Python-created threads becoming +blocked while executing C extension code, you can use +:c:func:`PyThread_TryAcquireFinalizeBlock` and +:c:func:`PyThread_ReleaseFinalizeBlock`. + +For example, to deliver an asynchronous notification to Python from a C +extension, you might be inclined to write the following code that is *not* safe +to execute during finalization: + +.. code-block:: c + + // some non-Python created thread that wants to send Python an async notification + PyGILState_STATE state = PyGILState_Ensure(); // may hang thread + // call `call_soon_threadsafe` on some event loop object + PyGILState_Release(state); + +To avoid the possibility of the thread hanging during finalization, and also +support older Python versions: + +.. code-block:: c + + // some non-Python created thread that wants to send Python an async notification + PyGILState_STATE state; + #if PY_VERSION_HEX >= 0x030c0000 // API added in Python 3.12 + int acquired = PyThread_TryAcquireFinalizeBlock(); + if (!acquired) { + // skip sending notification since python is exiting + return; + } + #endif // PY_VERSION_HEX + state = PyGILState_Ensure(); // safe now + // call `call_soon_threadsafe` on some event loop object + PyGILState_Release(state); + #if PY_VERSION_HEX >= 0x030c0000 // API added in Python 3.12 + PyThread_ReleaseFinalizeBlock(); + #endif // PY_VERSION_HEX + +Or with the convenience interface (requires Python >=3.12): + +.. code-block:: c + + // some non-Python created thread that wants to send Python an async notification + PyGILState_TRY_STATE state = PyGILState_TryAcquireFinalizeBlockAndGIL(); + if (!state) { + // skip sending notification since python is exiting + return; + } + // call `call_soon_threadsafe` on some event loop object + PyGILState_ReleaseGILAndFinalizeBlock(state); High-level API -------------- @@ -1082,11 +1158,14 @@ code, or when embedding the Python interpreter: ensues. .. note:: - Calling this function from a thread when the runtime is finalizing - will terminate the thread, even if the thread was not created by Python. - You can use :c:func:`_Py_IsFinalizing` or :func:`sys.is_finalizing` to - check if the interpreter is in process of being finalized before calling - this function to avoid unwanted termination. + Calling this function from a thread when the runtime is finalizing will + hang the thread until the program exits, even if the thread was not + created by Python. Refer to + :ref:`cautions-regarding-runtime-finalization` for more details. + + .. versionchanged:: 3.12 + Hangs the current thread, rather than terminating it, if called while the + interpreter is finalizing. .. c:function:: PyThreadState* PyThreadState_Get() @@ -1128,11 +1207,14 @@ with sub-interpreters: to call arbitrary Python code. Failure is a fatal error. .. note:: - Calling this function from a thread when the runtime is finalizing - will terminate the thread, even if the thread was not created by Python. - You can use :c:func:`_Py_IsFinalizing` or :func:`sys.is_finalizing` to - check if the interpreter is in process of being finalized before calling - this function to avoid unwanted termination. + Calling this function from a thread when the runtime is finalizing will + hang the thread until the program exits, even if the thread was not + created by Python. Refer to + :ref:`cautions-regarding-runtime-finalization` for more details. + + .. versionchanged:: 3.12 + Hangs the current thread, rather than terminating it, if called while the + interpreter is finalizing. .. c:function:: void PyGILState_Release(PyGILState_STATE) @@ -1144,6 +1226,36 @@ with sub-interpreters: Every call to :c:func:`PyGILState_Ensure` must be matched by a call to :c:func:`PyGILState_Release` on the same thread. +.. c:function:: PyGILState_TRY_STATE PyGILState_AcquireFinalizeBlockAndGIL() + + Attempts to acquire a :ref:`finalize + block`, and if successful, acquires + the :term:`GIL`. + + This is a simple convenience interface that saves having to call + :c:func:`PyThread_TryAcquireFinalizeBlock` and :c:func:`PyGILState_Ensure` + separately. + + Returns ``PyGILState_TRY_LOCK_FAILED`` (equal to 0) if the interpreter is + already waiting to finalize. In this case, the :term:`GIL` is not acquired + and Python C APIs that require the :term:`GIL` must not be called. + + Otherwise, acquires a finalize block and then acquires the :term:`GIL`. + + Each call that is successful (i.e. returns a non-zero + ``PyGILState_TRY_STATE`` value) must be paired with a subsequent call to + :c:func:`PyGILState_ReleaseGILAndFinalizeBlock` with the same value returned + by this function. Calling :c:func:`PyGILState_ReleaseGILAndFinalizeBlock` with the + error value ``PyGILState_TRY_LOCK_FAILED`` is safe and does nothing. + + .. versionadded:: 3.12 + +.. c:function:: void PyGILState_ReleaseGILAndFinalizeBlock(PyGILState_TRY_STATE) + + Releases any locks acquired by the corresponding call to + :c:func:`PyGILState_AcquireFinalizeBlockAndGIL`. + + .. versionadded:: 3.12 .. c:function:: PyThreadState* PyGILState_GetThisThreadState() @@ -1410,17 +1522,20 @@ All of the following functions must be called after :c:func:`Py_Initialize`. If this thread already has the lock, deadlock ensues. .. note:: - Calling this function from a thread when the runtime is finalizing - will terminate the thread, even if the thread was not created by Python. - You can use :c:func:`_Py_IsFinalizing` or :func:`sys.is_finalizing` to - check if the interpreter is in process of being finalized before calling - this function to avoid unwanted termination. + Calling this function from a thread when the runtime is finalizing will + hang the thread until the program exits, even if the thread was not + created by Python. Refer to + :ref:`cautions-regarding-runtime-finalization` for more details. .. versionchanged:: 3.8 Updated to be consistent with :c:func:`PyEval_RestoreThread`, :c:func:`Py_END_ALLOW_THREADS`, and :c:func:`PyGILState_Ensure`, and terminate the current thread if called while the interpreter is finalizing. + .. versionchanged:: 3.12 + Hangs the current thread, rather than terminating it, if called while the + interpreter is finalizing. + :c:func:`PyEval_RestoreThread` is a higher-level function which is always available (even when threads have not been initialized). @@ -1448,17 +1563,19 @@ All of the following functions must be called after :c:func:`Py_Initialize`. instead. .. note:: - Calling this function from a thread when the runtime is finalizing - will terminate the thread, even if the thread was not created by Python. - You can use :c:func:`_Py_IsFinalizing` or :func:`sys.is_finalizing` to - check if the interpreter is in process of being finalized before calling - this function to avoid unwanted termination. + Calling this function from a thread when the runtime is finalizing will + hang the thread until the program exits, even if the thread was not + created by Python. Refer to + :ref:`cautions-regarding-runtime-finalization` for more details. .. versionchanged:: 3.8 Updated to be consistent with :c:func:`PyEval_RestoreThread`, :c:func:`Py_END_ALLOW_THREADS`, and :c:func:`PyGILState_Ensure`, and terminate the current thread if called while the interpreter is finalizing. + .. versionchanged:: 3.12 + Hangs the current thread, rather than terminating it, if called while the + interpreter is finalizing. .. c:function:: void PyEval_ReleaseLock() @@ -1469,6 +1586,32 @@ All of the following functions must be called after :c:func:`Py_Initialize`. :c:func:`PyEval_SaveThread` or :c:func:`PyEval_ReleaseThread` instead. +.. c:function:: int PyThread_AcquireFinalizeBlock() + + Attempts to acquire a block on Python finalization. + + While the *finalize block* is held, the Python interpreter will block before + it begins finalization. Holding a finalize block ensures that the + :term:`GIL` can be safely acquired without the risk of hanging the thread. + Refer to :ref:`cautions-regarding-runtime-finalization` for more details. + + If successful, returns 1. If the interpreter is already finalizing, or about + to begin finalization and waiting for all previously-acquired finalize blocks + to be released, returns 0 without acquiring a finalize block. + + Every successful call must be paired with a call to + :c:func:`PyThread_ReleaseFinalizeBlock`. + + This function may be safely called with or without holding the :term:`GIL`. + + .. versionadded:: 3.12 + +.. c:function:: void PyThread_ReleaseFinalizeBlock() + + Releases a finalize block acquired by a prior successful call to + :c:func:`PyThread_AcquireFinalizeBlock` (return value of 1). + + .. versionadded:: 3.12 .. _sub-interpreter-support: @@ -1983,4 +2126,3 @@ be used in new code. .. c:function:: void* PyThread_get_key_value(int key) .. c:function:: void PyThread_delete_key_value(int key) .. c:function:: void PyThread_ReInitTLS() - diff --git a/Doc/data/stable_abi.dat b/Doc/data/stable_abi.dat index fde62eacd00a7c..9777697eb3e0cb 100644 --- a/Doc/data/stable_abi.dat +++ b/Doc/data/stable_abi.dat @@ -295,7 +295,9 @@ function,PyGC_IsEnabled,3.10,, function,PyGILState_Ensure,3.2,, function,PyGILState_GetThisThreadState,3.2,, function,PyGILState_Release,3.2,, +function,PyGILState_ReleaseGILAndFinalizeBlock,3.12,, type,PyGILState_STATE,3.2,, +function,PyGILState_TryAcquireFinalizeBlockAndGIL,3.12,, type,PyGetSetDef,3.2,,full-abi var,PyGetSetDescr_Type,3.2,, function,PyImport_AddModule,3.2,, @@ -616,6 +618,8 @@ function,PyThreadState_SetAsyncExc,3.2,, function,PyThreadState_Swap,3.2,, function,PyThread_GetInfo,3.3,, function,PyThread_ReInitTLS,3.2,, +function,PyThread_ReleaseFinalizeBlock,3.12,, +function,PyThread_TryAcquireFinalizeBlock,3.12,, function,PyThread_acquire_lock,3.2,, function,PyThread_acquire_lock_timed,3.2,, function,PyThread_allocate_lock,3.2,, diff --git a/Doc/tools/extensions/pyspecific.py b/Doc/tools/extensions/pyspecific.py index da15abdf637260..cd56fcc6b0504a 100644 --- a/Doc/tools/extensions/pyspecific.py +++ b/Doc/tools/extensions/pyspecific.py @@ -73,9 +73,11 @@ def issue_role(typ, rawtext, text, lineno, inliner, options={}, content=[]): def gh_issue_role(typ, rawtext, text, lineno, inliner, options={}, content=[]): issue = utils.unescape(text) - # sanity check: all GitHub issues have ID >= 32426 + # sanity check: new GitHub issues have ID >= 28525 # even though some of them are also valid BPO IDs - if int(issue) < 32426: + # Note: there are actually some older open pull requests; this check may + # need to be adjusted. + if int(issue) < 28525: msg = inliner.reporter.error(f'The GitHub ID {text!r} seems too low -- ' 'use :issue:`...` for BPO IDs', line=lineno) prb = inliner.problematic(rawtext, rawtext, msg) diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 2c04ead45869fc..e149b5d4b7def9 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -85,6 +85,19 @@ typedef struct pyruntimestate { to access it, don't access it directly. */ _Py_atomic_address _finalizing; + /* Tracks the finalize blocks. + + Bit 0 is set to 1 by `Py_FinalizeEx` to indicate it is waiting to set `_finalizing`. + + The remaining bits are a count of the number of finalize blocks that are + currently held. Once bit 0 is set to 1, the number of finalize blocks is + not allowed to increase. + + Protected by `ceval.gil.mutex`, and `ceval.gil.cond` must be broadcast + when it becomes 1. + */ + uintptr_t finalize_blocks; + struct pyinterpreters { PyThread_type_lock mutex; /* The linked list of interpreters, newest first. */ diff --git a/Include/pystate.h b/Include/pystate.h index e6b4de979c87b8..96a9efa470812c 100644 --- a/Include/pystate.h +++ b/Include/pystate.h @@ -119,6 +119,53 @@ PyAPI_FUNC(void) PyGILState_Release(PyGILState_STATE); */ PyAPI_FUNC(PyThreadState *) PyGILState_GetThisThreadState(void); +/* Attempts to acquire a block on interpreter finalization. + + Returns 1 on success, or 0 if the interpreter is already waiting to finalize. + + While the lock is held, the interpreter will not enter the finalization + state. + + Each call that returns 1 must be paired with a subsequent call to + `PyThread_ReleaseFinalizeBlock`. + + It is not necessary to hold the GIL. While holding a block on interpreter + finalization, a non-main thread can safely acquire the GIL without risking + becoming permanently blocked. + */ +PyAPI_FUNC(int) PyThread_TryAcquireFinalizeBlock(void); + +/* Releases the block acquired by a successful call to + `PyThread_TryAcquireFinalizeBlock`. */ +PyAPI_FUNC(void) PyThread_ReleaseFinalizeBlock(void); + +typedef enum { + PyGILState_TRY_LOCK_FAILED, + PyGILState_TRY_LOCK_LOCKED, + PyGILState_TRY_LOCK_UNLOCKED +} PyGILState_TRY_STATE; + +/* Attempts to acquire a finalize block, and if successful, acquires the GIL. + + This is a simple convenience interface that saves having to call + `PyThread_TryAcquireFinalizeBlock()` and `PyGILState_Ensure()` separately. + + Returns `PyGILState_TRY_LOCK_FAILED` (equal to 0) if the interpreter is + already waiting to finalize. In this case, the GIL is not acquired and + Python C APIs that require the GIL must not be called. + + Otherwise, acquires a finalize block and then acquires the GIL. + + Each call that is successful (i.e. returns a non-zero `PyGILState_TRY_STATE` + value) must be paired with a subsequent call to + `PyGILState_ReleaseGILAndFinalizeBlock` with the same value returned by this + function. Calling `PyGILState_ReleaseGILAndFinalizeBlock` with the error + value `PyGILState_TRY_LOCK_FAILED` is safe and does nothing. */ +PyAPI_FUNC(PyGILState_TRY_STATE) PyGILState_TryAcquireFinalizeBlockAndGIL(void); + +/* Releases any locks acquired by the corresponding call to + `PyGILState_TryAcquireFinalizeBlockAndGIL`. */ +PyAPI_FUNC(void) PyGILState_ReleaseGILAndFinalizeBlock(PyGILState_TRY_STATE); #ifndef Py_LIMITED_API # define Py_CPYTHON_PYSTATE_H diff --git a/Include/pythread.h b/Include/pythread.h index 63714437c496b7..2374fe47ec3f0b 100644 --- a/Include/pythread.h +++ b/Include/pythread.h @@ -17,7 +17,37 @@ typedef enum PyLockStatus { PyAPI_FUNC(void) PyThread_init_thread(void); PyAPI_FUNC(unsigned long) PyThread_start_new_thread(void (*)(void *), void *); -PyAPI_FUNC(void) _Py_NO_RETURN PyThread_exit_thread(void); +/* Terminates the current thread. + * + * WARNING: This function is only safe to call if all functions in the full call + * stack are written to safely allow it. Additionally, the behavior is + * platform-dependent. This function should be avoided, and is no longer called + * by Python itself. It is retained only for compatibility with existing C + * extension code. + * + * With pthreads, calls `pthread_exit` which attempts to unwind the stack and + * call C++ destructors. If a `noexcept` function is reached, the program is + * terminated. + * + * On Windows, calls `_endthreadex` which kills the thread without calling C++ + * destructors. + * + * In either case there is a risk of invalid references remaining to data on the + * thread stack. + */ +Py_DEPRECATED(3.12) PyAPI_FUNC(void) _Py_NO_RETURN PyThread_exit_thread(void); + +#ifndef Py_LIMITED_API +/* Hangs the thread indefinitely without exiting it. + * + * bpo-42969: There is no safe way to exit a thread other than returning + * normally from its start function. This is used during finalization in lieu + * of actually exiting the thread. Since the program is expected to terminate + * soon anyway, it does not matter if the thread stack stays around until then. + */ +PyAPI_FUNC(void) _Py_NO_RETURN _PyThread_hang_thread(void); +#endif /* !Py_LIMITED_API */ + PyAPI_FUNC(unsigned long) PyThread_get_thread_ident(void); #if (defined(__APPLE__) || defined(__linux__) || defined(_WIN32) \ diff --git a/Lib/test/test_stable_abi_ctypes.py b/Lib/test/test_stable_abi_ctypes.py index a803e3a5025985..733b838a4f5b17 100644 --- a/Lib/test/test_stable_abi_ctypes.py +++ b/Lib/test/test_stable_abi_ctypes.py @@ -321,6 +321,8 @@ def test_windows_feature_macros(self): "PyGILState_Ensure", "PyGILState_GetThisThreadState", "PyGILState_Release", + "PyGILState_ReleaseGILAndFinalizeBlock", + "PyGILState_TryAcquireFinalizeBlockAndGIL", "PyGetSetDescr_Type", "PyImport_AddModule", "PyImport_AddModuleObject", @@ -625,6 +627,8 @@ def test_windows_feature_macros(self): "PyThreadState_Swap", "PyThread_GetInfo", "PyThread_ReInitTLS", + "PyThread_ReleaseFinalizeBlock", + "PyThread_TryAcquireFinalizeBlock", "PyThread_acquire_lock", "PyThread_acquire_lock_timed", "PyThread_allocate_lock", diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index dcd27697bb4839..1cb3dc10f5a944 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -976,6 +976,125 @@ def import_threading(): self.assertEqual(out, b'') self.assertEqual(err, b'') + @cpython_only + def test_finalize_daemon_thread_hang(self): + # bpo-42969: tests that daemon threads hang during finalization + script = textwrap.dedent(''' + import os + import sys + import threading + import time + import _testcapi + + lock = threading.Lock() + lock.acquire() + def thread_func(): + try: + _testcapi.finalize_thread_hang(lock.acquire) + finally: + # Control must not reach here. + os._exit(2) + + t = threading.Thread(target=thread_func) + t.daemon = True + t.start() + # Sleep to ensure daemon thread is blocked on `lock.acquire` + # + # Note: This test is designed so that in the unlikely case that + # `0.1` seconds is not sufficient time for the thread to become + # blocked on `lock.acquire`, the test will still pass, it just won't + # be properly testing the thread behavior during finalization. + time.sleep(0.1) + + def run_during_finalization(): + # Wake up daemon thread + lock.release() + # Sleep to give the daemon thread time to crash if it is going to. + time.sleep(0.1) + # If control reaches here, the test succeeded. + os._exit(0) + + # Replace sys.stderr.flush as a way to run code during finalization + orig_flush = sys.stderr.flush + def do_flush(*args, **kwargs): + orig_flush(*args, **kwargs) + if not sys.is_finalizing: + return + sys.stderr.flush = orig_flush + run_during_finalization() + + sys.stderr.flush = do_flush + + # If the follow exit code is retained, `run_during_finalization` + # did not run. + sys.exit(1) + ''') + assert_python_ok("-c", script) + + @cpython_only + def test_finalize_block(self): + # bpo-42969: tests `PyThread_{Acquire,Release}FinalizeBlock` + script = textwrap.dedent(''' + import os + import sys + import threading + import time + import _testcapi + + sem = threading.Semaphore(0) + def thread_func(): + # Should not return False + if not _testcapi.acquire_finalize_block(): + os._exit(5) + try: + sem.release() + # Sleep to allow Python interpreter to begin exiting. + time.sleep(0.1) + sys.stdout.write('A') + sys.stdout.flush() + finally: + _testcapi.release_finalize_block() + + time.sleep(0.1) + # Thread should hang before control reaches here. + os._exit(1) + + + t = threading.Thread(target=thread_func) + t.daemon = True + t.start() + + # Wait until thread has blocked finalization. + sem.acquire() + + def run_during_finalization(): + # Should return False since this is during finalization. + if _testcapi.acquire_finalize_block(): + os._exit(4) + sys.stdout.write('B') + sys.stdout.flush() + # Sleep to ensure the daemon thread doesn't crash + time.sleep(0.2) + # If control reaches here, the test succeeded. + os._exit(0) + + # Replace sys.stderr.flush as a way to run code during finalization + orig_flush = sys.stderr.flush + def do_flush(*args, **kwargs): + orig_flush(*args, **kwargs) + if not sys.is_finalizing: + return + sys.stderr.flush = orig_flush + run_during_finalization() + + sys.stderr.flush = do_flush + + # If the follow exit code is retained, `run_during_finalization` + # did not run. + sys.exit(2) + ''') + _, out, _ = assert_python_ok("-c", script) + assert out == b"AB" class ThreadJoinOnShutdown(BaseTestCase): diff --git a/Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-28525.SCNBYj.rst b/Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-28525.SCNBYj.rst new file mode 100644 index 00000000000000..6803ca925878bd --- /dev/null +++ b/Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-28525.SCNBYj.rst @@ -0,0 +1,17 @@ +Attempting to acquire the GIL after runtime finalization has begun in a +different thread now causes the thread to hang rather than terminate, which +avoids potential crashes or memory corruption caused by attempting to +terminate a thread that is running code not specifically designed to support +termination. In most cases this hanging is harmless since the process will +soon exit anyway, but in cases where a thread must avoid hanging, the new +API functions :c:func:`PyThread_TryAcquireFinalizeBlock` or +:c:func:`PyGILState_AcquireFinalizeBlockAndGIL` may be used. + +The ``PyThread_exit_thread`` function is now deprecated. Its behavior is +inconsistent across platforms, and it can only be used safely in the +unlikely case that every function in the entire call stack has been designed +to support the platform-dependent termination mechanism. It is recommended +that users of this function change their design to not require thread +termination. In the unlikely case that thread termination is needed and can +be done safely, users may migrate to calling platform-specific APIs such as +``pthread_exit`` (POSIX) or ``_endthreadex`` (Windows) directly. diff --git a/Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst b/Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst new file mode 100644 index 00000000000000..e5e03baa6ac0df --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2021-09-22-14-19-02.bpo-42969.8Z2mth.rst @@ -0,0 +1,3 @@ +Daemon threads and other threads not created by Python are now paused rather +than unsafely terminated if they attempt to acquire the GIL during Python +finalization. diff --git a/Misc/stable_abi.toml b/Misc/stable_abi.toml index 4da002a0586299..1012c84c6f9817 100644 --- a/Misc/stable_abi.toml +++ b/Misc/stable_abi.toml @@ -2286,3 +2286,11 @@ added = '3.12' [typedef.vectorcallfunc] added = '3.12' +[function.PyGILState_ReleaseGILAndFinalizeBlock] + added = '3.12' +[function.PyGILState_TryAcquireFinalizeBlockAndGIL] + added = '3.12' +[function.PyThread_ReleaseFinalizeBlock] + added = '3.12' +[function.PyThread_TryAcquireFinalizeBlock] + added = '3.12' diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 2d4c73cfe97097..9ab4f23b781358 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -5458,6 +5458,48 @@ test_macros(PyObject *self, PyObject *Py_UNUSED(args)) Py_RETURN_NONE; } +// Used by `finalize_thread_hang`. +#ifdef _POSIX_THREADS +static void finalize_thread_hang_cleanup_callback(void *Py_UNUSED(arg)) { + // Should not reach here. + assert(0 && "pthread thread termination was triggered unexpectedly"); +} +#endif + +// Tests that finalization does not trigger pthread cleanup. +// +// Must be called with a single nullary callable function that should block +// (with GIL released) until finalization is in progress. +static PyObject * +finalize_thread_hang(PyObject *self, PyObject *arg) +{ +#ifdef _POSIX_THREADS + pthread_cleanup_push(finalize_thread_hang_cleanup_callback, NULL); +#endif + PyObject_CallNoArgs(arg); + // Should not reach here. + assert(0 && "thread unexpectedly did not hang"); +#ifdef _POSIX_THREADS + pthread_cleanup_pop(0); +#endif + Py_INCREF(Py_None); + return Py_None; +} + +static PyObject * +acquire_finalize_block(PyObject *self, PyObject *Py_UNUSED(args)) { + PyObject *result; + result = PyThread_TryAcquireFinalizeBlock() ? Py_True : Py_False; + Py_INCREF(result); + return result; +} + +static PyObject * +release_finalize_block(PyObject *self, PyObject *Py_UNUSED(args)) { + PyThread_ReleaseFinalizeBlock(); + Py_INCREF(Py_None); + return Py_None; +} static PyObject *test_buildvalue_issue38913(PyObject *, PyObject *); static PyObject *getargs_s_hash_int(PyObject *, PyObject *, PyObject*); @@ -5734,6 +5776,9 @@ static PyMethodDef TestMethods[] = { {"settrace_to_record", settrace_to_record, METH_O, NULL}, {"test_macros", test_macros, METH_NOARGS, NULL}, {"clear_managed_dict", clear_managed_dict, METH_O, NULL}, + {"finalize_thread_hang", finalize_thread_hang, METH_O, NULL}, + {"acquire_finalize_block", acquire_finalize_block, METH_NOARGS, NULL}, + {"release_finalize_block", release_finalize_block, METH_NOARGS, NULL}, {NULL, NULL} /* sentinel */ }; diff --git a/PC/python3dll.c b/PC/python3dll.c index 89bbd05932b853..4855877c615130 100755 --- a/PC/python3dll.c +++ b/PC/python3dll.c @@ -282,6 +282,8 @@ EXPORT_FUNC(PyGC_IsEnabled) EXPORT_FUNC(PyGILState_Ensure) EXPORT_FUNC(PyGILState_GetThisThreadState) EXPORT_FUNC(PyGILState_Release) +EXPORT_FUNC(PyGILState_ReleaseGILAndFinalizeBlock) +EXPORT_FUNC(PyGILState_TryAcquireFinalizeBlockAndGIL) EXPORT_FUNC(PyImport_AddModule) EXPORT_FUNC(PyImport_AddModuleObject) EXPORT_FUNC(PyImport_AppendInittab) @@ -569,9 +571,11 @@ EXPORT_FUNC(PyThread_GetInfo) EXPORT_FUNC(PyThread_init_thread) EXPORT_FUNC(PyThread_ReInitTLS) EXPORT_FUNC(PyThread_release_lock) +EXPORT_FUNC(PyThread_ReleaseFinalizeBlock) EXPORT_FUNC(PyThread_set_key_value) EXPORT_FUNC(PyThread_set_stacksize) EXPORT_FUNC(PyThread_start_new_thread) +EXPORT_FUNC(PyThread_TryAcquireFinalizeBlock) EXPORT_FUNC(PyThread_tss_alloc) EXPORT_FUNC(PyThread_tss_create) EXPORT_FUNC(PyThread_tss_delete) diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index a6790866766795..5ec4c525f7d2ba 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -179,43 +179,6 @@ is_tstate_valid(PyThreadState *tstate) #include "condvar.h" -#define MUTEX_INIT(mut) \ - if (PyMUTEX_INIT(&(mut))) { \ - Py_FatalError("PyMUTEX_INIT(" #mut ") failed"); }; -#define MUTEX_FINI(mut) \ - if (PyMUTEX_FINI(&(mut))) { \ - Py_FatalError("PyMUTEX_FINI(" #mut ") failed"); }; -#define MUTEX_LOCK(mut) \ - if (PyMUTEX_LOCK(&(mut))) { \ - Py_FatalError("PyMUTEX_LOCK(" #mut ") failed"); }; -#define MUTEX_UNLOCK(mut) \ - if (PyMUTEX_UNLOCK(&(mut))) { \ - Py_FatalError("PyMUTEX_UNLOCK(" #mut ") failed"); }; - -#define COND_INIT(cond) \ - if (PyCOND_INIT(&(cond))) { \ - Py_FatalError("PyCOND_INIT(" #cond ") failed"); }; -#define COND_FINI(cond) \ - if (PyCOND_FINI(&(cond))) { \ - Py_FatalError("PyCOND_FINI(" #cond ") failed"); }; -#define COND_SIGNAL(cond) \ - if (PyCOND_SIGNAL(&(cond))) { \ - Py_FatalError("PyCOND_SIGNAL(" #cond ") failed"); }; -#define COND_WAIT(cond, mut) \ - if (PyCOND_WAIT(&(cond), &(mut))) { \ - Py_FatalError("PyCOND_WAIT(" #cond ") failed"); }; -#define COND_TIMED_WAIT(cond, mut, microseconds, timeout_result) \ - { \ - int r = PyCOND_TIMEDWAIT(&(cond), &(mut), (microseconds)); \ - if (r < 0) \ - Py_FatalError("PyCOND_WAIT(" #cond ") failed"); \ - if (r) /* 1 == timeout, 2 == impl. can't say, so assume timeout */ \ - timeout_result = 1; \ - else \ - timeout_result = 0; \ - } \ - - #define DEFAULT_INTERVAL 5000 static void _gil_initialize(struct _gil_runtime_state *gil) @@ -346,12 +309,12 @@ take_gil(PyThreadState *tstate) if (tstate_must_exit(tstate)) { /* bpo-39877: If Py_Finalize() has been called and tstate is not the - thread which called Py_Finalize(), exit immediately the thread. + thread which called Py_Finalize(), bpo-42969: hang the thread. This code path can be reached by a daemon thread after Py_Finalize() completes. In this case, tstate is a dangling pointer: points to PyThreadState freed memory. */ - PyThread_exit_thread(); + _PyThread_hang_thread(); } assert(is_tstate_valid(tstate)); @@ -384,7 +347,8 @@ take_gil(PyThreadState *tstate) { if (tstate_must_exit(tstate)) { MUTEX_UNLOCK(gil->mutex); - PyThread_exit_thread(); + /* bpo-42969: hang the thread */ + _PyThread_hang_thread(); } assert(is_tstate_valid(tstate)); @@ -414,7 +378,7 @@ take_gil(PyThreadState *tstate) if (tstate_must_exit(tstate)) { /* bpo-36475: If Py_Finalize() has been called and tstate is not - the thread which called Py_Finalize(), exit immediately the + the thread which called Py_Finalize(), bpo-42969: hang the thread. This code path can be reached by a daemon thread which was waiting @@ -422,7 +386,7 @@ take_gil(PyThreadState *tstate) wait_for_thread_shutdown() from Py_Finalize(). */ MUTEX_UNLOCK(gil->mutex); drop_gil(ceval, ceval2, tstate); - PyThread_exit_thread(); + _PyThread_hang_thread(); } assert(is_tstate_valid(tstate)); diff --git a/Python/condvar.h b/Python/condvar.h index 4ddc5311cf8fad..2b0f819a48bfe7 100644 --- a/Python/condvar.h +++ b/Python/condvar.h @@ -306,4 +306,44 @@ PyCOND_BROADCAST(PyCOND_T *cv) #endif /* _POSIX_THREADS, NT_THREADS */ +/* Convenience interfaces that terminate the program in case of an error. */ +#define MUTEX_INIT(mut) \ + if (PyMUTEX_INIT(&(mut))) { \ + Py_FatalError("PyMUTEX_INIT(" #mut ") failed"); }; +#define MUTEX_FINI(mut) \ + if (PyMUTEX_FINI(&(mut))) { \ + Py_FatalError("PyMUTEX_FINI(" #mut ") failed"); }; +#define MUTEX_LOCK(mut) \ + if (PyMUTEX_LOCK(&(mut))) { \ + Py_FatalError("PyMUTEX_LOCK(" #mut ") failed"); }; +#define MUTEX_UNLOCK(mut) \ + if (PyMUTEX_UNLOCK(&(mut))) { \ + Py_FatalError("PyMUTEX_UNLOCK(" #mut ") failed"); }; + +#define COND_INIT(cond) \ + if (PyCOND_INIT(&(cond))) { \ + Py_FatalError("PyCOND_INIT(" #cond ") failed"); }; +#define COND_FINI(cond) \ + if (PyCOND_FINI(&(cond))) { \ + Py_FatalError("PyCOND_FINI(" #cond ") failed"); }; +#define COND_SIGNAL(cond) \ + if (PyCOND_SIGNAL(&(cond))) { \ + Py_FatalError("PyCOND_SIGNAL(" #cond ") failed"); }; +#define COND_BROADCAST(cond) \ + if (PyCOND_BROADCAST(&(cond))) { \ + Py_FatalError("PyCOND_BROADCAST(" #cond ") failed"); }; +#define COND_WAIT(cond, mut) \ + if (PyCOND_WAIT(&(cond), &(mut))) { \ + Py_FatalError("PyCOND_WAIT(" #cond ") failed"); }; +#define COND_TIMED_WAIT(cond, mut, microseconds, timeout_result) \ + { \ + int r = PyCOND_TIMEDWAIT(&(cond), &(mut), (microseconds)); \ + if (r < 0) \ + Py_FatalError("PyCOND_WAIT(" #cond ") failed"); \ + if (r) /* 1 == timeout, 2 == impl. can't say, so assume timeout */ \ + timeout_result = 1; \ + else \ + timeout_result = 0; \ + } \ + #endif /* _CONDVAR_IMPL_H_ */ diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index bb646f1a0ee2d0..a3852852ecb502 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2,6 +2,7 @@ #include "Python.h" +#include "condvar.h" #include "pycore_bytesobject.h" // _PyBytes_InitTypes() #include "pycore_ceval.h" // _PyEval_FiniGIL() #include "pycore_context.h" // _PyContext_Init() @@ -1746,6 +1747,38 @@ finalize_interp_delete(PyInterpreterState *interp) PyInterpreterState_Delete(interp); } +/* Prevents new exit blocks from being acquired and waits for existing blocks to + * be released. + * + * This is only to be called from `Py_FinalizeEx`. + */ +static void wait_for_exit_blocks_to_be_released() +{ + _PyRuntimeState *runtime = &_PyRuntime; + + /* We release the GIL to avoid deadlock. */ + Py_BEGIN_ALLOW_THREADS + + MUTEX_LOCK(runtime->ceval.gil.mutex) + runtime->finalize_blocks |= 1; + /* Note: It is possible that there is another concurrent call to + `Py_FinalizeEx` in a different thread. In that case, the LSB of + `runtime->finalize_blocks` may have already been set to 1. Finalization + will still work correctly, though, because only one thread will + successfully acquire the GIL from `Py_END_ALLOW_THREADS` below. That + thread will then initiate finalization, and the other thread will then + hang in `Py_END_ALLOW_THREADS` until the process exits. + + Calling `Py_FinalizeEx` recursively, e.g. by an atexit handler, is *not* + allowed and will not work correctly. + */ + while(runtime->finalize_blocks != 1) { + COND_WAIT(runtime->ceval.gil.cond, runtime->ceval.gil.mutex) + } + MUTEX_UNLOCK(runtime->ceval.gil.mutex) + + Py_END_ALLOW_THREADS +} int Py_FinalizeEx(void) @@ -1778,6 +1811,8 @@ Py_FinalizeEx(void) _PyAtExit_Call(tstate->interp); + wait_for_exit_blocks_to_be_released(); + /* Copy the core config, PyInterpreterState_Delete() free the core config memory */ #ifdef Py_REF_DEBUG diff --git a/Python/pystate.c b/Python/pystate.c index a11f1622ecd4ab..e1d73abf49aceb 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2,6 +2,7 @@ /* Thread and interpreter state structures and their interfaces */ #include "Python.h" +#include "condvar.h" #include "pycore_ceval.h" #include "pycore_code.h" // stats #include "pycore_frame.h" @@ -1753,6 +1754,54 @@ PyGILState_Release(PyGILState_STATE oldstate) PyEval_SaveThread(); } +int +PyThread_TryAcquireFinalizeBlock(void) +{ + int ret; + _PyRuntimeState *runtime = &_PyRuntime; + MUTEX_LOCK(runtime->ceval.gil.mutex) + if (runtime->finalize_blocks & 1) { + ret = 0; + } else { + ret = 1; + runtime->finalize_blocks += 2; + } + MUTEX_UNLOCK(runtime->ceval.gil.mutex) + return ret; +} + +void +PyThread_ReleaseFinalizeBlock(void) +{ + _PyRuntimeState *runtime = &_PyRuntime; + MUTEX_LOCK(runtime->ceval.gil.mutex) + assert(runtime->finalize_blocks >= 2); + if ((runtime->finalize_blocks -= 2) == 0) { + COND_BROADCAST(runtime->ceval.gil.cond) + } + MUTEX_UNLOCK(runtime->ceval.gil.mutex) +} + +PyGILState_TRY_STATE +PyGILState_TryAcquireFinalizeBlockAndGIL(void) +{ + if (!PyThread_TryAcquireFinalizeBlock()) { + return PyGILState_TRY_LOCK_FAILED; + } + return PyGILState_Ensure() == PyGILState_LOCKED + ? PyGILState_TRY_LOCK_LOCKED + : PyGILState_TRY_LOCK_UNLOCKED; +} + +void +PyGILState_ReleaseGILAndFinalizeBlock(PyGILState_TRY_STATE state) { + if (state == PyGILState_TRY_LOCK_FAILED) { + return; + } + PyGILState_Release(state == PyGILState_TRY_LOCK_LOCKED + ? PyGILState_LOCKED + : PyGILState_UNLOCKED); +} /**************************/ /* cross-interpreter data */ diff --git a/Python/thread_nt.h b/Python/thread_nt.h index d1f1323948a6c6..f940bc3e055160 100644 --- a/Python/thread_nt.h +++ b/Python/thread_nt.h @@ -256,6 +256,14 @@ PyThread_exit_thread(void) _endthreadex(0); } +void _Py_NO_RETURN +_PyThread_hang_thread(void) +{ + while (1) { + SleepEx(INFINITE, TRUE); + } +} + /* * Lock support. It has to be implemented as semaphores. * I [Dag] tried to implement it with mutex but I could find a way to diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 1c5b320813af83..d143d447e9f46e 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -356,6 +356,16 @@ PyThread_exit_thread(void) pthread_exit(0); } +void _Py_NO_RETURN +_PyThread_hang_thread(void) +{ + while (1) { /* loop just in case sigsuspend returns for some reason */ + sigset_t set; /* we don't want to receive any signal */ + sigfillset(&set); + sigsuspend(&set); + } +} + #ifdef USE_SEMAPHORES /* From 703d7990456c6884c8f86971f9402e3b26ef5232 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 20 Sep 2022 16:26:01 -0700 Subject: [PATCH 2/8] Correct news blurb filename, revert Doc/tools edit --- Doc/tools/extensions/pyspecific.py | 6 ++---- ...Yj.rst => 2022-08-05-19-41-20.gh-issue-87135.SCNBYj.rst} | 0 2 files changed, 2 insertions(+), 4 deletions(-) rename Misc/NEWS.d/next/C API/{2022-08-05-19-41-20.gh-issue-28525.SCNBYj.rst => 2022-08-05-19-41-20.gh-issue-87135.SCNBYj.rst} (100%) diff --git a/Doc/tools/extensions/pyspecific.py b/Doc/tools/extensions/pyspecific.py index cd56fcc6b0504a..da15abdf637260 100644 --- a/Doc/tools/extensions/pyspecific.py +++ b/Doc/tools/extensions/pyspecific.py @@ -73,11 +73,9 @@ def issue_role(typ, rawtext, text, lineno, inliner, options={}, content=[]): def gh_issue_role(typ, rawtext, text, lineno, inliner, options={}, content=[]): issue = utils.unescape(text) - # sanity check: new GitHub issues have ID >= 28525 + # sanity check: all GitHub issues have ID >= 32426 # even though some of them are also valid BPO IDs - # Note: there are actually some older open pull requests; this check may - # need to be adjusted. - if int(issue) < 28525: + if int(issue) < 32426: msg = inliner.reporter.error(f'The GitHub ID {text!r} seems too low -- ' 'use :issue:`...` for BPO IDs', line=lineno) prb = inliner.problematic(rawtext, rawtext, msg) diff --git a/Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-28525.SCNBYj.rst b/Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-87135.SCNBYj.rst similarity index 100% rename from Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-28525.SCNBYj.rst rename to Misc/NEWS.d/next/C API/2022-08-05-19-41-20.gh-issue-87135.SCNBYj.rst From 40222ec321bb2a31cb1681e3711711b93856feaf Mon Sep 17 00:00:00 2001 From: Jeremy Maitin-Shepard Date: Wed, 21 Sep 2022 09:14:26 -0700 Subject: [PATCH 3/8] Apply suggestions from code review Co-authored-by: Victor Stinner --- Modules/_testcapimodule.c | 6 ++---- Python/pylifecycle.c | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index dfac9803468eeb..1d56ad47f36a88 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -5492,15 +5492,13 @@ static PyObject * acquire_finalize_block(PyObject *self, PyObject *Py_UNUSED(args)) { PyObject *result; result = PyThread_TryAcquireFinalizeBlock() ? Py_True : Py_False; - Py_INCREF(result); - return result; + return Py_NewRef(result); } static PyObject * release_finalize_block(PyObject *self, PyObject *Py_UNUSED(args)) { PyThread_ReleaseFinalizeBlock(); - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject *test_buildvalue_issue38913(PyObject *, PyObject *); diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 9bd8f945f41fe3..7990c8d4a8581e 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1758,7 +1758,7 @@ finalize_interp_delete(PyInterpreterState *interp) * * This is only to be called from `Py_FinalizeEx`. */ -static void wait_for_exit_blocks_to_be_released() +static void wait_for_exit_blocks_to_be_released(void) { _PyRuntimeState *runtime = &_PyRuntime; From ea5cc5410d3b66c56d4c1660723b524dfe644ebf Mon Sep 17 00:00:00 2001 From: Jeremy Maitin-Shepard Date: Wed, 21 Sep 2022 09:29:56 -0700 Subject: [PATCH 4/8] Revise race condition test cases to use threading.Event in addition to sleep --- Lib/test/test_threading.py | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 9c33e688d0fe5c..8ed0201a3dbfaa 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1047,8 +1047,10 @@ def test_finalize_daemon_thread_hang(self): lock = threading.Lock() lock.acquire() + thread_started_event = threading.Event() def thread_func(): try: + thread_started_event.set() _testcapi.finalize_thread_hang(lock.acquire) finally: # Control must not reach here. @@ -1057,18 +1059,25 @@ def thread_func(): t = threading.Thread(target=thread_func) t.daemon = True t.start() + thread_started_event.wait() # Sleep to ensure daemon thread is blocked on `lock.acquire` # # Note: This test is designed so that in the unlikely case that # `0.1` seconds is not sufficient time for the thread to become - # blocked on `lock.acquire`, the test will still pass, it just won't - # be properly testing the thread behavior during finalization. + # blocked on `lock.acquire`, the test will still pass, it just + # won't be properly testing the thread behavior during + # finalization. time.sleep(0.1) def run_during_finalization(): # Wake up daemon thread lock.release() - # Sleep to give the daemon thread time to crash if it is going to. + # Sleep to give the daemon thread time to crash if it is going + # to. + # + # Note: If due to an exceptionally slow execution this delay is + # insufficient, the test will still pass but will simply be + # ineffective as a test. time.sleep(0.1) # If control reaches here, the test succeeded. os._exit(0) @@ -1094,6 +1103,7 @@ def do_flush(*args, **kwargs): def test_finalize_block(self): # bpo-42969: tests `PyThread_{Acquire,Release}FinalizeBlock` script = textwrap.dedent(''' + import atexit import os import sys import threading @@ -1101,13 +1111,23 @@ def test_finalize_block(self): import _testcapi sem = threading.Semaphore(0) + atexit_handlers_called = threading.Event() + atexit.register(atexit_handlers_called.set) + def thread_func(): # Should not return False if not _testcapi.acquire_finalize_block(): os._exit(5) try: sem.release() - # Sleep to allow Python interpreter to begin exiting. + # Wait until `atexit` handlers are called. + atexit_handlers_called.wait() + # Sleep to allow Python interpreter to begin exiting, so + # that we are properly testing the finalize block. + # + # Note: If for some reason this delay is insufficient, the + # test will still pass but will simply be ineffective as a + # test. time.sleep(0.1) sys.stdout.write('A') sys.stdout.flush() @@ -1132,7 +1152,12 @@ def run_during_finalization(): os._exit(4) sys.stdout.write('B') sys.stdout.flush() - # Sleep to ensure the daemon thread doesn't crash + # Sleep to give the daemon thread time to crash if it is going + # to. + # + # Note: If for some reason this delay is insufficient, the + # test will still pass but will simply be ineffective as a + # test. time.sleep(0.2) # If control reaches here, the test succeeded. os._exit(0) From 2b519956dc610c143a23eb7a5c929cbd95b2f074 Mon Sep 17 00:00:00 2001 From: Jeremy Maitin-Shepard Date: Tue, 2 May 2023 13:18:35 -0700 Subject: [PATCH 5/8] Revert back to using pause rather than sigsuspend --- Python/thread_pthread.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Python/thread_pthread.h b/Python/thread_pthread.h index e8a8742d95b2fb..bcf953f34e8748 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -362,10 +362,8 @@ PyThread_exit_thread(void) void _Py_NO_RETURN _PyThread_hang_thread(void) { - while (1) { /* loop just in case sigsuspend returns for some reason */ - sigset_t set; /* we don't want to receive any signal */ - sigfillset(&set); - sigsuspend(&set); + while (1) { + pause(); } } From 8022ebe841bf5c82206b94ad6ce8ec2153fd2170 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 25 May 2023 14:42:55 -0700 Subject: [PATCH 6/8] Py_FatalError instead of assert(0) in _testcapimodule --- Modules/_testcapimodule.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index b4f71cda08dba1..a75e6b39b80c0a 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3543,12 +3543,11 @@ finalize_thread_hang(PyObject *self, PyObject *arg) #endif PyObject_CallNoArgs(arg); // Should not reach here. - assert(0 && "thread unexpectedly did not hang"); + Py_FatalError("thread unexpectedly did not hang"); #ifdef _POSIX_THREADS pthread_cleanup_pop(0); #endif - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject * From 6f8503665734f35a577e9f8ad9f7079a3890d8d6 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Fri, 26 May 2023 00:44:43 +0000 Subject: [PATCH 7/8] Port this branch to per-interpreter-GIL. The finalized blocks is now protected by the main interpreters GIL. --- Include/internal/pycore_runtime.h | 6 +++--- Python/pylifecycle.c | 7 ++++--- Python/pystate.c | 12 +++++++----- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 949465b14fda49..da040c61a16b5f 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -92,10 +92,10 @@ typedef struct pyruntimestate { currently held. Once bit 0 is set to 1, the number of finalize blocks is not allowed to increase. - Protected by `ceval.gil.mutex`, and `ceval.gil.cond` must be broadcast - when it becomes 1. + Protected by the main interpreter's GIL `main_interp->ceval.gil->mutex`; + `main_interp->ceval.gil->cond` must be broadcast when it becomes 1. */ - uintptr_t finalize_blocks; + unsigned long finalize_blocks; struct _pymem_allocators allocators; struct _obmalloc_global_state obmalloc; diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 55f56099c727ea..f926dbdf1f3979 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1752,7 +1752,8 @@ static void wait_for_exit_blocks_to_be_released(void) /* We release the GIL to avoid deadlock. */ Py_BEGIN_ALLOW_THREADS - MUTEX_LOCK(runtime->ceval.gil.mutex) + PyInterpreterState *main_interp = PyInterpreterState_Main(); + MUTEX_LOCK(main_interp->ceval.gil->mutex) runtime->finalize_blocks |= 1; /* Note: It is possible that there is another concurrent call to `Py_FinalizeEx` in a different thread. In that case, the LSB of @@ -1766,9 +1767,9 @@ static void wait_for_exit_blocks_to_be_released(void) allowed and will not work correctly. */ while(runtime->finalize_blocks != 1) { - COND_WAIT(runtime->ceval.gil.cond, runtime->ceval.gil.mutex) + COND_WAIT(main_interp->ceval.gil->cond, main_interp->ceval.gil->mutex) } - MUTEX_UNLOCK(runtime->ceval.gil.mutex) + MUTEX_UNLOCK(main_interp->ceval.gil->mutex) Py_END_ALLOW_THREADS } diff --git a/Python/pystate.c b/Python/pystate.c index 19f6bba3a2c3d9..839a7a48d18a84 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2299,14 +2299,15 @@ PyThread_TryAcquireFinalizeBlock(void) { int ret; _PyRuntimeState *runtime = &_PyRuntime; - MUTEX_LOCK(runtime->ceval.gil.mutex) + PyInterpreterState *main_interp = PyInterpreterState_Main(); + MUTEX_LOCK(main_interp->ceval.gil->mutex) if (runtime->finalize_blocks & 1) { ret = 0; } else { ret = 1; runtime->finalize_blocks += 2; } - MUTEX_UNLOCK(runtime->ceval.gil.mutex) + MUTEX_UNLOCK(main_interp->ceval.gil->mutex) return ret; } @@ -2314,12 +2315,13 @@ void PyThread_ReleaseFinalizeBlock(void) { _PyRuntimeState *runtime = &_PyRuntime; - MUTEX_LOCK(runtime->ceval.gil.mutex) + PyInterpreterState *main_interp = PyInterpreterState_Main(); + MUTEX_LOCK(main_interp->ceval.gil->mutex) assert(runtime->finalize_blocks >= 2); if ((runtime->finalize_blocks -= 2) == 0) { - COND_BROADCAST(runtime->ceval.gil.cond) + COND_BROADCAST(main_interp->ceval.gil->cond) } - MUTEX_UNLOCK(runtime->ceval.gil.mutex) + MUTEX_UNLOCK(main_interp->ceval.gil->mutex) } PyGILState_TRY_STATE From bdebcf832d13e4e79204e67d852b790fc950f98b Mon Sep 17 00:00:00 2001 From: Jeremy Maitin-Shepard Date: Thu, 1 Jun 2023 22:06:50 -0700 Subject: [PATCH 8/8] Update Doc/c-api/init.rst Co-authored-by: Eric Snow --- Doc/c-api/init.rst | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index f267ccafdf046d..2e549c3b6f1388 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -1588,19 +1588,24 @@ All of the following functions must be called after :c:func:`Py_Initialize`. .. c:function:: int PyThread_AcquireFinalizeBlock() - Attempts to acquire a block on Python finalization. + Attempts to prevent finalizing the current interpreter. - While the *finalize block* is held, the Python interpreter will block before - it begins finalization. Holding a finalize block ensures that the - :term:`GIL` can be safely acquired without the risk of hanging the thread. - Refer to :ref:`cautions-regarding-runtime-finalization` for more details. + If the interpreter is already finalizing then this returns 0 and has no + effect. Likewise if the interpreter is about to begin finalization but + is waiting for earlier calls to ``PyThread_AcquireFinalizeBlock()`` to be + resolved. The caller should then proceed knowing that they should + not use this interpreter any more. - If successful, returns 1. If the interpreter is already finalizing, or about - to begin finalization and waiting for all previously-acquired finalize blocks - to be released, returns 0 without acquiring a finalize block. + If the interpreter is not finalizing (nor about to) then it is immediately + prevented from finalizing, though this does not otherwise affect it. + This function returns 1 for this case. Every successful call must be paired with a call to - :c:func:`PyThread_ReleaseFinalizeBlock`. + :c:func:`PyThread_ReleaseFinalizeBlock`. Until that happens, the + interpreter will be prevented from finalizing. During this period of + time, the caller is guaranteed that the :term:`GIL` can be safely + acquired without the risk of hanging the thread. + Refer to :ref:`cautions-regarding-runtime-finalization` for more details. This function may be safely called with or without holding the :term:`GIL`.