fix: re-acquire the GIL in class_::init_instance before instance registration (#6172)
* fix: re-acquire GIL in init_instance for thread-safe instance registration
With a factory-based py::init combined with py::call_guard<py::gil_scoped_release>,
init_instance -> register_instance runs while the GIL is released, racing on
internals.registered_instances with GIL-holding threads. This corrupts the
instance map and leads to 'pybind11_object_dealloc(): Tried to deallocate
unregistered instance!' -> std::terminate. Acquire the GIL (no-op if already
held) in both class_::init_instance overloads; free-threaded builds keep using
their sharded mutex and are unaffected.
* tests: avoid std::make_unique in test_gil_scoped (C++14 only)
The iOS/Android wheel CI jobs build the test suite with -std=gnu++11.
* tests: skip test_init_factory_gil_released_concurrent_construction on free-threaded builds
On free-threaded builds py::gil_scoped_release detaches the thread state and the
constructor machinery is not safe when called detached, so the test segfaults
(pre-existing limitation of call_guard<gil_scoped_release>, unrelated to the
init_instance fix). The instance map is mutex-protected on free-threaded builds
anyway, so there is nothing to test there. An early return inside the function
covers the _run_in_process parametrizations, where pytest skip markers do not
apply.
* tests: run the init_instance race regression only as a direct test
The test was in ALL_BASIC_TESTS, so it also ran in the _run_in_process
parametrizations, whose subprocesses impose a 10s timeout; on Windows
(sequential variant) the extra sleep timer granularity and GIL handoff
overhead pushed that over the limit. It is a data-race regression, not a
deadlock check, so define it after ALL_BASIC_TESTS like the
test_run_in_process_* functions and drop the now-unneeded free-threaded
early return.
* chore: retrigger CI (GCC 9 job failed on apt 404 for python3-setuptools, unrelated)
* fix: make init_instance GIL acquisition unconditional
The `#if !defined(Py_GIL_DISABLED)` guards skipped the acquire on free-threaded builds, where `gil_scoped_release` detaches the thread state. `init_instance` then crashes in `PyCriticalSection_BeginMutex` (via `get_type_info`) even single-threaded, because the critical section requires an attached thread state.
`gil_scoped_acquire` attaches the thread state without taking a global lock, so this is safe on free-threaded builds and does not serialize threads.
* test: run init_instance regression on free-threaded builds too
The acquire in init_instance is now unconditional, so the regression test also covers the free-threaded detached-thread-state crash. Drop the PY_GIL_DISABLED skip.
* docs: cover free-threaded thread-state attach in init_instance comments
The unconditional gil_scoped_acquire is also needed on free-threaded builds, where gil_scoped_release detaches the thread state and get_type_info requires it to be attached. Mention this in the init_instance comment and the regression-test docstring.
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 3687983..c57312d 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -2799,6 +2799,16 @@
template <typename H = holder_type,
detail::enable_if_t<!detail::is_smart_holder<H>::value, int> = 0>
static void init_instance(detail::instance *inst, const void *holder_ptr) {
+ // A factory-based `py::init` keeps the `py::call_guard<py::gil_scoped_release>`
+ // alive across the `construct()` call that invokes this function, so
+ // `init_instance` may run with the GIL released. Acquire it (a no-op if it is
+ // already held) so that `register_instance` and `init_holder` only touch
+ // `internals.registered_instances` while the GIL is held.
+ //
+ // On free-threaded builds `gil_scoped_release` detaches the thread state instead:
+ // `gil_scoped_acquire` attaches it again without taking a global lock, as required
+ // by the critical section inside `get_type_info`.
+ gil_scoped_acquire gil;
auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type)));
if (!v_h.instance_registered()) {
register_instance(inst, v_h.value_ptr(), v_h.type);
@@ -2839,6 +2849,9 @@
// void (*init_instance)(instance *, const void *);
auto *holder_void_ptr = const_cast<void *>(holder_const_void_ptr);
+ // See the comment in the non-smart_holder `init_instance` above.
+ gil_scoped_acquire gil;
+
auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type)));
if (!v_h.instance_registered()) {
register_instance(inst, v_h.value_ptr(), v_h.type);
diff --git a/tests/test_gil_scoped.cpp b/tests/test_gil_scoped.cpp
index f136086..191284b 100644
--- a/tests/test_gil_scoped.cpp
+++ b/tests/test_gil_scoped.cpp
@@ -11,6 +11,8 @@
#include "pybind11_tests.h"
+#include <chrono>
+#include <memory>
#include <string>
#include <thread>
@@ -27,6 +29,11 @@
virtual void pure_virtual_func() = 0;
};
+class SlowInit {
+public:
+ explicit SlowInit(int) {}
+};
+
class PyVirtClass : public VirtClass {
void virtual_func() override { PYBIND11_OVERRIDE(void, VirtClass, virtual_func, ); }
void pure_virtual_func() override {
@@ -50,6 +57,16 @@
.def("virtual_func", &VirtClass::virtual_func)
.def("pure_virtual_func", &VirtClass::pure_virtual_func);
+ py::class_<SlowInit>(m, "SlowInit")
+ .def(py::init([](int state) {
+ // Sleep to widen the window in which `init_instance` runs with the GIL
+ // released by the call_guard, making the instance-map race (without the
+ // `init_instance` GIL-acquire fix) much more likely to surface.
+ std::this_thread::sleep_for(std::chrono::milliseconds(1));
+ return std::unique_ptr<SlowInit>(new SlowInit(state));
+ }),
+ py::call_guard<py::gil_scoped_release>());
+
m.def("test_callback_py_obj", [](py::object &func) { func(); });
m.def("test_callback_std_func", [](const std::function<void()> &func) { func(); });
m.def("test_callback_virtual_func", [](VirtClass &virt) { virt.virtual_func(); });
diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py
index fc998b0..8c0ac95 100644
--- a/tests/test_gil_scoped.py
+++ b/tests/test_gil_scoped.py
@@ -160,6 +160,37 @@
assert len(ALL_BASIC_TESTS) == num_found
+# Defined after ALL_BASIC_TESTS on purpose: this test is a regression for the
+# `gil_scoped_release` + factory `py::init` path, not a deadlock check, so it should not
+# run in the _run_in_process parametrizations above (whose subprocesses impose a 10s
+# timeout; on Windows this test is much slower there due to sleep timer granularity and
+# GIL handoff costs).
+@pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads")
+def test_init_factory_gil_released_concurrent_construction():
+ """Concurrent construction via a factory `py::init` with `call_guard<gil_scoped_release>`.
+
+ `init_instance` runs while the GIL is released and must internally acquire the GIL
+ before touching the instance map. Without that fix this aborts with
+ "pybind11_object_dealloc(): Tried to deallocate unregistered instance!" (races on
+ `internals.registered_instances`, which is unguarded on GIL builds). On free-threaded
+ builds the detached thread state instead segfaults in `PyCriticalSection_BeginMutex`
+ (via `get_type_info`), even without concurrency.
+ """
+ num_threads = 8
+ iterations = 100
+
+ def construct_many():
+ for _ in range(iterations):
+ instance = m.SlowInit(0)
+ del instance # Destructor runs with the GIL held (deregistration).
+
+ threads = [threading.Thread(target=construct_many) for _ in range(num_threads)]
+ for thread in threads:
+ thread.start()
+ for thread in threads:
+ thread.join()
+
+
def _intentional_deadlock():
m.intentional_deadlock()
