diff --git a/Include/cpython/pyatomic.h b/Include/cpython/pyatomic.h index ab182381b39f00..ce23e13bf3838c 100644 --- a/Include/cpython/pyatomic.h +++ b/Include/cpython/pyatomic.h @@ -501,3 +501,20 @@ static inline void _Py_atomic_fence_release(void); #else # error "no available pyatomic implementation for this platform/compiler" #endif + + +// --- aliases --------------------------------------------------------------- + +#if SIZEOF_LONG == 8 +# define _Py_atomic_load_ulong _Py_atomic_load_uint64 +# define _Py_atomic_load_ulong_relaxed _Py_atomic_load_uint64_relaxed +# define _Py_atomic_store_ulong _Py_atomic_store_uint64 +# define _Py_atomic_store_ulong_relaxed _Py_atomic_store_uint64_relaxed +#elif SIZEOF_LONG == 4 +# define _Py_atomic_load_ulong _Py_atomic_load_uint32 +# define _Py_atomic_load_ulong_relaxed _Py_atomic_load_uint32_relaxed +# define _Py_atomic_store_ulong _Py_atomic_store_uint32 +# define _Py_atomic_store_ulong_relaxed _Py_atomic_store_uint32_relaxed +#else +# error "long must be 4 or 8 bytes in size" +#endif // SIZEOF_LONG diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index ba5764e943e676..0912bd175fe4f7 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -93,6 +93,8 @@ struct _is { and _PyInterpreterState_SetFinalizing() to access it, don't access it directly. */ _Py_atomic_address _finalizing; + /* The ID of the OS thread in which we are finalizing. */ + unsigned long _finalizing_id; struct _gc_runtime_state gc; @@ -215,9 +217,23 @@ _PyInterpreterState_GetFinalizing(PyInterpreterState *interp) { return (PyThreadState*)_Py_atomic_load_relaxed(&interp->_finalizing); } +static inline unsigned long +_PyInterpreterState_GetFinalizingID(PyInterpreterState *interp) { + return _Py_atomic_load_ulong_relaxed(&interp->_finalizing_id); +} + static inline void _PyInterpreterState_SetFinalizing(PyInterpreterState *interp, PyThreadState *tstate) { _Py_atomic_store_relaxed(&interp->_finalizing, (uintptr_t)tstate); + if (tstate == NULL) { + _Py_atomic_store_ulong_relaxed(&interp->_finalizing_id, 0); + } + else { + // XXX Re-enable this assert once gh-109860 is fixed. + //assert(tstate->thread_id == PyThread_get_thread_ident()); + _Py_atomic_store_ulong_relaxed(&interp->_finalizing_id, + tstate->thread_id); + } } diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 9fc8ae903b2ac0..2e568f8aeeb152 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -36,8 +36,12 @@ _Py_IsMainInterpreter(PyInterpreterState *interp) static inline int _Py_IsMainInterpreterFinalizing(PyInterpreterState *interp) { - return (_PyRuntimeState_GetFinalizing(interp->runtime) != NULL && - interp == &interp->runtime->_main_interpreter); + /* bpo-39877: Access _PyRuntime directly rather than using + tstate->interp->runtime to support calls from Python daemon threads. + After Py_Finalize() has been called, tstate can be a dangling pointer: + point to PyThreadState freed memory. */ + return (_PyRuntimeState_GetFinalizing(&_PyRuntime) != NULL && + interp == &_PyRuntime._main_interpreter); } diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 0ddc405f221a1c..cc3a3420befa3d 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -171,6 +171,8 @@ typedef struct pyruntimestate { Use _PyRuntimeState_GetFinalizing() and _PyRuntimeState_SetFinalizing() to access it, don't access it directly. */ _Py_atomic_address _finalizing; + /* The ID of the OS thread in which we are finalizing. */ + unsigned long _finalizing_id; struct pyinterpreters { PyThread_type_lock mutex; @@ -303,9 +305,23 @@ _PyRuntimeState_GetFinalizing(_PyRuntimeState *runtime) { return (PyThreadState*)_Py_atomic_load_relaxed(&runtime->_finalizing); } +static inline unsigned long +_PyRuntimeState_GetFinalizingID(_PyRuntimeState *runtime) { + return _Py_atomic_load_ulong_relaxed(&runtime->_finalizing_id); +} + static inline void _PyRuntimeState_SetFinalizing(_PyRuntimeState *runtime, PyThreadState *tstate) { _Py_atomic_store_relaxed(&runtime->_finalizing, (uintptr_t)tstate); + if (tstate == NULL) { + _Py_atomic_store_ulong_relaxed(&runtime->_finalizing_id, 0); + } + else { + // XXX Re-enable this assert once gh-109860 is fixed. + //assert(tstate->thread_id == PyThread_get_thread_ident()); + _Py_atomic_store_ulong_relaxed(&runtime->_finalizing_id, + tstate->thread_id); + } } #ifdef __cplusplus diff --git a/Lib/test/test_interpreters.py b/Lib/test/test_interpreters.py index 90932c0f66f38f..9c0dac7d6c61fb 100644 --- a/Lib/test/test_interpreters.py +++ b/Lib/test/test_interpreters.py @@ -1,5 +1,6 @@ import contextlib import os +import sys import threading from textwrap import dedent import unittest @@ -487,6 +488,26 @@ def task(): pass +class FinalizationTests(TestBase): + + def test_gh_109793(self): + import subprocess + argv = [sys.executable, '-c', '''if True: + import _xxsubinterpreters as _interpreters + interpid = _interpreters.create() + raise Exception + '''] + proc = subprocess.run(argv, capture_output=True, text=True) + self.assertIn('Traceback', proc.stderr) + if proc.returncode == 0 and support.verbose: + print() + print("--- cmd unexpected succeeded ---") + print(f"stdout:\n{proc.stdout}") + print(f"stderr:\n{proc.stderr}") + print("------") + self.assertEqual(proc.returncode, 1) + + class TestIsShareable(TestBase): def test_default_shareables(self): diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst b/Misc/NEWS.d/next/Core and Builtins/2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst new file mode 100644 index 00000000000000..d2dc4c830a9031 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-09-25-09-24-10.gh-issue-109793.zFQBkv.rst @@ -0,0 +1,4 @@ +The main thread no longer exits prematurely when a subinterpreter +is cleaned up during runtime finalization. The bug was a problem +particularly because, when triggered, the Python process would +always return with a 0 exitcode, even if it failed. diff --git a/Python/pystate.c b/Python/pystate.c index dcc6c112215b30..570b5242600c0c 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2964,11 +2964,26 @@ _PyThreadState_MustExit(PyThreadState *tstate) tstate->interp->runtime to support calls from Python daemon threads. After Py_Finalize() has been called, tstate can be a dangling pointer: point to PyThreadState freed memory. */ + unsigned long finalizing_id = _PyRuntimeState_GetFinalizingID(&_PyRuntime); PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(&_PyRuntime); if (finalizing == NULL) { + // XXX This isn't completely safe from daemon thraeds, + // since tstate might be a dangling pointer. finalizing = _PyInterpreterState_GetFinalizing(tstate->interp); + finalizing_id = _PyInterpreterState_GetFinalizingID(tstate->interp); } - return (finalizing != NULL && finalizing != tstate); + // XXX else check &_PyRuntime._main_interpreter._initial_thread + if (finalizing == NULL) { + return 0; + } + else if (finalizing == tstate) { + return 0; + } + else if (finalizing_id == PyThread_get_thread_ident()) { + /* gh-109793: we must have switched interpreters. */ + return 0; + } + return 1; }