From 0122b4d7c92855e97912cf827dd81d836725c9a4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 27 Nov 2023 19:01:05 -0700 Subject: [PATCH 1/2] [3.12] gh-105716: Support Background Threads in Subinterpreters Consistently (gh-109921) (gh-110707) 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) --- Doc/data/python3.12.abi | 228 ++++++++++-------- 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 +++ 11 files changed, 385 insertions(+), 150 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/Doc/data/python3.12.abi b/Doc/data/python3.12.abi index 6abf43ca6a458b..b3a07fdf9bd19b 100644 --- a/Doc/data/python3.12.abi +++ b/Doc/data/python3.12.abi @@ -1124,11 +1124,14 @@ + + + @@ -1707,7 +1710,7 @@ - + @@ -2534,11 +2537,11 @@ - + - + @@ -3926,17 +3929,17 @@ - + - - - + + + - - - + + + @@ -7563,19 +7566,19 @@ - + - + - + - + @@ -16067,7 +16070,7 @@ - + @@ -16276,10 +16279,13 @@ - + - + + + + @@ -16296,18 +16302,18 @@ - + - + - + - + - + @@ -16590,7 +16596,7 @@ - + @@ -17469,7 +17475,7 @@ - + @@ -22073,11 +22079,11 @@ - + - + @@ -24951,7 +24957,7 @@ - + @@ -25140,15 +25146,15 @@ - + - + - + @@ -25495,7 +25501,7 @@ - + @@ -25514,142 +25520,154 @@ - - + + + + + + + + + + + + + + - - - + + + - - + + - - + + - - + + - - + + - - + + - - + + - + - - + + - - + + - - + + - - - + + + - - - + + + - + - + - + - + - - - - - - + + + + + + - - - - - - + + + + + + - - - + + + - - + + - - - + + + - - + + - - + + - - + + - - - + + + - - + + - - + + - - + + - - - + + + - - + + diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 2fbb9f1aec6154..b5d947cbb6ca9d 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -194,6 +194,8 @@ struct _is { struct _Py_interp_cached_objects cached_objects; struct _Py_interp_static_objects static_objects; + /* The thread currently executing in the __main__ module, if any. */ + PyThreadState *threads_main; /* The ID of the OS thread in which we are finalizing. We use _Py_atomic_address instead of adding a new _Py_atomic_ulong. */ _Py_atomic_address _finalizing_id; diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index e4186b3a921428..fba08ae55235d9 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -44,6 +44,11 @@ _Py_IsMainInterpreterFinalizing(PyInterpreterState *interp) interp == &_PyRuntime._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 38cf4b6fe737dd..cc4f4003b0359d 100644 --- a/Lib/test/test_interpreters.py +++ b/Lib/test/test_interpreters.py @@ -261,6 +261,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""" @@ -288,6 +298,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): @@ -389,6 +424,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): @@ -465,6 +531,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 5d4f1b97a25eba..c0958c65dd0d65 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" @@ -353,41 +360,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) { @@ -417,6 +397,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; @@ -432,6 +413,7 @@ _run_script(PyInterpreterState *interp, const char *codestr, } Py_XDECREF(excval); assert(!PyErr_Occurred()); + _PyInterpreterState_SetNotRunningMain(interp); return -1; } @@ -439,9 +421,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); @@ -452,8 +431,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); } @@ -473,8 +470,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) { @@ -569,12 +568,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); @@ -743,11 +750,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; @@ -758,6 +761,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 b9de2ecda92811..1b189b456162e6 100644 --- a/Modules/main.c +++ b/Modules/main.c @@ -613,6 +613,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); } @@ -636,6 +639,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 0430454432be7b..534e77fe2cbf55 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1049,6 +1049,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 //---------- @@ -2757,6 +2790,10 @@ _register_builtins_for_crossinterpreter_data(struct _xidregistry *xidregistry) } +/*************/ +/* Other API */ +/*************/ + _PyFrameEvalFunction _PyInterpreterState_GetEvalFrameFunc(PyInterpreterState *interp) { From 1e1a30f9f49ed5104a4be194d094b13703bcc0de Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 27 Nov 2023 19:36:29 -0700 Subject: [PATCH 2/2] [3.12] gh-110310: Add a Per-Interpreter XID Registry for Heap Types (gh-110311) (gh-110714) We do the following: * add a per-interpreter XID registry (PyInterpreterState.xidregistry) * put heap types there (keep static types in _PyRuntimeState.xidregistry) * clear the registries during interpreter/runtime finalization * avoid duplicate entries in the registry (when _PyCrossInterpreterData_RegisterClass() is called more than once for a type) * use Py_TYPE() instead of PyObject_Type() in _PyCrossInterpreterData_Lookup() The per-interpreter registry helps preserve isolation between interpreters. This is important when heap types are registered, which is something we haven't been doing yet but I will likely do soon. (cherry-picked from commit 80dc39e1dc2abc809f448cba5d2c5b9c1c631e11) --- Doc/data/python3.12.abi | 705 +++++++++++++++--------------- Include/internal/pycore_interp.h | 43 +- Include/internal/pycore_runtime.h | 5 +- Python/pystate.c | 158 +++++-- 4 files changed, 506 insertions(+), 405 deletions(-) diff --git a/Doc/data/python3.12.abi b/Doc/data/python3.12.abi index b3a07fdf9bd19b..ab5190f6966c02 100644 --- a/Doc/data/python3.12.abi +++ b/Doc/data/python3.12.abi @@ -1710,7 +1710,7 @@ - + @@ -2544,7 +2544,7 @@ - + @@ -7566,19 +7566,19 @@ - + - + - + - + @@ -10912,10 +10912,10 @@ - + - + @@ -13496,10 +13496,10 @@ - + - + @@ -13528,7 +13528,7 @@ - + @@ -13543,7 +13543,7 @@ - + @@ -13564,13 +13564,13 @@ - + - + - + @@ -13606,7 +13606,7 @@ - + @@ -13707,7 +13707,7 @@ - + @@ -14307,7 +14307,7 @@ - + @@ -14367,7 +14367,7 @@ - + @@ -14421,7 +14421,7 @@ - + @@ -14580,7 +14580,7 @@ - + @@ -14664,7 +14664,7 @@ - + @@ -15147,7 +15147,7 @@ - + @@ -15282,7 +15282,7 @@ - + @@ -15366,7 +15366,7 @@ - + @@ -15438,7 +15438,7 @@ - + @@ -15744,7 +15744,7 @@ - + @@ -15881,7 +15881,7 @@ - + @@ -16070,253 +16070,270 @@ - + - + - + - + - + - + - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - - - - + - - - - - - - - - + - - + + - - + + - + - + - + - + - + - + @@ -16324,68 +16341,68 @@ - + - + - - - + + + - + - + - + - + - + - + - + - + - + - + - + - - + + - + - + - + - + @@ -16394,17 +16411,17 @@ - + - + - + @@ -16428,34 +16445,34 @@ - + - + - + - - + + - + - + - - + + - + @@ -16464,7 +16481,7 @@ - + @@ -16472,42 +16489,42 @@ - + - + - + - + - + - + - + - + - + - + - + - + @@ -16515,22 +16532,22 @@ - - + + - + - + - + @@ -16541,34 +16558,34 @@ - + - + - + - + - + - + - + - + - + - + @@ -16576,7 +16593,7 @@ - + @@ -16584,9 +16601,9 @@ - + - + @@ -16595,8 +16612,8 @@ - - + + @@ -16616,109 +16633,109 @@ - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + @@ -16732,20 +16749,12 @@ - - - - - - - - - + - + - + @@ -16880,7 +16889,7 @@ - + @@ -16930,7 +16939,7 @@ - + @@ -16989,7 +16998,7 @@ - + @@ -17000,7 +17009,7 @@ - + @@ -17020,8 +17029,8 @@ - - + + @@ -17056,7 +17065,7 @@ - + @@ -17064,7 +17073,7 @@ - + @@ -17113,7 +17122,7 @@ - + @@ -17353,8 +17362,8 @@ - - + + @@ -17378,7 +17387,7 @@ - + @@ -17392,7 +17401,7 @@ - + @@ -17405,10 +17414,10 @@ - - - - + + + + @@ -17430,10 +17439,10 @@ + + - - - + @@ -19605,7 +19614,7 @@ - + @@ -20634,7 +20643,7 @@ - + @@ -23516,7 +23525,7 @@ - + @@ -24765,7 +24774,7 @@ - + @@ -24954,10 +24963,10 @@ - + - + @@ -25158,12 +25167,12 @@ - - + + - + @@ -25516,158 +25525,158 @@ - - + + - - + + - - + + - - + + - - + + - - - + + + - - + + - - + + - - + + - - + + - - + + - - + + - + - - + + - - + + - - + + - - - + + + - - - + + + - + - + - + - + - - - - - - + + + + + + - - - - - - + + + + + + - - - + + + - - + + - - - + + + - - + + - - + + - - + + - - - + + + - - + + - - + + - - + + - - - + + + - - + + @@ -26273,7 +26282,7 @@ - + @@ -26325,11 +26334,11 @@ - + - + diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index b5d947cbb6ca9d..37cc88ed081b72 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -39,6 +39,32 @@ struct _Py_long_state { int max_str_digits; }; + +/* cross-interpreter data registry */ + +/* For now we use a global registry of shareable classes. An + alternative would be to add a tp_* slot for a class's + crossinterpdatafunc. It would be simpler and more efficient. */ + +struct _xidregitem; + +struct _xidregitem { + struct _xidregitem *prev; + struct _xidregitem *next; + /* This can be a dangling pointer, but only if weakref is set. */ + PyTypeObject *cls; + /* This is NULL for builtin types. */ + PyObject *weakref; + size_t refcount; + crossinterpdatafunc getdata; +}; + +struct _xidregistry { + PyThread_type_lock mutex; + struct _xidregitem *head; +}; + + /* interpreter state */ /* PyInterpreterState holds the global state for one of the runtime's @@ -194,6 +220,8 @@ struct _is { struct _Py_interp_cached_objects cached_objects; struct _Py_interp_static_objects static_objects; + // XXX Remove this field once we have a tp_* slot. + struct _xidregistry xidregistry; /* The thread currently executing in the __main__ module, if any. */ PyThreadState *threads_main; /* The ID of the OS thread in which we are finalizing. @@ -235,21 +263,6 @@ _PyInterpreterState_SetFinalizing(PyInterpreterState *interp, PyThreadState *tst } -/* cross-interpreter data registry */ - -/* For now we use a global registry of shareable classes. An - alternative would be to add a tp_* slot for a class's - crossinterpdatafunc. It would be simpler and more efficient. */ - -struct _xidregitem; - -struct _xidregitem { - struct _xidregitem *prev; - struct _xidregitem *next; - PyObject *cls; // weakref to a PyTypeObject - crossinterpdatafunc getdata; -}; - PyAPI_FUNC(PyInterpreterState*) _PyInterpreterState_LookUpID(int64_t); PyAPI_FUNC(int) _PyInterpreterState_IDInitref(PyInterpreterState *); diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index bc972783fc94f1..99c4b0760bfb94 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -111,10 +111,7 @@ typedef struct pyruntimestate { tools. */ // XXX Remove this field once we have a tp_* slot. - struct _xidregistry { - PyThread_type_lock mutex; - struct _xidregitem *head; - } xidregistry; + struct _xidregistry xidregistry; struct _pymem_allocators allocators; struct _obmalloc_global_state obmalloc; diff --git a/Python/pystate.c b/Python/pystate.c index 534e77fe2cbf55..1337516aa59cbc 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -493,6 +493,8 @@ _PyRuntimeState_Init(_PyRuntimeState *runtime) return _PyStatus_OK(); } +static void _xidregistry_clear(struct _xidregistry *); + void _PyRuntimeState_Fini(_PyRuntimeState *runtime) { @@ -501,6 +503,8 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime) assert(runtime->object_state.interpreter_leaks == 0); #endif + _xidregistry_clear(&runtime->xidregistry); + if (gilstate_tss_initialized(runtime)) { gilstate_tss_fini(runtime); } @@ -550,6 +554,11 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) for (int i = 0; i < NUMLOCKS; i++) { reinit_err += _PyThread_at_fork_reinit(lockptrs[i]); } + /* PyOS_AfterFork_Child(), which calls this function, later calls + _PyInterpreterState_DeleteExceptMain(), so we only need to update + the main interpreter here. */ + assert(runtime->interpreters.main != NULL); + runtime->interpreters.main->xidregistry.mutex = runtime->xidregistry.mutex; PyMem_SetAllocator(PYMEM_DOMAIN_RAW, &old_alloc); @@ -699,6 +708,10 @@ init_interpreter(PyInterpreterState *interp, interp->dtoa = (struct _dtoa_state)_dtoa_state_INIT(interp); } interp->f_opcode_trace_set = false; + + assert(runtime->xidregistry.mutex != NULL); + interp->xidregistry.mutex = runtime->xidregistry.mutex; + interp->_initialized = 1; } @@ -891,6 +904,10 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate) Py_CLEAR(interp->builtins); Py_CLEAR(interp->interpreter_trampoline); + _xidregistry_clear(&interp->xidregistry); + /* The lock is owned by the runtime, so we don't free it here. */ + interp->xidregistry.mutex = NULL; + if (tstate->interp == interp) { /* We are now safe to fix tstate->_status.cleared. */ // XXX Do this (much) earlier? @@ -2529,23 +2546,27 @@ _PyCrossInterpreterData_ReleaseAndRawFree(_PyCrossInterpreterData *data) crossinterpdatafunc. It would be simpler and more efficient. */ static int -_xidregistry_add_type(struct _xidregistry *xidregistry, PyTypeObject *cls, - crossinterpdatafunc getdata) +_xidregistry_add_type(struct _xidregistry *xidregistry, + PyTypeObject *cls, crossinterpdatafunc getdata) { - // Note that we effectively replace already registered classes - // rather than failing. struct _xidregitem *newhead = PyMem_RawMalloc(sizeof(struct _xidregitem)); if (newhead == NULL) { return -1; } - // XXX Assign a callback to clear the entry from the registry? - newhead->cls = PyWeakref_NewRef((PyObject *)cls, NULL); - if (newhead->cls == NULL) { - PyMem_RawFree(newhead); - return -1; + *newhead = (struct _xidregitem){ + // We do not keep a reference, to avoid keeping the class alive. + .cls = cls, + .refcount = 1, + .getdata = getdata, + }; + if (cls->tp_flags & Py_TPFLAGS_HEAPTYPE) { + // XXX Assign a callback to clear the entry from the registry? + newhead->weakref = PyWeakref_NewRef((PyObject *)cls, NULL); + if (newhead->weakref == NULL) { + PyMem_RawFree(newhead); + return -1; + } } - newhead->getdata = getdata; - newhead->prev = NULL; newhead->next = xidregistry->head; if (newhead->next != NULL) { newhead->next->prev = newhead; @@ -2570,37 +2591,78 @@ _xidregistry_remove_entry(struct _xidregistry *xidregistry, if (next != NULL) { next->prev = entry->prev; } - Py_DECREF(entry->cls); + Py_XDECREF(entry->weakref); PyMem_RawFree(entry); return next; } +static void +_xidregistry_clear(struct _xidregistry *xidregistry) +{ + struct _xidregitem *cur = xidregistry->head; + xidregistry->head = NULL; + while (cur != NULL) { + struct _xidregitem *next = cur->next; + Py_XDECREF(cur->weakref); + PyMem_RawFree(cur); + cur = next; + } +} + static struct _xidregitem * _xidregistry_find_type(struct _xidregistry *xidregistry, PyTypeObject *cls) { struct _xidregitem *cur = xidregistry->head; while (cur != NULL) { - PyObject *registered = PyWeakref_GetObject(cur->cls); - if (registered == Py_None) { - // The weakly ref'ed object was freed. - cur = _xidregistry_remove_entry(xidregistry, cur); - } - else { - assert(PyType_Check(registered)); - if (registered == (PyObject *)cls) { - return cur; + if (cur->weakref != NULL) { + // cur is/was a heap type. + PyObject *registered = PyWeakref_GetObject(cur->weakref); + assert(registered != NULL); + if (registered == Py_None) { + // The weakly ref'ed object was freed. + cur = _xidregistry_remove_entry(xidregistry, cur); + continue; } - cur = cur->next; + assert(PyType_Check(registered)); + assert(cur->cls == (PyTypeObject *)registered); + assert(cur->cls->tp_flags & Py_TPFLAGS_HEAPTYPE); + //Py_DECREF(registered); } + if (cur->cls == cls) { + return cur; + } + cur = cur->next; } return NULL; } +static inline struct _xidregistry * +_get_xidregistry(PyInterpreterState *interp, PyTypeObject *cls) +{ + struct _xidregistry *xidregistry = &interp->runtime->xidregistry; + if (cls->tp_flags & Py_TPFLAGS_HEAPTYPE) { + assert(interp->xidregistry.mutex == xidregistry->mutex); + xidregistry = &interp->xidregistry; + } + return xidregistry; +} + static void _register_builtins_for_crossinterpreter_data(struct _xidregistry *xidregistry); +static inline void +_ensure_builtins_xid(PyInterpreterState *interp, struct _xidregistry *xidregistry) +{ + if (xidregistry != &interp->xidregistry) { + assert(xidregistry == &interp->runtime->xidregistry); + if (xidregistry->head == NULL) { + _register_builtins_for_crossinterpreter_data(xidregistry); + } + } +} + int _PyCrossInterpreterData_RegisterClass(PyTypeObject *cls, - crossinterpdatafunc getdata) + crossinterpdatafunc getdata) { if (!PyType_Check(cls)) { PyErr_Format(PyExc_ValueError, "only classes may be registered"); @@ -2611,12 +2673,23 @@ _PyCrossInterpreterData_RegisterClass(PyTypeObject *cls, return -1; } - struct _xidregistry *xidregistry = &_PyRuntime.xidregistry ; + int res = 0; + PyInterpreterState *interp = _PyInterpreterState_GET(); + struct _xidregistry *xidregistry = _get_xidregistry(interp, cls); PyThread_acquire_lock(xidregistry->mutex, WAIT_LOCK); - if (xidregistry->head == NULL) { - _register_builtins_for_crossinterpreter_data(xidregistry); + + _ensure_builtins_xid(interp, xidregistry); + + struct _xidregitem *matched = _xidregistry_find_type(xidregistry, cls); + if (matched != NULL) { + assert(matched->getdata == getdata); + matched->refcount += 1; + goto finally; } - int res = _xidregistry_add_type(xidregistry, cls, getdata); + + res = _xidregistry_add_type(xidregistry, cls, getdata); + +finally: PyThread_release_lock(xidregistry->mutex); return res; } @@ -2625,13 +2698,20 @@ int _PyCrossInterpreterData_UnregisterClass(PyTypeObject *cls) { int res = 0; - struct _xidregistry *xidregistry = &_PyRuntime.xidregistry ; + PyInterpreterState *interp = _PyInterpreterState_GET(); + struct _xidregistry *xidregistry = _get_xidregistry(interp, cls); PyThread_acquire_lock(xidregistry->mutex, WAIT_LOCK); + struct _xidregitem *matched = _xidregistry_find_type(xidregistry, cls); if (matched != NULL) { - (void)_xidregistry_remove_entry(xidregistry, matched); + assert(matched->refcount > 0); + matched->refcount -= 1; + if (matched->refcount == 0) { + (void)_xidregistry_remove_entry(xidregistry, matched); + } res = 1; } + PyThread_release_lock(xidregistry->mutex); return res; } @@ -2644,17 +2724,19 @@ _PyCrossInterpreterData_UnregisterClass(PyTypeObject *cls) crossinterpdatafunc _PyCrossInterpreterData_Lookup(PyObject *obj) { - struct _xidregistry *xidregistry = &_PyRuntime.xidregistry ; - PyObject *cls = PyObject_Type(obj); + PyTypeObject *cls = Py_TYPE(obj); + + PyInterpreterState *interp = _PyInterpreterState_GET(); + struct _xidregistry *xidregistry = _get_xidregistry(interp, cls); PyThread_acquire_lock(xidregistry->mutex, WAIT_LOCK); - if (xidregistry->head == NULL) { - _register_builtins_for_crossinterpreter_data(xidregistry); - } - struct _xidregitem *matched = _xidregistry_find_type(xidregistry, - (PyTypeObject *)cls); - Py_DECREF(cls); + + _ensure_builtins_xid(interp, xidregistry); + + struct _xidregitem *matched = _xidregistry_find_type(xidregistry, cls); + crossinterpdatafunc func = matched != NULL ? matched->getdata : NULL; + PyThread_release_lock(xidregistry->mutex); - return matched != NULL ? matched->getdata : NULL; + return func; } /* cross-interpreter data for builtin types */