bpo-36479: Exit threads when interpreter is finalizing rather than runtime by nanjekyejoannah · Pull Request #12679 · python/cpython · GitHub

I have added changes to exit threads when the interpreter is finalizing.

I have halted to update this until PR until GH-12667 is merged.

https://bugs.python.org/issue36479

Please check _Py_CURRENTLY_FINALIZING(). I suspect that it doesn't do the right thing. It may only matter for the main interpreter (e.g. during runtime finalization).

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

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.

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.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Blocking merge as per https://bugs.python.org/issue36479#msg341882 (we need to actively constrain the cleanup to daemon threads created by the interpreter being cleaned up, and simply report an error to other threads)

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.

I don't understand what is "runtime" here. _PyRuntimeState doesn't contain threads: PyInterpreterState does. IMHO the field should be moved from _PyRuntimeState to PyInterpreterState.

Right now, Py_EndInterpreter() doesn't call Py_FinalizeEx(): but that's wrong. To really have isolated interpreter, each interpreter should clear everything it owns. For example, each interpreter should join all threads that it spawned.

@vstinner PyFinalize_Ex kills the entire runtime, so we can't call it just because a subinterpreter went away. Instead, we have to migrate cleaning up particular resources from PyFinalize_Ex to Py_EndInterpreter as this PR does.

However, I also agree that for this PR to work there needs to be a new field in the individual interpreter state that tracks which thread is cleaning up that particular interpreter.

And then once we have that per-interpreter field and are using it consistently, then we can ask if we still need a separate runtime level "finalizing" marker, or if the finalizing marker for the main interpreter will be sufficient (which will mean addressing the fact that Py_EndInterpreter never actually gets called on the main interpreter - Py_FinalizeEx has its own copy of the pieces it wants to use, and does a bunch of other more destructive things while keeping at least one thread state alive until the last possible moment).

Closing this for now. I may revisit in the future in a new PR. Anyone else can look at this.

the-knights-who-say-ni added the CLA signed label Apr 3, 2019

bedevere-bot added the awaiting review label Apr 3, 2019

nanjekyejoannah changed the title bpo-36479: Exit threads when interpreter is finalizing rather than runtime [WIP]bpo-36479: Exit threads when interpreter is finalizing rather than runtime Apr 3, 2019

ericsnowcurrently reviewed Apr 3, 2019

View reviewed changes

nanjekyejoannah force-pushed the issue36479 branch 2 times, most recently from a132f03 to 68bc067 Compare April 25, 2019 17:19

nanjekyejoannah changed the title [WIP]bpo-36479: Exit threads when interpreter is finalizing rather than runtime bpo-36479: Exit threads when interpreter is finalizing rather than runtime [Witing on merge of GH-12667] Apr 25, 2019

nanjekyejoannah force-pushed the issue36479 branch 3 times, most recently from 607a345 to b8a1690 Compare April 25, 2019 22:25

nanjekyejoannah changed the title bpo-36479: Exit threads when interpreter is finalizing rather than runtime [Witing on merge of GH-12667] bpo-36479: Exit threads when interpreter is finalizing rather than runtime [Waiting on merge of GH-12667] Apr 25, 2019

ncoghlan reviewed Apr 26, 2019

View reviewed changes

Comment thread Doc/c-api/init.rst Outdated

Exit threads when interpreter is finalizing rather than runtime.

a19bbd4

nanjekyejoannah force-pushed the issue36479 branch from 5f145b3 to a19bbd4 Compare May 5, 2019 16:16

📜🤖 Added by blurb_it.

515e8bb

nanjekyejoannah changed the title bpo-36479: Exit threads when interpreter is finalizing rather than runtime [Waiting on merge of GH-12667] bpo-36479: Exit threads when interpreter is finalizing rather than runtime May 5, 2019

vstinner requested changes May 6, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels May 6, 2019

ncoghlan requested changes May 8, 2019

View reviewed changes

csabella added the DO-NOT-MERGE label Jun 1, 2019

nanjekyejoannah closed this Jun 24, 2019

bpo-36479: Exit threads when interpreter is finalizing rather than runtime by nanjekyejoannah · Pull Request #12679 · python/cpython · GitHub

Uh oh!

Uh oh!

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

bpo-36479: Exit threads when interpreter is finalizing rather than runtime#12679
nanjekyejoannah wants to merge 2 commits into
python:masterfrom
nanjekyejoannah:issue36479

nanjekyejoannah commented Apr 3, 2019 •
edited
Loading

Uh oh!

Uh oh!

nanjekyejoannah commented Apr 3, 2019

Uh oh!

ericsnowcurrently Apr 3, 2019

Uh oh!

Uh oh!

vstinner May 6, 2019

Uh oh!

ncoghlan May 8, 2019

Uh oh!

ncoghlan May 8, 2019

Uh oh!

bedevere-bot commented May 6, 2019

Uh oh!

ncoghlan left a comment

Uh oh!

vstinner commented May 10, 2019

Uh oh!

ncoghlan commented May 10, 2019

Uh oh!

nanjekyejoannah commented Jun 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

+                  PyInterpreterState *interp = tstate->interp;
                   /* _Py_Finalizing is protected by the GIL */
-                  if (_Py_IsFinalizing() && !_Py_CURRENTLY_FINALIZING(tstate)) {

bpo-36479: Exit threads when interpreter is finalizing rather than runtime by nanjekyejoannah · Pull Request #12679 · python/cpython · GitHub

Uh oh!

Uh oh!

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

bpo-36479: Exit threads when interpreter is finalizing rather than runtime#12679nanjekyejoannah wants to merge 2 commits intopython:masterpython/cpython:masterfrom nanjekyejoannah:issue36479nanjekyejoannah/cpython:issue36479Copy head branch name to clipboard

Conversation

nanjekyejoannah commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanjekyejoannah commented Apr 3, 2019

Uh oh!

ericsnowcurrently Apr 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner May 6, 2019

Choose a reason for hiding this comment

Uh oh!

ncoghlan May 8, 2019

Choose a reason for hiding this comment

Uh oh!

ncoghlan May 8, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 6, 2019

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented May 10, 2019

Uh oh!

ncoghlan commented May 10, 2019

Uh oh!

nanjekyejoannah commented Jun 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bpo-36479: Exit threads when interpreter is finalizing rather than runtime#12679
nanjekyejoannah wants to merge 2 commits into
python:masterfrom
nanjekyejoannah:issue36479

nanjekyejoannah commented Apr 3, 2019 •
edited
Loading