This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: [subinterpreters] Exit threads when interpreter is finalizing rather than runtime.
Type: behavior Stage: resolved
Components: Interpreter Core, Subinterpreters Versions: Python 3.8
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, ncoghlan, pablogsal, vstinner
Priority: normal Keywords: patch

Created on 2019-03-29 21:59 by eric.snow, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 12679 closed nanjekyejoannah, 2019-04-03 19:58
Messages (6)
msg339155 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-03-29 21:59
Currently when a thread acquires the GIL, it subsequently exits if the runtime is finalizing.  This helps with some cases like with stopping daemon threads.

This behavior should instead be triggered by the thread's interpreter finalizing rather than the runtime.  This implies the following changes:

* in Py_EndInterpreter() move "interp-finalizing = 1;" to right after the call to "wait_for_thread_shutdown()"
* in Py_FinalizeEx() add "interp-finalizing = 1;" right after the call to "wait_for_thread_shutdown()"
* update _PyEval_EvalFrameDefault() (the eval loop) to check "interp->finalizing" instead of the runtime
* likewise update PyEval_RestoreThread() (as well as PyEval_AcquireThread() and PyEval_AcquireLock(); see #36475)

The check will actually need to change from this:

  if (_Py_IsFinalizing() && !_Py_CURRENTLY_FINALIZING(tstate)) {

to look like this:

  if (interp->finalizing && !_Py_CURRENTLY_FINALIZING(tstate)) {

If we could guarantee that only the main thread will ever call Py_FinalizeEx() then it would look more like this:

  if (interp->finalizing && tstate.id != _PyRuntime.main_thread {
msg341882 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2019-05-08 14:57
Pablo pointed out a problem with this change at the PyCon sprints: the thread cleanup code doesn't currently distinguish between "Python created daemon thread" and "thread created by the embedding application".

That's already somewhat problematic if an embedding application goes through multiple initialize/finalize cycles, but it's even more critical to make the distinction correctly if we move the daemon thread cleanup to interpreter shutdown.
msg341884 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2019-05-08 15:04
Another potential issue here is if a thread belonging to another interpreter attempts to access the interpreter being cleaned up.
msg342039 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-10 02:06
IMHO PR 12679 change should wait until Py_EndInterpreter() is reworked to better cleanup its state and its threads.
msg364589 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-19 02:14
I suggest to close this issue, I don't think that it can be implemented because of daemon threads.

--

> This behavior should instead be triggered by the thread's interpreter finalizing rather than the runtime.

What is the motivation for that?

--

take_gil() cannot access interp->finalizing since interp memory is freed during Python finalization.

take_gil() can only rely on a global variable like _PyRuntime to check if a daemon thread must exit during the Python finalization.

I modified take_gil() recently to fix 2 or 3 different crashes caused by daemon threads during Python finalization. Extract of ceval.c:

/* Check if a Python thread must exit immediately, rather than taking the GIL
   if Py_Finalize() has been called.

   When this function is called by a daemon thread after Py_Finalize() has been
   called, the GIL does no longer exist.

   tstate must be non-NULL. */
static inline int
tstate_must_exit(PyThreadState *tstate)
{
    /* bpo-39877: Access _PyRuntime directly rather than using
       tstate->interp->runtime to support calls from Python daemon threads.
       After Py_Finalize() has been called, tstate can be a dangling pointer:
       point to PyThreadState freed memory. */
    _PyRuntimeState *runtime = &_PyRuntime;
    PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(runtime);
    return (finalizing != NULL && finalizing != tstate);
}


And simplified take_gil():

static void
take_gil(PyThreadState *tstate)
{
    if (tstate_must_exit(tstate)) {
        /* bpo-39877: If Py_Finalize() has been called and tstate is not the
           thread which called Py_Finalize(), exit immediately the thread.

           This code path can be reached by a daemon thread after Py_Finalize()
           completes. In this case, tstate is a dangling pointer: points to
           PyThreadState freed memory. */
        PyThread_exit_thread();
    }

    (... logic to acquire the GIL ...)

    if (must_exit) {
        /* bpo-36475: If Py_Finalize() has been called and tstate is not
           the thread which called Py_Finalize(), exit immediately the
           thread.

           This code path can be reached by a daemon thread which was waiting
           in take_gil() while the main thread called
           wait_for_thread_shutdown() from Py_Finalize(). */
        drop_gil(ceval, ceval2, tstate);
        PyThread_exit_thread();
    }
}
msg396680 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-06-28 23:54
I close the issue.
History
Date User Action Args
2022-04-11 14:59:13adminsetgithub: 80660
2021-06-28 23:54:03vstinnersetstatus: open -> closed
resolution: wont fix
messages: + msg396680

stage: patch review -> resolved
2020-05-15 00:44:59vstinnersetcomponents: + Subinterpreters
title: Exit threads when interpreter is finalizing rather than runtime. -> [subinterpreters] Exit threads when interpreter is finalizing rather than runtime.
2020-03-19 02:14:12vstinnersetmessages: + msg364589
2019-05-10 02:06:08vstinnersetmessages: + msg342039
2019-05-08 15:04:22ncoghlansetmessages: + msg341884
2019-05-08 14:57:24ncoghlansetnosy: + pablogsal, ncoghlan
messages: + msg341882
2019-04-29 08:54:56vstinnersetnosy: + vstinner
2019-04-03 19:58:38nanjekyejoannahsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request12606
2019-03-29 21:59:02eric.snowcreate