Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-128639: Don't assume one thread in subinterpreter finalization#128640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
025cc17
eea3ba9
149e0cf
1761c37
8febfbf
5a73ce1
a7cc3fe
7c44c0b
6551bfa
f19e28d
02da72a
a759540
76e4ec9
1555731
d61099e
09851dc
b223396
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -647,6 +647,59 @@ def test_created_with_capi(self): | ||
self.interp_exists(interpid)) | ||
def test_remaining_threads(self): | ||
r_interp, w_interp = self.pipe() | ||
FINISHED = b'F' | ||
# It's unlikely, but technically speaking, it's possible | ||
# that the thread could've finished before interp.close() is | ||
# reached, so this test might not properly exercise the case. | ||
# However, it's quite unlikely and I'm too lazy to deal with it. | ||
interp = interpreters.create() | ||
interp.exec(f"""if True: | ||
import os | ||
import threading | ||
import time | ||
def task(): | ||
time.sleep(1) | ||
ericsnowcurrently marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
os.write({w_interp}, {FINISHED!r}) | ||
threads = [threading.Thread(target=task) for _ in range(3)] | ||
for t in threads: | ||
t.start() | ||
""") | ||
interp.close() | ||
self.assertEqual(os.read(r_interp, 1), FINISHED) | ||
def test_remaining_daemon_threads(self): | ||
interp = _interpreters.create( | ||
types.SimpleNamespace( | ||
use_main_obmalloc=False, | ||
allow_fork=False, | ||
allow_exec=False, | ||
allow_threads=True, | ||
allow_daemon_threads=True, | ||
check_multi_interp_extensions=True, | ||
gil='own', | ||
) | ||
) | ||
_interpreters.exec(interp, f"""if True: | ||
import threading | ||
import time | ||
def task(): | ||
time.sleep(100) | ||
threads = [threading.Thread(target=task, daemon=True) for _ in range(3)] | ||
for t in threads: | ||
t.start() | ||
""") | ||
_interpreters.destroy(interp) | ||
class TestInterpreterPrepareMain(TestBase): | ||
def test_empty(self): | ||
@@ -756,7 +809,10 @@ def script(): | ||
spam.eggs() | ||
interp = interpreters.create() | ||
try: | ||
interp.exec(script) | ||
finally: | ||
interp.close() | ||
""") | ||
stdout, stderr = self.assert_python_failure(scriptfile) | ||
@@ -765,7 +821,7 @@ def script(): | ||
# File "{interpreters.__file__}", line 179, in exec | ||
self.assertEqual(stderr, dedent(f"""\ | ||
Traceback (most recent call last): | ||
File "{scriptfile}", line10, in <module> | ||
interp.exec(script) | ||
~~~~~~~~~~~^^^^^^^^ | ||
{interpmod_line.strip()} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -132,6 +132,7 @@ def test_sys_path_0(self): | ||
'sub': sys.path[0], | ||
}}, indent=4), flush=True) | ||
""") | ||
interp.close() | ||
ericsnowcurrently marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
''' | ||
# <tmp>/ | ||
# pkg/ | ||
@@ -172,7 +173,10 @@ def test_gh_109793(self): | ||
argv= [sys.executable,'-c','''if True: | ||
from test.support import interpreters | ||
interp = interpreters.create() | ||
try: | ||
raise Exception | ||
finally: | ||
interp.close() | ||
'''] | ||
proc=subprocess.run(argv,capture_output=True,text=True) | ||
self.assertIn('Traceback',proc.stderr) | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix a crash when using threads inside of a subinterpreter. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1424,9 +1424,12 @@ static int test_audit_subinterpreter(void) | ||
PySys_AddAuditHook(_audit_subinterpreter_hook, NULL); | ||
_testembed_Py_InitializeFromConfig(); | ||
PyThreadState *tstate = PyThreadState_Get(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I think this test working before was a fluke. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. It's probably okay but your change helps future-proof the test. | ||
for (int i = 0; i < 3; ++i) | ||
{ | ||
Py_EndInterpreter(Py_NewInterpreter()); | ||
PyThreadState_Swap(tstate); | ||
} | ||
Py_Finalize(); | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1982,6 +1982,7 @@ resolve_final_tstate(_PyRuntimeState *runtime) | ||
} | ||
else { | ||
/* Fall back to the current tstate. It's better than nothing. */ | ||
// XXX No it's not | ||
main_tstate = tstate; | ||
} | ||
} | ||
@@ -2027,6 +2028,16 @@ _Py_Finalize(_PyRuntimeState *runtime) | ||
_PyAtExit_Call(tstate->interp); | ||
/* Clean up any lingering subinterpreters. | ||
Two preconditions need to be met here: | ||
- This has to happen before _PyRuntimeState_SetFinalizing is | ||
called, or else threads might get prematurely blocked. | ||
- The world must not be stopped, as finalizers can run. | ||
*/ | ||
finalize_subinterpreters(); | ||
assert(_PyThreadState_GET() == tstate); | ||
/* Copy the core config, PyInterpreterState_Delete() free | ||
@@ -2114,9 +2125,6 @@ _Py_Finalize(_PyRuntimeState *runtime) | ||
_PyImport_FiniExternal(tstate->interp); | ||
finalize_modules(tstate); | ||
/* Print debug stats if any */ | ||
_PyEval_Fini(); | ||
@@ -2400,9 +2408,8 @@ Py_NewInterpreter(void) | ||
return tstate; | ||
} | ||
/* Delete an interpreter. This requires that the given thread state | ||
is current, and that the thread has no remaining frames. | ||
ericsnowcurrently marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
It is a fatal error to violate these constraints. | ||
(Py_FinalizeEx() doesn't have these constraints -- it zaps | ||
@@ -2432,14 +2439,15 @@ Py_EndInterpreter(PyThreadState *tstate) | ||
_Py_FinishPendingCalls(tstate); | ||
_PyAtExit_Call(tstate->interp); | ||
_PyRuntimeState *runtime = interp->runtime; | ||
_PyEval_StopTheWorldAll(runtime); | ||
ericsnowcurrently marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
PyThreadState *list = _PyThreadState_RemoveExcept(tstate); | ||
/* Remaining daemon threads will automatically exit | ||
when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ | ||
_PyInterpreterState_SetFinalizing(interp, tstate); | ||
_PyEval_StartTheWorldAll(runtime); | ||
_PyThreadState_DeleteList(list, /*is_after_fork=*/0); | ||
ericsnowcurrently marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
// XXX Call something like _PyImport_Disable() here? | ||
@@ -2470,6 +2478,8 @@ finalize_subinterpreters(void) | ||
PyInterpreterState *main_interp = _PyInterpreterState_Main(); | ||
assert(final_tstate->interp == main_interp); | ||
_PyRuntimeState *runtime = main_interp->runtime; | ||
assert(!runtime->stoptheworld.world_stopped); | ||
assert(_PyRuntimeState_GetFinalizing(runtime) == NULL); | ||
struct pyinterpreters *interpreters = &runtime->interpreters; | ||
/* Get the first interpreter in the list. */ | ||
@@ -2498,27 +2508,17 @@ finalize_subinterpreters(void) | ||
/* Clean up all remaining subinterpreters. */ | ||
while (interp != NULL) { | ||
/* Make a tstate for finalization. */ | ||
PyThreadState *tstate = _PyThreadState_NewBound(interp, _PyThreadState_WHENCE_FINI); | ||
ericsnowcurrently marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
if (tstate == NULL) { | ||
// XXX Some graceful way to always get a thread state? | ||
Py_FatalError("thread state allocation failed"); | ||
ZeroIntensity marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
} | ||
/*Enter the subinterpreter. */ | ||
_PyThreadState_Attach(tstate); | ||
/* Destroy the subinterpreter. */ | ||
Py_EndInterpreter(tstate); | ||
assert(_PyThreadState_GET() == NULL); | ||
Uh oh!
There was an error while loading.Please reload this page.