GeezerVison

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

Maybe we could add an assert.

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

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

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

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?

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

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).

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.

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.

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

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.

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.

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