fix(free-threading): cast self directly in cpp_function::dispatcher

include/pybind11/detail/function_record_pyobject.h

@@ -12,6 +12,7 @@
 
 #include "common.h"
 
+#include <cassert>
 #include <cstring>
 #include <utility>
 
@@ -102,6 +103,14 @@ inline PyTypeObject *get_function_record_PyTypeObject() {
     return py_type_obj;
 }
 
+// This works across extension modules, and does not need the internals lock.
+// Note that tp_name is versioned.
+inline bool function_record_PyTypeObject_name_matches(PyTypeObject *obj_type) {
+    return strcmp(obj_type->tp_name, function_record_PyTypeObject_methods::tp_qualname_impl) == 0
+           || strcmp(obj_type->tp_name, function_record_PyTypeObject_methods::tp_plainname_impl)
+                  == 0;
+}
+
 inline bool is_function_record_PyObject(PyObject *obj) {
     if (PyType_Check(obj) != 0) {
         return false;
@@ -114,13 +123,7 @@ inline bool is_function_record_PyObject(PyObject *obj) {
     if (obj_type == frtype) {
         return true;
     }
-    // This works across extension modules. Note that tp_name is versioned.
-    if (strcmp(obj_type->tp_name, function_record_PyTypeObject_methods::tp_qualname_impl) == 0
-        || strcmp(obj_type->tp_name, function_record_PyTypeObject_methods::tp_plainname_impl)
-               == 0) {
-        return true;
-    }
-    return false;
+    return function_record_PyTypeObject_name_matches(obj_type);
 }
 
 inline function_record *function_record_ptr_from_PyObject(PyObject *obj) {
@@ -130,6 +133,17 @@ inline function_record *function_record_ptr_from_PyObject(PyObject *obj) {
     return nullptr;
 }
 
+// The `self` of cpp_function::dispatcher() is always the function_record_PyObject that was
+// created for that dispatcher in cpp_function::initialize_generic(), so the type check can be
+// skipped. is_function_record_PyObject() is deliberately not used here: it calls
+// get_function_record_PyTypeObject(), which acquires the internals lock on every call.
+inline function_record *function_record_ptr_from_dispatcher_self(PyObject *self) {
+    assert(self != nullptr);
+    assert(PyType_Check(self) == 0);
+    assert(function_record_PyTypeObject_name_matches(Py_TYPE(self)));
+    return (reinterpret_cast<function_record_PyObject *>(self))->cpp_func_rec;
+}
+
 inline object function_record_PyObject_New() {
     auto *py_func_rec = PyObject_New(function_record_PyObject, get_function_record_PyTypeObject());
     if (py_func_rec == nullptr) {

include/pybind11/pybind11.h

@@ -967,7 +967,7 @@ class cpp_function : public function {
     static PyObject *
     dispatcher(PyObject *self, PyObject *const *args_in_arr, size_t nargsf, PyObject *kwnames_in) {
         using namespace detail;
-        const function_record *overloads = function_record_ptr_from_PyObject(self);
+        const function_record *overloads = function_record_ptr_from_dispatcher_self(self);
         assert(overloads != nullptr);
 
         /* Iterator over the list of potentially admissible overloads */

tests/test_thread.cpp

@@ -8,11 +8,16 @@
 */
 
 #include <pybind11/cast.h>
+#include <pybind11/detail/internals.h>
 #include <pybind11/pybind11.h>
 
 #include "pybind11_tests.h"
 
 #include <chrono>
+#include <condition_variable>
+#include <mutex>
+#include <stdexcept>
+#include <string>
 #include <thread>
 
 #if defined(PYBIND11_HAS_STD_BARRIER)
@@ -35,6 +40,144 @@ struct IntStruct {
 struct EmptyStruct {};
 EmptyStruct SharedInstance;
 
+#ifdef Py_GIL_DISABLED
+
+// Holds detail::internals::mutex on a native thread that never touches the Python C API, so that
+// a Python-level call can be checked for independence from the internals lock. The lock is a
+// no-op unless Py_GIL_DISABLED, hence the #ifdef. See test_dispatch_does_not_need_internals_lock.
+class internals_lock_holder {
+public:
+    internals_lock_holder() = default;
+    internals_lock_holder(const internals_lock_holder &) = delete;
+    internals_lock_holder &operator=(const internals_lock_holder &) = delete;
+    ~internals_lock_holder() {
+        if (thread.joinable()) {
+            request_release_and_join();
+        }
+    }
+
+    // Returns once the native thread holds the internals mutex. The thread releases it when
+    // release_and_join() is called, or after `watchdog` if nobody asks: that keeps the process
+    // moving when the call under test blocks on the mutex.
+    void start(py::detail::pymutex &internals_mutex, std::chrono::milliseconds watchdog) {
+        if (thread.joinable()) {
+            throw std::runtime_error("internals_lock_holder is already started");
+        }
+        held = false;
+        release_requested = false;
+        released_by_watchdog = false;
+        thread = std::thread([this, &internals_mutex, watchdog]() {
+            internals_mutex.lock();
+            {
+                std::lock_guard<std::mutex> lock(m);
+                held = true;
+            }
+            cv.notify_all();
+            {
+                std::unique_lock<std::mutex> lock(m);
+                released_by_watchdog
+                    = !cv.wait_for(lock, watchdog, [this]() { return release_requested; });
+            }
+            internals_mutex.unlock();
+        });
+        std::unique_lock<std::mutex> lock(m);
+        cv.wait(lock, [this]() { return held; });
+    }
+
+    // Returns true if the watchdog had already released the mutex.
+    bool release_and_join() {
+        if (!thread.joinable()) {
+            throw std::runtime_error("internals_lock_holder is not started");
+        }
+        return request_release_and_join();
+    }
+
+private:
+    bool request_release_and_join() {
+        {
+            std::lock_guard<std::mutex> lock(m);
+            release_requested = true;
+        }
+        cv.notify_all();
+        thread.join();
+        return released_by_watchdog;
+    }
+
+    std::mutex m;
+    std::condition_variable cv;
+    bool held = false;
+    bool release_requested = false;
+    bool released_by_watchdog = false;
+    std::thread thread;
+};
+
+internals_lock_holder &get_internals_lock_holder() {
+    static internals_lock_holder holder;
+    return holder;
+}
+
+// Deliberately raw PyCFunctions, not pybind11 bindings: these must be callable while the
+// internals mutex is held, and whether a pybind11 dispatch can be is exactly what the test checks.
+PyObject *start_internals_lock_holder(PyObject *, PyObject *watchdog_seconds_obj) {
+    const double watchdog_seconds = PyFloat_AsDouble(watchdog_seconds_obj);
+    if (watchdog_seconds == -1.0 && PyErr_Occurred() != nullptr) {
+        return nullptr;
+    }
+    const auto watchdog = std::chrono::duration_cast<std::chrono::milliseconds>(
+        std::chrono::duration<double>(watchdog_seconds));
+    // Looked up while attached; the holder thread itself never uses the Python C API.
+    auto &internals_mutex = py::detail::get_internals().mutex;
+    std::string error;
+    PyThreadState *save = PyEval_SaveThread();
+    try {
+        get_internals_lock_holder().start(internals_mutex, watchdog);
+    } catch (const std::exception &e) {
+        error = e.what();
+    }
+    PyEval_RestoreThread(save);
+    if (!error.empty()) {
+        PyErr_SetString(PyExc_RuntimeError, error.c_str());
+        return nullptr;
+    }
+    Py_RETURN_NONE;
+}
+
+PyObject *release_internals_lock_holder(PyObject *, PyObject *) {
+    bool released_by_watchdog = false;
+    std::string error;
+    PyThreadState *save = PyEval_SaveThread();
+    try {
+        released_by_watchdog = get_internals_lock_holder().release_and_join();
+    } catch (const std::exception &e) {
+        error = e.what();
+    }
+    PyEval_RestoreThread(save);
+    if (!error.empty()) {
+        PyErr_SetString(PyExc_RuntimeError, error.c_str());
+        return nullptr;
+    }
+    return PyBool_FromLong(released_by_watchdog ? 1 : 0);
+}
+
+// Positive control for the test: a call that is known to take the internals lock.
+PyObject *take_internals_lock(PyObject *, PyObject *) {
+    try {
+        py::detail::with_internals([](py::detail::internals &) {});
+    } catch (py::error_already_set &e) {
+        e.restore();
+        return nullptr;
+    }
+    Py_RETURN_NONE;
+}
+
+PyMethodDef internals_lock_methods[]
+    = {{"start_internals_lock_holder", start_internals_lock_holder, METH_O, nullptr},
+       {"release_internals_lock_holder", release_internals_lock_holder, METH_NOARGS, nullptr},
+       {"take_internals_lock", take_internals_lock, METH_NOARGS, nullptr},
+       {nullptr, nullptr, 0, nullptr}};
+
+#endif // Py_GIL_DISABLED
+
 } // namespace
 
 TEST_SUBMODULE(thread, m) {
@@ -103,6 +246,15 @@ TEST_SUBMODULE(thread, m) {
 #endif
     m.def("acquire_gil", []() { py::gil_scoped_acquire gil_acquired; });
 
+    // The call under test for test_dispatch_does_not_need_internals_lock: the smallest possible
+    // bound function, so that the only pybind11 machinery involved is cpp_function::dispatcher().
+    m.def("dispatch_noop", []() {});
+#ifdef Py_GIL_DISABLED
+    if (PyModule_AddFunctions(m.ptr(), internals_lock_methods) != 0) {
+        throw py::error_already_set();
+    }
+#endif
+
     // NOTE: std::string_view also uses loader_life_support to ensure that
     // the string contents remain alive, but that's a C++ 17 feature.
 }

tests/test_thread.py

@@ -1,11 +1,15 @@
 from __future__ import annotations
 
+import os
+import subprocess
 import sys
+import textwrap
 import threading
 
 import pytest
 
 import env
+import pybind11_tests
 from pybind11_tests import thread as m
 
 
@@ -78,3 +82,59 @@ def __del__(self):
             m.acquire_gil()
 
     m.test_pythread_state_clear_destructor(Foo)
+
+
+@pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads")
+@pytest.mark.skipif(env.ANDROID or env.IOS, reason="Requires subprocess support")
+@pytest.mark.skipif(
+    not env.PY_GIL_DISABLED, reason="The internals lock is a no-op with the GIL"
+)
+def test_dispatch_does_not_need_internals_lock():
+    """A bound call must not block on the internals mutex while another thread holds it.
+
+    Regression test for #6159 (item 59): cpp_function::dispatcher() used to reach its
+    function_record through get_function_record_PyTypeObject(), which takes the internals lock
+    on every call. Runs in a subprocess because on a regression the call under test blocks
+    until the holder's watchdog fires.
+    """
+    script = textwrap.dedent(
+        f"""
+        import sys
+
+        sys.path.insert(0, {os.path.dirname(env.__file__)!r})
+        sys.path.insert(0, {os.path.dirname(pybind11_tests.__file__)!r})
+
+        from pybind11_tests import thread as m
+
+        m.dispatch_noop()  # Warm up any one-time initialization.
+
+        # Positive control: the holder holds the very mutex that internals users take, so a
+        # call that needs it can only return after the holder's watchdog fires.
+        m.start_internals_lock_holder(1.0)
+        m.take_internals_lock()
+        assert m.release_internals_lock_holder(), "take_internals_lock() did not block"
+
+        # The call under test: a plain bound call must not need the internals lock at all.
+        m.start_internals_lock_holder(5.0)
+        m.dispatch_noop()
+        released_by_watchdog = m.release_internals_lock_holder()
+        assert not released_by_watchdog, "cpp_function::dispatcher() needed the internals lock"
+        """
+    )
+    try:
+        proc = subprocess.run(
+            [sys.executable, "-c", script],
+            capture_output=True,
+            text=True,
+            timeout=60,
+            check=False,
+        )
+    except subprocess.TimeoutExpired as ex:
+        pytest.fail(
+            f"Subprocess did not finish within {ex.timeout} s (deadlock?).\n"
+            f"Output:\n{ex.stdout}\n{ex.stderr}"
+        )
+    assert proc.returncode == 0, (
+        f"Subprocess failed with exit code {proc.returncode}.\n"
+        f"Output:\n{proc.stdout}\n{proc.stderr}"
+    )