gh-159485: Fix warnings lock deadlock during interpreter shutdown

[ngoldbaum]

Closes #159485

It looks like the 3.14 backport will need to happen manually but I think the same fix applies there as well if I adjust where the shutdown hook goes.

• Issue: Interpreter shutdown can deadlock when a daemon thread holds the warnings lock #159485

[ZeroIntensity]

An alternative (and simpler) approach would be to hold an interpreter guard while holding the warnings lock.

[ngoldbaum]

I don’t see a way to do that without a larger refactor. warnings.warn() holds the warnings lock while calling user-defined showwarning(), and the issue’s reproducer blocks forever in that callback.

If the guard lasts as long as the lock, shutdown waits forever for the guard. Releasing the guard before the callback doesn’t help if the lock remains held: the callback never returns, so the daemon never releases the lock, and the finalizing thread later blocks on it.

Did you have a different guard placement in mind, or were you suggesting a refactor that releases the lock before calling user code?

[ZeroIntensity]

I was thinking we could acquire a guard before acquiring the lock, and then release it after the lock is acquired. It's fine to call into Python code while a guard is held (and if it isn't, we need to fix that). A thread entering finalization must wait until all daemon threads have released their guards, ensuring that no one holds the lock.

FTR, this kind of issue is one of the reasons PyInterpreterGuard was created in the first place.

[ngoldbaum]

@ZeroIntensity is this closer to what you had in mind?

I think this is safe to backport to 3.15 as-is because the struct I'm changing is private. But what about 3.14 which doesn't have the guard API?

[ZeroIntensity]

I haven't looked into the correctness of this approach, but at a glance, this is significantly more complicated than what I was envisioning. Generally speaking, we shouldn't need to rely on private APIs or of PyInterpreterGuard internals here.

I think it would be much simpler to either:

1. Add a field to _PyThreadStateImpl that holds the guard pointer.
2. Return the guard from warnings_lock.

Conceptually, we want the locking to look something like this (psuedocode):

static inline PyInterpreterGuard *
warnings_lock(PyInterpreterState *interp)
{
    // ...
    PyInterpreterGuard *guard = PyInterpreterGuard_FromCurrent();
    if (guard == NULL) {
        // We can hang the thread or fail gracefully.
        return NULL;
    }

    // Acquire the warnings lock...

    return guard;
}

static inline void
warnings_unlock(PyInterpreterGuard *guard)
{
    // Release the warnings lock...
    PyInterpreterGuard_Close(guard);
}

Then, in the module methods, we do this:

PyInterpreterGuard *guard = warnings_lock(tstate->interp);
if (guard == NULL) {
    return NULL;
}
res = warn_explicit(/* ... */); // For example
warnings_unlock(guard);

[ngoldbaum]

I just pushed a commit that uses the public guard API and guards at the outermost locking level. Since the lock is exposed to python and there isn't a natural way to pass the guard around there, I added it to the thread state struct.

[ZeroIntensity]

Things like this make me very suspicious as to why we're using a recursive mutex in the first place. Is it solely because warnings can call into Python code? If so, we should drop the recursive mutex and switch to Py_BEGIN_CRITICAL_SECTION_MUT, which should solve the deadlock, make the code much simpler, and also give us some peace-of-mind (since we know critical sections are deadlock-safe).

If we go this route, we don't need the PEP 788 APIs either.

[ngoldbaum]

Maybe @nascheme has context for why using a critical section wouldn't work in the first place.

[nascheme]

A critical section is not the right tool in this situtation, IMHO. First, we need locking in the GIL-enable builds as well. Second, we have a Python implementation of warnings and there is no way to use critical sections from there (generally doesn't make sense either, which is why there is no API for that).

[ZeroIntensity]

First, we need locking in the GIL-enable builds as well.

What for?

[nascheme]

I think PyInterpreterGuard does not do what we want. The guards only interact with finalization, not with fork(). Following what's done for the import lock makes the most sense to me.

Your change to use a global rather than changing _warnings_runtime_state (which will change PyInterpreterState) seems like the correct thing to do for the backport. You would need to add the new global to Tools/c-analyzer.

An alternative idea, rather than a global: compare the lock owner against tstate->thread_id in the child. That field is set once in bind_tstate() and is not refreshed by PyOS_AfterFork_Child(), so it still holds the forking thread's pre-fork ident even on platforms where pthread_self() changes across fork() (gh-126688). The lock's thread field was written from the same ident, so the comparison does not depend on the post-fork value. No struct layout change, no file-static global.

[ZeroIntensity]

Isn't deadlocking with threads + fork a known issue? (And even so, couldn't we reset the lock separately in PyOS_AfterFork_Child?)

Apologies for my potential ignorance here. I'm largely out of the loop on this issue.