GeezerVison

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

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.

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!)?

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

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.

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?