Skip to content

Commit 82ae5a6

Browse files
[3.12] gh-109793: Allow Switching Interpreters During Finalization (gh-109794) (gh-110705)
Essentially, we should check the thread ID rather than the thread state pointer.
1 parent daf9ff9 commit 82ae5a6

File tree

7 files changed

+118
-34
lines changed

7 files changed

+118
-34
lines changed

Doc/data/python3.12.abi

+37-31
Large diffs are not rendered by default.

Include/internal/pycore_interp.h

+18
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,10 @@ struct _is {
194194
struct _Py_interp_cached_objects cached_objects;
195195
struct _Py_interp_static_objects static_objects;
196196

197+
/* The ID of the OS thread in which we are finalizing.
198+
We use _Py_atomic_address instead of adding a new _Py_atomic_ulong. */
199+
_Py_atomic_address _finalizing_id;
200+
197201
/* the initial PyInterpreterState.threads.head */
198202
PyThreadState _initial_thread;
199203
};
@@ -209,9 +213,23 @@ _PyInterpreterState_GetFinalizing(PyInterpreterState *interp) {
209213
return (PyThreadState*)_Py_atomic_load_relaxed(&interp->_finalizing);
210214
}
211215

216+
static inline unsigned long
217+
_PyInterpreterState_GetFinalizingID(PyInterpreterState *interp) {
218+
return (unsigned long)_Py_atomic_load_relaxed(&interp->_finalizing_id);
219+
}
220+
212221
static inline void
213222
_PyInterpreterState_SetFinalizing(PyInterpreterState *interp, PyThreadState *tstate) {
214223
_Py_atomic_store_relaxed(&interp->_finalizing, (uintptr_t)tstate);
224+
if (tstate == NULL) {
225+
_Py_atomic_store_relaxed(&interp->_finalizing_id, 0);
226+
}
227+
else {
228+
// XXX Re-enable this assert once gh-109860 is fixed.
229+
//assert(tstate->thread_id == PyThread_get_thread_ident());
230+
_Py_atomic_store_relaxed(&interp->_finalizing_id,
231+
(uintptr_t)tstate->thread_id);
232+
}
215233
}
216234

217235

Include/internal/pycore_pystate.h

+6-2
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,12 @@ _Py_IsMainInterpreter(PyInterpreterState *interp)
3636
static inline int
3737
_Py_IsMainInterpreterFinalizing(PyInterpreterState *interp)
3838
{
39-
return (_PyRuntimeState_GetFinalizing(interp->runtime) != NULL &&
40-
interp == &interp->runtime->_main_interpreter);
39+
/* bpo-39877: Access _PyRuntime directly rather than using
40+
tstate->interp->runtime to support calls from Python daemon threads.
41+
After Py_Finalize() has been called, tstate can be a dangling pointer:
42+
point to PyThreadState freed memory. */
43+
return (_PyRuntimeState_GetFinalizing(&_PyRuntime) != NULL &&
44+
interp == &_PyRuntime._main_interpreter);
4145
}
4246

4347

Include/internal/pycore_runtime.h

+17
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,9 @@ typedef struct pyruntimestate {
163163
struct _Py_static_objects static_objects;
164164
struct _Py_cached_objects cached_objects;
165165

166+
/* The ID of the OS thread in which we are finalizing.
167+
We use _Py_atomic_address instead of adding a new _Py_atomic_ulong. */
168+
_Py_atomic_address _finalizing_id;
166169
/* The value to use for sys.path[0] in new subinterpreters.
167170
Normally this would be part of the PyConfig struct. However,
168171
we cannot add it there in 3.12 since that's an ABI change. */
@@ -210,9 +213,23 @@ _PyRuntimeState_GetFinalizing(_PyRuntimeState *runtime) {
210213
return (PyThreadState*)_Py_atomic_load_relaxed(&runtime->_finalizing);
211214
}
212215

216+
static inline unsigned long
217+
_PyRuntimeState_GetFinalizingID(_PyRuntimeState *runtime) {
218+
return (unsigned long)_Py_atomic_load_relaxed(&runtime->_finalizing_id);
219+
}
220+
213221
static inline void
214222
_PyRuntimeState_SetFinalizing(_PyRuntimeState *runtime, PyThreadState *tstate) {
215223
_Py_atomic_store_relaxed(&runtime->_finalizing, (uintptr_t)tstate);
224+
if (tstate == NULL) {
225+
_Py_atomic_store_relaxed(&runtime->_finalizing_id, 0);
226+
}
227+
else {
228+
// XXX Re-enable this assert once gh-109860 is fixed.
229+
//assert(tstate->thread_id == PyThread_get_thread_ident());
230+
_Py_atomic_store_relaxed(&runtime->_finalizing_id,
231+
(uintptr_t)tstate->thread_id);
232+
}
216233
}
217234

218235
#ifdef __cplusplus

Lib/test/test_interpreters.py

+20
Original file line numberDiff line numberDiff line change
@@ -639,6 +639,26 @@ def test_sys_path_0(self):
639639
self.assertEqual(sp0_sub, expected)
640640

641641

642+
class FinalizationTests(TestBase):
643+
644+
def test_gh_109793(self):
645+
import subprocess
646+
argv = [sys.executable, '-c', '''if True:
647+
import _xxsubinterpreters as _interpreters
648+
interpid = _interpreters.create()
649+
raise Exception
650+
''']
651+
proc = subprocess.run(argv, capture_output=True, text=True)
652+
self.assertIn('Traceback', proc.stderr)
653+
if proc.returncode == 0 and support.verbose:
654+
print()
655+
print("--- cmd unexpected succeeded ---")
656+
print(f"stdout:\n{proc.stdout}")
657+
print(f"stderr:\n{proc.stderr}")
658+
print("------")
659+
self.assertEqual(proc.returncode, 1)
660+
661+
642662
class TestIsShareable(TestBase):
643663

644664
def test_default_shareables(self):
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
The main thread no longer exits prematurely when a subinterpreter
2+
is cleaned up during runtime finalization. The bug was a problem
3+
particularly because, when triggered, the Python process would
4+
always return with a 0 exitcode, even if it failed.

Python/pystate.c

+16-1
Original file line numberDiff line numberDiff line change
@@ -2916,11 +2916,26 @@ _PyThreadState_MustExit(PyThreadState *tstate)
29162916
tstate->interp->runtime to support calls from Python daemon threads.
29172917
After Py_Finalize() has been called, tstate can be a dangling pointer:
29182918
point to PyThreadState freed memory. */
2919+
unsigned long finalizing_id = _PyRuntimeState_GetFinalizingID(&_PyRuntime);
29192920
PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(&_PyRuntime);
29202921
if (finalizing == NULL) {
2922+
// XXX This isn't completely safe from daemon thraeds,
2923+
// since tstate might be a dangling pointer.
29212924
finalizing = _PyInterpreterState_GetFinalizing(tstate->interp);
2925+
finalizing_id = _PyInterpreterState_GetFinalizingID(tstate->interp);
29222926
}
2923-
return (finalizing != NULL && finalizing != tstate);
2927+
// XXX else check &_PyRuntime._main_interpreter._initial_thread
2928+
if (finalizing == NULL) {
2929+
return 0;
2930+
}
2931+
else if (finalizing == tstate) {
2932+
return 0;
2933+
}
2934+
else if (finalizing_id == PyThread_get_thread_ident()) {
2935+
/* gh-109793: we must have switched interpreters. */
2936+
return 0;
2937+
}
2938+
return 1;
29242939
}
29252940

29262941

0 commit comments

Comments
 (0)