From e19af2ca44f3dd424fe616ae03ae7f48673210b7 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 2 Oct 2023 14:12:12 -0600 Subject: [PATCH 1/9] [3.12] gh-105716: Support Background Threads in Subinterpreters Consistently (gh-109921) The existence of background threads running on a subinterpreter was preventing interpreters from getting properly destroyed, as well as impacting the ability to run the interpreter again. It also affected how we wait for non-daemon threads to finish. We add PyInterpreterState.threads.main, with some internal C-API functions. (cherry-picked from commit 1dd9dee45d2591b4e701039d1673282380696849) --- Include/internal/pycore_interp.h | 2 + Include/internal/pycore_pystate.h | 5 + Lib/test/test_interpreters.py | 97 +++++++++++++++++++ Lib/test/test_threading.py | 49 ++++++++++ Lib/threading.py | 4 +- ...-09-26-14-00-25.gh-issue-105716.SUJkW1.rst | 3 + Modules/_threadmodule.c | 16 ++- Modules/_xxsubinterpretersmodule.c | 90 +++++++++-------- Modules/main.c | 4 + Python/pystate.c | 37 +++++++ 10 files changed, 262 insertions(+), 45 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-09-26-14-00-25.gh-issue-105716.SUJkW1.rst diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 1db23145a539cb..3eaf08e122daa2 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -68,6 +68,8 @@ struct _is { uint64_t next_unique_id; /* The linked list of threads, newest first. */ PyThreadState *head; + /* The thread currently executing in the __main__ module, if any. */ + PyThreadState *main; /* Used in Modules/_threadmodule.c. */ long count; /* Support for runtime thread stack size tuning. diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 5be0ff6764c693..5bb8ff50d54d00 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -40,6 +40,11 @@ _Py_IsMainInterpreterFinalizing(PyInterpreterState *interp) interp == &interp->runtime->_main_interpreter); } +// Export for _xxsubinterpreters module. +PyAPI_FUNC(int) _PyInterpreterState_SetRunningMain(PyInterpreterState *); +PyAPI_FUNC(void) _PyInterpreterState_SetNotRunningMain(PyInterpreterState *); +PyAPI_FUNC(int) _PyInterpreterState_IsRunningMain(PyInterpreterState *); + static inline const PyConfig * _Py_GetMainConfig(void) diff --git a/Lib/test/test_interpreters.py b/Lib/test/test_interpreters.py index 27a143c7f5f38d..00e772aaca9fa3 100644 --- a/Lib/test/test_interpreters.py +++ b/Lib/test/test_interpreters.py @@ -257,6 +257,16 @@ def test_subinterpreter(self): self.assertTrue(interp.is_running()) self.assertFalse(interp.is_running()) + def test_finished(self): + r, w = os.pipe() + interp = interpreters.create() + interp.run(f"""if True: + import os + os.write({w}, b'x') + """) + self.assertFalse(interp.is_running()) + self.assertEqual(os.read(r, 1), b'x') + def test_from_subinterpreter(self): interp = interpreters.create() out = _run_output(interp, dedent(f""" @@ -284,6 +294,31 @@ def test_bad_id(self): with self.assertRaises(ValueError): interp.is_running() + def test_with_only_background_threads(self): + r_interp, w_interp = os.pipe() + r_thread, w_thread = os.pipe() + + DONE = b'D' + FINISHED = b'F' + + interp = interpreters.create() + interp.run(f"""if True: + import os + import threading + + def task(): + v = os.read({r_thread}, 1) + assert v == {DONE!r} + os.write({w_interp}, {FINISHED!r}) + t = threading.Thread(target=task) + t.start() + """) + self.assertFalse(interp.is_running()) + + os.write(w_thread, DONE) + interp.run('t.join()') + self.assertEqual(os.read(r_interp, 1), FINISHED) + class TestInterpreterClose(TestBase): @@ -385,6 +420,37 @@ def test_still_running(self): interp.close() self.assertTrue(interp.is_running()) + def test_subthreads_still_running(self): + r_interp, w_interp = os.pipe() + r_thread, w_thread = os.pipe() + + FINISHED = b'F' + + interp = interpreters.create() + interp.run(f"""if True: + import os + import threading + import time + + done = False + + def notify_fini(): + global done + done = True + t.join() + threading._register_atexit(notify_fini) + + def task(): + while not done: + time.sleep(0.1) + os.write({w_interp}, {FINISHED!r}) + t = threading.Thread(target=task) + t.start() + """) + interp.close() + + self.assertEqual(os.read(r_interp, 1), FINISHED) + class TestInterpreterRun(TestBase): @@ -461,6 +527,37 @@ def test_bytes_for_script(self): with self.assertRaises(TypeError): interp.run(b'print("spam")') + def test_with_background_threads_still_running(self): + r_interp, w_interp = os.pipe() + r_thread, w_thread = os.pipe() + + RAN = b'R' + DONE = b'D' + FINISHED = b'F' + + interp = interpreters.create() + interp.run(f"""if True: + import os + import threading + + def task(): + v = os.read({r_thread}, 1) + assert v == {DONE!r} + os.write({w_interp}, {FINISHED!r}) + t = threading.Thread(target=task) + t.start() + os.write({w_interp}, {RAN!r}) + """) + interp.run(f"""if True: + os.write({w_interp}, {RAN!r}) + """) + + os.write(w_thread, DONE) + interp.run('t.join()') + self.assertEqual(os.read(r_interp, 1), RAN) + self.assertEqual(os.read(r_interp, 1), RAN) + self.assertEqual(os.read(r_interp, 1), FINISHED) + # test_xxsubinterpreters covers the remaining Interpreter.run() behavior. diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index f63e5c6184ef0b..ce477d2df7d7d4 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -26,6 +26,11 @@ from test import lock_tests from test import support +try: + from test.support import interpreters +except ModuleNotFoundError: + interpreters = None + threading_helper.requires_working_threading(module=True) # Between fork() and exec(), only async-safe functions are allowed (issues @@ -45,6 +50,12 @@ def skip_unless_reliable_fork(test): return test +def requires_subinterpreters(meth): + """Decorator to skip a test if subinterpreters are not supported.""" + return unittest.skipIf(interpreters is None, + 'subinterpreters required')(meth) + + def restore_default_excepthook(testcase): testcase.addCleanup(setattr, threading, 'excepthook', threading.excepthook) threading.excepthook = threading.__excepthook__ @@ -1296,6 +1307,44 @@ def f(): # The thread was joined properly. self.assertEqual(os.read(r, 1), b"x") + @requires_subinterpreters + def test_threads_join_with_no_main(self): + r_interp, w_interp = self.pipe() + + INTERP = b'I' + FINI = b'F' + DONE = b'D' + + interp = interpreters.create() + interp.run(f"""if True: + import os + import threading + import time + + done = False + + def notify_fini(): + global done + done = True + os.write({w_interp}, {FINI!r}) + t.join() + threading._register_atexit(notify_fini) + + def task(): + while not done: + time.sleep(0.1) + os.write({w_interp}, {DONE!r}) + t = threading.Thread(target=task) + t.start() + + os.write({w_interp}, {INTERP!r}) + """) + interp.close() + + self.assertEqual(os.read(r_interp, 1), INTERP) + self.assertEqual(os.read(r_interp, 1), FINI) + self.assertEqual(os.read(r_interp, 1), DONE) + @cpython_only def test_daemon_threads_fatal_error(self): subinterp_code = f"""if 1: diff --git a/Lib/threading.py b/Lib/threading.py index a746dee5708124..624e7ed8f07c8f 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -37,6 +37,7 @@ _allocate_lock = _thread.allocate_lock _set_sentinel = _thread._set_sentinel get_ident = _thread.get_ident +_is_main_interpreter = _thread._is_main_interpreter try: get_native_id = _thread.get_native_id _HAVE_THREAD_NATIVE_ID = True @@ -1566,7 +1567,7 @@ def _shutdown(): # the main thread's tstate_lock - that won't happen until the interpreter # is nearly dead. So we release it here. Note that just calling _stop() # isn't enough: other threads may already be waiting on _tstate_lock. - if _main_thread._is_stopped: + if _main_thread._is_stopped and _is_main_interpreter(): # _shutdown() was already called return @@ -1619,6 +1620,7 @@ def main_thread(): In normal conditions, the main thread is the thread from which the Python interpreter was started. """ + # XXX Figure this out for subinterpreters. (See gh-75698.) return _main_thread # get thread-local implementation, either from the thread diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-09-26-14-00-25.gh-issue-105716.SUJkW1.rst b/Misc/NEWS.d/next/Core and Builtins/2023-09-26-14-00-25.gh-issue-105716.SUJkW1.rst new file mode 100644 index 00000000000000..b35550fa650dcc --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-09-26-14-00-25.gh-issue-105716.SUJkW1.rst @@ -0,0 +1,3 @@ +Subinterpreters now correctly handle the case where they have threads +running in the background. Before, such threads would interfere with +cleaning up and destroying them, as well as prevent running another script. diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 5edb6e9875d1ab..568fe8375d1eb4 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1604,6 +1604,18 @@ PyDoc_STRVAR(excepthook_doc, \n\ Handle uncaught Thread.run() exception."); +static PyObject * +thread__is_main_interpreter(PyObject *module, PyObject *Py_UNUSED(ignored)) +{ + PyInterpreterState *interp = _PyInterpreterState_GET(); + return PyBool_FromLong(_Py_IsMainInterpreter(interp)); +} + +PyDoc_STRVAR(thread__is_main_interpreter_doc, +"_is_main_interpreter()\n\ +\n\ +Return True if the current interpreter is the main Python interpreter."); + static PyMethodDef thread_methods[] = { {"start_new_thread", (PyCFunction)thread_PyThread_start_new_thread, METH_VARARGS, start_new_doc}, @@ -1633,8 +1645,10 @@ static PyMethodDef thread_methods[] = { METH_VARARGS, stack_size_doc}, {"_set_sentinel", thread__set_sentinel, METH_NOARGS, _set_sentinel_doc}, - {"_excepthook", thread_excepthook, + {"_excepthook", thread_excepthook, METH_O, excepthook_doc}, + {"_is_main_interpreter", thread__is_main_interpreter, + METH_NOARGS, thread__is_main_interpreter_doc}, {NULL, NULL} /* sentinel */ }; diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c index 4801f37d6f6c5f..4d4066788ca704 100644 --- a/Modules/_xxsubinterpretersmodule.c +++ b/Modules/_xxsubinterpretersmodule.c @@ -2,7 +2,14 @@ /* interpreters module */ /* low-level access to interpreter primitives */ +#ifndef Py_BUILD_CORE_BUILTIN +# define Py_BUILD_CORE_MODULE 1 +#endif + #include "Python.h" +#include "pycore_initconfig.h" // _PyErr_SetFromPyStatus() +#include "pycore_pyerrors.h" // _PyErr_ChainExceptions1() +#include "pycore_pystate.h" // _PyInterpreterState_SetRunningMain() #include "interpreteridobject.h" @@ -360,41 +367,14 @@ exceptions_init(PyObject *mod) } static int -_is_running(PyInterpreterState *interp) -{ - PyThreadState *tstate = PyInterpreterState_ThreadHead(interp); - if (PyThreadState_Next(tstate) != NULL) { - PyErr_SetString(PyExc_RuntimeError, - "interpreter has more than one thread"); - return -1; - } - - assert(!PyErr_Occurred()); - struct _PyInterpreterFrame *frame = tstate->cframe->current_frame; - if (frame == NULL) { - return 0; - } - return 1; -} - -static int -_ensure_not_running(PyInterpreterState *interp) +_run_script(PyInterpreterState *interp, const char *codestr, + _sharedns *shared, _sharedexception *sharedexc) { - int is_running = _is_running(interp); - if (is_running < 0) { + if (_PyInterpreterState_SetRunningMain(interp) < 0) { + // We skip going through the shared exception. return -1; } - if (is_running) { - PyErr_Format(PyExc_RuntimeError, "interpreter already running"); - return -1; - } - return 0; -} -static int -_run_script(PyInterpreterState *interp, const char *codestr, - _sharedns *shared, _sharedexception *sharedexc) -{ PyObject *excval = NULL; PyObject *main_mod = _PyInterpreterState_GetMainModule(interp); if (main_mod == NULL) { @@ -424,6 +404,7 @@ _run_script(PyInterpreterState *interp, const char *codestr, else { Py_DECREF(result); // We throw away the result. } + _PyInterpreterState_SetNotRunningMain(interp); *sharedexc = no_exception; return 0; @@ -439,6 +420,7 @@ _run_script(PyInterpreterState *interp, const char *codestr, } Py_XDECREF(excval); assert(!PyErr_Occurred()); + _PyInterpreterState_SetNotRunningMain(interp); return -1; } @@ -446,9 +428,6 @@ static int _run_script_in_interpreter(PyObject *mod, PyInterpreterState *interp, const char *codestr, PyObject *shareables) { - if (_ensure_not_running(interp) < 0) { - return -1; - } module_state *state = get_module_state(mod); _sharedns *shared = _get_shared_ns(shareables); @@ -459,8 +438,26 @@ _run_script_in_interpreter(PyObject *mod, PyInterpreterState *interp, // Switch to interpreter. PyThreadState *save_tstate = NULL; if (interp != PyInterpreterState_Get()) { - // XXX Using the "head" thread isn't strictly correct. + // XXX gh-109860: Using the "head" thread isn't strictly correct. PyThreadState *tstate = PyInterpreterState_ThreadHead(interp); + assert(tstate != NULL); + // Hack (until gh-109860): The interpreter's initial thread state + // is least likely to break. + while(tstate->next != NULL) { + tstate = tstate->next; + } + // We must do this check before switching interpreters, so any + // exception gets raised in the right one. + // XXX gh-109860: Drop this redundant check once we stop + // re-using tstates that might already be in use. + if (_PyInterpreterState_IsRunningMain(interp)) { + PyErr_SetString(PyExc_RuntimeError, + "interpreter already running"); + if (shared != NULL) { + _sharedns_free(shared); + } + return -1; + } // XXX Possible GILState issues? save_tstate = PyThreadState_Swap(tstate); } @@ -480,8 +477,10 @@ _run_script_in_interpreter(PyObject *mod, PyInterpreterState *interp, _sharedexception_apply(&exc, state->RunFailedError); } else if (result != 0) { - // We were unable to allocate a shared exception. - PyErr_NoMemory(); + if (!PyErr_Occurred()) { + // We were unable to allocate a shared exception. + PyErr_NoMemory(); + } } if (shared != NULL) { @@ -576,12 +575,20 @@ interp_destroy(PyObject *self, PyObject *args, PyObject *kwds) // Ensure the interpreter isn't running. /* XXX We *could* support destroying a running interpreter but aren't going to worry about it for now. */ - if (_ensure_not_running(interp) < 0) { + if (_PyInterpreterState_IsRunningMain(interp)) { + PyErr_Format(PyExc_RuntimeError, "interpreter running"); return NULL; } // Destroy the interpreter. + // XXX gh-109860: Using the "head" thread isn't strictly correct. PyThreadState *tstate = PyInterpreterState_ThreadHead(interp); + assert(tstate != NULL); + // Hack (until gh-109860): The interpreter's initial thread state + // is least likely to break. + while(tstate->next != NULL) { + tstate = tstate->next; + } // XXX Possible GILState issues? PyThreadState *save_tstate = PyThreadState_Swap(tstate); Py_EndInterpreter(tstate); @@ -750,11 +757,7 @@ interp_is_running(PyObject *self, PyObject *args, PyObject *kwds) if (interp == NULL) { return NULL; } - int is_running = _is_running(interp); - if (is_running < 0) { - return NULL; - } - if (is_running) { + if (_PyInterpreterState_IsRunningMain(interp)) { Py_RETURN_TRUE; } Py_RETURN_FALSE; @@ -765,6 +768,7 @@ PyDoc_STRVAR(is_running_doc, \n\ Return whether or not the identified interpreter is running."); + static PyMethodDef module_functions[] = { {"create", _PyCFunction_CAST(interp_create), METH_VARARGS | METH_KEYWORDS, create_doc}, diff --git a/Modules/main.c b/Modules/main.c index 7edfeb3365b4c6..c277e1ed3175ff 100644 --- a/Modules/main.c +++ b/Modules/main.c @@ -597,6 +597,9 @@ pymain_run_python(int *exitcode) pymain_header(config); + _PyInterpreterState_SetRunningMain(interp); + assert(!PyErr_Occurred()); + if (config->run_command) { *exitcode = pymain_run_command(config->run_command); } @@ -620,6 +623,7 @@ pymain_run_python(int *exitcode) *exitcode = pymain_exit_err_print(); done: + _PyInterpreterState_SetNotRunningMain(interp); Py_XDECREF(main_importer_path); } diff --git a/Python/pystate.c b/Python/pystate.c index 6f60c3dccce222..fb4cdd82586b42 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1045,6 +1045,39 @@ _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime) #endif +int +_PyInterpreterState_SetRunningMain(PyInterpreterState *interp) +{ + if (interp->threads.main != NULL) { + PyErr_SetString(PyExc_RuntimeError, + "interpreter already running"); + return -1; + } + PyThreadState *tstate = current_fast_get(&_PyRuntime); + _Py_EnsureTstateNotNULL(tstate); + if (tstate->interp != interp) { + PyErr_SetString(PyExc_RuntimeError, + "current tstate has wrong interpreter"); + return -1; + } + interp->threads.main = tstate; + return 0; +} + +void +_PyInterpreterState_SetNotRunningMain(PyInterpreterState *interp) +{ + assert(interp->threads.main == current_fast_get(&_PyRuntime)); + interp->threads.main = NULL; +} + +int +_PyInterpreterState_IsRunningMain(PyInterpreterState *interp) +{ + return (interp->threads.main != NULL); +} + + //---------- // accessors //---------- @@ -2728,6 +2761,10 @@ _register_builtins_for_crossinterpreter_data(struct _xidregistry *xidregistry) } +/*************/ +/* Other API */ +/*************/ + _PyFrameEvalFunction _PyInterpreterState_GetEvalFrameFunc(PyInterpreterState *interp) { From b1e4a2495a8b6eac5b36b085db9f74b424cea7a9 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 19 Sep 2023 15:01:34 -0600 Subject: [PATCH 2/9] gh-76785: Use Pending Calls When Releasing Cross-Interpreter Data (gh-109556) This fixes some crashes in the _xxinterpchannels module, due to a race between interpreters. (cherry picked from commit fd7e08a6f35581e1189b9bf12feb51f7167a86c5) Co-authored-by: Eric Snow --- Include/cpython/pystate.h | 1 + Include/internal/pycore_ceval.h | 2 +- Include/internal/pycore_ceval_state.h | 4 +- Modules/_xxinterpchannelsmodule.c | 30 ++++++--- Modules/_xxsubinterpretersmodule.c | 29 ++++----- Python/ceval_gil.c | 8 +-- Python/pystate.c | 91 +++++++++++++++++---------- 7 files changed, 98 insertions(+), 67 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 628f2e0996e469..27d3279f80320c 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -431,6 +431,7 @@ PyAPI_FUNC(void) _PyCrossInterpreterData_Clear( PyAPI_FUNC(int) _PyObject_GetCrossInterpreterData(PyObject *, _PyCrossInterpreterData *); PyAPI_FUNC(PyObject *) _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *); PyAPI_FUNC(int) _PyCrossInterpreterData_Release(_PyCrossInterpreterData *); +PyAPI_FUNC(int) _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *); PyAPI_FUNC(int) _PyObject_CheckCrossInterpreterData(PyObject *); diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index fc0f72efdae48e..2d9aefcda31edb 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -26,7 +26,7 @@ extern void _PyEval_FiniState(struct _ceval_state *ceval); PyAPI_FUNC(void) _PyEval_SignalReceived(PyInterpreterState *interp); PyAPI_FUNC(int) _PyEval_AddPendingCall( PyInterpreterState *interp, - int (*func)(void *), + _Py_pending_call_func func, void *arg, int mainthreadonly); PyAPI_FUNC(void) _PyEval_SignalAsyncExc(PyInterpreterState *interp); diff --git a/Include/internal/pycore_ceval_state.h b/Include/internal/pycore_ceval_state.h index e56e43c6e0c6a7..2d54c6bd343076 100644 --- a/Include/internal/pycore_ceval_state.h +++ b/Include/internal/pycore_ceval_state.h @@ -13,6 +13,8 @@ extern "C" { #include "pycore_gil.h" // struct _gil_runtime_state +typedef int (*_Py_pending_call_func)(void *); + struct _pending_calls { int busy; PyThread_type_lock lock; @@ -24,7 +26,7 @@ struct _pending_calls { int async_exc; #define NPENDINGCALLS 32 struct _pending_call { - int (*func)(void *); + _Py_pending_call_func func; void *arg; } calls[NPENDINGCALLS]; int first; diff --git a/Modules/_xxinterpchannelsmodule.c b/Modules/_xxinterpchannelsmodule.c index 1d7e7f1d71af3e..9d3a1e5118ae89 100644 --- a/Modules/_xxinterpchannelsmodule.c +++ b/Modules/_xxinterpchannelsmodule.c @@ -161,14 +161,24 @@ add_new_type(PyObject *mod, PyType_Spec *spec, crossinterpdatafunc shared) return cls; } +#define XID_IGNORE_EXC 1 +#define XID_FREE 2 + static int -_release_xid_data(_PyCrossInterpreterData *data, int ignoreexc) +_release_xid_data(_PyCrossInterpreterData *data, int flags) { + int ignoreexc = flags & XID_IGNORE_EXC; PyObject *exc; if (ignoreexc) { exc = PyErr_GetRaisedException(); } - int res = _PyCrossInterpreterData_Release(data); + int res; + if (flags & XID_FREE) { + res = _PyCrossInterpreterData_ReleaseAndRawFree(data); + } + else { + res = _PyCrossInterpreterData_Release(data); + } if (res < 0) { /* The owning interpreter is already destroyed. */ if (ignoreexc) { @@ -176,6 +186,9 @@ _release_xid_data(_PyCrossInterpreterData *data, int ignoreexc) PyErr_Clear(); } } + if (flags & XID_FREE) { + /* Either way, we free the data. */ + } if (ignoreexc) { PyErr_SetRaisedException(exc); } @@ -367,9 +380,8 @@ static void _channelitem_clear(_channelitem *item) { if (item->data != NULL) { - (void)_release_xid_data(item->data, 1); // It was allocated in _channel_send(). - GLOBAL_FREE(item->data); + (void)_release_xid_data(item->data, XID_IGNORE_EXC & XID_FREE); item->data = NULL; } item->next = NULL; @@ -1440,14 +1452,12 @@ _channel_recv(_channels *channels, int64_t id, PyObject **res) PyObject *obj = _PyCrossInterpreterData_NewObject(data); if (obj == NULL) { assert(PyErr_Occurred()); - (void)_release_xid_data(data, 1); - // It was allocated in _channel_send(). - GLOBAL_FREE(data); + // It was allocated in _channel_send(), so we free it. + (void)_release_xid_data(data, XID_IGNORE_EXC | XID_FREE); return -1; } - int release_res = _release_xid_data(data, 0); - // It was allocated in _channel_send(). - GLOBAL_FREE(data); + // It was allocated in _channel_send(), so we free it. + int release_res = _release_xid_data(data, XID_FREE); if (release_res < 0) { // The source interpreter has been destroyed already. assert(PyErr_Occurred()); diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c index 4d4066788ca704..c0958c65dd0d65 100644 --- a/Modules/_xxsubinterpretersmodule.c +++ b/Modules/_xxsubinterpretersmodule.c @@ -60,24 +60,17 @@ add_new_exception(PyObject *mod, const char *name, PyObject *base) add_new_exception(MOD, MODULE_NAME "." Py_STRINGIFY(NAME), BASE) static int -_release_xid_data(_PyCrossInterpreterData *data, int ignoreexc) +_release_xid_data(_PyCrossInterpreterData *data) { - PyObject *exc; - if (ignoreexc) { - exc = PyErr_GetRaisedException(); - } + PyObject *exc = PyErr_GetRaisedException(); int res = _PyCrossInterpreterData_Release(data); if (res < 0) { /* The owning interpreter is already destroyed. */ _PyCrossInterpreterData_Clear(NULL, data); - if (ignoreexc) { - // XXX Emit a warning? - PyErr_Clear(); - } - } - if (ignoreexc) { - PyErr_SetRaisedException(exc); + // XXX Emit a warning? + PyErr_Clear(); } + PyErr_SetRaisedException(exc); return res; } @@ -147,7 +140,7 @@ _sharednsitem_clear(struct _sharednsitem *item) PyMem_RawFree((void *)item->name); item->name = NULL; } - (void)_release_xid_data(&item->data, 1); + (void)_release_xid_data(&item->data); } static int @@ -176,16 +169,16 @@ typedef struct _sharedns { static _sharedns * _sharedns_new(Py_ssize_t len) { - _sharedns *shared = PyMem_NEW(_sharedns, 1); + _sharedns *shared = PyMem_RawCalloc(sizeof(_sharedns), 1); if (shared == NULL) { PyErr_NoMemory(); return NULL; } shared->len = len; - shared->items = PyMem_NEW(struct _sharednsitem, len); + shared->items = PyMem_RawCalloc(sizeof(struct _sharednsitem), len); if (shared->items == NULL) { PyErr_NoMemory(); - PyMem_Free(shared); + PyMem_RawFree(shared); return NULL; } return shared; @@ -197,8 +190,8 @@ _sharedns_free(_sharedns *shared) for (Py_ssize_t i=0; i < shared->len; i++) { _sharednsitem_clear(&shared->items[i]); } - PyMem_Free(shared->items); - PyMem_Free(shared); + PyMem_RawFree(shared->items); + PyMem_RawFree(shared); } static _sharedns * diff --git a/Python/ceval_gil.c b/Python/ceval_gil.c index c1ab5883568e7d..18ab4ccc176284 100644 --- a/Python/ceval_gil.c +++ b/Python/ceval_gil.c @@ -757,7 +757,7 @@ _PyEval_SignalReceived(PyInterpreterState *interp) /* Push one item onto the queue while holding the lock. */ static int _push_pending_call(struct _pending_calls *pending, - int (*func)(void *), void *arg) + _Py_pending_call_func func, void *arg) { int i = pending->last; int j = (i + 1) % NPENDINGCALLS; @@ -804,7 +804,7 @@ _pop_pending_call(struct _pending_calls *pending, int _PyEval_AddPendingCall(PyInterpreterState *interp, - int (*func)(void *), void *arg, + _Py_pending_call_func func, void *arg, int mainthreadonly) { assert(!mainthreadonly || _Py_IsMainInterpreter(interp)); @@ -828,7 +828,7 @@ _PyEval_AddPendingCall(PyInterpreterState *interp, } int -Py_AddPendingCall(int (*func)(void *), void *arg) +Py_AddPendingCall(_Py_pending_call_func func, void *arg) { /* Legacy users of this API will continue to target the main thread (of the main interpreter). */ @@ -872,7 +872,7 @@ _make_pending_calls(struct _pending_calls *pending) { /* perform a bounded number of calls, in case of recursion */ for (int i=0; ifree != NULL) { - data->free(data->data); + // _PyCrossInterpreterData only has two members that need to be + // cleaned up, if set: "data" must be freed and "obj" must be decref'ed. + // In both cases the original (owning) interpreter must be used, + // which is the caller's responsibility to ensure. + if (data->data != NULL) { + if (data->free != NULL) { + data->free(data->data); + } + data->data = NULL; } - data->data = NULL; Py_CLEAR(data->obj); } @@ -2440,40 +2446,32 @@ _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *data) return data->new_object(data); } -typedef void (*releasefunc)(PyInterpreterState *, void *); - -static void -_call_in_interpreter(PyInterpreterState *interp, releasefunc func, void *arg) +static int +_release_xidata_pending(void *data) { - /* We would use Py_AddPendingCall() if it weren't specific to the - * main interpreter (see bpo-33608). In the meantime we take a - * naive approach. - */ - _PyRuntimeState *runtime = interp->runtime; - PyThreadState *save_tstate = NULL; - if (interp != current_fast_get(runtime)->interp) { - // XXX Using the "head" thread isn't strictly correct. - PyThreadState *tstate = PyInterpreterState_ThreadHead(interp); - // XXX Possible GILState issues? - save_tstate = _PyThreadState_Swap(runtime, tstate); - } - - // XXX Once the GIL is per-interpreter, this should be called with the - // calling interpreter's GIL released and the target interpreter's held. - func(interp, arg); + _xidata_clear((_PyCrossInterpreterData *)data); + return 0; +} - // Switch back. - if (save_tstate != NULL) { - _PyThreadState_Swap(runtime, save_tstate); - } +static int +_xidata_release_and_rawfree_pending(void *data) +{ + _xidata_clear((_PyCrossInterpreterData *)data); + PyMem_RawFree(data); + return 0; } -int -_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) +static int +_xidata_release(_PyCrossInterpreterData *data, int rawfree) { - if (data->free == NULL && data->obj == NULL) { + if ((data->data == NULL || data->free == NULL) && data->obj == NULL) { // Nothing to release! - data->data = NULL; + if (rawfree) { + PyMem_RawFree(data); + } + else { + data->data = NULL; + } return 0; } @@ -2484,15 +2482,42 @@ _PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) // This function shouldn't have been called. // XXX Someone leaked some memory... assert(PyErr_Occurred()); + if (rawfree) { + PyMem_RawFree(data); + } return -1; } // "Release" the data and/or the object. - _call_in_interpreter(interp, - (releasefunc)_PyCrossInterpreterData_Clear, data); + if (interp == current_fast_get(interp->runtime)->interp) { + _xidata_clear(data); + if (rawfree) { + PyMem_RawFree(data); + } + } + else { + _Py_pending_call_func func = _release_xidata_pending; + if (rawfree) { + func = _xidata_release_and_rawfree_pending; + } + // XXX Emit a warning if this fails? + _PyEval_AddPendingCall(interp, func, data, 0); + } return 0; } +int +_PyCrossInterpreterData_Release(_PyCrossInterpreterData *data) +{ + return _xidata_release(data, 0); +} + +int +_PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *data) +{ + return _xidata_release(data, 1); +} + /* registry of {type -> crossinterpdatafunc} */ /* For now we use a global registry of shareable classes. An From aeaeceeddffb983b68817c706c02568e05d63f2c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 11 Oct 2023 07:58:32 -0600 Subject: [PATCH 3/9] Move _PyCrossInterpreterData_ReleaseAndRawFree to internal API. --- Include/internal/pycore_pystate.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 5bb8ff50d54d00..97c5239ed8db9e 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -153,6 +153,8 @@ extern PyStatus _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime); extern void _PySignal_AfterFork(void); #endif +PyAPI_FUNC(int) _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *); + PyAPI_FUNC(int) _PyState_AddModule( PyThreadState *tstate, From 1940c7815a0bb7ab8d6900ecc7e8db22b4501fbf Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 11 Oct 2023 07:58:34 -0600 Subject: [PATCH 4/9] Move _PyCrossInterpreterData_ReleaseAndRawFree to internal API. --- Include/cpython/pystate.h | 1 - 1 file changed, 1 deletion(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 27d3279f80320c..628f2e0996e469 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -431,7 +431,6 @@ PyAPI_FUNC(void) _PyCrossInterpreterData_Clear( PyAPI_FUNC(int) _PyObject_GetCrossInterpreterData(PyObject *, _PyCrossInterpreterData *); PyAPI_FUNC(PyObject *) _PyCrossInterpreterData_NewObject(_PyCrossInterpreterData *); PyAPI_FUNC(int) _PyCrossInterpreterData_Release(_PyCrossInterpreterData *); -PyAPI_FUNC(int) _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *); PyAPI_FUNC(int) _PyObject_CheckCrossInterpreterData(PyObject *); From 49f99ca376ad0859f00e886d1b901785414192fc Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 11 Oct 2023 08:29:08 -0600 Subject: [PATCH 5/9] Set Py_BUILD_CORE_MODULE for _xxinterpchannelsmodule. --- Modules/_xxinterpchannelsmodule.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Modules/_xxinterpchannelsmodule.c b/Modules/_xxinterpchannelsmodule.c index 9d3a1e5118ae89..6bee11c1218518 100644 --- a/Modules/_xxinterpchannelsmodule.c +++ b/Modules/_xxinterpchannelsmodule.c @@ -2,8 +2,13 @@ /* interpreters module */ /* low-level access to interpreter primitives */ +#ifndef Py_BUILD_CORE_BUILTIN +# define Py_BUILD_CORE_MODULE 1 +#endif + #include "Python.h" #include "interpreteridobject.h" +#include "pycore_pystate.h" // _PyCrossInterpreterData_ReleaseAndRawFree() /* From f945ed26c07ba5efa83551dbd68a1e0a26d3d534 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 2 Oct 2023 13:59:05 -0600 Subject: [PATCH 6/9] [3.12] gh-109853: Fix sys.path[0] For Subinterpreters (gh-109994) This change makes sure sys.path[0] is set properly for subinterpreters. Before, it wasn't getting set at all. This change does not address the broader concerns from gh-109853. (cherry-picked from commit a040a32ea2f13f16172394d3e3e3f80f47f25a68) --- Include/cpython/initconfig.h | 3 + Lib/test/test_embed.py | 3 + Lib/test/test_interpreters.py | 152 ++++++++++++++++++ ...-09-27-18-01-06.gh-issue-109853.coQQiL.rst | 1 + Modules/main.c | 38 +++-- Python/initconfig.c | 5 + Python/pylifecycle.c | 25 +++ 7 files changed, 217 insertions(+), 10 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-09-27-18-01-06.gh-issue-109853.coQQiL.rst diff --git a/Include/cpython/initconfig.h b/Include/cpython/initconfig.h index cbae97f12f5377..affd366011af8e 100644 --- a/Include/cpython/initconfig.h +++ b/Include/cpython/initconfig.h @@ -205,6 +205,9 @@ typedef struct PyConfig { wchar_t *run_module; wchar_t *run_filename; + /* --- Set by Py_Main() -------------------------- */ + wchar_t *sys_path_0; + /* --- Private fields ---------------------------- */ // Install importlib? If equals to 0, importlib is not initialized at all. diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 24617ab24c6958..1d8d9f5b604835 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -500,6 +500,7 @@ class InitConfigTests(EmbeddingTestsMixin, unittest.TestCase): 'run_command': None, 'run_module': None, 'run_filename': None, + 'sys_path_0': None, '_install_importlib': 1, 'check_hash_pycs_mode': 'default', @@ -1119,6 +1120,7 @@ def test_init_run_main(self): 'program_name': './python3', 'run_command': code + '\n', 'parse_argv': 2, + 'sys_path_0': '', } self.check_all_configs("test_init_run_main", config, api=API_PYTHON) @@ -1134,6 +1136,7 @@ def test_init_main(self): 'run_command': code + '\n', 'parse_argv': 2, '_init_main': 0, + 'sys_path_0': '', } self.check_all_configs("test_init_main", config, api=API_PYTHON, diff --git a/Lib/test/test_interpreters.py b/Lib/test/test_interpreters.py index 00e772aaca9fa3..0e930e7958ab3c 100644 --- a/Lib/test/test_interpreters.py +++ b/Lib/test/test_interpreters.py @@ -1,5 +1,8 @@ import contextlib +import json import os +import os.path +import sys import threading from textwrap import dedent import unittest @@ -8,6 +11,7 @@ from test import support from test.support import import_helper from test.support import threading_helper +from test.support import os_helper _interpreters = import_helper.import_module('_xxsubinterpreters') _channels = import_helper.import_module('_xxinterpchannels') from test.support import interpreters @@ -585,6 +589,154 @@ def task(): pass +class StartupTests(TestBase): + + # We want to ensure the initial state of subinterpreters + # matches expectations. + + _subtest_count = 0 + + @contextlib.contextmanager + def subTest(self, *args): + with super().subTest(*args) as ctx: + self._subtest_count += 1 + try: + yield ctx + finally: + if self._debugged_in_subtest: + if self._subtest_count == 1: + # The first subtest adds a leading newline, so we + # compensate here by not printing a trailing newline. + print('### end subtest debug ###', end='') + else: + print('### end subtest debug ###') + self._debugged_in_subtest = False + + def debug(self, msg, *, header=None): + if header: + self._debug(f'--- {header} ---') + if msg: + if msg.endswith(os.linesep): + self._debug(msg[:-len(os.linesep)]) + else: + self._debug(msg) + self._debug('') + self._debug('------') + else: + self._debug(msg) + + _debugged = False + _debugged_in_subtest = False + def _debug(self, msg): + if not self._debugged: + print() + self._debugged = True + if self._subtest is not None: + if True: + if not self._debugged_in_subtest: + self._debugged_in_subtest = True + print('### start subtest debug ###') + print(msg) + else: + print(msg) + + def create_temp_dir(self): + import tempfile + tmp = tempfile.mkdtemp(prefix='test_interpreters_') + tmp = os.path.realpath(tmp) + self.addCleanup(os_helper.rmtree, tmp) + return tmp + + def write_script(self, *path, text): + filename = os.path.join(*path) + dirname = os.path.dirname(filename) + if dirname: + os.makedirs(dirname, exist_ok=True) + with open(filename, 'w', encoding='utf-8') as outfile: + outfile.write(dedent(text)) + return filename + + @support.requires_subprocess() + def run_python(self, argv, *, cwd=None): + # This method is inspired by + # EmbeddingTestsMixin.run_embedded_interpreter() in test_embed.py. + import shlex + import subprocess + if isinstance(argv, str): + argv = shlex.split(argv) + argv = [sys.executable, *argv] + try: + proc = subprocess.run( + argv, + cwd=cwd, + capture_output=True, + text=True, + ) + except Exception as exc: + self.debug(f'# cmd: {shlex.join(argv)}') + if isinstance(exc, FileNotFoundError) and not exc.filename: + if os.path.exists(argv[0]): + exists = 'exists' + else: + exists = 'does not exist' + self.debug(f'{argv[0]} {exists}') + raise # re-raise + assert proc.stderr == '' or proc.returncode != 0, proc.stderr + if proc.returncode != 0 and support.verbose: + self.debug(f'# python3 {shlex.join(argv[1:])} failed:') + self.debug(proc.stdout, header='stdout') + self.debug(proc.stderr, header='stderr') + self.assertEqual(proc.returncode, 0) + self.assertEqual(proc.stderr, '') + return proc.stdout + + def test_sys_path_0(self): + # The main interpreter's sys.path[0] should be used by subinterpreters. + script = ''' + import sys + from test.support import interpreters + + orig = sys.path[0] + + interp = interpreters.create() + interp.run(f"""if True: + import json + import sys + print(json.dumps({{ + 'main': {orig!r}, + 'sub': sys.path[0], + }}, indent=4), flush=True) + """) + ''' + # / + # pkg/ + # __init__.py + # __main__.py + # script.py + # script.py + cwd = self.create_temp_dir() + self.write_script(cwd, 'pkg', '__init__.py', text='') + self.write_script(cwd, 'pkg', '__main__.py', text=script) + self.write_script(cwd, 'pkg', 'script.py', text=script) + self.write_script(cwd, 'script.py', text=script) + + cases = [ + ('script.py', cwd), + ('-m script', cwd), + ('-m pkg', cwd), + ('-m pkg.script', cwd), + ('-c "import script"', ''), + ] + for argv, expected in cases: + with self.subTest(f'python3 {argv}'): + out = self.run_python(argv, cwd=cwd) + data = json.loads(out) + sp0_main, sp0_sub = data['main'], data['sub'] + self.assertEqual(sp0_sub, sp0_main) + self.assertEqual(sp0_sub, expected) + # XXX Also check them all with the -P cmdline flag? + + class TestIsShareable(TestBase): def test_default_shareables(self): diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-09-27-18-01-06.gh-issue-109853.coQQiL.rst b/Misc/NEWS.d/next/Core and Builtins/2023-09-27-18-01-06.gh-issue-109853.coQQiL.rst new file mode 100644 index 00000000000000..45de3ba8877b01 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-09-27-18-01-06.gh-issue-109853.coQQiL.rst @@ -0,0 +1 @@ +``sys.path[0]`` is now set correctly for subinterpreters. diff --git a/Modules/main.c b/Modules/main.c index c277e1ed3175ff..6e9ec74ad2c2de 100644 --- a/Modules/main.c +++ b/Modules/main.c @@ -559,6 +559,11 @@ pymain_run_python(int *exitcode) goto error; } + // XXX Calculate config->sys_path_0 in getpath.py. + // The tricky part is that we can't check the path importers yet + // at that point. + assert(config->sys_path_0 == NULL); + if (config->run_filename != NULL) { /* If filename is a package (ex: directory or ZIP file) which contains __main__.py, main_importer_path is set to filename and will be @@ -574,24 +579,37 @@ pymain_run_python(int *exitcode) // import readline and rlcompleter before script dir is added to sys.path pymain_import_readline(config); + PyObject *path0 = NULL; if (main_importer_path != NULL) { - if (pymain_sys_path_add_path0(interp, main_importer_path) < 0) { - goto error; - } + path0 = Py_NewRef(main_importer_path); } else if (!config->safe_path) { - PyObject *path0 = NULL; int res = _PyPathConfig_ComputeSysPath0(&config->argv, &path0); if (res < 0) { goto error; } - - if (res > 0) { - if (pymain_sys_path_add_path0(interp, path0) < 0) { - Py_DECREF(path0); - goto error; - } + else if (res == 0) { + Py_CLEAR(path0); + } + } + // XXX Apply config->sys_path_0 in init_interp_main(). We have + // to be sure to get readline/rlcompleter imported at the correct time. + if (path0 != NULL) { + wchar_t *wstr = PyUnicode_AsWideCharString(path0, NULL); + if (wstr == NULL) { Py_DECREF(path0); + goto error; + } + config->sys_path_0 = _PyMem_RawWcsdup(wstr); + PyMem_Free(wstr); + if (config->sys_path_0 == NULL) { + Py_DECREF(path0); + goto error; + } + int res = pymain_sys_path_add_path0(interp, path0); + Py_DECREF(path0); + if (res < 0) { + goto error; } } diff --git a/Python/initconfig.c b/Python/initconfig.c index 4e5d4bb9876e3b..7e5cc4e20d8734 100644 --- a/Python/initconfig.c +++ b/Python/initconfig.c @@ -739,6 +739,7 @@ PyConfig_Clear(PyConfig *config) CLEAR(config->exec_prefix); CLEAR(config->base_exec_prefix); CLEAR(config->platlibdir); + CLEAR(config->sys_path_0); CLEAR(config->filesystem_encoding); CLEAR(config->filesystem_errors); @@ -993,6 +994,7 @@ _PyConfig_Copy(PyConfig *config, const PyConfig *config2) COPY_WSTR_ATTR(exec_prefix); COPY_WSTR_ATTR(base_exec_prefix); COPY_WSTR_ATTR(platlibdir); + COPY_WSTR_ATTR(sys_path_0); COPY_ATTR(site_import); COPY_ATTR(bytes_warning); @@ -1102,6 +1104,7 @@ _PyConfig_AsDict(const PyConfig *config) SET_ITEM_WSTR(exec_prefix); SET_ITEM_WSTR(base_exec_prefix); SET_ITEM_WSTR(platlibdir); + SET_ITEM_WSTR(sys_path_0); SET_ITEM_INT(site_import); SET_ITEM_INT(bytes_warning); SET_ITEM_INT(warn_default_encoding); @@ -1403,6 +1406,7 @@ _PyConfig_FromDict(PyConfig *config, PyObject *dict) GET_WSTR_OPT(pythonpath_env); GET_WSTR_OPT(home); GET_WSTR(platlibdir); + GET_WSTR(sys_path_0); // Path configuration output GET_UINT(module_search_paths_set); @@ -3165,6 +3169,7 @@ _Py_DumpPathConfig(PyThreadState *tstate) PySys_WriteStderr(" import site = %i\n", config->site_import); PySys_WriteStderr(" is in build tree = %i\n", config->_is_python_build); DUMP_CONFIG("stdlib dir", stdlib_dir); + DUMP_CONFIG("sys.path[0]", sys_path_0); #undef DUMP_CONFIG #define DUMP_SYS(NAME) \ diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 29771e07ae6a2c..ba4fd5c1c1c1e5 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1200,6 +1200,31 @@ init_interp_main(PyThreadState *tstate) #endif } + if (!is_main_interp) { + // The main interpreter is handled in Py_Main(), for now. + if (config->sys_path_0 != NULL) { + PyObject *path0 = PyUnicode_FromWideChar(config->sys_path_0, -1); + if (path0 == NULL) { + return _PyStatus_ERR("can't initialize sys.path[0]"); + } + PyObject *sysdict = interp->sysdict; + if (sysdict == NULL) { + Py_DECREF(path0); + return _PyStatus_ERR("can't initialize sys.path[0]"); + } + PyObject *sys_path = PyDict_GetItemWithError(sysdict, &_Py_ID(path)); + if (sys_path == NULL) { + Py_DECREF(path0); + return _PyStatus_ERR("can't initialize sys.path[0]"); + } + int res = PyList_Insert(sys_path, 0, path0); + Py_DECREF(path0); + if (res) { + return _PyStatus_ERR("can't initialize sys.path[0]"); + } + } + } + assert(!_PyErr_Occurred(tstate)); return _PyStatus_OK(); From ea57f0c4cb5211f24687714f0e7b4da5adb6bac8 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 11 Oct 2023 04:59:43 -0600 Subject: [PATCH 7/9] Do not modify PyConfig. Add sys_path_0 to _PyRuntimeState instead. --- Include/cpython/initconfig.h | 3 --- Include/internal/pycore_runtime.h | 5 +++++ Lib/test/test_embed.py | 3 --- Modules/main.c | 13 ++++++++----- Python/initconfig.c | 5 ----- Python/pylifecycle.c | 5 +++-- Python/pystate.c | 4 ++++ 7 files changed, 20 insertions(+), 18 deletions(-) diff --git a/Include/cpython/initconfig.h b/Include/cpython/initconfig.h index affd366011af8e..cbae97f12f5377 100644 --- a/Include/cpython/initconfig.h +++ b/Include/cpython/initconfig.h @@ -205,9 +205,6 @@ typedef struct PyConfig { wchar_t *run_module; wchar_t *run_filename; - /* --- Set by Py_Main() -------------------------- */ - wchar_t *sys_path_0; - /* --- Private fields ---------------------------- */ // Install importlib? If equals to 0, importlib is not initialized at all. diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 5ed97e9715b2b0..de504b15945041 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -105,6 +105,11 @@ typedef struct pyruntimestate { unsigned long main_thread; + /* The value to use for sys.path[0] in new subinterpreters. + Normally this would be part of the PyConfig struct. However, + we cannot add it there in 3.12 since that's an ABI change. */ + wchar_t *sys_path_0; + /* ---------- IMPORTANT --------------------------- The fields above this line are declared as early as possible to facilitate out-of-process observability diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 1d8d9f5b604835..24617ab24c6958 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -500,7 +500,6 @@ class InitConfigTests(EmbeddingTestsMixin, unittest.TestCase): 'run_command': None, 'run_module': None, 'run_filename': None, - 'sys_path_0': None, '_install_importlib': 1, 'check_hash_pycs_mode': 'default', @@ -1120,7 +1119,6 @@ def test_init_run_main(self): 'program_name': './python3', 'run_command': code + '\n', 'parse_argv': 2, - 'sys_path_0': '', } self.check_all_configs("test_init_run_main", config, api=API_PYTHON) @@ -1136,7 +1134,6 @@ def test_init_main(self): 'run_command': code + '\n', 'parse_argv': 2, '_init_main': 0, - 'sys_path_0': '', } self.check_all_configs("test_init_main", config, api=API_PYTHON, diff --git a/Modules/main.c b/Modules/main.c index 6e9ec74ad2c2de..07eaa5befe0d68 100644 --- a/Modules/main.c +++ b/Modules/main.c @@ -559,10 +559,10 @@ pymain_run_python(int *exitcode) goto error; } - // XXX Calculate config->sys_path_0 in getpath.py. + // XXX Calculate runtime->sys_path_0 in getpath.py. // The tricky part is that we can't check the path importers yet // at that point. - assert(config->sys_path_0 == NULL); + assert(interp->runtime->sys_path_0 == NULL); if (config->run_filename != NULL) { /* If filename is a package (ex: directory or ZIP file) which contains @@ -592,7 +592,7 @@ pymain_run_python(int *exitcode) Py_CLEAR(path0); } } - // XXX Apply config->sys_path_0 in init_interp_main(). We have + // XXX Apply runtime->sys_path_0 in init_interp_main(). We have // to be sure to get readline/rlcompleter imported at the correct time. if (path0 != NULL) { wchar_t *wstr = PyUnicode_AsWideCharString(path0, NULL); @@ -600,9 +600,12 @@ pymain_run_python(int *exitcode) Py_DECREF(path0); goto error; } - config->sys_path_0 = _PyMem_RawWcsdup(wstr); + PyMemAllocatorEx old_alloc; + _PyMem_SetDefaultAllocator(PYMEM_DOMAIN_RAW, &old_alloc); + interp->runtime->sys_path_0 = _PyMem_RawWcsdup(wstr); + PyMem_SetAllocator(PYMEM_DOMAIN_RAW, &old_alloc); PyMem_Free(wstr); - if (config->sys_path_0 == NULL) { + if (interp->runtime->sys_path_0 == NULL) { Py_DECREF(path0); goto error; } diff --git a/Python/initconfig.c b/Python/initconfig.c index 7e5cc4e20d8734..4e5d4bb9876e3b 100644 --- a/Python/initconfig.c +++ b/Python/initconfig.c @@ -739,7 +739,6 @@ PyConfig_Clear(PyConfig *config) CLEAR(config->exec_prefix); CLEAR(config->base_exec_prefix); CLEAR(config->platlibdir); - CLEAR(config->sys_path_0); CLEAR(config->filesystem_encoding); CLEAR(config->filesystem_errors); @@ -994,7 +993,6 @@ _PyConfig_Copy(PyConfig *config, const PyConfig *config2) COPY_WSTR_ATTR(exec_prefix); COPY_WSTR_ATTR(base_exec_prefix); COPY_WSTR_ATTR(platlibdir); - COPY_WSTR_ATTR(sys_path_0); COPY_ATTR(site_import); COPY_ATTR(bytes_warning); @@ -1104,7 +1102,6 @@ _PyConfig_AsDict(const PyConfig *config) SET_ITEM_WSTR(exec_prefix); SET_ITEM_WSTR(base_exec_prefix); SET_ITEM_WSTR(platlibdir); - SET_ITEM_WSTR(sys_path_0); SET_ITEM_INT(site_import); SET_ITEM_INT(bytes_warning); SET_ITEM_INT(warn_default_encoding); @@ -1406,7 +1403,6 @@ _PyConfig_FromDict(PyConfig *config, PyObject *dict) GET_WSTR_OPT(pythonpath_env); GET_WSTR_OPT(home); GET_WSTR(platlibdir); - GET_WSTR(sys_path_0); // Path configuration output GET_UINT(module_search_paths_set); @@ -3169,7 +3165,6 @@ _Py_DumpPathConfig(PyThreadState *tstate) PySys_WriteStderr(" import site = %i\n", config->site_import); PySys_WriteStderr(" is in build tree = %i\n", config->_is_python_build); DUMP_CONFIG("stdlib dir", stdlib_dir); - DUMP_CONFIG("sys.path[0]", sys_path_0); #undef DUMP_CONFIG #define DUMP_SYS(NAME) \ diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index ba4fd5c1c1c1e5..ee0eb79e0151ee 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1202,8 +1202,9 @@ init_interp_main(PyThreadState *tstate) if (!is_main_interp) { // The main interpreter is handled in Py_Main(), for now. - if (config->sys_path_0 != NULL) { - PyObject *path0 = PyUnicode_FromWideChar(config->sys_path_0, -1); + wchar_t *sys_path_0 = interp->runtime->sys_path_0; + if (sys_path_0 != NULL) { + PyObject *path0 = PyUnicode_FromWideChar(sys_path_0, -1); if (path0 == NULL) { return _PyStatus_ERR("can't initialize sys.path[0]"); } diff --git a/Python/pystate.c b/Python/pystate.c index d5670cba7ed514..b9d098c379c159 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -524,6 +524,10 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime) } #undef FREE_LOCK + if (runtime->sys_path_0 != NULL) { + PyMem_RawFree(runtime->sys_path_0); + runtime->sys_path_0 = NULL; + } PyMem_SetAllocator(PYMEM_DOMAIN_RAW, &old_alloc); } From 9a3588e11e2cd0ec141bbfe19d60c62c1e0bdaaa Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 3 Oct 2023 09:20:48 -0600 Subject: [PATCH 8/9] [3.12] gh-109860: Use a New Thread State When Switching Interpreters, When Necessary (gh-110245) In a few places we switch to another interpreter without knowing if it has a thread state associated with the current thread. For the main interpreter there wasn't much of a problem, but for subinterpreters we were *mostly* okay re-using the tstate created with the interpreter (located via PyInterpreterState_ThreadHead()). There was a good chance that tstate wasn't actually in use by another thread. However, there are no guarantees of that. Furthermore, re-using an already used tstate is currently fragile. To address this, now we create a new thread state in each of those places and use it. One consequence of this change is that PyInterpreterState_ThreadHead() may not return NULL (though that won't happen for the main interpreter). (cherry-picked from commit f5198b09e16bca1886f8245fa88203d07d51ec11) --- Include/cpython/pystate.h | 9 ++ Include/internal/pycore_pystate.h | 5 +- Include/internal/pycore_runtime_init.h | 1 + Lib/threading.py | 7 +- Modules/_threadmodule.c | 2 +- Modules/_xxsubinterpretersmodule.c | 111 +++++++++++++++---------- Python/pylifecycle.c | 6 +- Python/pystate.c | 78 +++++++++++++---- 8 files changed, 151 insertions(+), 68 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index 628f2e0996e469..a1130381886ca7 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -143,6 +143,15 @@ struct _ts { /* padding to align to 4 bytes */ unsigned int :24; } _status; +#ifdef Py_BUILD_CORE +# define _PyThreadState_WHENCE_NOTSET -1 +# define _PyThreadState_WHENCE_UNKNOWN 0 +# define _PyThreadState_WHENCE_INTERP 1 +# define _PyThreadState_WHENCE_THREADING 2 +# define _PyThreadState_WHENCE_GILSTATE 3 +# define _PyThreadState_WHENCE_EXEC 4 +#endif + int _whence; int py_recursion_remaining; int py_recursion_limit; diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 97c5239ed8db9e..08fe216a50f314 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -44,6 +44,7 @@ _Py_IsMainInterpreterFinalizing(PyInterpreterState *interp) PyAPI_FUNC(int) _PyInterpreterState_SetRunningMain(PyInterpreterState *); PyAPI_FUNC(void) _PyInterpreterState_SetNotRunningMain(PyInterpreterState *); PyAPI_FUNC(int) _PyInterpreterState_IsRunningMain(PyInterpreterState *); +PyAPI_FUNC(int) _PyInterpreterState_FailIfRunningMain(PyInterpreterState *); static inline const PyConfig * @@ -132,7 +133,9 @@ static inline PyInterpreterState* _PyInterpreterState_GET(void) { // PyThreadState functions -PyAPI_FUNC(PyThreadState *) _PyThreadState_New(PyInterpreterState *interp); +PyAPI_FUNC(PyThreadState *) _PyThreadState_New( + PyInterpreterState *interp, + int whence); PyAPI_FUNC(void) _PyThreadState_Bind(PyThreadState *tstate); // We keep this around exclusively for stable ABI compatibility. PyAPI_FUNC(void) _PyThreadState_Init( diff --git a/Include/internal/pycore_runtime_init.h b/Include/internal/pycore_runtime_init.h index 7aace9f86119c4..035e0f595b92d8 100644 --- a/Include/internal/pycore_runtime_init.h +++ b/Include/internal/pycore_runtime_init.h @@ -127,6 +127,7 @@ extern PyTypeObject _PyExc_MemoryError; #define _PyThreadState_INIT \ { \ + ._whence = _PyThreadState_WHENCE_NOTSET, \ .py_recursion_limit = Py_DEFAULT_RECURSION_LIMIT, \ .context_ver = 1, \ } diff --git a/Lib/threading.py b/Lib/threading.py index 624e7ed8f07c8f..c881f1107014c2 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -1585,8 +1585,11 @@ def _shutdown(): # The main thread isn't finished yet, so its thread state lock can't # have been released. assert tlock is not None - assert tlock.locked() - tlock.release() + if tlock.locked(): + # It should have been released already by + # _PyInterpreterState_SetNotRunningMain(), but there may be + # embedders that aren't calling that yet. + tlock.release() _main_thread._stop() else: # bpo-1596321: _shutdown() must be called in the main thread. diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 568fe8375d1eb4..598c096d8e9ee4 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1203,7 +1203,7 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) if (boot == NULL) { return PyErr_NoMemory(); } - boot->tstate = _PyThreadState_New(interp); + boot->tstate = _PyThreadState_New(interp, _PyThreadState_WHENCE_THREADING); if (boot->tstate == NULL) { PyMem_RawFree(boot); if (!PyErr_Occurred()) { diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c index c0958c65dd0d65..2006e44c79ea22 100644 --- a/Modules/_xxsubinterpretersmodule.c +++ b/Modules/_xxsubinterpretersmodule.c @@ -243,6 +243,11 @@ _sharedns_apply(_sharedns *shared, PyObject *ns) // of the exception in the calling interpreter. typedef struct _sharedexception { + PyInterpreterState *interp; +#define ERR_NOT_SET 0 +#define ERR_NO_MEMORY 1 +#define ERR_ALREADY_RUNNING 2 + int code; const char *name; const char *msg; } _sharedexception; @@ -264,14 +269,26 @@ _sharedexception_clear(_sharedexception *exc) } static const char * -_sharedexception_bind(PyObject *exc, _sharedexception *sharedexc) +_sharedexception_bind(PyObject *exc, int code, _sharedexception *sharedexc) { + if (sharedexc->interp == NULL) { + sharedexc->interp = PyInterpreterState_Get(); + } + + if (code != ERR_NOT_SET) { + assert(exc == NULL); + assert(code > 0); + sharedexc->code = code; + return NULL; + } + assert(exc != NULL); const char *failure = NULL; PyObject *nameobj = PyUnicode_FromFormat("%S", Py_TYPE(exc)); if (nameobj == NULL) { failure = "unable to format exception type name"; + code = ERR_NO_MEMORY; goto error; } sharedexc->name = _copy_raw_string(nameobj); @@ -282,6 +299,7 @@ _sharedexception_bind(PyObject *exc, _sharedexception *sharedexc) } else { failure = "unable to encode and copy exception type name"; } + code = ERR_NO_MEMORY; goto error; } @@ -289,6 +307,7 @@ _sharedexception_bind(PyObject *exc, _sharedexception *sharedexc) PyObject *msgobj = PyUnicode_FromFormat("%S", exc); if (msgobj == NULL) { failure = "unable to format exception message"; + code = ERR_NO_MEMORY; goto error; } sharedexc->msg = _copy_raw_string(msgobj); @@ -299,6 +318,7 @@ _sharedexception_bind(PyObject *exc, _sharedexception *sharedexc) } else { failure = "unable to encode and copy exception message"; } + code = ERR_NO_MEMORY; goto error; } } @@ -309,7 +329,10 @@ _sharedexception_bind(PyObject *exc, _sharedexception *sharedexc) assert(failure != NULL); PyErr_Clear(); _sharedexception_clear(sharedexc); - *sharedexc = no_exception; + *sharedexc = (_sharedexception){ + .interp = sharedexc->interp, + .code = code, + }; return failure; } @@ -317,6 +340,7 @@ static void _sharedexception_apply(_sharedexception *exc, PyObject *wrapperclass) { if (exc->name != NULL) { + assert(exc->code == ERR_NOT_SET); if (exc->msg != NULL) { PyErr_Format(wrapperclass, "%s: %s", exc->name, exc->msg); } @@ -325,9 +349,19 @@ _sharedexception_apply(_sharedexception *exc, PyObject *wrapperclass) } } else if (exc->msg != NULL) { + assert(exc->code == ERR_NOT_SET); PyErr_SetString(wrapperclass, exc->msg); } + else if (exc->code == ERR_NO_MEMORY) { + PyErr_NoMemory(); + } + else if (exc->code == ERR_ALREADY_RUNNING) { + assert(exc->interp != NULL); + assert(_PyInterpreterState_IsRunningMain(exc->interp)); + _PyInterpreterState_FailIfRunningMain(exc->interp); + } else { + assert(exc->code == ERR_NOT_SET); PyErr_SetNone(wrapperclass); } } @@ -363,9 +397,16 @@ static int _run_script(PyInterpreterState *interp, const char *codestr, _sharedns *shared, _sharedexception *sharedexc) { + int errcode = ERR_NOT_SET; + if (_PyInterpreterState_SetRunningMain(interp) < 0) { - // We skip going through the shared exception. - return -1; + assert(PyErr_Occurred()); + // 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. + PyErr_Clear(); + errcode = ERR_ALREADY_RUNNING; + goto error; } PyObject *excval = NULL; @@ -404,16 +445,17 @@ _run_script(PyInterpreterState *interp, const char *codestr, error: excval = PyErr_GetRaisedException(); - const char *failure = _sharedexception_bind(excval, sharedexc); + const char *failure = _sharedexception_bind(excval, errcode, sharedexc); if (failure != NULL) { fprintf(stderr, "RunFailedError: script raised an uncaught exception (%s)", failure); - PyErr_Clear(); } Py_XDECREF(excval); + if (errcode != ERR_ALREADY_RUNNING) { + _PyInterpreterState_SetNotRunningMain(interp); + } assert(!PyErr_Occurred()); - _PyInterpreterState_SetNotRunningMain(interp); return -1; } @@ -422,6 +464,7 @@ _run_script_in_interpreter(PyObject *mod, PyInterpreterState *interp, const char *codestr, PyObject *shareables) { module_state *state = get_module_state(mod); + assert(state != NULL); _sharedns *shared = _get_shared_ns(shareables); if (shared == NULL && PyErr_Occurred()) { @@ -430,50 +473,30 @@ _run_script_in_interpreter(PyObject *mod, PyInterpreterState *interp, // Switch to interpreter. PyThreadState *save_tstate = NULL; + PyThreadState *tstate = NULL; if (interp != PyInterpreterState_Get()) { - // XXX gh-109860: Using the "head" thread isn't strictly correct. - PyThreadState *tstate = PyInterpreterState_ThreadHead(interp); - assert(tstate != NULL); - // Hack (until gh-109860): The interpreter's initial thread state - // is least likely to break. - while(tstate->next != NULL) { - tstate = tstate->next; - } - // We must do this check before switching interpreters, so any - // exception gets raised in the right one. - // XXX gh-109860: Drop this redundant check once we stop - // re-using tstates that might already be in use. - if (_PyInterpreterState_IsRunningMain(interp)) { - PyErr_SetString(PyExc_RuntimeError, - "interpreter already running"); - if (shared != NULL) { - _sharedns_free(shared); - } - return -1; - } + tstate = PyThreadState_New(interp); + tstate->_whence = _PyThreadState_WHENCE_EXEC; // XXX Possible GILState issues? save_tstate = PyThreadState_Swap(tstate); } // Run the script. - _sharedexception exc = {NULL, NULL}; + _sharedexception exc = (_sharedexception){ .interp = interp }; int result = _run_script(interp, codestr, shared, &exc); // Switch back. if (save_tstate != NULL) { + PyThreadState_Clear(tstate); PyThreadState_Swap(save_tstate); + PyThreadState_Delete(tstate); } // Propagate any exception out to the caller. - if (exc.name != NULL) { - assert(state != NULL); + if (result < 0) { + assert(!PyErr_Occurred()); _sharedexception_apply(&exc, state->RunFailedError); - } - else if (result != 0) { - if (!PyErr_Occurred()) { - // We were unable to allocate a shared exception. - PyErr_NoMemory(); - } + assert(PyErr_Occurred()); } if (shared != NULL) { @@ -503,6 +526,7 @@ interp_create(PyObject *self, PyObject *args, PyObject *kwds) const PyInterpreterConfig config = isolated ? (PyInterpreterConfig)_PyInterpreterConfig_INIT : (PyInterpreterConfig)_PyInterpreterConfig_LEGACY_INIT; + // XXX Possible GILState issues? PyThreadState *tstate = NULL; PyStatus status = Py_NewInterpreterFromConfig(&tstate, &config); @@ -518,6 +542,7 @@ interp_create(PyObject *self, PyObject *args, PyObject *kwds) return NULL; } assert(tstate != NULL); + PyInterpreterState *interp = PyThreadState_GetInterpreter(tstate); PyObject *idobj = _PyInterpreterState_GetIDObject(interp); if (idobj == NULL) { @@ -527,6 +552,10 @@ interp_create(PyObject *self, PyObject *args, PyObject *kwds) PyThreadState_Swap(save_tstate); return NULL; } + + PyThreadState_Clear(tstate); + PyThreadState_Delete(tstate); + _PyInterpreterState_RequireIDRef(interp, 1); return idobj; } @@ -574,14 +603,8 @@ interp_destroy(PyObject *self, PyObject *args, PyObject *kwds) } // Destroy the interpreter. - // XXX gh-109860: Using the "head" thread isn't strictly correct. - PyThreadState *tstate = PyInterpreterState_ThreadHead(interp); - assert(tstate != NULL); - // Hack (until gh-109860): The interpreter's initial thread state - // is least likely to break. - while(tstate->next != NULL) { - tstate = tstate->next; - } + PyThreadState *tstate = PyThreadState_New(interp); + tstate->_whence = _PyThreadState_WHENCE_INTERP; // XXX Possible GILState issues? PyThreadState *save_tstate = PyThreadState_Swap(tstate); Py_EndInterpreter(tstate); diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index ee0eb79e0151ee..8df4e0d2b841e3 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -650,7 +650,8 @@ pycore_create_interpreter(_PyRuntimeState *runtime, return status; } - PyThreadState *tstate = _PyThreadState_New(interp); + PyThreadState *tstate = _PyThreadState_New(interp, + _PyThreadState_WHENCE_INTERP); if (tstate == NULL) { return _PyStatus_ERR("can't make first thread"); } @@ -2051,7 +2052,8 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) return _PyStatus_OK(); } - PyThreadState *tstate = _PyThreadState_New(interp); + PyThreadState *tstate = _PyThreadState_New(interp, + _PyThreadState_WHENCE_INTERP); if (tstate == NULL) { PyInterpreterState_Delete(interp); *tstate_p = NULL; diff --git a/Python/pystate.c b/Python/pystate.c index b9d098c379c159..24ed0498fee499 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1052,9 +1052,7 @@ _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime) int _PyInterpreterState_SetRunningMain(PyInterpreterState *interp) { - if (interp->threads.main != NULL) { - PyErr_SetString(PyExc_RuntimeError, - "interpreter already running"); + if (_PyInterpreterState_FailIfRunningMain(interp) < 0) { return -1; } PyThreadState *tstate = current_fast_get(&_PyRuntime); @@ -1071,7 +1069,20 @@ _PyInterpreterState_SetRunningMain(PyInterpreterState *interp) void _PyInterpreterState_SetNotRunningMain(PyInterpreterState *interp) { - assert(interp->threads.main == current_fast_get(&_PyRuntime)); + PyThreadState *tstate = interp->threads.main; + assert(tstate == current_fast_get(&_PyRuntime)); + + if (tstate->on_delete != NULL) { + // The threading module was imported for the first time in this + // thread, so it was set as threading._main_thread. (See gh-75698.) + // The thread has finished running the Python program so we mark + // the thread object as finished. + assert(tstate->_whence != _PyThreadState_WHENCE_THREADING); + tstate->on_delete(tstate->on_delete_data); + tstate->on_delete = NULL; + tstate->on_delete_data = NULL; + } + interp->threads.main = NULL; } @@ -1081,6 +1092,17 @@ _PyInterpreterState_IsRunningMain(PyInterpreterState *interp) return (interp->threads.main != NULL); } +int +_PyInterpreterState_FailIfRunningMain(PyInterpreterState *interp) +{ + if (interp->threads.main != NULL) { + PyErr_SetString(PyExc_RuntimeError, + "interpreter already running"); + return -1; + } + return 0; +} + //---------- // accessors @@ -1141,8 +1163,10 @@ _PyInterpreterState_IDDecref(PyInterpreterState *interp) PyThread_release_lock(interp->id_mutex); if (refcount == 0 && interp->requires_idref) { - // XXX Using the "head" thread isn't strictly correct. - PyThreadState *tstate = PyInterpreterState_ThreadHead(interp); + PyThreadState *tstate = _PyThreadState_New(interp, + _PyThreadState_WHENCE_INTERP); + _PyThreadState_Bind(tstate); + // XXX Possible GILState issues? PyThreadState *save_tstate = _PyThreadState_Swap(runtime, tstate); Py_EndInterpreter(tstate); @@ -1296,7 +1320,14 @@ free_threadstate(PyThreadState *tstate) { // The initial thread state of the interpreter is allocated // as part of the interpreter state so should not be freed. - if (tstate != &tstate->interp->_initial_thread) { + if (tstate == &tstate->interp->_initial_thread) { + // Restore to _PyThreadState_INIT. + tstate = &tstate->interp->_initial_thread; + memcpy(tstate, + &initial._main_interpreter._initial_thread, + sizeof(*tstate)); + } + else { PyMem_RawFree(tstate); } } @@ -1311,7 +1342,7 @@ free_threadstate(PyThreadState *tstate) static void init_threadstate(PyThreadState *tstate, - PyInterpreterState *interp, uint64_t id) + PyInterpreterState *interp, uint64_t id, int whence) { if (tstate->_status.initialized) { Py_FatalError("thread state already initialized"); @@ -1324,6 +1355,10 @@ init_threadstate(PyThreadState *tstate, assert(tstate->next == NULL); assert(tstate->prev == NULL); + assert(tstate->_whence == _PyThreadState_WHENCE_NOTSET); + assert(whence >= 0 && whence <= _PyThreadState_WHENCE_EXEC); + tstate->_whence = whence; + assert(id > 0); tstate->id = id; @@ -1353,8 +1388,6 @@ add_threadstate(PyInterpreterState *interp, PyThreadState *tstate, PyThreadState *next) { assert(interp->threads.head != tstate); - assert((next != NULL && tstate->id != 1) || - (next == NULL && tstate->id == 1)); if (next != NULL) { assert(next->prev == NULL || next->prev == tstate); next->prev = tstate; @@ -1365,7 +1398,7 @@ add_threadstate(PyInterpreterState *interp, PyThreadState *tstate, } static PyThreadState * -new_threadstate(PyInterpreterState *interp) +new_threadstate(PyInterpreterState *interp, int whence) { PyThreadState *tstate; _PyRuntimeState *runtime = interp->runtime; @@ -1388,10 +1421,10 @@ new_threadstate(PyInterpreterState *interp) PyThreadState *old_head = interp->threads.head; if (old_head == NULL) { // It's the interpreter's initial thread state. - assert(id == 1); used_newtstate = 0; tstate = &interp->_initial_thread; } + // XXX Re-use interp->_initial_thread if not in use? else { // Every valid interpreter must have at least one thread. assert(id > 1); @@ -1404,7 +1437,7 @@ new_threadstate(PyInterpreterState *interp) sizeof(*tstate)); } - init_threadstate(tstate, interp, id); + init_threadstate(tstate, interp, id, whence); add_threadstate(interp, tstate, old_head); HEAD_UNLOCK(runtime); @@ -1418,7 +1451,8 @@ new_threadstate(PyInterpreterState *interp) PyThreadState * PyThreadState_New(PyInterpreterState *interp) { - PyThreadState *tstate = new_threadstate(interp); + PyThreadState *tstate = new_threadstate(interp, + _PyThreadState_WHENCE_UNKNOWN); if (tstate) { bind_tstate(tstate); // This makes sure there's a gilstate tstate bound @@ -1432,16 +1466,16 @@ PyThreadState_New(PyInterpreterState *interp) // This must be followed by a call to _PyThreadState_Bind(); PyThreadState * -_PyThreadState_New(PyInterpreterState *interp) +_PyThreadState_New(PyInterpreterState *interp, int whence) { - return new_threadstate(interp); + return new_threadstate(interp, whence); } // We keep this for stable ABI compabibility. PyThreadState * _PyThreadState_Prealloc(PyInterpreterState *interp) { - return _PyThreadState_New(interp); + return _PyThreadState_New(interp, _PyThreadState_WHENCE_UNKNOWN); } // We keep this around for (accidental) stable ABI compatibility. @@ -1538,6 +1572,12 @@ PyThreadState_Clear(PyThreadState *tstate) Py_CLEAR(tstate->context); if (tstate->on_delete != NULL) { + // For the "main" thread of each interpreter, this is meant + // to be done in _PyInterpreterState_SetNotRunningMain(). + // That leaves threads created by the threading module, + // and any threads killed by forking. + // However, we also accommodate "main" threads that still + // don't call _PyInterpreterState_SetNotRunningMain() yet. tstate->on_delete(tstate->on_delete_data); } @@ -2198,7 +2238,9 @@ PyGILState_Ensure(void) int has_gil; if (tcur == NULL) { /* Create a new Python thread state for this thread */ - tcur = new_threadstate(runtime->gilstate.autoInterpreterState); + // XXX Use PyInterpreterState_EnsureThreadState()? + tcur = new_threadstate(runtime->gilstate.autoInterpreterState, + _PyThreadState_WHENCE_GILSTATE); if (tcur == NULL) { Py_FatalError("Couldn't create thread-state for new thread"); } From 4fb7f0fc698c86036d05319b4c3aefc71396c0fa Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 11 Oct 2023 06:05:43 -0600 Subject: [PATCH 9/9] Drop PyThreadState.whence. --- Include/cpython/pystate.h | 9 --------- Include/internal/pycore_pystate.h | 4 +--- Include/internal/pycore_runtime_init.h | 1 - Modules/_threadmodule.c | 2 +- Modules/_xxsubinterpretersmodule.c | 2 -- Python/pylifecycle.c | 6 ++---- Python/pystate.c | 26 +++++++++----------------- 7 files changed, 13 insertions(+), 37 deletions(-) diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h index a1130381886ca7..628f2e0996e469 100644 --- a/Include/cpython/pystate.h +++ b/Include/cpython/pystate.h @@ -143,15 +143,6 @@ struct _ts { /* padding to align to 4 bytes */ unsigned int :24; } _status; -#ifdef Py_BUILD_CORE -# define _PyThreadState_WHENCE_NOTSET -1 -# define _PyThreadState_WHENCE_UNKNOWN 0 -# define _PyThreadState_WHENCE_INTERP 1 -# define _PyThreadState_WHENCE_THREADING 2 -# define _PyThreadState_WHENCE_GILSTATE 3 -# define _PyThreadState_WHENCE_EXEC 4 -#endif - int _whence; int py_recursion_remaining; int py_recursion_limit; diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 08fe216a50f314..9ae82372c7203c 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -133,9 +133,7 @@ static inline PyInterpreterState* _PyInterpreterState_GET(void) { // PyThreadState functions -PyAPI_FUNC(PyThreadState *) _PyThreadState_New( - PyInterpreterState *interp, - int whence); +PyAPI_FUNC(PyThreadState *) _PyThreadState_New(PyInterpreterState *interp); PyAPI_FUNC(void) _PyThreadState_Bind(PyThreadState *tstate); // We keep this around exclusively for stable ABI compatibility. PyAPI_FUNC(void) _PyThreadState_Init( diff --git a/Include/internal/pycore_runtime_init.h b/Include/internal/pycore_runtime_init.h index 035e0f595b92d8..7aace9f86119c4 100644 --- a/Include/internal/pycore_runtime_init.h +++ b/Include/internal/pycore_runtime_init.h @@ -127,7 +127,6 @@ extern PyTypeObject _PyExc_MemoryError; #define _PyThreadState_INIT \ { \ - ._whence = _PyThreadState_WHENCE_NOTSET, \ .py_recursion_limit = Py_DEFAULT_RECURSION_LIMIT, \ .context_ver = 1, \ } diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 598c096d8e9ee4..568fe8375d1eb4 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1203,7 +1203,7 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) if (boot == NULL) { return PyErr_NoMemory(); } - boot->tstate = _PyThreadState_New(interp, _PyThreadState_WHENCE_THREADING); + boot->tstate = _PyThreadState_New(interp); if (boot->tstate == NULL) { PyMem_RawFree(boot); if (!PyErr_Occurred()) { diff --git a/Modules/_xxsubinterpretersmodule.c b/Modules/_xxsubinterpretersmodule.c index 2006e44c79ea22..bb7a76912330cb 100644 --- a/Modules/_xxsubinterpretersmodule.c +++ b/Modules/_xxsubinterpretersmodule.c @@ -476,7 +476,6 @@ _run_script_in_interpreter(PyObject *mod, PyInterpreterState *interp, PyThreadState *tstate = NULL; if (interp != PyInterpreterState_Get()) { tstate = PyThreadState_New(interp); - tstate->_whence = _PyThreadState_WHENCE_EXEC; // XXX Possible GILState issues? save_tstate = PyThreadState_Swap(tstate); } @@ -604,7 +603,6 @@ interp_destroy(PyObject *self, PyObject *args, PyObject *kwds) // Destroy the interpreter. PyThreadState *tstate = PyThreadState_New(interp); - tstate->_whence = _PyThreadState_WHENCE_INTERP; // XXX Possible GILState issues? PyThreadState *save_tstate = PyThreadState_Swap(tstate); Py_EndInterpreter(tstate); diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 8df4e0d2b841e3..ee0eb79e0151ee 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -650,8 +650,7 @@ pycore_create_interpreter(_PyRuntimeState *runtime, return status; } - PyThreadState *tstate = _PyThreadState_New(interp, - _PyThreadState_WHENCE_INTERP); + PyThreadState *tstate = _PyThreadState_New(interp); if (tstate == NULL) { return _PyStatus_ERR("can't make first thread"); } @@ -2052,8 +2051,7 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) return _PyStatus_OK(); } - PyThreadState *tstate = _PyThreadState_New(interp, - _PyThreadState_WHENCE_INTERP); + PyThreadState *tstate = _PyThreadState_New(interp); if (tstate == NULL) { PyInterpreterState_Delete(interp); *tstate_p = NULL; diff --git a/Python/pystate.c b/Python/pystate.c index 24ed0498fee499..d512f2aafc8f61 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1077,7 +1077,6 @@ _PyInterpreterState_SetNotRunningMain(PyInterpreterState *interp) // thread, so it was set as threading._main_thread. (See gh-75698.) // The thread has finished running the Python program so we mark // the thread object as finished. - assert(tstate->_whence != _PyThreadState_WHENCE_THREADING); tstate->on_delete(tstate->on_delete_data); tstate->on_delete = NULL; tstate->on_delete_data = NULL; @@ -1163,8 +1162,7 @@ _PyInterpreterState_IDDecref(PyInterpreterState *interp) PyThread_release_lock(interp->id_mutex); if (refcount == 0 && interp->requires_idref) { - PyThreadState *tstate = _PyThreadState_New(interp, - _PyThreadState_WHENCE_INTERP); + PyThreadState *tstate = _PyThreadState_New(interp); _PyThreadState_Bind(tstate); // XXX Possible GILState issues? @@ -1342,7 +1340,7 @@ free_threadstate(PyThreadState *tstate) static void init_threadstate(PyThreadState *tstate, - PyInterpreterState *interp, uint64_t id, int whence) + PyInterpreterState *interp, uint64_t id) { if (tstate->_status.initialized) { Py_FatalError("thread state already initialized"); @@ -1355,10 +1353,6 @@ init_threadstate(PyThreadState *tstate, assert(tstate->next == NULL); assert(tstate->prev == NULL); - assert(tstate->_whence == _PyThreadState_WHENCE_NOTSET); - assert(whence >= 0 && whence <= _PyThreadState_WHENCE_EXEC); - tstate->_whence = whence; - assert(id > 0); tstate->id = id; @@ -1398,7 +1392,7 @@ add_threadstate(PyInterpreterState *interp, PyThreadState *tstate, } static PyThreadState * -new_threadstate(PyInterpreterState *interp, int whence) +new_threadstate(PyInterpreterState *interp) { PyThreadState *tstate; _PyRuntimeState *runtime = interp->runtime; @@ -1437,7 +1431,7 @@ new_threadstate(PyInterpreterState *interp, int whence) sizeof(*tstate)); } - init_threadstate(tstate, interp, id, whence); + init_threadstate(tstate, interp, id); add_threadstate(interp, tstate, old_head); HEAD_UNLOCK(runtime); @@ -1451,8 +1445,7 @@ new_threadstate(PyInterpreterState *interp, int whence) PyThreadState * PyThreadState_New(PyInterpreterState *interp) { - PyThreadState *tstate = new_threadstate(interp, - _PyThreadState_WHENCE_UNKNOWN); + PyThreadState *tstate = new_threadstate(interp); if (tstate) { bind_tstate(tstate); // This makes sure there's a gilstate tstate bound @@ -1466,16 +1459,16 @@ PyThreadState_New(PyInterpreterState *interp) // This must be followed by a call to _PyThreadState_Bind(); PyThreadState * -_PyThreadState_New(PyInterpreterState *interp, int whence) +_PyThreadState_New(PyInterpreterState *interp) { - return new_threadstate(interp, whence); + return new_threadstate(interp); } // We keep this for stable ABI compabibility. PyThreadState * _PyThreadState_Prealloc(PyInterpreterState *interp) { - return _PyThreadState_New(interp, _PyThreadState_WHENCE_UNKNOWN); + return _PyThreadState_New(interp); } // We keep this around for (accidental) stable ABI compatibility. @@ -2239,8 +2232,7 @@ PyGILState_Ensure(void) if (tcur == NULL) { /* Create a new Python thread state for this thread */ // XXX Use PyInterpreterState_EnsureThreadState()? - tcur = new_threadstate(runtime->gilstate.autoInterpreterState, - _PyThreadState_WHENCE_GILSTATE); + tcur = new_threadstate(runtime->gilstate.autoInterpreterState); if (tcur == NULL) { Py_FatalError("Couldn't create thread-state for new thread"); }