diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 8cf3f9c4bb..4d574b9bd7 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1714,6 +1714,13 @@ class class_ : public detail::generic_type { } } + static void register_instance_once(detail::instance *inst, detail::value_and_holder &v_h) { + if (!v_h.instance_registered()) { + v_h.set_instance_registered(); + register_instance(inst, v_h.value_ptr(), v_h.type); + } + } + /// Performs instance initialization including constructing a holder and registering the known /// instance. Should be called as soon as the `type` value_ptr is set for an instance. Takes /// an optional pointer to an existing holder to use; if not specified and the instance is @@ -1732,11 +1739,19 @@ class class_ : public detail::generic_type { // 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); - v_h.set_instance_registered(); + // Registering publishes `inst` to other threads, so it should come last. Holders that + // may run user code still register first, since that code can look `inst` up on this + // thread (e.g. via PYBIND11_OVERRIDE). Other threads can then find such an instance before + // its holder is constructed. A custom unique_ptr deleter counts as user code. + constexpr bool register_first = !std::is_same>::value + && !detail::is_shared_ptr::value; + if (register_first) { + register_instance_once(inst, v_h); } init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr()); + if (!register_first) { + register_instance_once(inst, v_h); + } } template @@ -1775,10 +1790,6 @@ class class_ : public detail::generic_type { 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); - v_h.set_instance_registered(); - } auto *uninitialized_location = std::addressof(v_h.holder()); auto *value_ptr_w_t = v_h.value_ptr(); // Try downcast from `type` to `type_alias`: @@ -1799,6 +1810,8 @@ class class_ : public detail::generic_type { } } v_h.set_holder_constructed(); + // Nothing above runs user code, so registering last is always safe here. + register_instance_once(inst, v_h); } // Deallocates an instance; via holder, if constructed; otherwise via operator delete. diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 10f6843202..dc2245f387 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -352,6 +352,45 @@ struct ElementList { std::vector> l; }; +// test_holder_ctor_calls_python_override +class RetainNotifier { +public: + virtual ~RetainNotifier() = default; + virtual void on_retain() {} + void add_ref() { + ++ref_count_; + on_retain(); + } + void release() { + if (--ref_count_ == 0) { + delete this; + } + } + +private: + int ref_count_ = 0; +}; + +class PyRetainNotifier : public RetainNotifier { +public: + void on_retain() override { PYBIND11_OVERRIDE(void, RetainNotifier, on_retain, ); } +}; + +// Holder whose constructor calls back into the object, so constructing it runs user code +// (including Python overrides of on_retain()). +template +class notifying_intrusive_ptr { + T *ptr_; + +public: + explicit notifying_intrusive_ptr(T *p) : ptr_(p) { ptr_->add_ref(); } + notifying_intrusive_ptr(const notifying_intrusive_ptr &other) + : notifying_intrusive_ptr(other.ptr_) {} + notifying_intrusive_ptr &operator=(const notifying_intrusive_ptr &) = delete; + ~notifying_intrusive_ptr() { ptr_->release(); } + T *get() const { return ptr_; } +}; + } // namespace // ref is a wrapper for 'Object' which uses intrusive reference counting @@ -377,6 +416,7 @@ PYBIND11_DECLARE_HOLDER_TYPE(T, huge_unique_ptr, ) PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr, ) PYBIND11_DECLARE_HOLDER_TYPE(T, shared_ptr_with_addressof_operator, ) PYBIND11_DECLARE_HOLDER_TYPE(T, unique_ptr_with_addressof_operator, ) +PYBIND11_DECLARE_HOLDER_TYPE(T, notifying_intrusive_ptr, true) namespace holder_caster_traits_test { struct example_base {}; @@ -658,5 +698,10 @@ TEST_SUBMODULE(smart_ptr, m) { .def_readwrite("ptr", &ContainerUsingPrivateESFT::ptr); // <- access ESFT through shared_ptr + // test_holder_ctor_calls_python_override + py::class_>( + m, "RetainNotifier") + .def(py::init<>()); + holder_caster_traits_test::wrap(m); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index 326b768c40..aaf980969c 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -392,3 +392,14 @@ def test_shared_ptr_cast_for_custom_holder_throws(cast_shared_ptr): ), ): cast_shared_ptr() + + +def test_holder_ctor_calls_python_override(): + calls = [] + + class PyNotifier(m.RetainNotifier): + def on_retain(self): + calls.append("retain") + + PyNotifier() + assert calls == ["retain"] diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp index f64a166850..68adc890fc 100644 --- a/tests/test_thread.cpp +++ b/tests/test_thread.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -40,6 +41,18 @@ struct IntStruct { struct EmptyStruct {}; EmptyStruct SharedInstance; +// Templated so each holder kind gets its own C++ type, since a type can only be bound once. +template +struct SharedObject { + int value() const { return 42; } +}; + +template +std::shared_ptr get_shared() { + static const auto instance = std::make_shared(); + return instance; +} + #ifdef Py_GIL_DISABLED // Holds detail::internals::mutex on a native thread that never touches the Python C API, so that @@ -213,6 +226,14 @@ TEST_SUBMODULE(thread, m) { py::class_(m, "EmptyStruct") .def_readonly_static("SharedInstance", &SharedInstance); + using SharedObjectClassic = SharedObject<0>; + using SharedObjectSh = SharedObject<1>; + py::class_>(m, "SharedObjectClassic") + .def("value", &SharedObjectClassic::value); + m.def("get_shared_object_classic", &get_shared); + py::classh(m, "SharedObjectSh").def("value", &SharedObjectSh::value); + m.def("get_shared_object_sh", &get_shared); + #if defined(PYBIND11_HAS_STD_BARRIER) // In the free-threaded build, during PyThreadState_Clear, removing the thread from the biased // reference counting table may call destructors. Make sure that it doesn't crash. diff --git a/tests/test_thread.py b/tests/test_thread.py index d688f9ee91..2be7492927 100644 --- a/tests/test_thread.py +++ b/tests/test_thread.py @@ -73,6 +73,28 @@ def access_shared_instance(): thread.join() +@pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads") +@pytest.mark.parametrize( + "get_shared", [m.get_shared_object_classic, m.get_shared_object_sh] +) +def test_concurrent_cast_of_shared_cpp_object(get_shared): + # Only fails under ThreadSanitizer on free-threaded builds; otherwise the race is silent. + nb_threads = 4 + b = threading.Barrier(nb_threads) + + def cast_shared_object(): + b.wait() + for _ in range(1000): + # get_shared() alone never reads the wrapper's flags. Calling value() does. + get_shared().value() + + threads = [threading.Thread(target=cast_shared_object) for _ in range(nb_threads)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + @pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads") @pytest.mark.skipif(not m.defined_PYBIND11_HAS_STD_BARRIER, reason="no ") @pytest.mark.skipif(env.sys_is_gil_enabled(), reason="Deadlock with the GIL")