-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-136003: Execute pre-finalization callbacks in a loop #136004
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 24 commits
354e6b5
3090524
43038b8
8d4151c
add4d33
3edc3a8
ec57918
970153b
cfd62b8
859070f
37098a0
8a1aa13
cbcd552
54613a1
9ccdb5f
9cd75b7
1360059
475538a
a794188
2dda7a4
f1460af
1e1301d
51a20d4
cf2dc1e
6ea3792
8c12e6c
8b87014
e57bfde
e202848
0e66c88
c8cac69
2ab28b0
c9d5f4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Fix :class:`threading.Thread` objects becoming incorrectly daemon when | ||
| created from an :mod:`atexit` callback or a pending call | ||
| (:c:func:`Py_AddPendingCall`). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2011,6 +2011,105 @@ resolve_final_tstate(_PyRuntimeState *runtime) | |
| return main_tstate; | ||
| } | ||
|
|
||
| #ifdef Py_GIL_DISABLED | ||
| #define ASSERT_WORLD_STOPPED(interp) assert(interp->runtime->stoptheworld.world_stopped) | ||
| #else | ||
| #define ASSERT_WORLD_STOPPED(interp) | ||
| #endif | ||
|
|
||
| static int | ||
| interp_has_threads(PyInterpreterState *interp) | ||
| { | ||
| /* This needs to check for non-daemon threads only, otherwise we get stuck | ||
| * in an infinite loop. */ | ||
| assert(interp != NULL); | ||
| ASSERT_WORLD_STOPPED(interp); | ||
| assert(interp->threads.head != NULL); | ||
| if (interp->threads.head->next == NULL) { | ||
| // No other threads active, easy way out. | ||
| return 0; | ||
| } | ||
|
|
||
| // We don't have to worry about locking this because the | ||
| // world is stopped. | ||
| _Py_FOR_EACH_TSTATE_UNLOCKED(interp, tstate) { | ||
| if (tstate->_whence == _PyThreadState_WHENCE_THREADING) { | ||
| return 1; | ||
| } | ||
| } | ||
|
Comment on lines
+2037
to
+2041
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. Using 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 mean a counter on 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. That better approach doesn't need to block this PR, but we'd need to follow up with the better approach soon after. 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. Sure, I don't mind refactoring afterwards. |
||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| static int | ||
| interp_has_pending_calls(PyInterpreterState *interp) | ||
| { | ||
| assert(interp != NULL); | ||
| ASSERT_WORLD_STOPPED(interp); | ||
| return interp->ceval.pending.npending != 0; | ||
| } | ||
|
|
||
| static int | ||
| interp_has_atexit_callbacks(PyInterpreterState *interp) | ||
| { | ||
| assert(interp != NULL); | ||
| assert(interp->atexit.callbacks != NULL); | ||
| ASSERT_WORLD_STOPPED(interp); | ||
| assert(PyList_CheckExact(interp->atexit.callbacks)); | ||
| return PyList_GET_SIZE(interp->atexit.callbacks) != 0; | ||
| } | ||
|
|
||
| static void | ||
| make_pre_finalization_calls(PyThreadState *tstate) | ||
| { | ||
| assert(tstate != NULL); | ||
| PyInterpreterState *interp = tstate->interp; | ||
| /* Each of these functions can start one another, e.g. a pending call | ||
| * could start a thread or vice versa. To ensure that we properly clean | ||
| * call everything, we run these in a loop until none of them run anything. */ | ||
| for (;;) { | ||
|
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. Would it make sense to add an arbitrary limit to detect infinite loop? For example, log an error after 16 attemps. 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. Well, it would prevent deadlocks in rare cases, but it would cause crashes in other equally rare cases. Maybe it would be better to emit a fatal error when there are too many iterations? |
||
| assert(!interp->runtime->stoptheworld.world_stopped); | ||
|
|
||
| // Wrap up existing "threading"-module-created, non-daemon threads. | ||
| wait_for_thread_shutdown(tstate); | ||
|
Comment on lines
+2085
to
+2086
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. We don't necessarily need to worry about it here, but it would probably also be worth waiting here for the interpreter to not be "running main". 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. Let's do that in a follow-up (probably tomorrow). |
||
|
|
||
| // Make any remaining pending calls. | ||
| _Py_FinishPendingCalls(tstate); | ||
|
|
||
| /* The interpreter is still entirely intact at this point, and the | ||
| * exit funcs may be relying on that. In particular, if some thread | ||
| * or exit func is still waiting to do an import, the import machinery | ||
| * expects Py_IsInitialized() to return true. So don't say the | ||
| * runtime is uninitialized until after the exit funcs have run. | ||
| * Note that Threading.py uses an exit func to do a join on all the | ||
| * threads created thru it, so this also protects pending imports in | ||
| * the threads created via Threading. | ||
| */ | ||
|
|
||
| _PyAtExit_Call(tstate->interp); | ||
|
|
||
| /* Stop the world to prevent other threads from creating threads or | ||
| * atexit callbacks. On the default build, this is simply locked by | ||
| * the GIL. For pending calls, we acquire the dedicated mutex, because | ||
| * Py_AddPendingCall() can be called without an attached thread state. | ||
| */ | ||
|
|
||
| // XXX Why does _PyThreadState_DeleteList() rely on all interpreters | ||
| // being stopped? | ||
| PyMutex_Lock(&interp->ceval.pending.mutex); | ||
|
||
| _PyEval_StopTheWorldAll(interp->runtime); | ||
ericsnowcurrently marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| int should_continue = (interp_has_threads(interp) | ||
| || interp_has_atexit_callbacks(interp) | ||
| || interp_has_pending_calls(interp)); | ||
| if (!should_continue) { | ||
| break; | ||
| } | ||
| _PyEval_StartTheWorldAll(interp->runtime); | ||
| PyMutex_Unlock(&interp->ceval.pending.mutex); | ||
| } | ||
| ASSERT_WORLD_STOPPED(interp); | ||
| } | ||
|
|
||
| static int | ||
| _Py_Finalize(_PyRuntimeState *runtime) | ||
| { | ||
|
|
@@ -2027,23 +2126,7 @@ _Py_Finalize(_PyRuntimeState *runtime) | |
| // Block some operations. | ||
| tstate->interp->finalizing = 1; | ||
|
|
||
| // Wrap up existing "threading"-module-created, non-daemon threads. | ||
| wait_for_thread_shutdown(tstate); | ||
|
|
||
| // Make any remaining pending calls. | ||
| _Py_FinishPendingCalls(tstate); | ||
|
|
||
| /* The interpreter is still entirely intact at this point, and the | ||
| * exit funcs may be relying on that. In particular, if some thread | ||
| * or exit func is still waiting to do an import, the import machinery | ||
| * expects Py_IsInitialized() to return true. So don't say the | ||
| * runtime is uninitialized until after the exit funcs have run. | ||
| * Note that Threading.py uses an exit func to do a join on all the | ||
| * threads created thru it, so this also protects pending imports in | ||
| * the threads created via Threading. | ||
| */ | ||
|
|
||
| _PyAtExit_Call(tstate->interp); | ||
| make_pre_finalization_calls(tstate); | ||
ericsnowcurrently marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| assert(_PyThreadState_GET() == tstate); | ||
|
|
||
|
|
@@ -2061,7 +2144,7 @@ _Py_Finalize(_PyRuntimeState *runtime) | |
| #endif | ||
|
|
||
| /* Ensure that remaining threads are detached */ | ||
| _PyEval_StopTheWorldAll(runtime); | ||
| ASSERT_WORLD_STOPPED(tstate->interp); | ||
|
|
||
| /* Remaining daemon threads will be trapped in PyThread_hang_thread | ||
| when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ | ||
|
|
@@ -2082,6 +2165,7 @@ _Py_Finalize(_PyRuntimeState *runtime) | |
| _PyThreadState_SetShuttingDown(p); | ||
| } | ||
| _PyEval_StartTheWorldAll(runtime); | ||
| PyMutex_Unlock(&tstate->interp->ceval.pending.mutex); | ||
|
|
||
| /* Clear frames of other threads to call objects destructors. Destructors | ||
| will be called in the current Python thread. Since | ||
|
|
@@ -2441,13 +2525,7 @@ Py_EndInterpreter(PyThreadState *tstate) | |
| } | ||
| interp->finalizing = 1; | ||
|
|
||
| // Wrap up existing "threading"-module-created, non-daemon threads. | ||
| wait_for_thread_shutdown(tstate); | ||
|
|
||
| // Make any remaining pending calls. | ||
| _Py_FinishPendingCalls(tstate); | ||
|
|
||
| _PyAtExit_Call(tstate->interp); | ||
| make_pre_finalization_calls(tstate); | ||
ericsnowcurrently marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if (tstate != interp->threads.head || tstate->next != NULL) { | ||
| Py_FatalError("not the last thread"); | ||
|
|
@@ -2456,6 +2534,8 @@ Py_EndInterpreter(PyThreadState *tstate) | |
| /* Remaining daemon threads will automatically exit | ||
| when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ | ||
| _PyInterpreterState_SetFinalizing(interp, tstate); | ||
| _PyEval_StartTheWorldAll(interp->runtime); | ||
| PyMutex_Unlock(&interp->ceval.pending.mutex); | ||
|
|
||
| // XXX Call something like _PyImport_Disable() here? | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.