From b8e4ae83d867bbaf6d79fedb0e125f8c06b1efca Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Sat, 16 Nov 2024 10:11:32 -0500
Subject: [PATCH 01/21] Initial implementation.

---
 Include/internal/pycore_atexit.h |  2 ++
 Modules/atexitmodule.c           | 41 +++++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/Include/internal/pycore_atexit.h b/Include/internal/pycore_atexit.h
index 507a5c03cbc792..98f2702d63f91f 100644
--- a/Include/internal/pycore_atexit.h
+++ b/Include/internal/pycore_atexit.h
@@ -52,6 +52,8 @@ struct atexit_state {
     atexit_py_callback **callbacks;
     int ncallbacks;
     int callback_len;
+
+    PyMutex lock;
 };
 
 // Export for '_interpchannels' shared extension
diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index 297a8d74ba3bf4..1b60b3c618b881 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -12,6 +12,11 @@
 #include "pycore_interp.h"        // PyInterpreterState.atexit
 #include "pycore_pystate.h"       // _PyInterpreterState_GET
 
+/* Note: anything declared as static in this file assumes the lock is held
+ * (except for the Python-level functions) */
+#define _PyAtExit_LOCK(state) PyMutex_Lock(&state->lock);
+#define _PyAtExit_UNLOCK(state) PyMutex_Unlock(&state->lock);
+
 /* ===================================================================== */
 /* Callback machinery. */
 
@@ -38,6 +43,7 @@ PyUnstable_AtExit(PyInterpreterState *interp,
     callback->next = NULL;
 
     struct atexit_state *state = &interp->atexit;
+    _PyAtExit_LOCK(state);
     if (state->ll_callbacks == NULL) {
         state->ll_callbacks = callback;
         state->last_ll_callback = callback;
@@ -45,6 +51,7 @@ PyUnstable_AtExit(PyInterpreterState *interp,
     else {
         state->last_ll_callback->next = callback;
     }
+    _PyAtExit_UNLOCK(state);
     return 0;
 }
 
@@ -99,6 +106,8 @@ void
 _PyAtExit_Fini(PyInterpreterState *interp)
 {
     struct atexit_state *state = &interp->atexit;
+    // XXX Can this be unlocked?
+    _PyAtExit_LOCK(state);
     atexit_cleanup(state);
     PyMem_Free(state->callbacks);
     state->callbacks = NULL;
@@ -114,6 +123,7 @@ _PyAtExit_Fini(PyInterpreterState *interp)
         PyMem_Free(callback);
         exitfunc(data);
     }
+    _PyAtExit_UNLOCK(state);
 }
 
 
@@ -155,7 +165,9 @@ void
 _PyAtExit_Call(PyInterpreterState *interp)
 {
     struct atexit_state *state = &interp->atexit;
+    _PyAtExit_LOCK(state);
     atexit_callfuncs(state);
+    _PyAtExit_UNLOCK(state);
 }
 
 
@@ -192,12 +204,17 @@ atexit_register(PyObject *module, PyObject *args, PyObject *kwargs)
     }
 
     struct atexit_state *state = get_atexit_state();
+    /* In theory, we could hold the lock for a shorter amount of time
+     * using some fancy compare-exchanges with the length. However, I'm lazy.
+     */
+    _PyAtExit_LOCK(state);
     if (state->ncallbacks >= state->callback_len) {
         atexit_py_callback **r;
         state->callback_len += 16;
         size_t size = sizeof(atexit_py_callback*) * (size_t)state->callback_len;
         r = (atexit_py_callback**)PyMem_Realloc(state->callbacks, size);
         if (r == NULL) {
+            _PyAtExit_UNLOCK(state);
             return PyErr_NoMemory();
         }
         state->callbacks = r;
@@ -205,18 +222,21 @@ atexit_register(PyObject *module, PyObject *args, PyObject *kwargs)
 
     atexit_py_callback *callback = PyMem_Malloc(sizeof(atexit_py_callback));
     if (callback == NULL) {
+        _PyAtExit_UNLOCK(state);
         return PyErr_NoMemory();
     }
 
     callback->args = PyTuple_GetSlice(args, 1, PyTuple_GET_SIZE(args));
     if (callback->args == NULL) {
         PyMem_Free(callback);
+        _PyAtExit_UNLOCK(state);
         return NULL;
     }
     callback->func = Py_NewRef(func);
     callback->kwargs = Py_XNewRef(kwargs);
 
     state->callbacks[state->ncallbacks++] = callback;
+    _PyAtExit_UNLOCK(state);
 
     return Py_NewRef(func);
 }
@@ -233,7 +253,9 @@ static PyObject *
 atexit_run_exitfuncs(PyObject *module, PyObject *unused)
 {
     struct atexit_state *state = get_atexit_state();
+    _PyAtExit_LOCK(state);
     atexit_callfuncs(state);
+    _PyAtExit_UNLOCK(state);
     Py_RETURN_NONE;
 }
 
@@ -246,7 +268,10 @@ Clear the list of previously registered exit functions.");
 static PyObject *
 atexit_clear(PyObject *module, PyObject *unused)
 {
-    atexit_cleanup(get_atexit_state());
+    struct atexit_state *state = get_atexit_state();
+    _PyAtExit_LOCK(state);
+    atexit_cleanup(state);
+    _PyAtExit_UNLOCK(state);
     Py_RETURN_NONE;
 }
 
@@ -260,7 +285,10 @@ static PyObject *
 atexit_ncallbacks(PyObject *module, PyObject *unused)
 {
     struct atexit_state *state = get_atexit_state();
-    return PyLong_FromSsize_t(state->ncallbacks);
+    _PyAtExit_LOCK(state);
+    PyObject *res = PyLong_FromSsize_t(state->ncallbacks);
+    _PyAtExit_UNLOCK(state);
+    return res;
 }
 
 PyDoc_STRVAR(atexit_unregister__doc__,
@@ -276,6 +304,7 @@ static PyObject *
 atexit_unregister(PyObject *module, PyObject *func)
 {
     struct atexit_state *state = get_atexit_state();
+    _PyAtExit_LOCK(state);
     for (int i = 0; i < state->ncallbacks; i++)
     {
         atexit_py_callback *cb = state->callbacks[i];
@@ -283,14 +312,20 @@ atexit_unregister(PyObject *module, PyObject *func)
             continue;
         }
 
-        int eq = PyObject_RichCompareBool(cb->func, func, Py_EQ);
+        // We need to hold our own reference to this
+        // in case another thread is trying to unregister as well.
+        PyObject *to_compare = cb->func;
+        _PyAtExit_UNLOCK(state);
+        int eq = PyObject_RichCompareBool(to_compare, func, Py_EQ);
         if (eq < 0) {
             return NULL;
         }
+        _PyAtExit_LOCK(state);
         if (eq) {
             atexit_delete_cb(state, i);
         }
     }
+    _PyAtExit_UNLOCK(state);
     Py_RETURN_NONE;
 }
 

From d54bca9d274870bf9ca39a033dac1cc463c7bb0f Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Sat, 16 Nov 2024 10:19:36 -0500
Subject: [PATCH 02/21] Fix race with unregistration.

---
 Modules/atexitmodule.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index 1b60b3c618b881..4160fd4fd9bbc3 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -311,16 +311,21 @@ atexit_unregister(PyObject *module, PyObject *func)
         if (cb == NULL) {
             continue;
         }
-
-        // We need to hold our own reference to this
-        // in case another thread is trying to unregister as well.
         PyObject *to_compare = cb->func;
+
+        // Unlock for fear of a custom __eq__ causing re-entrancy
         _PyAtExit_UNLOCK(state);
         int eq = PyObject_RichCompareBool(to_compare, func, Py_EQ);
         if (eq < 0) {
             return NULL;
         }
         _PyAtExit_LOCK(state);
+        if (state->callbacks[i] == NULL)
+        {
+            // Edge case: another thread might have
+            // unregistered the function while we released the lock.
+            continue;
+        }
         if (eq) {
             atexit_delete_cb(state, i);
         }

From 0601a6da3fdbff1516d6b41f6def376a6d22a757 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Sat, 16 Nov 2024 10:24:49 -0500
Subject: [PATCH 03/21] Add a test.

---
 Lib/test/test_atexit.py | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Lib/test/test_atexit.py b/Lib/test/test_atexit.py
index 913b7556be83af..789183484251a8 100644
--- a/Lib/test/test_atexit.py
+++ b/Lib/test/test_atexit.py
@@ -4,6 +4,7 @@
 import unittest
 from test import support
 from test.support import script_helper
+from test.support import threading_helper
 
 
 class GeneralTest(unittest.TestCase):
@@ -46,6 +47,34 @@ def test_atexit_instances(self):
         self.assertEqual(res.out.decode().splitlines(), ["atexit2", "atexit1"])
         self.assertFalse(res.err)
 
+    @threading_helper.requires_working_threading()
+    @support.requires_resource("cpu")
+    @unittest.skipUnless(support.Py_GIL_DISABLED, "only meaningful without the GIL")
+    def test_atexit_thread_safety(self):
+        # GH-126907: atexit was not thread safe on the free-threaded build
+
+        # I'm not certain this needs to be in a script runner, but
+        # let's do it anyway.
+        code = textwrap.dedent("""
+        from threading import Thread
+
+        def dummy():
+            pass
+
+
+        def thready():
+            for _ in range(100):
+                atexit.register(dummy)
+                atexit._clear()
+                atexit.register(dummy)
+                atexit.unregister(dummy)
+
+
+        for x in range(100):
+            Thread(target=thready, args=()).start()
+        """)
+        script_helper.assert_python_ok("-c", code)
+
 
 @support.cpython_only
 class SubinterpreterTest(unittest.TestCase):

From 2fcee90b0ed006ca7b5712d510b8cc029994a1cd Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Sat, 16 Nov 2024 10:36:18 -0500
Subject: [PATCH 04/21] Fix the tests.

---
 Lib/test/test_atexit.py | 11 +++--------
 Modules/atexitmodule.c  | 12 ++++++++++--
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/Lib/test/test_atexit.py b/Lib/test/test_atexit.py
index 789183484251a8..4d9da43597dc29 100644
--- a/Lib/test/test_atexit.py
+++ b/Lib/test/test_atexit.py
@@ -52,10 +52,6 @@ def test_atexit_instances(self):
     @unittest.skipUnless(support.Py_GIL_DISABLED, "only meaningful without the GIL")
     def test_atexit_thread_safety(self):
         # GH-126907: atexit was not thread safe on the free-threaded build
-
-        # I'm not certain this needs to be in a script runner, but
-        # let's do it anyway.
-        code = textwrap.dedent("""
         from threading import Thread
 
         def dummy():
@@ -70,10 +66,9 @@ def thready():
                 atexit.unregister(dummy)
 
 
-        for x in range(100):
-            Thread(target=thready, args=()).start()
-        """)
-        script_helper.assert_python_ok("-c", code)
+        threads = [Thread(target=thready) for _ in range(100)]
+        with threading_helper.start_threads(threads):
+            pass
 
 
 @support.cpython_only
diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index 4160fd4fd9bbc3..b02099e3dcb4fa 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -62,9 +62,12 @@ atexit_delete_cb(struct atexit_state *state, int i)
     atexit_py_callback *cb = state->callbacks[i];
     state->callbacks[i] = NULL;
 
+    // This can execute code via finalizers
+    _PyAtExit_UNLOCK(state);
     Py_DECREF(cb->func);
     Py_DECREF(cb->args);
     Py_XDECREF(cb->kwargs);
+    _PyAtExit_LOCK(state);
     PyMem_Free(cb);
 }
 
@@ -143,8 +146,12 @@ atexit_callfuncs(struct atexit_state *state)
         }
 
         // bpo-46025: Increment the refcount of cb->func as the call itself may unregister it
-        PyObject* the_func = Py_NewRef(cb->func);
-        PyObject *res = PyObject_Call(cb->func, cb->args, cb->kwargs);
+        PyObject *the_func = Py_NewRef(cb->func);
+        PyObject *the_args = cb->args;
+        PyObject *the_kwargs = cb->kwargs;
+        // Unlock for re-entrancy problems
+        _PyAtExit_UNLOCK(state);
+        PyObject *res = PyObject_Call(the_func, the_args, the_kwargs);
         if (res == NULL) {
             PyErr_FormatUnraisable(
                 "Exception ignored in atexit callback %R", the_func);
@@ -153,6 +160,7 @@ atexit_callfuncs(struct atexit_state *state)
             Py_DECREF(res);
         }
         Py_DECREF(the_func);
+        _PyAtExit_LOCK(state);
     }
 
     atexit_cleanup(state);

From 9eeea2e193ccaa38d9f146c691d0f941c72f5130 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Sat, 16 Nov 2024 10:37:34 -0500
Subject: [PATCH 05/21] Don't lock on the GIL-ful build.

---
 Include/internal/pycore_atexit.h | 3 ++-
 Modules/atexitmodule.c           | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Include/internal/pycore_atexit.h b/Include/internal/pycore_atexit.h
index 98f2702d63f91f..40c913640d87c4 100644
--- a/Include/internal/pycore_atexit.h
+++ b/Include/internal/pycore_atexit.h
@@ -52,8 +52,9 @@ struct atexit_state {
     atexit_py_callback **callbacks;
     int ncallbacks;
     int callback_len;
-
+#ifdef Py_GIL_DISABLED
     PyMutex lock;
+#endif
 };
 
 // Export for '_interpchannels' shared extension
diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index b02099e3dcb4fa..364cfc04b7b5c7 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -12,10 +12,15 @@
 #include "pycore_interp.h"        // PyInterpreterState.atexit
 #include "pycore_pystate.h"       // _PyInterpreterState_GET
 
+#ifdef Py_GIL_DISABLED
 /* Note: anything declared as static in this file assumes the lock is held
  * (except for the Python-level functions) */
 #define _PyAtExit_LOCK(state) PyMutex_Lock(&state->lock);
 #define _PyAtExit_UNLOCK(state) PyMutex_Unlock(&state->lock);
+#else
+#define _PyAtExit_LOCK(state)
+#define _PyAtExit_UNLOCK(state)
+#endif
 
 /* ===================================================================== */
 /* Callback machinery. */

From 60bb43f59af9a4822a8dd64a1546485db3c396e2 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Sat, 16 Nov 2024 10:39:07 -0500
Subject: [PATCH 06/21] Add blurb

---
 .../next/Library/2024-11-16-10-39-02.gh-issue-126907.fWRL_R.rst | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 Misc/NEWS.d/next/Library/2024-11-16-10-39-02.gh-issue-126907.fWRL_R.rst

diff --git a/Misc/NEWS.d/next/Library/2024-11-16-10-39-02.gh-issue-126907.fWRL_R.rst b/Misc/NEWS.d/next/Library/2024-11-16-10-39-02.gh-issue-126907.fWRL_R.rst
new file mode 100644
index 00000000000000..d33d2aa23b21b3
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2024-11-16-10-39-02.gh-issue-126907.fWRL_R.rst
@@ -0,0 +1,2 @@
+Fix crash when using :mod:`atexit` concurrently on the :term:`free-threaded
+<free threading>` build.

From fd8a0df9cfed911fe3d52555330a23cd05c9ec8c Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Mon, 9 Dec 2024 10:16:00 -0500
Subject: [PATCH 07/21] Update Modules/atexitmodule.c

Co-authored-by: Victor Stinner <vstinner@python.org>
---
 Modules/atexitmodule.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index 364cfc04b7b5c7..a3371b88af8644 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -15,11 +15,11 @@
 #ifdef Py_GIL_DISABLED
 /* Note: anything declared as static in this file assumes the lock is held
  * (except for the Python-level functions) */
-#define _PyAtExit_LOCK(state) PyMutex_Lock(&state->lock);
-#define _PyAtExit_UNLOCK(state) PyMutex_Unlock(&state->lock);
+#  define _PyAtExit_LOCK(state) PyMutex_Lock(&state->lock);
+#  define _PyAtExit_UNLOCK(state) PyMutex_Unlock(&state->lock);
 #else
-#define _PyAtExit_LOCK(state)
-#define _PyAtExit_UNLOCK(state)
+#  define _PyAtExit_LOCK(state)
+#  define _PyAtExit_UNLOCK(state)
 #endif
 
 /* ===================================================================== */

From cca1bd5fd7921809b88550dc5339773aa5d95e99 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Mon, 9 Dec 2024 10:16:31 -0500
Subject: [PATCH 08/21] Update Modules/atexitmodule.c

Co-authored-by: Victor Stinner <vstinner@python.org>
---
 Modules/atexitmodule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index a3371b88af8644..d75508777d196f 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -326,7 +326,7 @@ atexit_unregister(PyObject *module, PyObject *func)
         }
         PyObject *to_compare = cb->func;
 
-        // Unlock for fear of a custom __eq__ causing re-entrancy
+        // Unlock for custom __eq__ causing re-entrancy
         _PyAtExit_UNLOCK(state);
         int eq = PyObject_RichCompareBool(to_compare, func, Py_EQ);
         if (eq < 0) {

From e4ce835d09f8698f9804e30f3813c9dfce9251a2 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Mon, 9 Dec 2024 10:23:49 -0500
Subject: [PATCH 09/21] Remove `the_` prefix

---
 Modules/atexitmodule.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index d75508777d196f..68c36adf272549 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -151,12 +151,12 @@ atexit_callfuncs(struct atexit_state *state)
         }
 
         // bpo-46025: Increment the refcount of cb->func as the call itself may unregister it
-        PyObject *the_func = Py_NewRef(cb->func);
-        PyObject *the_args = cb->args;
-        PyObject *the_kwargs = cb->kwargs;
+        PyObject *func = Py_NewRef(cb->func);
+        PyObject *args = cb->args;
+        PyObject *kwargs = cb->kwargs;
         // Unlock for re-entrancy problems
         _PyAtExit_UNLOCK(state);
-        PyObject *res = PyObject_Call(the_func, the_args, the_kwargs);
+        PyObject *res = PyObject_Call(func, args, kwargs);
         if (res == NULL) {
             PyErr_FormatUnraisable(
                 "Exception ignored in atexit callback %R", the_func);
@@ -164,7 +164,7 @@ atexit_callfuncs(struct atexit_state *state)
         else {
             Py_DECREF(res);
         }
-        Py_DECREF(the_func);
+        Py_DECREF(func);
         _PyAtExit_LOCK(state);
     }
 

From d1acb86316b52a4ed54f96ecb769b858017f5e30 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Mon, 9 Dec 2024 10:32:01 -0500
Subject: [PATCH 10/21] Add a newline and don't re-lock for deallocating

---
 Modules/atexitmodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index 68c36adf272549..3b3537ef58fff4 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -67,12 +67,12 @@ atexit_delete_cb(struct atexit_state *state, int i)
     atexit_py_callback *cb = state->callbacks[i];
     state->callbacks[i] = NULL;
 
-    // This can execute code via finalizers
+    // We don't need to hold the lock; the entry isn't in the array anymore
     _PyAtExit_UNLOCK(state);
+
     Py_DECREF(cb->func);
     Py_DECREF(cb->args);
     Py_XDECREF(cb->kwargs);
-    _PyAtExit_LOCK(state);
     PyMem_Free(cb);
 }
 

From 801c4f1793ed53b35d16ef567fb29f6b6a2b24b9 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Mon, 9 Dec 2024 10:35:28 -0500
Subject: [PATCH 11/21] Clarify the reference for the parameters

---
 Modules/atexitmodule.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index 3b3537ef58fff4..c7f158edfe4296 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -152,8 +152,10 @@ atexit_callfuncs(struct atexit_state *state)
 
         // bpo-46025: Increment the refcount of cb->func as the call itself may unregister it
         PyObject *func = Py_NewRef(cb->func);
+        // No need to hold a strong reference to the arguments though
         PyObject *args = cb->args;
         PyObject *kwargs = cb->kwargs;
+
         // Unlock for re-entrancy problems
         _PyAtExit_UNLOCK(state);
         PyObject *res = PyObject_Call(func, args, kwargs);

From a48f2ac27e5bfe4cf17dae2587331cf0fe65c976 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Mon, 9 Dec 2024 10:36:19 -0500
Subject: [PATCH 12/21] Fix failing build.

---
 Modules/atexitmodule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index c7f158edfe4296..c60ab88b23c535 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -161,7 +161,7 @@ atexit_callfuncs(struct atexit_state *state)
         PyObject *res = PyObject_Call(func, args, kwargs);
         if (res == NULL) {
             PyErr_FormatUnraisable(
-                "Exception ignored in atexit callback %R", the_func);
+                "Exception ignored in atexit callback %R", func);
         }
         else {
             Py_DECREF(res);

From 968836130046972a1ba00584514681bbbe08cbe0 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Mon, 9 Dec 2024 16:47:04 -0500
Subject: [PATCH 13/21] Add some assertions and suffix functions with _locked

---
 Modules/atexitmodule.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index c60ab88b23c535..d41dc9aa2f082f 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -13,13 +13,15 @@
 #include "pycore_pystate.h"       // _PyInterpreterState_GET
 
 #ifdef Py_GIL_DISABLED
-/* Note: anything declared as static in this file assumes the lock is held
- * (except for the Python-level functions) */
 #  define _PyAtExit_LOCK(state) PyMutex_Lock(&state->lock);
 #  define _PyAtExit_UNLOCK(state) PyMutex_Unlock(&state->lock);
+#  define _PyAtExit_ASSERT_LOCKED(state) assert(PyMutex_IsLocked(&state->lock));
+#  define _PyAtExit_ASSERT_UNLOCKED(state) assert(!PyMutex_IsLocked(&state->lock));
 #else
 #  define _PyAtExit_LOCK(state)
 #  define _PyAtExit_UNLOCK(state)
+#  define _PyAtExit_ASSERT_LOCKED()
+#  define _PyAtExit_ASSERT_UNLOCKED()
 #endif
 
 /* ===================================================================== */
@@ -62,24 +64,27 @@ PyUnstable_AtExit(PyInterpreterState *interp,
 
 
 static void
-atexit_delete_cb(struct atexit_state *state, int i)
+atexit_delete_cb_locked(struct atexit_state *state, int i)
 {
     atexit_py_callback *cb = state->callbacks[i];
     state->callbacks[i] = NULL;
 
-    // We don't need to hold the lock; the entry isn't in the array anymore
+    // Finalizers might be re-entrant. Technically, we don't need
+    // the lock anymore, but the caller assumes that it still holds the lock.
     _PyAtExit_UNLOCK(state);
 
     Py_DECREF(cb->func);
     Py_DECREF(cb->args);
     Py_XDECREF(cb->kwargs);
     PyMem_Free(cb);
+
+    _PyAtExit_LOCK(state);
 }
 
 
 /* Clear all callbacks without calling them */
 static void
-atexit_cleanup(struct atexit_state *state)
+atexit_cleanup_locked(struct atexit_state *state)
 {
     atexit_py_callback *cb;
     for (int i = 0; i < state->ncallbacks; i++) {
@@ -87,7 +92,7 @@ atexit_cleanup(struct atexit_state *state)
         if (cb == NULL)
             continue;
 
-        atexit_delete_cb(state, i);
+        atexit_delete_cb_locked(state, i);
     }
     state->ncallbacks = 0;
 }
@@ -99,6 +104,7 @@ _PyAtExit_Init(PyInterpreterState *interp)
     struct atexit_state *state = &interp->atexit;
     // _PyAtExit_Init() must only be called once
     assert(state->callbacks == NULL);
+    _PyAtExit_ASSERT_UNLOCKED(state);
 
     state->callback_len = 32;
     state->ncallbacks = 0;
@@ -114,9 +120,10 @@ void
 _PyAtExit_Fini(PyInterpreterState *interp)
 {
     struct atexit_state *state = &interp->atexit;
-    // XXX Can this be unlocked?
-    _PyAtExit_LOCK(state);
-    atexit_cleanup(state);
+    // Only one thread can call this, no need to lock it
+    _PyAtExit_ASSERT_UNLOCKED(state);
+
+    atexit_cleanup_locked(state);
     PyMem_Free(state->callbacks);
     state->callbacks = NULL;
 
@@ -131,13 +138,13 @@ _PyAtExit_Fini(PyInterpreterState *interp)
         PyMem_Free(callback);
         exitfunc(data);
     }
-    _PyAtExit_UNLOCK(state);
 }
 
 
 static void
-atexit_callfuncs(struct atexit_state *state)
+atexit_callfuncs_locked(struct atexit_state *state)
 {
+    _PyAtExit_ASSERT_LOCKED(state);
     assert(!PyErr_Occurred());
 
     if (state->ncallbacks == 0) {
@@ -151,8 +158,8 @@ atexit_callfuncs(struct atexit_state *state)
         }
 
         // bpo-46025: Increment the refcount of cb->func as the call itself may unregister it
-        PyObject *func = Py_NewRef(cb->func);
         // No need to hold a strong reference to the arguments though
+        PyObject *func = Py_NewRef(cb->func);
         PyObject *args = cb->args;
         PyObject *kwargs = cb->kwargs;
 
@@ -170,8 +177,7 @@ atexit_callfuncs(struct atexit_state *state)
         _PyAtExit_LOCK(state);
     }
 
-    atexit_cleanup(state);
-
+    atexit_cleanup_locked(state);
     assert(!PyErr_Occurred());
 }
 
@@ -181,7 +187,7 @@ _PyAtExit_Call(PyInterpreterState *interp)
 {
     struct atexit_state *state = &interp->atexit;
     _PyAtExit_LOCK(state);
-    atexit_callfuncs(state);
+    atexit_callfuncs_locked(state);
     _PyAtExit_UNLOCK(state);
 }
 
@@ -269,7 +275,7 @@ atexit_run_exitfuncs(PyObject *module, PyObject *unused)
 {
     struct atexit_state *state = get_atexit_state();
     _PyAtExit_LOCK(state);
-    atexit_callfuncs(state);
+    atexit_callfuncs_locked(state);
     _PyAtExit_UNLOCK(state);
     Py_RETURN_NONE;
 }
@@ -285,7 +291,7 @@ atexit_clear(PyObject *module, PyObject *unused)
 {
     struct atexit_state *state = get_atexit_state();
     _PyAtExit_LOCK(state);
-    atexit_cleanup(state);
+    atexit_cleanup_locked(state);
     _PyAtExit_UNLOCK(state);
     Py_RETURN_NONE;
 }
@@ -342,7 +348,7 @@ atexit_unregister(PyObject *module, PyObject *func)
             continue;
         }
         if (eq) {
-            atexit_delete_cb(state, i);
+            atexit_delete_cb_locked(state, i);
         }
     }
     _PyAtExit_UNLOCK(state);

From 826dbe35e851c7eaa0d345ca2e63200fbf400760 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Mon, 9 Dec 2024 16:51:54 -0500
Subject: [PATCH 14/21] Add my name to the comment.

---
 Modules/atexitmodule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index d41dc9aa2f082f..298907fa903315 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -225,7 +225,7 @@ atexit_register(PyObject *module, PyObject *args, PyObject *kwargs)
     }
 
     struct atexit_state *state = get_atexit_state();
-    /* In theory, we could hold the lock for a shorter amount of time
+    /* (Peter Bierma/ZeroIntensity) In theory, we could hold the lock for a shorter amount of time
      * using some fancy compare-exchanges with the length. However, I'm lazy.
      */
     _PyAtExit_LOCK(state);

From 48c1b114752d8f299f2be1dd1e0cd2c453d4ff74 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Mon, 9 Dec 2024 16:53:47 -0500
Subject: [PATCH 15/21] Fix state restoration upon allocation failure.

---
 Modules/atexitmodule.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index 298907fa903315..1855df0907fb38 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -235,6 +235,7 @@ atexit_register(PyObject *module, PyObject *args, PyObject *kwargs)
         size_t size = sizeof(atexit_py_callback*) * (size_t)state->callback_len;
         r = (atexit_py_callback**)PyMem_Realloc(state->callbacks, size);
         if (r == NULL) {
+            state->callback_len -= 16;
             _PyAtExit_UNLOCK(state);
             return PyErr_NoMemory();
         }

From a2d4afa725e8b9d4eaac5e1a18faa4eb5ae0078a Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Tue, 10 Dec 2024 07:02:28 -0500
Subject: [PATCH 16/21] Update atexitmodule.c

Co-authored-by: Victor Stinner <vstinner@python.org>
---
 Modules/atexitmodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index 1855df0907fb38..d3d15007b47c54 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -20,8 +20,8 @@
 #else
 #  define _PyAtExit_LOCK(state)
 #  define _PyAtExit_UNLOCK(state)
-#  define _PyAtExit_ASSERT_LOCKED()
-#  define _PyAtExit_ASSERT_UNLOCKED()
+#  define _PyAtExit_ASSERT_LOCKED(state)
+#  define _PyAtExit_ASSERT_UNLOCKED(state)
 #endif
 
 /* ===================================================================== */

From 5cc26fee969ff565370abab652dfcf6eb9dad931 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Tue, 10 Dec 2024 13:16:54 +0000
Subject: [PATCH 17/21] Revert callback length restoration.

---
 Modules/atexitmodule.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index d3d15007b47c54..c5060a234a0678 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -235,7 +235,6 @@ atexit_register(PyObject *module, PyObject *args, PyObject *kwargs)
         size_t size = sizeof(atexit_py_callback*) * (size_t)state->callback_len;
         r = (atexit_py_callback**)PyMem_Realloc(state->callbacks, size);
         if (r == NULL) {
-            state->callback_len -= 16;
             _PyAtExit_UNLOCK(state);
             return PyErr_NoMemory();
         }

From 32e7415876fec9833e503afd87d49450ea119745 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Tue, 10 Dec 2024 13:18:22 +0000
Subject: [PATCH 18/21] Lock around atexit_cleanup

---
 Modules/atexitmodule.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index c5060a234a0678..8da50fd5dd94f2 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -120,10 +120,12 @@ void
 _PyAtExit_Fini(PyInterpreterState *interp)
 {
     struct atexit_state *state = &interp->atexit;
-    // Only one thread can call this, no need to lock it
+    // Only one thread can call this, but atexit_cleanup_locked() assumes
+    // that the lock is held, so let's hold it anyway.
     _PyAtExit_ASSERT_UNLOCKED(state);
-
+    _PyAtExit_LOCK(state);
     atexit_cleanup_locked(state);
+    _PyAtExit_UNLOCK(state);
     PyMem_Free(state->callbacks);
     state->callbacks = NULL;
 

From 11f7e1673aedf521ad1395ec6087f03f95c14fd5 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Tue, 10 Dec 2024 13:20:07 +0000
Subject: [PATCH 19/21] Run test in a subprocess.

---
 Lib/test/test_atexit.py | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Lib/test/test_atexit.py b/Lib/test/test_atexit.py
index 4d9da43597dc29..71fc722e35898b 100644
--- a/Lib/test/test_atexit.py
+++ b/Lib/test/test_atexit.py
@@ -52,6 +52,7 @@ def test_atexit_instances(self):
     @unittest.skipUnless(support.Py_GIL_DISABLED, "only meaningful without the GIL")
     def test_atexit_thread_safety(self):
         # GH-126907: atexit was not thread safe on the free-threaded build
+        source = """
         from threading import Thread
 
         def dummy():
@@ -67,9 +68,14 @@ def thready():
 
 
         threads = [Thread(target=thready) for _ in range(100)]
-        with threading_helper.start_threads(threads):
-            pass
+        for thread in threads:
+            thread.start()
+
+        for thread in threads:
+            thread.join()
+        """
 
+        assert_python_ok(textwrap.dedent(source))
 
 @support.cpython_only
 class SubinterpreterTest(unittest.TestCase):

From 9a8edf99e0cec245bae371cfbaac3f20e97a293a Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Tue, 10 Dec 2024 14:05:41 +0000
Subject: [PATCH 20/21] Fit within PEP 7

---
 Modules/atexitmodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
index 8da50fd5dd94f2..8cb3d8730dbf83 100644
--- a/Modules/atexitmodule.c
+++ b/Modules/atexitmodule.c
@@ -227,8 +227,9 @@ atexit_register(PyObject *module, PyObject *args, PyObject *kwargs)
     }
 
     struct atexit_state *state = get_atexit_state();
-    /* (Peter Bierma/ZeroIntensity) In theory, we could hold the lock for a shorter amount of time
-     * using some fancy compare-exchanges with the length. However, I'm lazy.
+    /* (Peter Bierma/ZeroIntensity) In theory, we could hold the
+     * lock for a shorter amount of time using some fancy compare-exchanges
+     * with the length. However, I'm lazy.
      */
     _PyAtExit_LOCK(state);
     if (state->ncallbacks >= state->callback_len) {

From f4beed7c06cfa66b64c1cfb92f643d5a3b4f2c13 Mon Sep 17 00:00:00 2001
From: Peter Bierma <zintensitydev@gmail.com>
Date: Tue, 10 Dec 2024 14:06:42 +0000
Subject: [PATCH 21/21] Clarify the subprocess with a comment.

---
 Lib/test/test_atexit.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Lib/test/test_atexit.py b/Lib/test/test_atexit.py
index 71fc722e35898b..2311e4b3c64497 100644
--- a/Lib/test/test_atexit.py
+++ b/Lib/test/test_atexit.py
@@ -75,6 +75,8 @@ def thready():
             thread.join()
         """
 
+        # atexit._clear() has some evil side effects, and we don't
+        # want them to affect the rest of the tests.
         assert_python_ok(textwrap.dedent(source))
 
 @support.cpython_only