diff --git a/include/pybind11/gil_safe_call_once.h b/include/pybind11/gil_safe_call_once.h index af26272d48..d733683c85 100644 --- a/include/pybind11/gil_safe_call_once.h +++ b/include/pybind11/gil_safe_call_once.h @@ -135,6 +135,9 @@ class gil_safe_call_once_and_store { // subinterpreter has its own separate state. The cached result may not shareable across // interpreters (e.g., imported modules and their members). +template +class gil_safe_call_once_and_store; + PYBIND11_NAMESPACE_BEGIN(detail) template @@ -143,9 +146,16 @@ struct call_once_storage { std::once_flag once_flag; void (*finalize)(T &) = nullptr; std::atomic_bool is_initialized{false}; + // The `gil_safe_call_once_and_store` which caches a pointer into `storage`, if any. + // It is a global static, therefore it outlives this storage, which the interpreter owns. + gil_safe_call_once_and_store *owner = nullptr; call_once_storage() = default; ~call_once_storage() { + // The interpreter destroys this storage, therefore the owner must forget its cache. + if (owner != nullptr) { + owner->invalidate_cache(reinterpret_cast(storage)); + } if (is_initialized) { if (finalize != nullptr) { finalize(*reinterpret_cast(storage)); @@ -193,6 +203,8 @@ class gil_safe_call_once_and_store { ::new (value->storage) T(fn()); value->finalize = finalize_fn; value->is_initialized = true; + // Let the storage reset the cache below when the interpreter destroys it. + value->owner = this; // Publish the cached pointer before setting the validity flag, so that any reader // which observes the flag as true is guaranteed to also observe this pointer. last_storage_ptr_ = reinterpret_cast(value->storage); @@ -216,6 +228,7 @@ class gil_safe_call_once_and_store { gil_scoped_acquire gil_acq; auto *value = get_or_create_storage_in_state_dict(); result = reinterpret_cast(value->storage); + value->owner = this; last_storage_ptr_ = result; } assert(result != nullptr); @@ -236,6 +249,17 @@ class gil_safe_call_once_and_store { private: using storage_type = detail::call_once_storage; + friend storage_type; + + // Called from the destructor of the storage, i.e. when the interpreter which owns the storage + // is finalized. The flag is cleared first, so that a reader which observes the flag as true + // never loads a null pointer. + void invalidate_cache(T *storage_ptr) { + if (last_storage_ptr_.load() == storage_ptr) { + is_initialized_by_at_least_one_interpreter_ = false; + last_storage_ptr_ = nullptr; + } + } // Indicator of fast path for single-interpreter case. bool is_last_storage_valid() const { diff --git a/tests/test_with_catch/test_interpreter.cpp b/tests/test_with_catch/test_interpreter.cpp index e39f51c274..a72f8c1c94 100644 --- a/tests/test_with_catch/test_interpreter.cpp +++ b/tests/test_with_catch/test_interpreter.cpp @@ -95,6 +95,15 @@ PYBIND11_EMBEDDED_MODULE(enum_module, m, py::multiple_interpreters::per_interpre .value("value2", SomeEnum::value2); } +class SomeCppException : public std::runtime_error { // Added for gh-6159 + using std::runtime_error::runtime_error; +}; + +PYBIND11_EMBEDDED_MODULE(exception_module, m, py::multiple_interpreters::per_interpreter_gil()) { + py::register_exception(m, "SomeCppException"); + m.def("raise_it", []() { throw SomeCppException("C++ Error"); }); +} + PYBIND11_EMBEDDED_MODULE(throw_exception, , py::multiple_interpreters::not_supported()) { throw std::runtime_error("C++ Error"); } @@ -357,18 +366,65 @@ TEST_CASE("Enum module survives restart") { // Added in PR #6015 // calls process_attributes::init after initialize_generic's strdup loop, // leaving arg names as string literals. Without the fix, destruct() would // call free() on those literals during interpreter finalization. - PYBIND11_CATCH2_SKIP_IF(PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION == 12, - "Pre-existing crash in enum cleanup during finalize on Python 3.12"); + { + // Scoped: the reference must not outlive the interpreter which owns it. + auto enum_mod = py::module_::import("enum_module"); + REQUIRE(enum_mod.attr("SomeEnum").attr("value1").attr("name").cast() + == "value1"); + } + + py::finalize_interpreter(); + py::initialize_interpreter(); auto enum_mod = py::module_::import("enum_module"); - REQUIRE(enum_mod.attr("SomeEnum").attr("value1").attr("name").cast() == "value1"); + REQUIRE(enum_mod.attr("SomeEnum").attr("value2").attr("name").cast() == "value2"); +} + +#ifdef PYBIND11_HAS_SUBINTERPRETER_SUPPORT +// Added for gh-6159: the per-interpreter storage is destroyed with the interpreter, therefore the +// fast-path cache in `gil_safe_call_once_and_store` must be reset, too. +PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store restart_test_storage; +static int restart_test_call_count = 0; + +TEST_CASE("gil_safe_call_once_and_store survives restart") { // Added for gh-6159 + auto import_sys_modules = []() { + restart_test_call_count++; + return py::module_::import("sys").attr("modules"); + }; + + auto &first = restart_test_storage.call_once_and_store_result(import_sys_modules).get_stored(); + REQUIRE(restart_test_call_count == 1); + REQUIRE(first.ptr() == py::module_::import("sys").attr("modules").ptr()); py::finalize_interpreter(); py::initialize_interpreter(); - enum_mod = py::module_::import("enum_module"); - REQUIRE(enum_mod.attr("SomeEnum").attr("value2").attr("name").cast() == "value2"); + auto &second + = restart_test_storage.call_once_and_store_result(import_sys_modules).get_stored(); + // The stored value belongs to the finalized interpreter, therefore it must be stored again. + REQUIRE(restart_test_call_count == 2); + REQUIRE(second.ptr() == py::module_::import("sys").attr("modules").ptr()); +} + +TEST_CASE("Exception module survives restart") { // Added for gh-6159 + // Regression test for item 3 of gh-6159: `py::register_exception` stores the Python exception + // type in a `gil_safe_call_once_and_store`. Without the fix, the stale cache made the second + // import a no-op, and the module had no `SomeCppException` attribute (use after free). + { + // Scoped: the reference must not outlive the interpreter which owns it. + auto exc_mod = py::module_::import("exception_module"); + REQUIRE(py::hasattr(exc_mod, "SomeCppException")); + REQUIRE_THROWS_WITH(exc_mod.attr("raise_it")(), "SomeCppException: C++ Error"); + } + + py::finalize_interpreter(); + py::initialize_interpreter(); + + auto exc_mod = py::module_::import("exception_module"); + REQUIRE(py::hasattr(exc_mod, "SomeCppException")); + REQUIRE_THROWS_WITH(exc_mod.attr("raise_it")(), "SomeCppException: C++ Error"); } +#endif TEST_CASE("Execution frame") { // When the interpreter is embedded, there is no execution frame, but `py::exec`