Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-32604: Expose the subinterpreters C-API in a "private" stdlib module.#1748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-32604: Expose the subinterpreters C-API in a "private" stdlib module. #1748
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ericsnowcurrently commented May 23, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
e0611a8 to bee1baaComparebee1baa to 060a720CompareDoc/library/_interpreters.rst Outdated
| @@ -0,0 +1,69 @@ | |||
| :mod:`_interpreters` --- Low-level interpreters API | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a warning about the module being private, and/or the API being subject to change without notice, IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Lib/test/test__interpreters.py Outdated
| import threading | ||
| import _interpreters | ||
| def f(): | ||
| _interpreters.run_string(id, dedent(''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand where the id variable is coming from... is it the built-in id function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
060a720 to 2fd0e64Compare114bda4 to 52ca360Compare52ca360 to 9991d75Compare9991d75 to e9d9b04Compare81de3c8 to bfc025fComparee05d02e to 396e294Compare5266080 to c86647aCompareff4ae72 to 0981dbfCompare0981dbf to 0be6314CompareDoc/library/_interpreters.rst Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call the function run as per the PEP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Include/internal/pystate.h Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Modules/_interpretersmodule.c Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to check for NULL here, PyThreadState_Get aborts if it can't lookup the state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Modules/_interpretersmodule.c Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per PEP 7 please use curly braces always :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Modules/_interpretersmodule.c Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if shared is NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Modules/_interpretersmodule.c Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant? And need to free err?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd meant to return err on the next line rather than NULL. :)
8ae8507 to 2ebc68cCompareericsnowcurrently commented Jan 27, 2018
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. |
ebbf591 to 9718b91Compareedec009 to 092fcccCompare092fccc to b07e928Compareericsnowcurrently commented Jan 27, 2018
@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 commented Jan 29, 2018
@python/windows-team Any of you have ideas on how I can get this PR to link on Windows? |
ericsnowcurrently commented Jan 30, 2018
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. |
| // | ||
| // We use the ID rather than the PyInterpreterState to avoid issues | ||
| // with deleted interpreters. | ||
| int64_tinterp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
https://bugs.python.org/issue32604