fix(free-threading): cast `self` directly in `cpp_function::dispatcher` (#6177) * fix(free-threading): cast `self` directly in `cpp_function::dispatcher` Part of #6159 (item 59). Under Py_GIL_DISABLED, cpp_function::dispatcher() took the internals lock on every bound call: it reached its function_record through function_record_ptr_from_PyObject(), which calls get_function_record_PyTypeObject() (PYBIND11_LOCK_INTERNALS on internals.mutex). dispatcher is installed in exactly one place (initialize_generic), and the PyCFunction there is created with m_self set to the function_record_PyObject allocated a few lines above, so self is always that object. The new function_record_ptr_from_dispatcher_self() states that invariant once and casts directly; its assert compares the versioned tp_name instead of calling is_function_record_PyObject(), which would reacquire the lock in non-NDEBUG builds. is_function_record_PyObject() and function_record_ptr_from_PyObject() are unchanged, so the call sites that can see a foreign object keep the checked conversion. Adds test_dispatch_does_not_need_internals_lock (free-threaded builds only): a native thread holds internals.mutex through raw PyCFunction helpers and the test checks that a bound call returns before the holder's watchdog releases the mutex, with a positive control proving the held mutex is the one internals users take. Fails without the fix, passes with it. Assisted-by: Claude Code:claude-opus-5 * test: put the source tests dir on the subprocess path pybind11_tests imports custom_exceptions from the source tests directory at init, so the subprocess must see that directory as well as the build directory, as test_custom_type_setup.py does. Assisted-by: Claude Code:claude-opus-5 * style: qualify function-record name comparisons with std::strcmp Apply the review suggestion to both sibling comparisons; retain the existing cstring include. Assisted-by: Codex:GPT-6
diff --git a/include/pybind11/detail/function_record_pyobject.h b/include/pybind11/detail/function_record_pyobject.h index 42b0e96..2e66944 100644 --- a/include/pybind11/detail/function_record_pyobject.h +++ b/include/pybind11/detail/function_record_pyobject.h
@@ -12,6 +12,7 @@ #include "common.h" +#include <cassert> #include <cstring> #include <utility> @@ -102,6 +103,16 @@ 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 std::strcmp(obj_type->tp_name, function_record_PyTypeObject_methods::tp_qualname_impl) + == 0 + || std::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 +125,7 @@ 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 +135,17 @@ 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) {
diff --git a/include/pybind11/pybind11-inl.h b/include/pybind11/pybind11-inl.h index 24e1359..0cfe9fc 100644 --- a/include/pybind11/pybind11-inl.h +++ b/include/pybind11/pybind11-inl.h
@@ -801,7 +801,7 @@ 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 */
diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp index 131bd87..f64a166 100644 --- a/tests/test_thread.cpp +++ b/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 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 @@ #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. }
diff --git a/tests/test_thread.py b/tests/test_thread.py index d302c38..d688f9e 100644 --- a/tests/test_thread.py +++ b/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 @@ 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}" + )