GeezerVison

Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

Sorry, @ericsnowcurrently, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 32466c97c06ee5812923d695195394c736eeb707 3.12

Sorry, @ericsnowcurrently, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 32466c9 3.12

Is it really reasonable to backport this major change in a stable branch? IMO it's too late to change that, no?

This change introduced a regression: Python started to crash again randomly at exit when they are daemon threads: see issue gh-110052. I confirmed (by manual testing) that the commit 32466c9 introduced the regression:

• Before this commit, I cannot reproduce the issue in 5 minutes.
• At this commit, I easily reproduce the issue in less than a minute.

@ericsnowcurrently: Please have a look. If you don't have the bandwidth to repair the regression, I will revert soon the change.

This code is very fragile :-( I fixed multiple bugs to try to handle all cases:

• https://vstinner.github.io/gil-bugfixes-daemon-threads-python39.html
• https://vstinner.github.io/daemon-threads-python-finalization-python32.html

Note: By the way, recently, I fixed a race condition in _thread.start_new_thread(), but it doesn't seem to be related: commit 517cd82.

Before the commit, the test was fine:

vstinner@mona$ ./python -m test test_threading -m test_4_daemon_threads -j50 -F --fail-env-changed  
(...)
0:07:34 load avg: 91.03 [1640] test_threading passed
0:07:34 load avg: 91.03 [1641] test_threading passed
^C

A system load of 91.03 is high knowning that my laptop CPU has 6 cores / 12 threads.

I'm not comfortable with this change which has comments like:

        // XXX This isn't completely safe from daemon thraeds,
        // since tstate might be a dangling pointer.

Well, if it's not safe: don't do it.

Note: _PyRuntimeState_GetFinalizing() uses the "old" pycore_atomic.h for API, whereas _PyRuntimeState_GetFinalizingID() uses the "new" pyatomic.h API.

static inline PyThreadState*
_PyRuntimeState_GetFinalizing(_PyRuntimeState *runtime) {
    // pycore_atomic.h                                                    
    return (PyThreadState*)_Py_atomic_load_relaxed(&runtime->_finalizing);     
}

static inline unsigned long
_PyRuntimeState_GetFinalizingID(_PyRuntimeState *runtime) {        
    // pyatomic.h
    return _Py_atomic_load_ulong_relaxed(&runtime->_finalizing_id);         
}                  

I'm looking into the crashes right now.

At this commit, I easily reproduce the issue in less than a minute.

I'm going to follow up on gh-110052.

I'm not comfortable with this change which has comments like:

   // XXX This isn't completely safe from daemon thraeds,
  // since tstate might be a dangling pointer.

Well, if it's not safe: don't do it.

That's a comment I added about existing code. 😄

That's a comment I added about existing code. 😄

In Python 3.11, _PyThreadState_MustExit() does not deference tstate because it can be a dangling pointer: it's explained in the comment.

// Check if a Python thread must exit immediately, rather than taking the GIL
// if Py_Finalize() has been called.
//              
// When this function is called by a daemon thread after Py_Finalize() has been
// called, the GIL does no longer exist.
//              
// tstate can be a dangling pointer (point to freed memory): only tstate value
// is used, the pointer is not deferenced.
//              
// tstate must be non-NULL.
int         
_PyThreadState_MustExit(PyThreadState *tstate)
{           
    /* bpo-39877: Access _PyRuntime directly rather than using
       tstate->interp->runtime to support calls from Python daemon threads.
       After Py_Finalize() has been called, tstate can be a dangling pointer:
       point to PyThreadState freed memory. */
    PyThreadState *finalizing = _PyRuntimeState_GetFinalizing(&_PyRuntime);
    return (finalizing != NULL && finalizing != tstate);
}

By the way, in the main branch, the function still has this comment:

// tstate can be a dangling pointer (point to freed memory): only tstate value
// is used, the pointer is not deferenced.

Is it really reasonable to backport this major change in a stable branch? IMO it's too late to change that, no?

The main motivation is to address the situation where a subinterpreter gets cleaned up while the runtime is finalizing. Without this fix, the process immediately exits with an exitcode of 0, even if it would have exited with 1 (and the rest of runtime finalization is skipped). That seems like it makes a backport worth it.

In Python 3.11, _PyThreadState_MustExit() does not deference tstate because it can be a dangling pointer

I added the _Py_IsInterpreterFinalizing(tstate->interp) check in 26baa74 (May 2023). A dangling pointer with that check isn't an issue during runtime finalization since we just verified that the runtime isn't finalizing yet.

The cases where it could be a problem are strictly where a subinterpreter has daemon threads. That is unlikely since subinterpreters have daemon threads disabled by default.

FYI, any backport will need to include the fix in gh-110053 (6364873). Likewise, any revert will need to revert that change.

GH-110705 is a backport of this pull request to the 3.12 branch.