gh-126644: Fix various thread safety issues in _interpreters

Include/internal/pycore_crossinterp.h

@@ -210,6 +210,7 @@ typedef enum error_code{
  _PyXI_ERR_MAIN_NS_FAILURE = -5,
  _PyXI_ERR_APPLY_NS_FAILURE = -6,
  _PyXI_ERR_NOT_SHAREABLE = -7,
+ _PyXI_ERR_SHUTTING_DOWN = -8,
 } _PyXI_errcode;
 
 

Include/internal/pycore_interp.h

@@ -139,6 +139,18 @@ struct _is{
  or the size specified by the THREAD_STACK_SIZE macro. */
  /* Used in Python/thread.c. */
  size_t stacksize;
+#ifdef Py_GIL_DISABLED
+ /*
+ * Whether setting the main thread should be prevented.
+ * This stops any data race problems during interpreter
+ * finalization, because it ensures that no other threads
+ * can be trying to access the subinterpreter on the free-threaded
+ * build.
+ *
+ * (See GH-126644)
+ */
+ int prevented;
+#endif
  } threads;
 
  /* Reference to the _PyRuntime global variable. This field exists
@@ -403,6 +415,14 @@ PyAPI_FUNC(PyStatus) _PyInterpreterState_New(
  PyThreadState *tstate,
  PyInterpreterState **pinterp);
 
+PyAPI_FUNC(int)
+_PyInterpreterState_PreventMain(PyInterpreterState *interp);
+
+PyAPI_FUNC(int)
+_PyInterpreterState_IsRunningAllowed(PyInterpreterState *interp);
+
+PyAPI_FUNC(PyThreadState *)
+_PyInterpreterState_ThreadHeadSafe(PyInterpreterState *interp);
 
 #define RARE_EVENT_INTERP_INC(interp, name) \
  do{\

Lib/test/test_free_threading/test_interpreters.py

@@ -0,0 +1,27 @@
+import threading
+import unittest
+
+from test import support
+from test.support import import_helper
+from test.support import threading_helper
+_interpreters = import_helper.import_module('_interpreters')
+
+
+@threading_helper.requires_working_threading()
+class StressTests(unittest.TestCase):
+
+ @support.requires_resource('cpu')
+ def test_subinterpreter_thread_safety(self):
+ # GH-126644: _interpreters had thread safety problems on the free-threaded build
+ interp = _interpreters.create()
+ threads = [threading.Thread(target=_interpreters.run_string, args=(interp, "1")) for _ in range(1000)]
+ threads.extend([threading.Thread(target=_interpreters.destroy, args=(interp,)) for _ in range(1000)])
+ # This will spam all kinds of subinterpreter errors, but we don't care.
+ # We just want to make sure that it doesn't crash.
+ with threading_helper.start_threads(threads):
+ pass
+
+
+if __name__ == '__main__':
+ # Test needs to be a package, so we can do relative imports.
+ unittest.main()

Lib/test/test_interpreters/__init__.py

@@ -1,9 +1,5 @@
 import os
-from test.support import load_package_tests, Py_GIL_DISABLED
-import unittest
-
-if Py_GIL_DISABLED:
- raise unittest.SkipTest("GIL disabled")
+from test.support import load_package_tests
 
 def load_tests(*args):
  return load_package_tests(os.path.dirname(__file__), *args)

Lib/test/test_interpreters/test_api.py

@@ -568,6 +568,8 @@ def test_still_running(self):
  interp.close()
  self.assertTrue(interp.is_running())
 
+ # TODO: Figure this out
+ @unittest.skipIf(support.Py_GIL_DISABLED, "does not work on the free-threaded build")
  def test_subthreads_still_running(self):
  r_interp, w_interp = self.pipe()
  r_thread, w_thread = self.pipe()
@@ -594,9 +596,9 @@ def task():
  os.write({w_interp},{FINISHED!r})
  t = threading.Thread(target=task)
  t.start()
+ t.join()
  """)
  interp.close()
-
  self.assertEqual(os.read(r_interp, 1), FINISHED)
 
  def test_created_with_capi(self):

Misc/NEWS.d/next/Library/2024-11-10-13-46-21.gh-issue-126644.mnjYQT.rst

@@ -0,0 +1,2 @@
+Fix crash when using subinterpreters in multiple threads on the
+:term:`free-threaded <free threading>` build.

Modules/_interpretersmodule.c

@@ -704,6 +704,33 @@ interp_destroy(PyObject *self, PyObject *args, PyObject *kwds)
  return NULL;
  }
 
+#ifdef Py_GIL_DISABLED
+ if (_PyInterpreterState_PreventMain(interp) == 0){
+ PyErr_SetString(PyExc_InterpreterError,
+ "interpreter started running during shutdown");
+ return NULL;
+ }
+
+ // It would be nice if we could just lock the runtime here, but
+ // unfortunately we don't have access to private PyMutex functions from this module.
+ if (_PyInterpreterState_ThreadHeadSafe(interp) != NULL)
+{
+ /*
+ * We're in a weird limbo where the main thread isn't set but
+ * the thread states haven't fully cleared yet.
+ */
+ PyErr_SetString(PyExc_InterpreterError, "interpreter is still finishing");
+ return NULL;
+ }
+
+ /*
+ * Running main has now been prevented, the interpreter will
+ * never run again (GH-126644).
+ */
+ assert(!is_running_main(interp));
+ assert(!_PyInterpreterState_IsRunningAllowed(interp));
+#endif
+
  // Destroy the interpreter.
  _PyXI_EndInterpreter(interp, NULL, NULL);
 

Python/crossinterp.c

@@ -986,8 +986,11 @@ _PyXI_ApplyErrorCode(_PyXI_errcode code, PyInterpreterState *interp)
  break;
  case _PyXI_ERR_ALREADY_RUNNING:
  assert(interp != NULL);
- assert(_PyInterpreterState_IsRunningMain(interp));
- _PyInterpreterState_FailIfRunningMain(interp);
+ // We can't use _PyInterpreterState_FailIfRunningMain, because
+ // it's possible that another thread has already changed the state
+ // of the interpreter by now.
+ PyErr_SetString(PyExc_InterpreterError,
+ "interpreter already running");
  break;
  case _PyXI_ERR_MAIN_NS_FAILURE:
  PyErr_SetString(PyExc_InterpreterError,
@@ -1000,6 +1003,10 @@ _PyXI_ApplyErrorCode(_PyXI_errcode code, PyInterpreterState *interp)
  case _PyXI_ERR_NOT_SHAREABLE:
  _set_xid_lookup_failure(interp, NULL, NULL);
  break;
+ case _PyXI_ERR_SHUTTING_DOWN:
+ PyErr_SetString(PyExc_InterpreterError,
+ "interpreter is shutting down");
+ break;
  default:
 #ifdef Py_DEBUG
  Py_UNREACHABLE();
@@ -1716,9 +1723,17 @@ _PyXI_Enter(_PyXI_session *session,
  // In the case where we didn't switch interpreters, it would
  // be more efficient to leave the exception in place and return
  // immediately. However, life is simpler if we don't.
+#ifdef Py_GIL_DISABLED
+ errcode = _PyInterpreterState_IsRunningAllowed(interp) == 1
+ ? _PyXI_ERR_ALREADY_RUNNING
+ : _PyXI_ERR_SHUTTING_DOWN;
+#else
  errcode = _PyXI_ERR_ALREADY_RUNNING;
+#endif
  goto error;
  }
+
+ assert(_PyInterpreterState_IsRunningAllowed(interp));
  session->running = 1;
 
  // Cache __main__.__dict__.

Python/pystate.c

@@ -1047,9 +1047,80 @@ get_main_thread(PyInterpreterState *interp)
  return _Py_atomic_load_ptr_relaxed(&interp->threads.main);
 }
 
+#define _PyInterpreterState_RUNNING_OK 0
+#define _PyInterpreterState_RUNNING_PREVENTED 1
+
+/*
+ * Is this interpreter allowed to set the main thread anymore?
+ */
+int
+_PyInterpreterState_IsRunningAllowed(PyInterpreterState *interp)
+{
+ assert(interp != NULL);
+#ifdef Py_GIL_DISABLED
+ return _Py_atomic_load_int(&interp->threads.prevented) == _PyInterpreterState_RUNNING_OK;
+#else
+ return 1;
+#endif
+}
+
+/*
+ * Atomically prevent future setting of the main thread.
+ *
+ * If something goes wrong, then this returns 0 without
+ * an exception, and 1 otherwise.
+ *
+ * On the GIL-ful build, this is a no-op.
+ */
+int
+_PyInterpreterState_PreventMain(PyInterpreterState *interp)
+{
+ assert(interp != NULL);
+#ifdef Py_GIL_DISABLED
+ if (_PyInterpreterState_IsRunningMain(interp))
+{
+ // Interpreter is running, can't prevent it yet.
+ return 0;
+ }
+
+ int expected_prevented = _PyInterpreterState_RUNNING_OK;
+ if (_Py_atomic_compare_exchange_int(&interp->threads.prevented,
+ &expected_prevented,
+ _PyInterpreterState_RUNNING_PREVENTED) == 0)
+{
+ // Another thread beat us!
+ return 0;
+ }
+
+ if (_PyInterpreterState_IsRunningMain(interp))
+{
+ // Another thread started right in between the
+ // prevention period!
+
+ // XXX Is having the prevented flag set a problem for a
+ // running interpreter?
+ return 0;
+ }
+
+ assert(!_PyInterpreterState_IsRunningAllowed(interp));
+ assert(!_PyInterpreterState_IsRunningMain(interp));
+ return 1;
+#else
+ // The GIL protects us on the default build, so this
+ // case shouldn't be possible.
+ assert(!_PyInterpreterState_IsRunningMain(interp));
+ return 1;
+#endif
+}
+
 int
 _PyInterpreterState_SetRunningMain(PyInterpreterState *interp)
 {
+ if (!_PyInterpreterState_IsRunningAllowed(interp))
+{
+ PyErr_SetString(PyExc_RuntimeError, "cannot run this interpreter anymore");
+ return -1;
+ }
  if (_PyInterpreterState_FailIfRunningMain(interp) < 0){
  return -1;
  }
@@ -1513,15 +1584,19 @@ new_threadstate(PyInterpreterState *interp, int whence)
  PyMem_RawFree(new_tstate);
  return NULL;
  }
+
+ /* _Py_ReserveTLBCIndex has thread safety issues */
+ HEAD_LOCK(runtime);
+
  int32_t tlbc_idx = _Py_ReserveTLBCIndex(interp);
  if (tlbc_idx < 0){
  PyMem_RawFree(new_tstate);
  return NULL;
  }
-#endif
-
+#else
  /* We serialize concurrent creation to protect global state. */
  HEAD_LOCK(runtime);
+#endif
 
  interp->threads.next_unique_id += 1;
  uint64_t id = interp->threads.next_unique_id;
@@ -2482,6 +2557,16 @@ PyThreadState_Next(PyThreadState *tstate){
 }
 
 
+PyThreadState *
+_PyInterpreterState_ThreadHeadSafe(PyInterpreterState *interp)
+{
+ HEAD_LOCK(&_PyRuntime);
+ PyThreadState *tstate = interp->threads.head;
+ HEAD_UNLOCK(&_PyRuntime);
+ return tstate;
+}
+
+
 /********************************************/
 /* reporting execution state of all threads */
 /********************************************/