Fix lock discipline; use newer graalpy with fewer workarounds needed

Author: oremanj

.codespell-ignore-lines

@@ -2,7 +2,7 @@ template <op_id id, op_type ot, typename L = undefined_t, typename R = undefined
     template <typename ThisT>
         auto &this_ = static_cast<ThisT &>(*this);
                 if (load_impl<ThisT>(temp, false)) {
-                return load_impl<ThisT>(src, false);
+                    return load_impl<ThisT>(src, false);
         ssize_t nd = 0;
         auto trivial = broadcast(buffers, nd, shape);
         auto ndim = (size_t) nd;

.github/workflows/ci.yml

@@ -51,7 +51,7 @@ jobs:
             python-version: 'pypy-3.11-v7.3.23'
             cmake-args: -DCMAKE_CXX_STANDARD=17
           - runs-on: ubuntu-latest
-            python-version: 'graalpy-25.0'
+            python-version: 'graalpy-25.2.4'
             cmake-args: -DCMAKE_CXX_STANDARD=20
           - runs-on: ubuntu-latest
             python-version: '3.15'
@@ -112,7 +112,7 @@ jobs:
           - runs-on: macos-latest
             python-version: 'pypy-3.11-v7.3.23'
           - runs-on: macos-latest
-            python-version: 'graalpy-25.0'
+            python-version: 'graalpy-25.2.4'
 
           - runs-on: windows-latest
             python-version: '3.9'

include/pybind11/contrib/pymetabind.h

@@ -7,7 +7,9 @@
  * This functionality is intended to be used by the framework itself,
  * rather than by users of the framework.
  *
- * This is version 0.4 of pymetabind. Changelog:
+ * This is version 0.4+dev of pymetabind. Changelog:
+ *
+ *      Unreleased: Use critical section API to avoid lock ordering issues.
  *
  *     Version 0.4: Properly return NULL if registry capsule creation fails.
  *      2026-01-23  Support concurrent calls to `pymb_get_registry`.
@@ -270,20 +272,47 @@ struct pymb_registry {
 #endif
 };
 
-#if defined(Py_GIL_DISABLED)
-PYMB_INLINE void pymb_lock_registry(struct pymb_registry* registry) {
-    PyMutex_Lock(&registry->mutex);
+#if !defined(Py_GIL_DISABLED)
+
+struct pymb_lock_ticket {
+    char unused;
+};
+PYMB_INLINE void pymb_lock_registry(struct pymb_lock_ticket* ticket,
+                                    struct pymb_registry* registry) {
+    (void) ticket;
+    (void) registry;
+}
+PYMB_INLINE void pymb_unlock_registry(struct pymb_lock_ticket* ticket) {
+    (void) ticket;
 }
-PYMB_INLINE void pymb_unlock_registry(struct pymb_registry* registry) {
-    PyMutex_Unlock(&registry->mutex);
+
+#elif PY_VERSION_HEX >= 0x030E00C1 // 3.14.0rc1 has PyCriticalSection_BeginMutex
+
+struct pymb_lock_ticket {
+    PyCriticalSection cs;
+};
+PYMB_INLINE void pymb_lock_registry(struct pymb_lock_ticket* ticket,
+                                    struct pymb_registry* registry) {
+    PyCriticalSection_BeginMutex(&ticket->cs, &registry->mutex);
 }
+PYMB_INLINE void pymb_unlock_registry(struct pymb_lock_ticket* ticket) {
+    PyCriticalSection_End(&ticket->cs);
+}
+
 #else
-PYMB_INLINE void pymb_lock_registry(struct pymb_registry* registry) {
-    (void) registry;
+
+struct pymb_lock_ticket {
+    struct pymb_registry* registry;
+};
+PYMB_INLINE void pymb_lock_registry(struct pymb_lock_ticket* ticket,
+                                    struct pymb_registry* registry) {
+    PyMutex_Lock(&registry->mutex);
+    ticket->registry = registry;
 }
-PYMB_INLINE void pymb_unlock_registry(struct pymb_registry* registry) {
-    (void) registry;
+PYMB_INLINE void pymb_unlock_registry(struct pymb_lock_ticket* ticket) {
+    PyMutex_Unlock(&ticket->registry->mutex);
 }
+
 #endif
 
 struct pymb_binding;
@@ -908,7 +937,9 @@ PYMB_FUNC void pymb_add_framework(struct pymb_registry* registry,
     framework->link.next = NULL;
     framework->link.prev = NULL;
     framework->registry = registry;
-    pymb_lock_registry(registry);
+
+    struct pymb_lock_ticket ticket;
+    pymb_lock_registry(&ticket, registry);
     PYMB_LIST_FOREACH(struct pymb_framework*, other, registry->frameworks) {
         // Intern `abi_extra` strings so they can be compared by pointer
         if (other->abi_extra && framework->abi_extra &&
@@ -930,7 +961,7 @@ PYMB_FUNC void pymb_add_framework(struct pymb_registry* registry,
             framework->add_foreign_binding(binding);
         }
     }
-    pymb_unlock_registry(registry);
+    pymb_unlock_registry(&ticket);
 }
 
 /*
@@ -1036,14 +1067,15 @@ PYMB_FUNC void pymb_add_binding(struct pymb_binding* binding,
     }
     Py_DECREF(binding->capsule); // keep only a borrowed reference
 
-    pymb_lock_registry(registry);
+    struct pymb_lock_ticket ticket;
+    pymb_lock_registry(&ticket, registry);
     pymb_list_append(&registry->bindings, &binding->link);
     PYMB_LIST_FOREACH(struct pymb_framework*, other, registry->frameworks) {
         if (other != binding->framework) {
             other->add_foreign_binding(binding);
         }
     }
-    pymb_unlock_registry(registry);
+    pymb_unlock_registry(&ticket);
     return;
 
   error:
@@ -1083,11 +1115,12 @@ PYMB_FUNC void pymb_remove_binding_internal(struct pymb_binding* binding,
 
     // Since we need to obtain it anyway, use the registry lock to serialize
     // concurrent attempts to remove the same binding
-    pymb_lock_registry(registry);
+    struct pymb_lock_ticket ticket;
+    pymb_lock_registry(&ticket, registry);
     if (!binding->capsule) {
         // Binding was concurrently removed from multiple places; the first
         // one to get the registry lock wins.
-        pymb_unlock_registry(registry);
+        pymb_unlock_registry(&ticket);
         return;
     }
 
@@ -1130,7 +1163,7 @@ PYMB_FUNC void pymb_remove_binding_internal(struct pymb_binding* binding,
             other->remove_foreign_binding(binding);
         }
     }
-    pymb_unlock_registry(registry);
+    pymb_unlock_registry(&ticket);
 
 #if !defined(Py_GIL_DISABLED)
     // On GIL builds, there's no need to delay deallocation

include/pybind11/detail/foreign.h

@@ -684,14 +684,17 @@ inline void foreign_internals::enable_autoimport() {
     });
     if (enabled_by_us) {
         // Note lock order: pymb_registry lock is 'outside' our internals lock.
-        pymb_lock_registry(registry);
+        // (This doesn't matter after 3.14 since the critical section API fixes
+        // lock order issues.)
+        struct pymb_lock_ticket ticket;
+        pymb_lock_registry(&ticket, registry);
         // NOLINTNEXTLINE(modernize-use-auto)
         PYMB_LIST_FOREACH(struct pymb_binding *, binding, registry->bindings) {
             if (binding->framework != self.get()) {
                 foreign_cb_add_foreign_binding(binding);
             }
         }
-        pymb_unlock_registry(registry);
+        pymb_unlock_registry(&ticket);
     }
 }
 
@@ -814,17 +817,18 @@ export_to_foreign(const std::type_info *cpptype, PyTypeObject *pytype, type_info
     ++foreign_internals.bindings_update_count;
     lst.push_back(binding);
 
-#ifdef Py_GIL_DISABLED
-    // Call pymb_add_binding() with unlocked internals in order to maintain
-    // consistent lock order: the pymb_registry lock is locked outside our
+#if defined(Py_GIL_DISABLED) && PY_VERSION_HEX < 0x030E00C1
+    // This interpreter doesn't have PyCriticalSection_BeginMutex(), so
+    // we must ensure a consistent lock order between our internals lock
+    // and the pymb_registry lock. The pymb_registry lock is locked outside our
     // internals lock in enable_autoimport(), so it must not be locked inside
     // our internals lock here. pymb_add_binding() is noexcept so we don't
     // need a scope guard.
     auto &internals = get_internals();
     internals.mutex.unlock();
 #endif
     pymb_add_binding(binding, /* tp_finalize_will_remove */ 0);
-#ifdef Py_GIL_DISABLED
+#if defined(Py_GIL_DISABLED) && PY_VERSION_HEX < 0x030E00C1
     internals.mutex.lock();
 #endif
 }
@@ -881,7 +885,9 @@ PYBIND11_NOINLINE void *try_foreign_bindings(const std::type_info *type,
             {
 #ifdef Py_GIL_DISABLED
                 // attempt() might execute Python code; drop the internals lock
-                // to avoid a deadlock
+                // to avoid a deadlock. This can be removed once we require
+                // 3.14, since using PyCriticalSection for the internals lock
+                // is already sufficient to prevent a deadlock.
                 auto guard = lock.temporarily_drop();
 #endif
                 void *result = attempt(closure, binding);

tests/test_foreign.py

@@ -2,6 +2,7 @@
 from __future__ import annotations
 
 import collections
+import contextlib
 import gc
 import itertools
 import sys
@@ -58,8 +59,7 @@ def delattr_and_ensure_destroyed(*specs):
 @pytest.fixture(autouse=True)
 def clean_after():
     yield
-    if sys.implementation.name != "graalpy":
-        t3.clear_foreign_bindings()
+    t3.clear_foreign_bindings()
 
     if not types_are_immortal:
         delattr_and_ensure_destroyed(
@@ -75,11 +75,9 @@ def clean_after():
         # this helps prevent different tests from interfering with each other
         for mod in (t1, t2, t3, t4, t5):
             for name in ("Shared", "SharedEnum", "RawShared"):
-                if (
-                    (ty := getattr(mod, name, None))
-                    and hasattr(ty, "__pymetabind_binding__")
-                ):
-                    del ty.__pymetabind_binding__
+                if ty := getattr(mod, name, None):
+                    with contextlib.suppress(AttributeError):
+                        del ty.__pymetabind_binding__
 
     if sys.implementation.name in ("pypy", "graalpy"):
         pytest.gc_collect()
@@ -89,12 +87,9 @@ def clean_after():
 
 
 def check_stats(mod, **entries):
-    if mod is None or sys.implementation.name == "graalpy":
-        # graalpy seems to not do a full collection when we gc_collect(); there
-        # will be too few destructions in one check_stats() and then correspondingly
-        # too many in the next one for the same module, so just skip the check
+    if mod is None:
         return
-    if sys.implementation.name == "pypy":
+    if sys.implementation.name in ("pypy", "graalpy"):
         pytest.gc_collect()
     stats = mod.pull_stats()
     if 0 < (move_diff := stats["move"] - entries.get("move", 0)) <= 2:
@@ -356,12 +351,10 @@ def test_import_export_errors():
     t3.create_raw_binding()
 
     # Can't import a type that doesn't have __pymetabind_binding__
-    try:
+    with contextlib.suppress(AttributeError):
         # Convertible gets a binding because it's in an export-by-default
         # module, but we don't rely on that binding anywhere
         del t1.Convertible.__pymetabind_binding__
-    except AttributeError:
-        pass
     with pytest.raises(
         RuntimeError, match="type does not define a __pymetabind_binding__"
     ):
@@ -851,12 +844,3 @@ def test_on_request_with_local_binding():
     # But t4 can now accept t1's types
     obj1 = t1.make(30)
     assert t4.check(obj1) == 30
-
-
-# =====================================================================
-# GraalPy has issues with GC-dependent tests
-# =====================================================================
-
-if sys.implementation.name == "graalpy":
-    del test_implicit_conversion_from_foreign
-    del test_three_module_interop

tests/test_foreign_3.cpp

@@ -213,21 +213,22 @@ PYBIND11_MODULE(test_foreign_3, m, py::mod_gil_not_used()) {
 
     m.def("clear_foreign_bindings", [hm]() {
         py::list bound;
-        pymb_lock_registry(registry);
+        struct pymb_lock_ticket ticket;
+        pymb_lock_registry(&ticket, registry);
         // NOLINTNEXTLINE(modernize-use-auto)
         PYMB_LIST_FOREACH(struct pymb_binding *, binding, registry->bindings) {
             bound.append(py::reinterpret_borrow<py::object>((PyObject *) binding->pytype));
         }
-        pymb_unlock_registry(registry);
+        pymb_unlock_registry(&ticket);
         for (auto type : bound) {
             py::delattr(type, "__pymetabind_binding__");
         }
 
         bool bindings_removed = false;
         for (int i = 0; i < 5; ++i) {
-            pymb_lock_registry(registry);
+            pymb_lock_registry(&ticket, registry);
             bindings_removed = pymb_list_is_empty(&registry->bindings);
-            pymb_unlock_registry(registry);
+            pymb_unlock_registry(&ticket);
             if (bindings_removed) {
                 break;
             }