bpo-32604: Expose the subinterpreters C-API in a "private" stdlib module.

[ericsnowcurrently]

https://bugs.python.org/issue32604

[pitrou]

There should be a warning about the module being private, and/or the API being subject to change without notice, IMHO.

[ericsnowcurrently]

Good point. For the most part I was following the precedent of the _thread module. However, in this case the module is relatively unstable still and there is no "interpreters" module yet. So a warning makes sense.

[ncoghlan]

Given the not-yet-approved PEP, I think it would be a good idea to use a name that makes it clear that this really is intended as an API for the CPython test suite, rather than for general use.

That would mean going with something like _xxsubinterpreters for now.

[ncoghlan]

Note: one consequence of that is that redistributors that remove the test suite would likely remove this module as well, and I think that's a desirable outcome: we want this as a learning tool for the further development of PEP 554 as a proposal for 3.8 (and to find and fix bugs in the subinterpreter support in 3.7), not as an inadvertent commitment to a particular API.

[pitrou]

I don't understand where the id variable is coming from... is it the built-in id function?

[ericsnowcurrently]

Good catch. That's a typo that I didn't notice because the resulting exception in the thread does not cause the process to fail.

FYI, adding the missing "id = _interpreters.create()" line causes the test to fail (correctly); the subinterpreter didn't get cleaned up before we reached Py_Finalize(), resulting in Py_FatalError. I'll sort that out separately.

[1st1]

Can we call the function run as per the PEP?

[ericsnowcurrently]

Note that the PEP is all about the high-level API we want to expose to users. In contrast, run_string() is the low-level function focused strictly as an analog to PyRun_String*(). As we support other runnable things (e.g. code objects, functions), we will add corresponding low-level functions. The eventual high-level method (i.e. Interpreter.run() will call the appropriate low-level function.

Also, I'm removing the docs for now.

[1st1]

The actual layout of the struct should go into an "internal" header file:

"Include/pystate.h":

typedefstruct_xid_PyCrossInterpreterData;

"Include/internal/pystate.h":

struct_xid {
void*data;
// etc
}

[ericsnowcurrently]

I haven't touched Include/pystate.h. If you are suggesting that I move the typedef (but not the layout) there, I don't see any benefit to that for 3.7. This is not a public API for now.

[1st1]

You don't need to check for NULL here, PyThreadState_Get aborts if it can't lookup the state.

[ericsnowcurrently]

fixed

[1st1]

per PEP 7 please use curly braces always :)

[ericsnowcurrently]

fixed

[1st1]

Check if shared is NULL

[ericsnowcurrently]

fixed

[1st1]

Redundant? And need to free err?

[ericsnowcurrently]

I'd meant to return err on the next line rather than NULL. :)

[ericsnowcurrently]

FYI, I've temporarily removed the Windows support to see if anything else breaks. I'll add it back in soon. I do not plan on merging this without it.

[ericsnowcurrently]

@zooba I suspect my problem is with Py_BUILD_CORE. When I don't set it I don't get the symbols out of Include/internal. When I do set it I end up missing stuff from python37.lib at link time. I tried adding a "Link" directive right below where I set Py_BUILD_CORE, but that did not help. :/ Any ideas on what I need to do?

[ericsnowcurrently]

@python/windows-team Any of you have ideas on how I can get this PR to link on Windows?

[ericsnowcurrently]

In the interest of the feature freeze I'm going to land the PR without Windows support. :/ The risk for beta1 is low since the PR does not have any OS-specific code and the new module is only for use in the test suite. I'll work on getting Windows support done ASAP.

[ncoghlan]

Something we'll need to keep an eye on here is not re-using interpreter IDs within the life of a process (otherwise we'll still run into issues with deleted interpreters).

Not that I've ever worked on systems where we introduced bugs by adding a freelist for objects where we assumed IDs would never be re-used or anything ;)

[ericsnowcurrently]

We picked int64_t so that it was both really large and would overflow to negative. When the next ID is negative PyInterpreterState_New() raises RuntimError.