PEP 788: Protecting the C API from Interpreter Finalization

[ZeroIntensity]

Hi,

I'd like the C API working group to begin considering PEP 788, "Protecting the C API from Interpreter Finalization", for recommendation to the SC.
The TL;DR is that we get a new PyThreadState_Ensure function to replace PyGILState_Ensure, which has a number of issues regarding subinterpreters and thread-safety. As part of introducing that, it includes several new interpreter "guarding" APIs that prevent an interpreter from shutting down, so things don't unexpectedly deallocate.

The main impact here is that the proposal deprecates PyGILState and schedules it for removal in a future version, so there will be a very huge migration from PyGILState_Ensure to PyThreadState_Ensure. APIs that use PyGILState_Ensure under the hood (for example, _PyObject_Dump in CPython) will probably need to be deprecated and given counterparts that take a correct PyInterpreterGuard parameter.

I'm not sure it's time to start formally voting on the proposal, but it would be good to hear some of the workgroup's feedback on the current design.

[vstinner]

I'm the sponsor of this PEP and I support it :-) It solves an old big design issue with C threads leading to crashes or hangs.

[encukou]

I'll post the discussion thread, but yes, I'm happy with the PEP overall. Thank you!

[vstinner]

Link to the discussion: https://discuss.python.org/t/pep-788-reimagining-native-threads-new-updates/93653

[encukou]

Looks like the discussion died down; let's vote!

• @encukou
• @erlend-aasland
• @mdboom (abstain)
• @serhiy-storchaka
• @vstinner
• @zooba

[ZeroIntensity]

Friendly bump

[encukou]

@erlend-aasland @mdboom @serhiy-storchaka @zooba, what are your thoughts on the PEP?

[zooba]

The int PyInterpreterRef_Get(PyInterpreterRef *ref) style functions might as well just return PyInterpreterRef, no? It's going to have to be an integer of some size (presumably pointer sized), so we can define 0/NULL as invalid, and simplify everyone's use of it.

A number of APIs "cannot fail" but they take a user-provided parameter, which means they can fail if given garbage input. What are they going to do in this case? Crash? Garbage out? Or better yet... fail 😉

Some also seem likely to involve memory allocation, unless you've got some real magic in mind for how to handle weak references (typically you need to allocate new memory for a weak reference to own, as well as adding it to a list owned by the live object so it can go through and mark them all as closed when it closes). Memory allocation can fail, which means functions like PyInterpreterWeakRef_Dup can fail (as well as probably needing to follow whatever rules we apply for allocator synchronisation and/or thread affinity). In short, "cannot fail" seems like a huge claim, and we're better off just saying that it could fail, even if failures are rare in practice.

I don't particularly like adding the dup model of reference counting at this stage, since we don't use it elsewhere (I know there's a proposal ongoing to bring it in, but that hasn't happened yet). How about PyInterpreterRef_NewRef (instead of _Dup) and PyInterpreterRef_NewWeakRef to get a weak reference?

AsStrong and AsInterpreter feel very unconventional. I'd have expected Get/GetTarget and GetInterpreterState here, to align with existing APIs that do similar things. Should also have a way to get a reference to a PyInterpreterState * (i.e. the opposite of AsInterpreter/GetInterpreterState).

Not a fan of PyInterpreter[Strong/Weak]Ref_Get for getting the current interpreter. We should only need to get a strong reference for the current interpreter (e.g. PyInterpreterRef_GetCurrent) and then can get a weak reference from that (NewWeakRef above).

What should I (as an embedder) be reporting if PyThreadState_Ensure fails? The example prints "out of memory", which seems presumptuous. Should I just be saying that some kind of internal Python error has occurred? "Failed to activate Python thread"?

PyInterpreterRef_Main - needs "get" somewhere in the name, and I think I'd prefer to see this as a PyRuntime_ function rather than PyInterpreterRef_. (We're also aiming to do away with the "main" interpreter as a concept, which means this kind of use will eventually be "an arbitrary default interpreter" rather than anything special. So an API to get "any"/"the first" active interpreter from the runtime is likely to align better with the intended design.)

[ZeroIntensity]

(It'd probably be better to discuss these on the DPO post.)

The int PyInterpreterRef_Get(PyInterpreterRef *ref) style functions might as well just return PyInterpreterRef, no? It's going to have to be an integer of some size (presumably pointer sized), so we can define 0/NULL as invalid, and simplify everyone's use of it.

Maybe. I don't have too much of a preference. I originally designed it that way, but I think @vstinner preferred the current design.

A number of APIs "cannot fail" but they take a user-provided parameter, which means they can fail if given garbage input. What are they going to do in this case? Crash? Garbage out? Or better yet... fail 😉

It seems totally fine to me for these to crash/fatal error if you feed them garbage. They're just pointers (at least in the reference implementation), so if you play silly games, you'll get silly segfaults. How are you envisioning these failing?

Memory allocation can fail, which means functions like PyInterpreterWeakRef_Dup can fail (as well as probably needing to follow whatever rules we apply for allocator synchronisation and/or thread affinity). In short, "cannot fail" seems like a huge claim, and we're better off just saying that it could fail, even if failures are rare in practice.

Dup functions reuse the allocation and increment a reference count, but I think I agree that these should be failable to be more future-proof. I'll update the proposal once we come to a consensus on how things should fail.

I don't particularly like adding the dup model of reference counting at this stage, since we don't use it elsewhere (I know there's a proposal ongoing to bring it in, but that hasn't happened yet). How about PyInterpreterRef_NewRef (instead of _Dup) and PyInterpreterRef_NewWeakRef to get a weak reference?

PyInterpreterRef_NewRef sounds fine, but I really don't like PyInterpreterRef_NewWeakRef. That will make weak references especially annoying to acquire, because you'll have to acquire and release a strong reference just to get a weak reference. The boilerplate would be like this:

PyInterpreterRef ref;
if (PyInterpreterRef_Get(&ref) < 0) {
    /* Fail ... */
}
PyInterpreterWeakRef wref;
if (PyInterpreterRef_NewWeakRef(ref, &wref) < 0) {
    /* Fail ... */
}
PyInterpreterRef_Close(ref);

AsStrong and AsInterpreter feel very unconventional. I'd have expected Get/GetTarget and GetInterpreterState here, to align with existing APIs that do similar things.

GetTarget seems really unclear, but I'm fine with GetInterpreterState. I just chose AsInterpreter because it was shorter.

Should also have a way to get a reference to a PyInterpreterState * (i.e. the opposite of AsInterpreter/GetInterpreterState).

That defeats the whole point of having an interpreter reference in the first place; a PyInterpreterState * can be deallocated at any moment if you don't hold an attached thread state or strong ref for it, so it isn't thread-safe.

Not a fan of PyInterpreter[Strong/Weak]Ref_Get for getting the current interpreter. We should only need to get a strong reference for the current interpreter (e.g. PyInterpreterRef_GetCurrent) and then can get a weak reference from that (NewWeakRef above).

Yeah, ditto. Weak and strong references should definitely be separate here.

What should I (as an embedder) be reporting if PyThreadState_Ensure fails? The example prints "out of memory", which seems presumptuous. Should I just be saying that some kind of internal Python error has occurred? "Failed to activate Python thread"?

I think "out of memory" is a good fit. The idea is that PyThreadState_Ensure can only fail if creating the thread state fails, because the interpreter is guaranteed to be in a state where it can attach a thread state (because the caller holds a strong reference).

PyInterpreterRef_Main - needs "get" somewhere in the name, and I think I'd prefer to see this as a PyRuntime_ function rather than PyInterpreterRef_. (We're also aiming to do away with the "main" interpreter as a concept, which means this kind of use will eventually be "an arbitrary default interpreter" rather than anything special. So an API to get "any"/"the first" active interpreter from the runtime is likely to align better with the intended design.)

Yeah, this has been brought up a few times. My rationale for PyInterpreterRef_Main was to be the "PyInterpreterRef equivalent of PyInterpreterState_Main", but I can see how the name is confusing when that context is missing. I'll rename it to PyInterpreterState_GetMain.

I don't think we need to put this under PyRuntime (we don't have anything public in the PyRuntime namespace, do we? ) or try to mark it as an "arbitrary default interpreter". The whole point of PyInterpreterState_Main is to make an easier migration for PyGILState_Ensure users that really don't want to support subinterpreters. The only reason it's supposed to be thread-safe is because the main interpreter is statically allocated, so an arbitrary PyInterpreterState * wouldn't be a good fit.

[zooba]

How are you envisioning these failing?

Not sure at this stage, but factoring in other implementations that want to follow CPython's API, and the fact that we know 100% that we can't predict the future, allowing us to later make them fail seems like a better option than declaring it impossible. (Deprecation is one thing that's better when we can support failure, though it's always hard to distinguish between "failed and you should give up" and "fail and you should try a different approach" when we don't set exceptions.)

There's also a possibility that one day we might be able to set exceptions here, which makes failure worth supporting. Part of the reason for having a C API WG is for us to guide designs by future, unpredictable things that we've seen happen in the past, rather than always designing the minimal API for the current implementation (which is usually the reason that those "unpredictable" things caused such trouble).

GetTarget seems really unclear

Yeah, must be a different language that uses target in their API - I was thinking of PyWeakref_GetObject as the existing pattern. So Get or GetInterpreterRef would be better than AsStrong.

That defeats the whole point of having an interpreter reference in the first place; a PyInterpreterState * can be deallocated at any moment if you don't hold an attached thread state or strong ref for it, so it isn't thread-safe.

There are plenty of circumstances where you can reasonably assume that someone else (in your callstack) is holding a strong reference, but has just passed you the state pointer. There's plenty of existing code out there that doesn't use references (obviously), but may well deal in interpreter states. We make porting a lot easier by allowing to go both directions.

I think "out of memory" is a good fit.

Okay, then write that explicitly into the specification, not just in an example. If you can confidently say "this will only ever fail due to out of memory" then we can specify it (FWIW, I don't think you should confidently say that, and should go more generic).

My rationale for PyInterpreterRef_Main was to be the "PyInterpreterRef equivalent of PyInterpreterState_Main", but I can see how the name is confusing when that context is missing. I'll rename it to PyInterpreterState_GetMain.

Or perhaps PyInterpreterState_GetDefault[Ref] (I like it better with Ref since it's not returning a state pointer)? Setting the default can be defined as internal only at this stage, but it leaves us in a better future position to update the model without having to deprecate your APIs (e.g. if the "default" is later allowed to shut down before other interpreters, then a new interpreter can become default - this seems incredibly possible in an embedding situation).

(It'd probably be better to discuss these on the DPO post.)

Then why come here asking for feedback? If the idea has passed DPO discussion, then now we're specifically checking the API design. Everything I've mentioned is in scope for C API WG review (you'll notice I haven't criticised the somewhat rambling motivation or misuse of "rationale" for "rationalisations" 😉).

[ZeroIntensity]

Not sure at this stage, but factoring in other implementations that want to follow CPython's API, and the fact that we know 100% that we can't predict the future, allowing us to later make them fail seems like a better option than declaring it impossible. (Deprecation is one thing that's better when we can support failure, though it's always hard to distinguish between "failed and you should give up" and "fail and you should try a different approach" when we don't set exceptions.)

Ok, that makes sense. My one concern is that some APIs (like PyInterpreterWeakRef_Dup) don't require you to hold an attached thread state, so Python can't really set an exception to denote what failed. I think that's probably work for a different proposal, though (maybe we could add a Py_errno for these cases)?

Yeah, must be a different language that uses target in their API - I was thinking of PyWeakref_GetObject as the existing pattern. So Get or GetInterpreterRef would be better than AsStrong.

I think that works for PyInterpreterRef_AsInterpreter (which I do see now is a little weirdly named in hindsight), but GetStrong is a little confusing to me, because in reality it's constructing something new rather than being a "getter". We could try a totally different name for it instead. Perhaps something like PyInterpreterWeakRef_Promote?

There are plenty of circumstances where you can reasonably assume that someone else (in your callstack) is holding a strong reference, but has just passed you the state pointer. There's plenty of existing code out there that doesn't use references (obviously), but may well deal in interpreter states. We make porting a lot easier by allowing to go both directions.

I see your point, but I'm really worried that people will use such an API to write unsafe code like this:

static void
blah(PyInterpreterState *interp)
{
    // Fresh thread; no interpreter reference anywhere
    PyInterpreterRef ref = PyInterpreterState_AsRef(interp);
    PyThreadState_Ensure(ref);
    /* ... The illusion of thread safety ... */
}

But anyway, if there's a PyInterpreterState * pointer available, then it's reasonable to assume that PyInterpreterState_Get returns that same interpreter, right? If it is, no need for AsRef, and if not, then it's not thread safe (because the thread isn't attached to the interpreter and it may be deallocated at any moment).

(FWIW, I don't think you should confidently say that, and should go more generic).

Ok, I'll update the proposal to change the failure message in the examples. The current documentation for PyThreadState_Ensure in the PEP simply reads "Return 0 on success, and -1 without an exception set on failure." Is that broad enough for future-proofing?

I like it better with Ref since it's not returning a state pointer

Well, since there's no state pointer involved (other than internally), we probably should avoid the PyInterpreterState prefix. I'm okay with PyInterpreterRef_GetDefault, but ideally we'll get rid of that API someday anyway. People should be using references for a specific interpreter, not the arbitrary "default" one. It solely acts as a compatibility shim for those who use PyGILState_Ensure (which will be removed), but don't have the time to try to support subinterpreters.

Then why come here asking for feedback? If the idea has passed DPO discussion, then now we're specifically checking the API design. Everything I've mentioned is in scope for C API WG review (you'll notice I haven't criticised the somewhat rambling motivation or misuse of "rationale" for "rationalisations" 😉).

I meant just for visibility (primarily for the SC who'll be reading the threads). I wasn't at all trying to say that I didn't want the feedback, I greatly appreciate it!

I'm also fine if you criticize the text of the proposal, here or on DPO; I'd rather the WG be able to comfortably understand the PEP even if it means going out of "scope" to get there :)

[encukou]

Good point about the fallible API.

In functions that don't need attached thread state, return -1 without raising an exception.

Deallocation functions (*_Close) are OK not failing.

The int PyInterpreterRef_Get(PyInterpreterRef *ref) style functions might as well just return PyInterpreterRef, no? It's going to have to be an integer of some size (presumably pointer sized), so we can define 0/NULL as invalid, and simplify everyone's use of it.

It's an opaque type, not an integer. You can't meaningfully compare it.

I don't particularly like adding the dup model of reference counting at this stage, since we don't use it elsewhere (I know there's a proposal ongoing to bring it in, but that hasn't happened yet). How about PyInterpreterRef_NewRef (instead of _Dup) and PyInterpreterRef_NewWeakRef to get a weak reference?

Well, if you're waiting for the proposal to bring it in, this is it. Regardless of what the operation is named.

To me personally, PyInterpreterRef_NewRef doesn't sounds right -- it should be _Copy, _Clone, or, well, _Dup.

AsStrong and AsInterpreter feel very unconventional. I'd have expected Get/GetTarget and GetInterpreterState here, to align with existing APIs that do similar things. Should also have a way to get a reference to a PyInterpreterState * (i.e. the opposite of AsInterpreter/GetInterpreterState).

Hm, that clashes with _Get for the constructor functions. Guess those should use _New or _From.