diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index f57514ae28..6c6fad350c 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2800,6 +2800,16 @@ class class_ : public detail::generic_type { template ::value, int> = 0> static void init_instance(detail::instance *inst, const void *holder_ptr) { + // A factory-based `py::init` keeps the `py::call_guard` + // 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); @@ -2840,6 +2850,9 @@ class class_ : public detail::generic_type { // void (*init_instance)(instance *, const void *); auto *holder_void_ptr = const_cast(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 f136086e84..191284b1a8 100644 --- a/tests/test_gil_scoped.cpp +++ b/tests/test_gil_scoped.cpp @@ -11,6 +11,8 @@ #include "pybind11_tests.h" +#include +#include #include #include @@ -27,6 +29,11 @@ class VirtClass { 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 @@ TEST_SUBMODULE(gil_scoped, m) { .def("virtual_func", &VirtClass::virtual_func) .def("pure_virtual_func", &VirtClass::pure_virtual_func); + py::class_(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(new SlowInit(state)); + }), + py::call_guard()); + m.def("test_callback_py_obj", [](py::object &func) { func(); }); m.def("test_callback_std_func", [](const std::function &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 fc998b0ed2..8c0ac9597d 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -160,6 +160,37 @@ def test_all_basic_tests_completeness(): 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`. + + `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()