gh-124622: Add PyThreadState_Ensure() function

[vstinner]

Add PyThreadState_Ensure() and PyThreadState_Release() functions.

Add new "OrFail" internal functions:

• _PyEval_AcquireLockOrFail()
• _PyEval_RestoreThreadOrFail()
• _PyThreadState_AttachOrFail()
• take_gil_or_fail()

• Issue: Fail PyGILState_Ensure() If Finalizing? #124622

---

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

[ZeroIntensity]

I'm a lot happier with this 😄! When this was first discussed a few months ago, Petr suggested using PyStatus as the return value to differentiate between failure reasons. So, something like this:

static int
my_thread(PyInterpreterState *interp)
{
    int state;
    PyStatus status = PyThreadState_Ensure(interp, &state);
    if (PyStatus_Exception(status)) {
        // Log the error somehow
        /* ... */
    }
    PyThreadState_Release(state);
}

I think that could be handy, especially for applications that want to continue running instead of just bailing out of the thread.

[ZeroIntensity]

If it's NULL, we should probably just fatal error or hang the thread.

[rruuaanng]

Maybe we could add an assert.

[ZeroIntensity]

That doesn't help much. Release builds will crash at finalization with spurious NULL derefs if we don't add a case for it.

[vstinner]

If possible, I would prefer to not change PyGILState_Ensure() in this PR which ("only") adds PyThreadState_Ensure().

[ZeroIntensity]

I'd be happy to do that in a follow-up then.

[ZeroIntensity]

Thinking out loud here: instead of manually passing an int around indicating the number of PyThreadState_Ensure calls, why not just store that number on the thread state itself?

[vstinner]

The oldstate is either PyGILState_UNLOCKED or PyGILState_LOCKED, it's not a counter.

[ZeroIntensity]

Oh, hm. Why do we need PyGILState here at all? The thread state should pretty much manage itself, there just needs to be a call to PyThreadState_Clear and PyThreadState_DeleteCurrent at the end (which is why I thought it would be done with a counter of calls).

[vstinner]

I understand that PyThreadState_Release() does basically nothing if oldstate is PyGILState_LOCKED: if the thread already hold the GIL when PyThreadState_Ensure() was called.

[ZeroIntensity]

I might just be used to dealing with thread states the subinterpreter way, but I think it could be worth avoiding any ints entirely in PyThreadState_Ensure. If we add a new field to PyThreadState (e.g., ensured) which is incremented for a call to PyThreadState_Ensure and then decremented upon a call to PyThreadState_Release, we could automatically clear and delete the thread state when that number hits zero from PyThreadState_Release. I think we should discuss this elsewhere, though. I think I'll create the WG discussion.

[vstinner]

The problem is that you can have nested calls to PyThreadState_Ensure(). How do you store such stack of states in PyThreadState?

[ZeroIntensity]

I was thinking we could set up a field for a linked list, so PyThreadState_Ensure/PyThreadState_Release would look something along the lines of:

int
PyThreadState_Ensure(PyInterpreterState *interp)
{
    PyThreadState *current = current_fast_get();
    if (current == NULL) {
        // Make a new thread state for *interp* and return it
        /* ... */
    }
    else {
        PyThreadState *new_tstate = _PyThreadState_NewBound(interp);
        assert(new_tstate->ensured == NULL);
        new_tstate->ensured = current;
        _PyThreadState_Detach(current);
        /* AttachOrFail(new_tstate) ... */
    }
}

void
PyThreadState_Release()
{
    PyThreadState *current = current_fast_get();
    _Py_EnsureTstateNotNULL(current);
    PyThreadState *to_restore = current->ensured;
    _PyThreadState_Detach(current);
    if (to_restore != NULL) {
        /* AttachOrFail(to_restore) */
    }
    // Nothing to restore, leave no attached tstate
    assert(current_fast_get() == NULL);
}

• We probably need a return value for PyThreadState_Release because it should be able to reattach to the previous thread state if it didn't have the requested interpreter (regardless of any linked list).
• If we go with my linked list idea, it would be nice to have a fast-path which does nothing if there's already an existing thread state that matches interp.

[vstinner]

I changed the API to int PyThreadState_Ensure(PyInterpreterState *interp, const char **errmsg): set an error message (if errmsg is not NULL) on error.

Currently, the only error message is: "Python is being finalized".

[ZeroIntensity]

Thanks for doing this, by the way :)

[colesbury]

I think PyThreadState_Release is missing code to restore the previous thread state for this case where the active thread state was from a different interpreter.

[ZeroIntensity]

It is, but the design is still being discussed. This PR is, more or less, a draft until the C API WG approves it.

[vstinner]

I prefer to close this PR for now until we agree on the API semantics.