From 23af5f567e4ff2014d7377168f3deb65bebc9745 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Thu, 21 Nov 2019 18:10:00 +0000 Subject: [PATCH 01/18] Add test suggested by ncoghlan --- Programs/_testembed.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index b98a38a1ba6783..d43e240bc179eb 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -85,6 +85,46 @@ static int test_repeated_init_and_subinterpreters(void) return 0; } +static int test_bpo36225(void) +{ + PyThreadState *mainstate; + PyGILState_STATE gilstate; + int i, j; + + for (i=0; i<15; i++) { + printf("--- Pass %d ---\n", i); + _testembed_Py_Initialize(); + mainstate = PyThreadState_Get(); + + PyEval_ReleaseThread(mainstate); + + gilstate = PyGILState_Ensure(); + print_subinterp(); + PyThreadState_Swap(NULL); + + for (j=0; j<2; j++) { + Py_NewInterpreter(); + print_subinterp(); + } + + PyThreadState_Swap(mainstate); + print_subinterp(); + + for (j=0; j<2; j++) { + Py_NewInterpreter(); + print_subinterp(); + } + + PyThreadState_Swap(mainstate); + print_subinterp(); + PyGILState_Release(gilstate); + + PyEval_RestoreThread(mainstate); + Py_Finalize(); + } + return 0; +} + /***************************************************** * Test forcing a particular IO encoding *****************************************************/ @@ -1603,6 +1643,7 @@ struct TestCase static struct TestCase TestCases[] = { {"test_forced_io_encoding", test_forced_io_encoding}, {"test_repeated_init_and_subinterpreters", test_repeated_init_and_subinterpreters}, + {"test_bpo36225", test_bpo36225}, {"test_pre_initialization_api", test_pre_initialization_api}, {"test_pre_initialization_sys_options", test_pre_initialization_sys_options}, {"test_bpo20891", test_bpo20891}, From 433663cbac435f9fc05b44cb25bd5843f1396354 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Wed, 11 Dec 2019 15:05:31 +0000 Subject: [PATCH 02/18] Finalise sub-interpreters in Py_FinalizeEx() --- Include/internal/pycore_pystate.h | 1 + Python/pylifecycle.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 936e9cbc65f7ad..10419e0e5a8d1b 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -226,6 +226,7 @@ typedef struct pyruntimestate { If that becomes a problem later then we can adjust, e.g. by using a Python int. */ int64_t next_id; + int finalizing; } interpreters; // XXX Remove this field once we have a tp_* slot. struct _xidregistry { diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 7591f069b455d2..c61179e40fcd1b 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1257,6 +1257,20 @@ Py_FinalizeEx(void) PyThreadState *tstate = _PyRuntimeState_GetThreadState(runtime); PyInterpreterState *interp = tstate->interp; + // Finalize sub-interpreters. + runtime->interpreters.finalizing = 1; + PyInterpreterState *subinterp = PyInterpreterState_Head(); + PyInterpreterState *next_interp; + while (subinterp != NULL) { + next_interp = PyInterpreterState_Next(subinterp); + if (subinterp != PyInterpreterState_Main()) { + PyThreadState_Swap(subinterp->tstate_head); + Py_EndInterpreter(subinterp->tstate_head); + } + subinterp = next_interp; + } + PyThreadState_Swap(tstate); + // Wrap up existing "threading"-module-created, non-daemon threads. wait_for_thread_shutdown(tstate); @@ -1454,6 +1468,10 @@ new_interpreter(PyThreadState **tstate_p) return _PyStatus_ERR("Py_Initialize must be called first"); } + if (runtime->interpreters.finalizing) { + return _PyStatus_ERR("Interpreters are being finalized"); + } + /* Issue #10915, #15751: The GIL API doesn't work with multiple interpreters: disable PyGILState_Check(). */ _PyGILState_check_enabled = 0; From 48e1cfc7a2f89a5f8e7a0d8efac2521a39b8430e Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Fri, 13 Dec 2019 17:49:38 +0000 Subject: [PATCH 03/18] Improve test name --- Programs/_testembed.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index d43e240bc179eb..9cfc1b144c42e7 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -85,7 +85,8 @@ static int test_repeated_init_and_subinterpreters(void) return 0; } -static int test_bpo36225(void) +/* bpo-36225: Implicitly tear down subinterpreters with Py_Finalize() */ +static int test_finalize_subinterps(void) { PyThreadState *mainstate; PyGILState_STATE gilstate; @@ -1643,7 +1644,7 @@ struct TestCase static struct TestCase TestCases[] = { {"test_forced_io_encoding", test_forced_io_encoding}, {"test_repeated_init_and_subinterpreters", test_repeated_init_and_subinterpreters}, - {"test_bpo36225", test_bpo36225}, + {"test_finalize_subinterps", test_finalize_subinterps}, {"test_pre_initialization_api", test_pre_initialization_api}, {"test_pre_initialization_sys_options", test_pre_initialization_sys_options}, {"test_bpo20891", test_bpo20891}, From 0400634c66a8b73779ec2d1bfb1622223a309aba Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Fri, 13 Dec 2019 18:23:01 +0000 Subject: [PATCH 04/18] Switch back to main threadstate in test_audit_subinterpreter before calling Py_Finalize() --- Programs/_testembed.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 9cfc1b144c42e7..487c4d2bb53449 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -1222,10 +1222,14 @@ static int test_audit_subinterpreter(void) PySys_AddAuditHook(_audit_subinterpreter_hook, NULL); _testembed_Py_Initialize(); + PyThreadState *mainstate = PyThreadState_Get(); + Py_NewInterpreter(); Py_NewInterpreter(); Py_NewInterpreter(); + // Currently unable to call Py_Finalize from subinterpreter thread, see bpo-37776. + PyThreadState_Swap(mainstate); Py_Finalize(); switch (_audit_subinterpreter_interpreter_count) { From b79649cbc96eb51683cbd0ebc2951ce8b98e3c1c Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sat, 14 Dec 2019 15:20:52 +0000 Subject: [PATCH 05/18] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Misc/NEWS.d/next/C API/2019-12-14-15-20-51.bpo-36225.z7Nnrq.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/C API/2019-12-14-15-20-51.bpo-36225.z7Nnrq.rst diff --git a/Misc/NEWS.d/next/C API/2019-12-14-15-20-51.bpo-36225.z7Nnrq.rst b/Misc/NEWS.d/next/C API/2019-12-14-15-20-51.bpo-36225.z7Nnrq.rst new file mode 100644 index 00000000000000..5a37976189dcf6 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2019-12-14-15-20-51.bpo-36225.z7Nnrq.rst @@ -0,0 +1 @@ +:func:`Py_FinalizeEx()` now implicitly cleans up subinterpreters, as the C API documentation suggests. \ No newline at end of file From 8b1e7d9a1dfb331254002c8486324cbbbef3a2bd Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Tue, 21 Jan 2020 23:35:31 +0000 Subject: [PATCH 06/18] Markups including: switch from 'finalizing' flag to 'allow_new', add test to test_embed.py --- Include/internal/pycore_pystate.h | 2 +- Lib/test/test_embed.py | 8 ++++++++ Programs/_testembed.c | 10 +--------- Python/pylifecycle.c | 29 +++++++++++++++-------------- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 10419e0e5a8d1b..53891745b4fbd9 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -226,7 +226,7 @@ typedef struct pyruntimestate { If that becomes a problem later then we can adjust, e.g. by using a Python int. */ int64_t next_id; - int finalizing; + int allow_new; } interpreters; // XXX Remove this field once we have a tp_* slot. struct _xidregistry { diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index b87863a372a6c4..23959668909139 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -190,6 +190,14 @@ def test_subinterps_distinct_state(self): self.assertNotEqual(sub.tstate, main.tstate) self.assertNotEqual(sub.modules, main.modules) + def test_finalize_subinterps(self): + """ + bpo-36225: Subinterpreters should implicitly be torn down by + Py_Finalize(). + """ + out, err = self.run_embedded_interpreter("test_finalize_subinterps") + self.assertEqual(err, "") + def test_forced_io_encoding(self): # Checks forced configuration of embedded interpreter IO streams env = dict(os.environ, PYTHONIOENCODING="utf-8:surrogateescape") diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 487c4d2bb53449..78bde082b3c9f5 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -17,7 +17,7 @@ /********************************************************* * Embedded interpreter tests that need a custom exe * - * Executed via 'EmbeddingTests' in Lib/test/test_capi.py + * Executed via 'EmbeddingTests' in Lib/test/test_embed.py *********************************************************/ /* Use path starting with "./" avoids a search along the PATH */ @@ -108,14 +108,6 @@ static int test_finalize_subinterps(void) print_subinterp(); } - PyThreadState_Swap(mainstate); - print_subinterp(); - - for (j=0; j<2; j++) { - Py_NewInterpreter(); - print_subinterp(); - } - PyThreadState_Swap(mainstate); print_subinterp(); PyGILState_Release(gilstate); diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index c61179e40fcd1b..19261c85933177 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -87,6 +87,7 @@ _PyRuntime_Initialize(void) return _PyStatus_OK(); } runtime_initialized = 1; + _PyRuntime.interpreters.allow_new = 0; return _PyRuntimeState_Init(&_PyRuntime); } @@ -946,6 +947,7 @@ pyinit_main(PyThreadState *tstate) * or pure Python code in the standard library won't work. */ runtime->initialized = 1; + runtime->interpreters.allow_new = 1; return _PyStatus_OK(); } @@ -1007,6 +1009,7 @@ pyinit_main(PyThreadState *tstate) } runtime->initialized = 1; + runtime->interpreters.allow_new = 1; if (config->site_import) { status = init_import_site(); @@ -1258,16 +1261,16 @@ Py_FinalizeEx(void) PyInterpreterState *interp = tstate->interp; // Finalize sub-interpreters. - runtime->interpreters.finalizing = 1; - PyInterpreterState *subinterp = PyInterpreterState_Head(); + runtime->interpreters.allow_new = 0; + PyInterpreterState *curr_interp = PyInterpreterState_Head(); PyInterpreterState *next_interp; - while (subinterp != NULL) { - next_interp = PyInterpreterState_Next(subinterp); - if (subinterp != PyInterpreterState_Main()) { - PyThreadState_Swap(subinterp->tstate_head); - Py_EndInterpreter(subinterp->tstate_head); + while (curr_interp != NULL) { + next_interp = PyInterpreterState_Next(curr_interp); + if (curr_interp != PyInterpreterState_Main()) { + PyThreadState_Swap(curr_interp->tstate_head); + Py_EndInterpreter(curr_interp->tstate_head); } - subinterp = next_interp; + curr_interp = next_interp; } PyThreadState_Swap(tstate); @@ -1464,12 +1467,10 @@ new_interpreter(PyThreadState **tstate_p) } _PyRuntimeState *runtime = &_PyRuntime; - if (!runtime->initialized) { - return _PyStatus_ERR("Py_Initialize must be called first"); - } - - if (runtime->interpreters.finalizing) { - return _PyStatus_ERR("Interpreters are being finalized"); + if (!runtime->interpreters.allow_new) { + return _PyStatus_ERR( + "New interpreters cannot currently be created - Py_Initialize must " + "be called first, and Py_Finalize must not have been called"); } /* Issue #10915, #15751: The GIL API doesn't work with multiple From 1095e66c2ecc5dc79829fc47bff175d4550de719 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Tue, 20 Oct 2020 14:55:46 +0100 Subject: [PATCH 07/18] Use '_' for unused variable in test_embed.py Co-authored-by: Eric Snow --- Lib/test/test_embed.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 414ad82fa737fd..9381104bb8fe0d 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -197,7 +197,7 @@ def test_finalize_subinterps(self): bpo-36225: Subinterpreters should implicitly be torn down by Py_Finalize(). """ - out, err = self.run_embedded_interpreter("test_finalize_subinterps") + _, err = self.run_embedded_interpreter("test_finalize_subinterps") self.assertEqual(err, "") def test_forced_io_encoding(self): From 675285dc04293757c220fa9a4802439fbb175e16 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Thu, 22 Oct 2020 17:16:12 +0100 Subject: [PATCH 08/18] Fix struct position of 'allow_new' flag --- Include/internal/pycore_runtime.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index dc960fbe326051..95994a90a72f03 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -83,8 +83,8 @@ typedef struct pyruntimestate { always have an ID of 0. Overflow results in a RuntimeError. If that becomes a problem later then we can adjust, e.g. by using a Python int. */ - int allow_new; int64_t next_id; + int allow_new; } interpreters; // XXX Remove this field once we have a tp_* slot. struct _xidregistry { From 8e21788e1fee05a55fcf2ecdc40ebeb0f8fcaa78 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Thu, 22 Oct 2020 23:19:46 +0100 Subject: [PATCH 09/18] Add handling for unsupported case of calling Py_Finalize() from a subinterpreter --- Python/pylifecycle.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index d292f67852ed03..3fddfa71817f6b 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1351,6 +1351,15 @@ Py_FinalizeEx(void) PyThreadState *tstate = _PyRuntimeState_GetThreadState(runtime); PyInterpreterState *interp = tstate->interp; + /* Check we're running in the main interpreter (not yet supported to call + * from any interpreter). + */ + if (interp != PyInterpreterState_Main()) { + fprintf(stderr, + "Py_FinalizeEx: error: must be called from the main interpreter\n"); + return -1; + } + // Finalize sub-interpreters. runtime->interpreters.allow_new = 0; PyInterpreterState *curr_interp = PyInterpreterState_Head(); From 606c068ed4759089fd4aa02f721084ecc66de36d Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Thu, 22 Oct 2020 23:20:16 +0100 Subject: [PATCH 10/18] Emit resource warning when calling Py_Finalize() with unfinalized subinterpreters --- Python/pylifecycle.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 3fddfa71817f6b..c581a059262963 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1364,16 +1364,25 @@ Py_FinalizeEx(void) runtime->interpreters.allow_new = 0; PyInterpreterState *curr_interp = PyInterpreterState_Head(); PyInterpreterState *next_interp; + int64_t num_destroyed = 0; while (curr_interp != NULL) { next_interp = PyInterpreterState_Next(curr_interp); - if (curr_interp != PyInterpreterState_Main()) { + if (curr_interp != interp) { PyThreadState_Swap(curr_interp->tstate_head); Py_EndInterpreter(curr_interp->tstate_head); + num_destroyed++; } curr_interp = next_interp; } PyThreadState_Swap(tstate); + if (num_destroyed > 0) { + if (PyErr_ResourceWarning(Py_None, 1, + "extra %zd interpreters", num_destroyed)) { + PyErr_WriteUnraisable(Py_None); + } + } + // Wrap up existing "threading"-module-created, non-daemon threads. wait_for_thread_shutdown(tstate); From e0789b0a3f01974ca07e9c5fdae39b095be54d59 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Thu, 22 Oct 2020 23:41:28 +0100 Subject: [PATCH 11/18] Update Py_FinalizeEx() docs --- Doc/c-api/init.rst | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 7f06648bcb4572..cb6b24cdf68f86 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -276,7 +276,8 @@ Initializing and finalizing the interpreter Undo all initializations made by :c:func:`Py_Initialize` and subsequent use of Python/C API functions, and destroy all sub-interpreters (see :c:func:`Py_NewInterpreter` below) that were created and not yet destroyed since - the last call to :c:func:`Py_Initialize`. Ideally, this frees all memory + the last call to :c:func:`Py_Initialize`. A resource warning is emitted if + there were remaining sub-interpreters. Ideally, this frees all memory allocated by the Python interpreter. This is a no-op when called for a second time (without calling :c:func:`Py_Initialize` again first). Normally the return value is ``0``. If there were errors during finalization @@ -299,7 +300,8 @@ Initializing and finalizing the interpreter freed. Some memory allocated by extension modules may not be freed. Some extensions may not work properly if their initialization routine is called more than once; this can happen if an application calls :c:func:`Py_Initialize` and - :c:func:`Py_FinalizeEx` more than once. + :c:func:`Py_FinalizeEx` more than once. Must be called from the main + interpreter, otherwise ``-1`` is returned and an error is output. .. audit-event:: cpython._PySys_ClearAuditHooks "" c.Py_FinalizeEx From dda99ced4ee2d10d38ac1df92049bf17034fdd9a Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Fri, 23 Oct 2020 18:55:43 +0100 Subject: [PATCH 12/18] Update test for resource warning when implicitly finalizing subinterpreters --- Lib/test/test_embed.py | 8 ++++++-- Programs/_testembed.c | 9 +++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 9381104bb8fe0d..4ed5b93c5a316f 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -197,8 +197,12 @@ def test_finalize_subinterps(self): bpo-36225: Subinterpreters should implicitly be torn down by Py_Finalize(). """ - _, err = self.run_embedded_interpreter("test_finalize_subinterps") - self.assertEqual(err, "") + out, err = self.run_embedded_interpreter("test_finalize_subinterps") + if support.verbose > 1: + print() + print(out) + print(err) + self.assertIn("ResourceWarning: extra 2 interpreters", err) def test_forced_io_encoding(self): # Checks forced configuration of embedded interpreter IO streams diff --git a/Programs/_testembed.c b/Programs/_testembed.c index da37266f2a63a2..3c475c9bd2c0b1 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -87,6 +87,7 @@ static int test_repeated_init_and_subinterpreters(void) static int test_finalize_subinterps(void) { PyThreadState *mainstate; + PyThreadState *interp_tstate; PyGILState_STATE gilstate; int i, j; @@ -101,11 +102,15 @@ static int test_finalize_subinterps(void) print_subinterp(); PyThreadState_Swap(NULL); - for (j=0; j<2; j++) { - Py_NewInterpreter(); + // Create 3 subinterpreters and destroy the last one. + for (j=0; j<3; j++) { + interp_tstate = Py_NewInterpreter(); print_subinterp(); } + PyThreadState_Swap(interp_tstate); + Py_EndInterpreter(interp_tstate); + // Switch back to the main interpreter and finalize the runtime. PyThreadState_Swap(mainstate); print_subinterp(); PyGILState_Release(gilstate); From 847e8d228df7c2deb0bd497db004af890c38979a Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Fri, 23 Oct 2020 19:44:52 +0100 Subject: [PATCH 13/18] Tidy up test_finalize_subinterps() testcase --- Lib/test/test_embed.py | 3 +-- Programs/_testembed.c | 42 ++++++++++++++++++++---------------------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 4ed5b93c5a316f..605fbef85f36b2 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -197,10 +197,9 @@ def test_finalize_subinterps(self): bpo-36225: Subinterpreters should implicitly be torn down by Py_Finalize(). """ - out, err = self.run_embedded_interpreter("test_finalize_subinterps") + _, err = self.run_embedded_interpreter("test_finalize_subinterps") if support.verbose > 1: print() - print(out) print(err) self.assertIn("ResourceWarning: extra 2 interpreters", err) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 3c475c9bd2c0b1..aa2c6cbb8f737f 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -89,35 +89,33 @@ static int test_finalize_subinterps(void) PyThreadState *mainstate; PyThreadState *interp_tstate; PyGILState_STATE gilstate; - int i, j; + int i; - for (i=0; i<15; i++) { - printf("--- Pass %d ---\n", i); - _testembed_Py_Initialize(); - mainstate = PyThreadState_Get(); + _testembed_Py_Initialize(); + mainstate = PyThreadState_Get(); - PyEval_ReleaseThread(mainstate); + PyEval_ReleaseThread(mainstate); - gilstate = PyGILState_Ensure(); + gilstate = PyGILState_Ensure(); + print_subinterp(); + PyThreadState_Swap(NULL); + + // Create 3 subinterpreters and destroy the last one. + for (i=0; i<3; i++) { + interp_tstate = Py_NewInterpreter(); print_subinterp(); - PyThreadState_Swap(NULL); + } + PyThreadState_Swap(interp_tstate); + Py_EndInterpreter(interp_tstate); - // Create 3 subinterpreters and destroy the last one. - for (j=0; j<3; j++) { - interp_tstate = Py_NewInterpreter(); - print_subinterp(); - } - PyThreadState_Swap(interp_tstate); - Py_EndInterpreter(interp_tstate); + // Switch back to the main interpreter and finalize the runtime. + PyThreadState_Swap(mainstate); + print_subinterp(); + PyGILState_Release(gilstate); - // Switch back to the main interpreter and finalize the runtime. - PyThreadState_Swap(mainstate); - print_subinterp(); - PyGILState_Release(gilstate); + PyEval_RestoreThread(mainstate); + Py_Finalize(); - PyEval_RestoreThread(mainstate); - Py_Finalize(); - } return 0; } From a2fb0fc43c44178deb5423a173d3f0b987c2a013 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Fri, 23 Oct 2020 19:45:13 +0100 Subject: [PATCH 14/18] Add testcase for calling Py_Finalize() from a subinterpreter --- Lib/test/test_embed.py | 10 ++++++++++ Programs/_testembed.c | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 605fbef85f36b2..37777ca1be649d 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -203,6 +203,16 @@ def test_finalize_subinterps(self): print(err) self.assertIn("ResourceWarning: extra 2 interpreters", err) + def test_finalize_from_subinterp(self): + """ + bpo-38865: Py_Finalize() should not be called from a subinterpreter. + """ + _, err = self.run_embedded_interpreter("test_finalize_from_subinterp", + returncode=255) + self.assertEqual( + err.strip(), + "Py_FinalizeEx: error: must be called from the main interpreter") + def test_forced_io_encoding(self): # Checks forced configuration of embedded interpreter IO streams env = dict(os.environ, PYTHONIOENCODING="utf-8:surrogateescape") diff --git a/Programs/_testembed.c b/Programs/_testembed.c index aa2c6cbb8f737f..0178fa102e0f0a 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -119,6 +119,24 @@ static int test_finalize_subinterps(void) return 0; } +/* bpo-38865: Py_Finalize() should not be called from a subinterpreter */ +static int test_finalize_from_subinterp(void) +{ + PyThreadState *subinterp_tstate; + int rc; + + _testembed_Py_Initialize(); + PyGILState_Ensure(); + PyThreadState_Swap(NULL); + + subinterp_tstate = Py_NewInterpreter(); + PyThreadState_Swap(subinterp_tstate); + + rc = Py_FinalizeEx(); + + return rc; +} + /***************************************************** * Test forcing a particular IO encoding *****************************************************/ @@ -1699,6 +1717,7 @@ static struct TestCase TestCases[] = { {"test_forced_io_encoding", test_forced_io_encoding}, {"test_repeated_init_and_subinterpreters", test_repeated_init_and_subinterpreters}, {"test_finalize_subinterps", test_finalize_subinterps}, + {"test_finalize_from_subinterp", test_finalize_from_subinterp}, {"test_pre_initialization_api", test_pre_initialization_api}, {"test_pre_initialization_sys_options", test_pre_initialization_sys_options}, {"test_bpo20891", test_bpo20891}, From d234528d90c0163fdddef0145f9a230aad77f2e8 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Mon, 23 Nov 2020 20:15:52 +0000 Subject: [PATCH 15/18] Tweak subinterpreters still running ResourceWarning handling --- Python/pylifecycle.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index c581a059262963..f8fa7f41f7ad92 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1377,9 +1377,12 @@ Py_FinalizeEx(void) PyThreadState_Swap(tstate); if (num_destroyed > 0) { - if (PyErr_ResourceWarning(Py_None, 1, + /* Sub-interpreters were still running, but should have be finalized + * before finalizing the runtime. + */ + if (PyErr_ResourceWarning(NULL, 1, "extra %zd interpreters", num_destroyed)) { - PyErr_WriteUnraisable(Py_None); + _PyErr_WriteUnraisableMsg("in PyFinalizeEx", NULL); } } From 46a861935142ec7f7bcb6acc364643dcf57e1c88 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Mon, 23 Nov 2020 20:25:59 +0000 Subject: [PATCH 16/18] Make calling PyFinalizeEx() from a subinterpreter a Py_FatalError --- Doc/c-api/init.rst | 2 +- Lib/test/test_embed.py | 8 ++++---- Python/pylifecycle.c | 4 +--- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index cb6b24cdf68f86..6658367130a2da 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -301,7 +301,7 @@ Initializing and finalizing the interpreter extensions may not work properly if their initialization routine is called more than once; this can happen if an application calls :c:func:`Py_Initialize` and :c:func:`Py_FinalizeEx` more than once. Must be called from the main - interpreter, otherwise ``-1`` is returned and an error is output. + interpreter. .. audit-event:: cpython._PySys_ClearAuditHooks "" c.Py_FinalizeEx diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 37777ca1be649d..efcfc8f964471b 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -208,10 +208,10 @@ def test_finalize_from_subinterp(self): bpo-38865: Py_Finalize() should not be called from a subinterpreter. """ _, err = self.run_embedded_interpreter("test_finalize_from_subinterp", - returncode=255) - self.assertEqual( - err.strip(), - "Py_FinalizeEx: error: must be called from the main interpreter") + returncode=-6) + self.assertIn( + "Fatal Python error: Py_FinalizeEx: must be called from the main interpreter", + err) def test_forced_io_encoding(self): # Checks forced configuration of embedded interpreter IO streams diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index f8fa7f41f7ad92..61963cc7b624b1 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1355,9 +1355,7 @@ Py_FinalizeEx(void) * from any interpreter). */ if (interp != PyInterpreterState_Main()) { - fprintf(stderr, - "Py_FinalizeEx: error: must be called from the main interpreter\n"); - return -1; + Py_FatalError("must be called from the main interpreter\n"); } // Finalize sub-interpreters. From c89c0e55c32fa9edb6ad479c4cbcacb13b089b03 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Mon, 23 Nov 2020 20:40:12 +0000 Subject: [PATCH 17/18] Acquire interpreters mutex before setting allow_new=0 in PyFinalizeEx() --- Python/pylifecycle.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 61963cc7b624b1..fd47d00c1322f2 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1359,7 +1359,9 @@ Py_FinalizeEx(void) } // Finalize sub-interpreters. + PyThread_acquire_lock(runtime->interpreters.mutex, WAIT_LOCK); runtime->interpreters.allow_new = 0; + PyThread_release_lock(runtime->interpreters.mutex); PyInterpreterState *curr_interp = PyInterpreterState_Head(); PyInterpreterState *next_interp; int64_t num_destroyed = 0; From 95cbfd495e753c4ddf027033a393b78ac99c4b88 Mon Sep 17 00:00:00 2001 From: Lewis Gaul Date: Mon, 23 Nov 2020 21:00:46 +0000 Subject: [PATCH 18/18] Add back in the 'interp' variable to PyFinalizeEx() to fix the build --- Python/pylifecycle.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 645b79b03ffa23..cd31c13c447454 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1649,6 +1649,7 @@ Py_FinalizeEx(void) /* Get current thread state and interpreter pointer */ PyThreadState *tstate = _PyRuntimeState_GetThreadState(runtime); + PyInterpreterState *interp = tstate->interp; /* Check we're running in the main interpreter (not yet supported to call * from any interpreter). @@ -1706,13 +1707,13 @@ Py_FinalizeEx(void) /* Copy the core config, PyInterpreterState_Delete() free the core config memory */ #ifdef Py_REF_DEBUG - int show_ref_count = tstate->interp->config.show_ref_count; + int show_ref_count = interp->config.show_ref_count; #endif #ifdef Py_TRACE_REFS - int dump_refs = tstate->interp->config.dump_refs; + int dump_refs = interp->config.dump_refs; #endif #ifdef WITH_PYMALLOC - int malloc_stats = tstate->interp->config.malloc_stats; + int malloc_stats = interp->config.malloc_stats; #endif /* Remaining daemon threads will automatically exit