Skip to content

bpo-36479: Exit threads when interpreter is finalizing rather than runtime #12679

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

bpo-36479: Exit threads when interpreter is finalizing rather than runtime #12679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a19bbd4
Exit threads when interpreter is finalizing rather than runtime.
nanjekyejoannah May 5, 2019
515e8bb
📜🤖 Added by blurb_it.
blurb-it[bot] May 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core and Builtins/2019-05-05-16-19-58.bpo-36479.FPztBY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Exit threads when interpreter is finalizing rather than runtime.
5 changes: 4 additions & 1 deletion Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,11 @@ _PyEval_FiniThreads(void)
static inline void
exit_thread_if_finalizing(PyThreadState *tstate)
{
/* Get interpreter pointer */
PyInterpreterState *interp = tstate->interp;

/* _Py_Finalizing is protected by the GIL */
if (_Py_IsFinalizing() && !_Py_CURRENTLY_FINALIZING(tstate)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, _Py_CURRENTLY_FINALIZING() becomes even more weird that it was: you rely on a global and unique PyRuntimeState to check which thread is currently being finalized. I don't think that it's correct. It should be possible to finalize 2 interpreters in parallel which both have their own thread.

For me, it would make more sense to move "PyThreadState *finalizing;" from _PyRuntimeState to PyInterpreterState and remove PyInterpreterState.finalizing.

What's your call Eric? @ericsnowcurrently

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thread state in finalizing actually means "This is the thread that is finalizing the runtime", so other threads can easily tell that another thread is responsible for cleaning things up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However @pablogsal pointed out a more serious problem here, which is that simply moving the existing check will result in:

  • potentially causing threads belonging to other subinterpreters to exit
  • potentially causing threads belonging to the embedding application to exit

The latter problem already exists, but moving the cleanup to EndInterpreter makes it easier to hit.

if (interp->finalizing && !_Py_CURRENTLY_FINALIZING(tstate)) {
drop_gil(tstate);
PyThread_exit_thread();
}
Expand Down
12 changes: 7 additions & 5 deletions Python/pylifecycle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,16 +1131,18 @@ Py_FinalizeEx(void)
return status;
}

/* Get current thread state and interpreter pointer */
PyThreadState *tstate = _PyThreadState_GET();
PyInterpreterState *interp = tstate->interp;

// Wrap up existing "threading"-module-created, non-daemon threads.
wait_for_thread_shutdown();
interp->finalizing = 1;


// Make any remaining pending calls.
_Py_FinishPendingCalls();

/* Get current thread state and interpreter pointer */
PyThreadState *tstate = _PyThreadState_GET();
PyInterpreterState *interp = tstate->interp;

/* 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
Expand Down Expand Up @@ -1546,10 +1548,10 @@ Py_EndInterpreter(PyThreadState *tstate)
Py_FatalError("Py_EndInterpreter: thread is not current");
if (tstate->frame != NULL)
Py_FatalError("Py_EndInterpreter: thread still has a frame");
interp->finalizing = 1;

// Wrap up existing "threading"-module-created, non-daemon threads.
wait_for_thread_shutdown();
interp->finalizing = 1;

call_py_exitfuncs(interp);

Expand Down