gh-109793: Allow Switching Interpreters During Finalization (gh-109794)

Essentially, we should check the thread ID rather than the thread state pointer.

diff --git a/Include/cpython/pyatomic.h b/Include/cpython/pyatomic.h
index ab182381b39f007f94cf0c37952cb02650ac87db..ce23e13bf3838c1e9a581448482fd508bf544acd 100644 (file)
--- 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 ba5764e943e676d6a95ba8f8ab2306f7ab4b2731..0912bd175fe4f71fd7342c31761b4ebdc98060ba 100644 (file)
--- 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 9fc8ae903b2ac05ea568968372d394c0ca066ed3..2e568f8aeeb152cfa443bf4cd05c3c623e6b83ef 100644 (file)
--- 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 0ddc405f221a1cc4ca8b356a8043181aa963403a..cc3a3420befa3d4a779a01087e7b1411f7e55a4a 100644 (file)
--- 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 90932c0f66f38f910985ab3dff9a234a825c72d7..9c0dac7d6c61fb44254eb672879ab2c00aaeffcd 100644 (file)
--- 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 @@ class StressTests(TestBase):
             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 (file)
index 0000000..d2dc4c8
--- /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 dcc6c112215b30ce7cdda9553777724772acd40c..570b5242600c0cfb31a8d19ce31c76ecd78ff017 100644 (file)
--- 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;
 }