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

LewisGaul · 2019-12-11T16:55:38Z

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 · 2019-12-11T17:49:07Z

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

…alling Py_Finalize()

LewisGaul · 2019-12-13T18:35:46Z

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

Python/pylifecycle.c

ericsnowcurrently · 2019-12-20T19:50:55Z

Python/pylifecycle.c

+        next_interp = PyInterpreterState_Next(subinterp);
+        if (subinterp != PyInterpreterState_Main()) {
+            PyThreadState_Swap(subinterp->tstate_head);
+            Py_EndInterpreter(subinterp->tstate_head);


This fails if the interp.tstate_head is still running (has a frame). We may want to consider a more graceful approach to dealing with subinterpreters that are still doing work.

Hmm yes, do you have any further thoughts on this?

I'll have to give it more thought (and take the time to queue up details into my mental cache 😄).

We've recently hit this problem with lingering subinterpreter with existing frames.

I solved it by adding Py_CLEAR before the Py_EndInterpreter call (which is similar to how PyThreadState_Clear handles existing frames) and now it seems to work and end gracefully. While that might not be the best solution (and I don't see in subinterpreters much to see possible issues with that), I guess it's not worse than Fatal error.

@ericsnowcurrently any new thoughts here? We might be better off having a call to chat through everything :)

Python/pylifecycle.c

Include/internal/pycore_pystate.h

Python/pylifecycle.c

ericsnowcurrently · 2019-12-20T20:24:30Z

Programs/_testembed.c

@@ -1181,10 +1222,14 @@ static int test_audit_subinterpreter(void)
    PySys_AddAuditHook(_audit_subinterpreter_hook, NULL);
    _testembed_Py_Initialize();

+    PyThreadState *mainstate = PyThreadState_Get();


You may want to double-check with @zooba on his intention here. It's pretty important to make sure that the auditing functionality works as expected.

Hi @zooba, as a consequence of my changes here, test_audit_subinterpreter() in _testembed.c started failing.

The change I'm making is to make Py_Finalize() implicitly clean up subinterpreters.

In the test, multiple subinterpreters are created, and then Py_Finalize() is called from the last-created subinterpreter. It seems there's currently an issue with calling Py_Finalize() from a subinterpreter (see bpo-37776), which caused this test to fail when getting Py_Finalize() to clean up subinterpreters.

The test passes if Py_Finalize() is instead called from the main interpreter tstate - which is the change I've made to the test. Just wanting to check whether that's taking anything away from what's intentionally being checked by this test?

See #19063 from @vstinner which was not merged, but also proposed to change the logic of this testcase. It seems like this testcase is doing something that is not currently working, and according to bpo-38865#msg357331 may not be supported in general?

I also confirmed that this test still fails on my branch without this change.

Programs/_testembed.c

ericsnowcurrently · 2019-12-20T20:34:45Z

Programs/_testembed.c

+        PyGILState_Release(gilstate);
+
+        PyEval_RestoreThread(mainstate);
+        Py_Finalize();


This certainly helps verify that finalization still works. There should probably also be something verifying that the subinterpreters were properly cleaned up at the beginning of finalization. (...perhaps with some artifact generated when each sub-interp is finalized.)

Also, what about the case where:

the subinterpreter has multiple threads still running?

what about daemon threads? (yeah, it's mean of me to ask 😉)

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?

Perhaps registering an "atexit" handler in each subinterpreter that prints something, and then confirming in the Python test case code that all the subinterpreter exit messages appear before the main interpreter's exit message?

This all sounds worth checking in a test, but I'm unclear how to implement it. Any advice would be appreciated, specifically:

Should all of the test logic be in _testembed.c, or should that just be performing the C API calls with most of the test logic being in test_embed.py?

I'm only aware of how to register an atexit handler from Python code.

How should the interaction between test_embed.py and _testembed.c work?

With a better understanding of the above I may be able to have a go at covering the above points in the tests, although it'll likely take quite a lot of thought given I'm pretty new to the C API! Any further guidance very welcome, and I can hopefully get this finished off without such a delay this time.

Should all of the test logic be in _testembed.c, or should that just be performing the C API calls with most of the test logic being in test_embed.py?

Put as much logic as you can in the Python code. _testembed.c should mostly be only what can't be done from Python (with exceptions where practicality dictates more).

I'm only aware of how to register an atexit handler from Python code.

You can call Python code from C if needed. Import the atexit module, get the appropriate function, and call it, all using the C-API. We do the same thing in various places, like Python/import.c. For me (not a C expert) searching the code base has always been the easiest way to see how to do something. 😄

(That assumes there isn't a C-API for atexit handlers.)

How should the interaction between test_embed.py and _testembed.c work?

I'll need more context.

This might be a good reason to pair up on a video call. Then we could walk through this stuff a bit more efficiently. What do you think?

At least some of the embedding tests already use PyRun_SimpleString() to run Python code inside the created interpreter, and that's also what I had in mind for the suggested atexit test case above.

It looks like there's a lot of considerations and things to check here, also fleshed out by Victor's message at bpo-36225#msg371571. Does everything here need addressing in this one PR, or can some of these points be split into separate issues to follow this fix? This feels like rather a lot to tackle all in one go - which of the cases you listed would you suggest starting with @ericsnowcurrently (perhaps the simplest to test!)?

Programs/_testembed.c

bedevere-bot · 2019-12-20T20:42:34Z

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.

Include/internal/pycore_pystate.h

csabella · 2020-01-12T13:35:26Z

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

LewisGaul · 2020-01-12T13:45:46Z

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

csabella · 2020-01-12T13:54:26Z

@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

Lib/test/test_embed.py

ncoghlan · 2020-01-25T15:41:58Z

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 · 2020-01-27T16:37:18Z

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

soltysh · 2020-02-07T15:08:23Z

Python/pylifecycle.c

@@ -1450,8 +1467,10 @@ new_interpreter(PyThreadState **tstate_p)
    }
    _PyRuntimeState *runtime = &_PyRuntime;

-    if (!runtime->initialized) {


I'd not remove this condition initialized is separate from allow_new, so having this check here still makes sense.

Sorry for any confusion, @soltysh. _PyRuntimeState,interpreters.allow_new was added in this PR to solve the problem of interpreters being created when they shouldn't be (e.g. during runtime finalization). You could say the flag specifically means "new_interpreter() can be called currently". 😄 So the change here is correct.

@ericsnowcurrently so runtime is always a single object (like an uber-object) and then you can create multiple interpreters. Right, but that still requires the runtime to be initialized. Even though the situation should not happen, because I'd assume the first invocation of python would initialize the runtime, it should not hurt having this here. Unless my thinking is wrong here.

I suppose your suggestion @soltysh would be to have two separate checks on runtime->initialized and runtime->interpreters.allow_new?

It's a while ago now, but I think my reasoning here was that this 'allow_new' flag encapsulates all information about whether a new interpreter can be created, so there should be no need to check things like whether the runtime is initialised (since 'allow_new' is only set to true when the runtime is initialised). Does that sounds reasonable?

I could be persuaded to change this if there are better suggestions :)

Co-authored-by: Eric Snow <[email protected]>

LewisGaul · 2020-10-20T16:03:28Z

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?

Include/internal/pycore_runtime.h

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.

Include/internal/pycore_runtime.h

Python/pylifecycle.c

ericsnowcurrently · 2020-10-22T21:32:29Z

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 · 2020-10-27T02:39:21Z

Python/pylifecycle.c

+    }
+
+    // Finalize sub-interpreters.
+    runtime->interpreters.allow_new = 0;


It may be safer to acquire _PyRuntime.interpreters.mutex beforing setting this variable. It may be better to move this code into pystate.c, since this file control the list of interpreters.

I presume you're suggesting refactoring the 'finalize subinterpreters' logic? It would seem to me a function that does 'finalizing' belongs better in pylifecycle than pystate? I believe this refactoring was referred to by Eric above, where he says this can be addressed separately.

I've added in an acquisition of the lock here for now.

Python/pylifecycle.c

vstinner · 2020-10-27T02:49:19Z

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 · 2020-11-23T20:03:04Z

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.

LewisGaul · 2020-11-23T21:20:54Z

Python/pylifecycle.c

+         * before finalizing the runtime.
+         */
+        if (PyErr_ResourceWarning(NULL, 1,
+                                  "extra %zd interpreters", num_destroyed)) {


I'm not sure why, but for some reason this warning isn't being output in the tests on Windows only. Anyone have any ideas?

python-cla-bot · 2025-04-06T14:35:09Z

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

[email protected]

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

Include/internal/pycore_pystate.h Outdated Show resolved Hide resolved

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

Lib/test/test_embed.py Outdated Show resolved Hide resolved

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 <[email protected]>

ericsnowcurrently reviewed Oct 22, 2020

View reviewed changes

Include/internal/pycore_runtime.h Outdated Show resolved Hide resolved

Fix struct position of 'allow_new' flag

675285d

ericsnowcurrently requested changes Oct 22, 2020

View reviewed changes

Include/internal/pycore_runtime.h Show resolved Hide resolved

Python/pylifecycle.c Outdated Show resolved Hide resolved

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()~~ gh-80406: Finalise subinterpreters in Py_FinalizeEx() Mar 28, 2024

Uh oh!

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

Are you sure you want to change the base?

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

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Dec 20, 2019

Uh oh!

Uh oh!

csabella commented Jan 12, 2020

Uh oh!

LewisGaul commented Jan 12, 2020

Uh oh!

csabella commented Jan 12, 2020

Uh oh!

Uh oh!

ncoghlan commented Jan 25, 2020 via email

Uh oh!

ericsnowcurrently commented Jan 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

LewisGaul commented Dec 11, 2019 •

edited by bedevere-app bot

Loading

LewisGaul Oct 20, 2020 •

edited

Loading

LewisGaul commented Oct 20, 2020 •

edited

Loading

ericsnowcurrently commented Oct 22, 2020 •

edited

Loading

LewisGaul Nov 23, 2020 •

edited

Loading