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

[nanjekyejoannah]

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

[nanjekyejoannah]

cc @ericsnowcurrently , @ncoghlan

[ericsnowcurrently]

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).

[vstinner]

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

[ncoghlan]

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.

[ncoghlan]

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.

[bedevere-bot]

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.

[ncoghlan]

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)

[vstinner]

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.

[ncoghlan]

@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).

[nanjekyejoannah]

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