gh-80406: Finalise subinterpreters in Py_FinalizeEx()#17575

LewisGaul

Added testcases in Programs/_testembed.c - previously failing, now passing
Loop over subinterpreters in Py_FinalizeEx() and call Py_EndInterpreter()
New flag _PyRuntime.interpreters.allow_new to prevent new subinterpreters from being created during finalisation
Error if calling Py_FinalizeEx() from a subinterpreter (to be changed at a later stage)

https://bugs.python.org/issue36225

Also addresses bpo-38865 and bpo-37776.

Issue: [subinterpreters] Lingering subinterpreters should be implicitly cleared on shutdown #80406

LewisGaul

I need to look into test_audit_subinterpreter() testcase, will do so at some point soon.

…alling Py_Finalize()

LewisGaul

As noted here, it seems test_audit_subinterpreter() was failing because it was trying to call Py_Finalize() from a subinterpreter. Hopefully now fixed by switching back to the main threadstate in that testcase.

ericsnowcurrently

Thanks for working on this! The approach makes sense, including the flag (and where you put it). My comments are basically:

questions about a few details
point out missing test code
a recommendation about the name (and meaning) of the new flag

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.

csabella

@LewisGaul, please take a look at the code reviews and requested changes. Thank you!

LewisGaul

@csabella Yes I haven't forgotten this, I've been a bit busy. I have plans to work on this, hopefully soon.

csabella

@LewisGaul, thank you for the update. I just wanted to make sure you were aware that it had been reviewed. 🙂

…test to test_embed.py

…to finalise-subinterps

ncoghlan

On Sat., 25 Jan. 2020, 7:45 am Eric Snow, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Include/internal/pycore_pystate.h <#17575 (comment)>: > @@ -226,6 +226,7 @@ typedef struct pyruntimestate { If that becomes a problem later then we can adjust, e.g. by using a Python int. */ int64_t next_id; + int finalizing; I guess it's a bit vague. "new" what? Honestly, the flag represents more than just the availability of subinterpreters. So I'd probably feel more comfortable with "ready" (or "available"), which would mean "the full capability of the C-API is available" (including subinterpreters). @ncoghlan <https://github.com/ncoghlan>, what do you think?

My expectation with the naming suggestion is that we use the flag to mean "`Py_NewInterpreter` will now work". It's true it could be seen as ambiguous with the more general meaning "allow new objects" (based on the name of the tp_new slot and associated Python magic method), so I'd also be OK with expanding it to be the more explicit "allow_new_interpreters". I don't think we'd be helping anyone by making the flag itself ambiguous, though, so I think we should stick with the concrete meaning of "`Py_NewInterpreter` will always fail immediately when this is not set".

ericsnowcurrently

I don't think we'd be helping anyone by making the flag itself ambiguous,

Fair enough. I guess I'm thinking about future stuff, but we can address that later (when needed). 😄

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

LewisGaul

I've resolved conflicts with upstream.

Current status:

Question from @vstinner over whether this behaviour should be changed [while there's still special treatment of the 'main' interpreter], see https://bugs.python.org/issue38865#msg364573.
Many additional cases need testing:
- the subinterpreter has multiple threads still running?
- what about daemon threads?
- the subinterpreter's tstate_head is still running?
- someone calls Py_NewInterpreter() while interpreters are being cleaned up?
- someone calls Py_NewInterpreter() while finalization is otherwise still running?
- someone calls Py_NewInterpreter() after finalization is finished?

ericsnowcurrently

Thanks for sticking with this! You're on the right track. I've noted 2 things that should be adjusted. I'll address the test cases separately.

We also discussed some refactoring that would help establish as safer order of operations during runtime finalization (and likewise initialization). However, those can be handled separately.

ericsnowcurrently

Question from @vstinner over whether this behaviour should be changed [while there's still special treatment of the 'main' interpreter], see https://bugs.python.org/issue38865#msg364573.

I don't see Victor's concerns as a problem for this PR. The impact is low and the result is an error before anything happens rather than doing anything improper or unexpected. I'll leave a note in the issue to that effect.

Many additional cases need testing:

the subinterpreter has multiple threads still running?

what about daemon threads?

the subinterpreter's tstate_head is still running?

someone calls Py_NewInterpreter() while interpreters are being cleaned up?

someone calls Py_NewInterpreter() while finalization is otherwise still running?

someone calls Py_NewInterpreter() after finalization is finished?

I still think each of those cases should be covered by tests, except maybe the one about daemon threads. I'm not sure how you would write a test that wouldn't fail sporadically with the current behavior. Maybe block a daemon thread and unblock it in an atexit handler (which would be triggered after non-daemon threads would have already finished. Then you can verify that the daemon thread did not block Py_FinalizeEx() from progressing past the wait-for-threads point.

…interpreter

…interpreters

…reters

vstinner

As I wrote in https://bugs.python.org/issue36225 I'm not excited by the idea of finalizing subinterpreters by executing their object finalizers in the main interpreter.

LewisGaul

As I wrote in https://bugs.python.org/issue36225 I'm not excited by the idea of finalizing subinterpreters by executing their object finalizers in the main interpreter.

Thanks for the input @vstinner. @ericsnowcurrently it would be good to get your thoughts here. I'm happy to change the implementation to satisfy whichever solution we settle on.

python-cla-bot

The following commit authors need to sign the Contributor License Agreement:

Lewis@scphillips.com

github-actions

This PR is stale because it has been open for 30 days with no activity.

StanFromIreland

Thanks for the interesting in contributing, however I'm closing as this is stale, awaiting changes, and CLA unsigned.

LewisGaul added 2 commits November 21, 2019 18:25

Add test suggested by ncoghlan

23af5f5

Finalise sub-interpreters in Py_FinalizeEx()

433663c

the-knights-who-say-ni added the CLA signed label Dec 11, 2019

bedevere-bot added the awaiting review label Dec 11, 2019

LewisGaul added 2 commits December 13, 2019 18:32

Improve test name

48e1cfc

Switch back to main threadstate in test_audit_subinterpreter before c… …

0400634

…alling Py_Finalize()

ericsnowcurrently self-requested a review December 14, 2019 00:08

📜🤖 Added by blurb_it.

b79649c

ericsnowcurrently requested changes Dec 20, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Dec 20, 2019

bedevere-bot added the awaiting changes label Dec 20, 2019

ncoghlan reviewed Dec 30, 2019

View reviewed changes

LewisGaul added 2 commits January 21, 2020 23:35

Markups including: switch from 'finalizing' flag to 'allow_new', add … …

8b1e7d9

…test to test_embed.py

Merge branch 'finalise-subinterps' of github.com:LewisGaul/cpython in… …

fd6073a

…to finalise-subinterps

ericsnowcurrently requested changes Jan 24, 2020

View reviewed changes

soltysh reviewed Feb 7, 2020

View reviewed changes

soltysh mentioned this pull request Feb 7, 2020

Lingering subinterpreters should be implicitly cleared on shutdown ericsnowcurrently/multi-core-python#57

Open

LewisGaul and others added 2 commits October 20, 2020 14:50

Merge branch 'master' into finalise-subinterps

4bbd58f

Use '_' for unused variable in test_embed.py …

1095e66

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

ericsnowcurrently reviewed Oct 22, 2020

View reviewed changes

Fix struct position of 'allow_new' flag

675285d

ericsnowcurrently requested changes Oct 22, 2020

View reviewed changes

LewisGaul added 6 commits October 22, 2020 23:19

Add handling for unsupported case of calling Py_Finalize() from a sub… …

8e21788

…interpreter

Emit resource warning when calling Py_Finalize() with unfinalized sub… …

606c068

…interpreters

Update Py_FinalizeEx() docs

e0789b0

Update test for resource warning when implicitly finalizing subinterp… …

dda99ce

…reters

Tidy up test_finalize_subinterps() testcase

847e8d2

Add testcase for calling Py_Finalize() from a subinterpreter

a2fb0fc

vstinner reviewed Oct 27, 2020

View reviewed changes

LewisGaul added 5 commits November 23, 2020 20:15

Tweak subinterpreters still running ResourceWarning handling

d234528

Make calling PyFinalizeEx() from a subinterpreter a Py_FatalError

46a8619

Acquire interpreters mutex before setting allow_new=0 in PyFinalizeEx()

c89c0e5

Merge remote-tracking branch 'upstream/master' into finalise-subinterps

c285f52

Add back in the 'interp' variable to PyFinalizeEx() to fix the build

95cbfd4

LewisGaul commented Nov 23, 2020

View reviewed changes

vstinner mentioned this pull request Apr 10, 2022

[subinterpreters] Can Py_Finalize() be called if the current interpreter is not the main interpreter? #83046

Closed

ncoghlan mentioned this pull request Apr 10, 2022

[subinterpreters] Lingering subinterpreters should be implicitly cleared on shutdown #80406

Closed

ezio-melotti removed the CLA signed label Jul 13, 2022

encukou changed the title ~~bpo-36225: Finalise subinterpreters in Py_FinalizeEx()~~ Mar 28, 2024

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 7, 2026

StanFromIreland closed this May 3, 2026

Conversation

LewisGaul commented Dec 11, 2019 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LewisGaul commented Dec 11, 2019

Uh oh!

LewisGaul commented Dec 13, 2019

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 20, 2019

Uh oh!

csabella commented Jan 12, 2020

Uh oh!

LewisGaul commented Jan 12, 2020

Uh oh!

csabella commented Jan 12, 2020

Uh oh!

ncoghlan commented Jan 25, 2020 via email

Uh oh!

ericsnowcurrently commented Jan 27, 2020

Uh oh!

LewisGaul commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Oct 27, 2020

Uh oh!

LewisGaul commented Nov 23, 2020

Uh oh!

python-cla-bot Bot commented Apr 6, 2025

Uh oh!

github-actions Bot commented Apr 7, 2026

Uh oh!

StanFromIreland commented May 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

LewisGaul commented Dec 11, 2019 •

edited by bedevere-app Bot

Loading

LewisGaul commented Oct 20, 2020 •

edited

Loading

ericsnowcurrently commented Oct 22, 2020 •

edited

Loading