Skip to content
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

gh-123672: Clarify the usage of PyGILState* for subinterpreters #123728

Closed
wants to merge 23 commits into from

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Sep 5, 2024

@encukou, this needs skip news, and backport to both 3.12 and 3.13.


📚 Documentation preview 📚: https://cpython-previews--123728.org.readthedocs.build/

@encukou encukou added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Sep 5, 2024
Doc/c-api/init.rst Outdated Show resolved Hide resolved
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
picnixz
picnixz previously approved these changes Sep 5, 2024
Doc/c-api/init.rst Outdated Show resolved Hide resolved
Doc/c-api/init.rst Outdated Show resolved Hide resolved
@ZeroIntensity
Copy link
Member Author

OK, as far as I can see, PyGILState_Ensure always returns the GIL state of the global interpreter, regardless of whether subinterpreters are active. Then, it seems that Py_NewInterpreterFromConfig releases the GIL of the global interpreter, and switches to the new one; this breaks things when using PyGILState_Release. Instead, it look likes you're supposed to call Py_EndInterpreter and omit any call to Release (this makes sense, if you think about it -- releasing will prevent you from shutting down the interpreter, and shutting down the interpreter indirectly releases it.) I've documented all of this now.

Doc/c-api/init.rst Outdated Show resolved Hide resolved
Doc/c-api/init.rst Outdated Show resolved Hide resolved
Doc/c-api/init.rst Outdated Show resolved Hide resolved
@picnixz picnixz dismissed their stale review September 7, 2024 17:14

The content changed.

ZeroIntensity and others added 3 commits September 7, 2024 13:25
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@encukou
Copy link
Member

encukou commented Sep 9, 2024

Keep in mind that you're writing the specification here; basing it on the current implementation is not necessarily the right thing to do. The implementation might be buggy, and some parts of it aren't thought out and tested. Be careful when promising that the API does something.

The current docs say “mixing multiple interpreters and the PyGILState_* API is unsupported”. Specifically, this means that creating a subinterpreter after PyGILState_Ensure is wrong: you should not do it at all, rather than omit PyGILState_Release if you do it.
See also the Bugs and Caveats section elsewhere on the page, repeating the incompatibility.

I don't think we can simply allow mixing the two APIs without any change to the code & tests.
“Activating Python from a callback, when there are multiple interpreters” is an unsupported feature, not an undocumented one: where the motivating issues ask “How should I...”, the answer is “sorry, you currently can't”.

Would you like to design this functionality, and the API for it? (Careful, I picked up a similar task 10 years ago and it basically took over my life...)

If no interpreter state has been initialized for the thread, then this function returns the state of the global interpreter

The term we use for “global interpreter” is the “main” interpreter. (And in my experience, any time we make the main interpreter special, except runtime init/teardown, we're creating technical debt.)

Activating the “main” interpreter by default is dangerous behavour: it's only valid in extensions that originally run in the “main” interpreter.
IMO, it follows that any extension that uses PyGILState_Ensure should restrict itself to only be loadable in the main interpreter -- otherwise there's a chance that PyGILState_Ensure will put them in an interpreter they didn't expect.
So, we should probably link to the new Py_MOD_MULTIPLE_INTERPRETERS_NOT_SUPPORTED here.

And perhaps Multi-phase initialization docs, which currently say

All modules created using multi-phase initialization are expected to support sub-interpreters.

... should specifically note that it's incompatible with PyGILState_*, unless the new Py_MOD_MULTIPLE_INTERPRETERS_NOT_SUPPORTED is used.

cc @ericsnowcurrently

@ericsnowcurrently
Copy link
Member

I'm mostly sure I fixed the situation with PyGILState_Ensure() and subinterpreters for 3.12 by moving us to a thread-local for the current thread state. I didn't think to update the docs. Of course, we should verify my assertion first.

(I'll reply further when I have a chance next week; this is a busy week of family stuff for me.)

@ZeroIntensity
Copy link
Member Author

Specifically, this means that creating a subinterpreter after PyGILState_Ensure is wrong

Oh! Though, it seems to work right now without issue. Is this something that's sort of an accident? If so, would it be that difficult to stabilize? (I'm guessing we just need to add some tests, and of course document it if that's the case.)

Would you like to design this functionality, and the API for it?

Is there all that much we need to design? The easy solution might just be to force PyGILState_Ensure to put you in the main interpreter if there’s no interpreter state for the thread. If not, I guess a PyGILState_EnsureMain could suffice.

In regards to this PR, maybe we should just document that if you use PyGILState_Ensure followed by creating a subinterpreter, then the extension must indicate that it’s incompatible with being loaded from a subinterpreter.

Co-authored-by: Savannah Ostrowski <savannahostrowski@gmail.com>
@encukou
Copy link
Member

encukou commented Sep 24, 2024

OK, so I just talked to Eric and I understand more about the issue now :)

The fix for PyGILState_Ensure was that Python now keeps track of the thread state in each thread, and will PyGILState_Ensure will activate the interpreter for the current thread.
If it's a fresh thread, in which Python did not run yet, then PyGILState_Ensure will give you... the main interpreter? I'm not clear on which one.

Here's a use case that Eric has not considered, which we need to test and document, and possibly add new API for:

  • You use a C library, to which you give a callback function and a PyObject* argument
  • The library calls the callback with the given argument, in an arbitrary thread (a fresh one, or one in which Python already ran).

In this case, we must somehow ensure that the PyObject* is only used in the interpreter it belongs to. But, PyGILState_Ensure does not do that.

IMO, the best way forward is:

  • Ensure there's API to get the current interpreter ID (I'm not sure if we have that)
  • Add a new API function that works like PyGILState_Ensure, but takes an explicit interpreter ID and switches to the given interpreter. (Note that an interpreter ID can become invalid, so this function can fail -- but can't exception mechanism for the failure. It can probably reuse PyStatus.)
  • Keep docs that say the current PyGILState_Ensure is incompatible with interpreters. But suggest the new alternative.

@ZeroIntensity
Copy link
Member Author

If it's a fresh thread, in which Python did not run yet, then PyGILState_Ensure will give you... the main interpreter? I'm not clear on which one.

I played around with this case last week, and it always gives you the main interpreter (including when subinterpreters are active). I think that's worth documenting in this PR. Maybe we should add some tests too?

  • Ensure there's API to get the current interpreter ID (I'm not sure if we have that)

There is! PyInterpreterState_GetID()

What we don't have is an API for getting a PyInterpreterState * from an ID, That shouldn't be too difficult to implement, I hope.

  • Add a new API function that works like PyGILState_Ensure, but takes an explicit interpreter ID and switches to the given interpreter. (Note that an interpreter ID can become invalid, so this function can fail -- but can't exception mechanism for the failure. It can probably reuse PyStatus.)

I think we do somewhat support this right now with PyThreadState_Swap, but I haven't tested it. Solely looking at the docs, might this work? (Psuedocode, too lazy to deal with error handling right now)

typedef struct {
    PyThreadState *tstate;
    PyObject *obj;
} transport;

int
func(transport thing)
{
    PyGILState_Ensure(); // Main interpreter, ready to call Python
    PyThreadState_Swap(thing.tstate);
    // Do something with obj in the subinterpreter
    // ??? PyGILState_Release() or PyThreadState_Clear?
}

int
thread_func(void *whatever)
{
    // Py_NewInterpreterFromConfig and whatnot
    PyObject *obj = Py_Something();
    transport thing = { PyThreadState_Get(), obj }
    call_in_another_thread(func, thing);
}

Though, if the thread state is supposed to be a thread local, then I think that would cause problems upon finalization. I'll test it tomorrow.

If it doesn't work, then we need an API (I'll make a new issue for that) -- maybe PyInterpreterState_Switch? (You're better at naming things than I am, feel free to bikeshed that!)

@ZeroIntensity
Copy link
Member Author

Ok, I've tested switching interpreters with PyThreadState_Swap and yeah, it doesn't work. I don't think the interpreter likes having the same thread state being used across different threads, so we do need an API if we want to support that.

In regards to this PR, does this look good in terms of clarifying at least what PyGILState_Ensure does in the case of subinterpreters being active?

@encukou
Copy link
Member

encukou commented Sep 26, 2024

I think that's worth documenting in this PR.

You should not document current behaviour. You should document the intended behaviour, once it matches what Python actually does.

Activating the main interpreter is an easy way to get an interpreter the user wasn't expecting, leading to crashes. We should not encourage that.

@ZeroIntensity
Copy link
Member Author

Wouldn't changing the interpreter selected by PyGILState_Ensure be a breaking change at this point?

@encukou
Copy link
Member

encukou commented Sep 27, 2024

It would, but that's not relevant here.
I believe the documentation should keep saying mixing multiple interpreters and the PyGILState_* API is unsupported. It will do the wrong thing, and we won't really help users by documenting what the wrong thing is.

@ZeroIntensity
Copy link
Member Author

I guess there's not much to do here then. I'll look into designing something better for subinterpreters next week.

@ZeroIntensity
Copy link
Member Author

ZeroIntensity commented Sep 29, 2024

cc @encukou, @ericsnowcurrently

Ok, I've put quite a bit of work into developing a proof of concept for a better interpreter-switching API. A quick example:

int
thread_func(void *arg)
{
    PyInterpreterState *subinterp = (PyInterpreterState *) arg;
    PyThreadState *main_tstate;
    PyThreadState *sub_tstate;

    PyInterpreterState_AttachToMain(&main_tstate);
    // We're now in the main interpreter

    PyInterpreterState_Attach(subinterp, &sub_tstate);
    // We're now in the subinterpreter!
    PyInterpreterState_Detach(sub_tstate);
    // Back in the main interpreter again!

    PyInterpreterState_Detach(main_tstate);
    // GIL is not held, current tstate is NULL
}

Unfortunately, there's a lot of PyThreadState * boilerplate here, so it looks a little nicer with some macros:

int
thread_func(void *arg)
{
    PyInterpreterState *subinterp = (PyInterpreterState *) arg;

    // These can be nested infinitely!
    Py_ENTER_MAIN_INTERPRETER();
    // Do something in the main interpreter...

    Py_ENTER_SUBINTERPRETER(subinterp);
    // Do something in the subinterpreter...
    Py_EXIT_SUBINTERPRETER();
    // Back in the main interpreter again!

    Py_EXIT_MAIN_INTERPRETER();
    // GIL is not held, current tstate is NULL
}

The main benefit here is that it's now possible to switch to another thread's interpreter, but you also know exactly what interpreter you're getting, so there's no gotchas if Python happened to have run in a subinterpreter in the current thread.

Although, this (now closed) PR doesn't seem like the best place to discuss things. Where should this get moved to? A new issue? DPO? Possibly even Discord? (Or, hopefully not, a PEP.)

@encukou
Copy link
Member

encukou commented Oct 31, 2024

Thank you so much for working on this!

I'm afraid that this will need a PEP, to explain to anyone who didn't read the discussions. (I admit I'm not caught up, myself.)
You'll need Rationale, Specification and Reference Implementation for any kind of discussion, and the rest of a PEP can be just a few sentences. And the first draft doesn't need to be in PEP format (though the recommended sections are pretty good for a change proposal).
If you write a draft for a discussion topic text I'll be happy to review it.

I'd prefer not treating the main interpreter as special. Extension authors -- stdlib or otherwise -- don't have a good way of knowing if their extenstion is in the main interpreter or not. If not, what kind of operations would make sense on the main interpreter? It's an environment where no PyObject* from the extension is valid.

Regarding the PR, I don't see the need for the stack. The ENTER/EXIT and Attach/Detach should nest, right. Are we worried about functions that switch interpreters behind your back when you call them?

@ZeroIntensity
Copy link
Member Author

ZeroIntensity commented Oct 31, 2024

If not, what kind of operations would make sense on the main interpreter?

I was thinking that it would be useful if a). you know that subinterpreters are active, so PyGILState_Ensure is a no-go, and b). you don't have any references to Python objects, but you still want to call the Python API--you need an interpreter for that, and the main interpreter will always exist.

Are we worried about functions that switch interpreters behind your back when you call them?

That was the idea, yeah. I added the stack to address the problem of Py_NewInterpreterFromConfig switching the thread state to the new interpreter.

I'll talk to Eric first on Discord before writing a PEP draft. At the time when I wrote that implementation, I wasn't aware of the _PyXI APIs; in hindsight, this looks quite similar to _PyXI_Enter and _PyXI_Exit, so maybe those should just get made public instead of designing something new.

@encukou
Copy link
Member

encukou commented Nov 4, 2024

The main interpreter will always* exist, but might be doing something unrelated -- for example, it might have sys.exc_info or even PyErr_Occurred set. Or is that handled somehow?
AFAIK, attaching to any interpreter is somewhat messy; you should use an interpreter you started, or one that your extension was imported in (ideally, in a nested call from your extension's code).

Things that aren't clear to me, since we're talking about C callback APIs with limited context being passed around: how do you know that a given PyInterpreterState* is still valid? Or that Py_Finalize wasn't called in the mean time?

@ZeroIntensity
Copy link
Member Author

Looking at the source, _PyXI_Enter doesn't deal with existing exceptions right now. But that should be a super easy fix if we need to change anything (just fail _PyXI_Enter if there are existing exceptions on the interpreter).

AFAIK, attaching to any interpreter is somewhat messy; you should use an interpreter you started, or one that your extension was imported in (ideally, in a nested call from your extension's code).

Yeah, but I'm not totally sure how we would implement that right now, I think we just have to trust the user that they're doing the right thing. (Maybe we could add an ob_interp slot to the object structure some day.)

Things that aren't clear to me, since we're talking about C callback APIs with limited context being passed around: how do you know that a given PyInterpreterState* is still valid? Or that Py_Finalize wasn't called in the mean time?

From my understanding, you basically don't know. But that's unspecific to the C API--_interpreters has this problem too (see #126016, for example). I'm going to try and hunt those issues down sooner or later, though.

@encukou
Copy link
Member

encukou commented Nov 4, 2024

_PyXI_Enter doesn't deal with existing exceptions right now. But that should be a super easy fix if we need to change anything

I mostly meant exceptions as an example of what could be “wrong” with attaching to an arbitrary interpreter. I don't think we need to fix them, especially if we don't add PyInterpreterState_AttachToMain.

I'm not totally sure how we would implement that right now, I think we just have to trust the user that they're doing the right thing.

In order to trust them, we should agree on and document what the right thing is -- which might then hint at new APIs to add to make it easier.

But that's unspecific to the C API--_interpreters has this problem too

IMO, that's one reason for the underscore in _interpreters :)

@ZeroIntensity
Copy link
Member Author

ZeroIntensity commented Nov 4, 2024

I mostly meant exceptions as an example of what could be “wrong” with attaching to an arbitrary interpreter. I don't think we need to fix them, especially if we don't add PyInterpreterState_AttachToMain.

Oh, I've more or less abandoned PyInterpreterState_AttachToMain in favor of a public PyXI_Enter (users could just pass PyInterpreterState_Main() to that anyway). The crossinterpreter APIs are pretty good at making sure nothing weird is going on, so I'm not particularly worried about the switching being the problem. My main concern is users doing silly things with an object and accidentally breaking things (for example, PyList_Append on a list from another interpreter will break if it needs to resize the array, because it will go to the wrong obmalloc state).

I think the only thing we need to document is "only use an object if you're certain that it came from this interpreter," right?

IMO, that's one reason for the underscore in _interpreters :)

I do think it's important that the C API be more lenient than Python for now, though. Subinterpreters need stress testing--that shouldn't only come from the _interpreters module.

@encukou
Copy link
Member

encukou commented Nov 11, 2024

I think the only thing we need to document is "only use an object if you're certain that it came from this interpreter," right?

That's the only rule that I'm aware of, yes. (It can be safely broken in some cases, but those are best left as implementation details.)


One thing I'm worried about is that this API seems to encourage users to store a PyInterpreterState * (or thread state) for a long time, which can be rather dangerous -- I don't think there's good API to properly invalidate such a pointer when the interpreter goes away.
If you store it in a module's state, things should be fine, but e.g. passing it as a data argument for a C callback function, to be called by some library, needs some careful thinking.

@ZeroIntensity
Copy link
Member Author

I don't think there's good API to properly invalidate such a pointer when the interpreter goes away.

An interesting thought: should we make it possible to statically allocate an interpreter state? That way, we could add some sort of PyInterpreterState_IsValid without having to worry about dangling pointers.

@encukou
Copy link
Member

encukou commented Nov 18, 2024

I don't think we can make that the only way to create interpreter states; we'd still need to support dynamically allocated state.

@ZeroIntensity
Copy link
Member Author

Yeah, that wasn't my plan, most subinterpreters (mainly allocated by _interpreters) would still be on the heap, but C API users could (optionally) use a static type for more control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review docs Documentation in the Doc dir needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants