From 361eacfb442d07c3773da22f9591acf7c7409e8d Mon Sep 17 00:00:00 2001 From: Rostan Tabet Date: Fri, 5 Sep 2025 13:22:47 +0200 Subject: [PATCH 1/4] Add a test reproducing the #5827 crash Signed-off-by: Rostan Tabet --- tests/test_thread.cpp | 39 ++++++++++++++++++++++++++++++++++++++- tests/test_thread.py | 12 ++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp index eabf39afa1..eb3e4eb586 100644 --- a/tests/test_thread.cpp +++ b/tests/test_thread.cpp @@ -15,6 +15,11 @@ #include #include +#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include() +# define PYBIND11_HAS_BARRIER 1 +# include +#endif + namespace py = pybind11; namespace { @@ -34,7 +39,6 @@ EmptyStruct SharedInstance; } // namespace TEST_SUBMODULE(thread, m) { - py::class_(m, "IntStruct").def(py::init([](const int i) { return IntStruct(i); })); // implicitly_convertible uses loader_life_support when an implicit @@ -67,6 +71,39 @@ TEST_SUBMODULE(thread, m) { py::class_(m, "EmptyStruct") .def_readonly_static("SharedInstance", &SharedInstance); +#if defined(PYBIND11_HAS_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. + m.def("test_pythread_state_clear_destructor", [](py::type cls) { + py::handle obj; + + std::barrier barrier{2}; + std::thread thread1{[&]() { + py::gil_scoped_acquire gil; + obj = cls().release(); + barrier.arrive_and_wait(); + }}; + std::thread thread2{[&]() { + py::gil_scoped_acquire gil; + barrier.arrive_and_wait(); + // ob_ref_shared becomes negative; transition to the queued state + obj.dec_ref(); + }}; + + // jthread is not supported by Apple Clang + thread1.join(); + thread2.join(); + }); +#endif + + m.attr("has_barrier") = +#ifdef PYBIND11_HAS_BARRIER + true; +#else + false; +#endif + m.def("acquire_gil", []() { py::gil_scoped_acquire gil_acquired; }); + // 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 e9d7bafb2f..034322c6da 100644 --- a/tests/test_thread.py +++ b/tests/test_thread.py @@ -5,6 +5,7 @@ import pytest +import env from pybind11_tests import thread as m @@ -66,3 +67,14 @@ def access_shared_instance(): thread.start() for thread in threads: thread.join() + + +@pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads") +@pytest.mark.skipif(not m.has_barrier, reason="no ") +@pytest.mark.skipif(env.sys_is_gil_enabled(), reason="Deadlock with the GIL") +def test_pythread_state_clear_destructor(): + class Foo: + def __del__(self): + m.acquire_gil() + + m.test_pythread_state_clear_destructor(Foo) From 15b5157a8f6aa3b3776098ec88977854a86fd5e7 Mon Sep 17 00:00:00 2001 From: Rostan Tabet Date: Fri, 5 Sep 2025 13:29:17 +0200 Subject: [PATCH 2/4] Fix #5827 Signed-off-by: Rostan Tabet --- include/pybind11/gil.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/pybind11/gil.h b/include/pybind11/gil.h index 4222a035f4..9e799b3cf7 100644 --- a/include/pybind11/gil.h +++ b/include/pybind11/gil.h @@ -120,7 +120,11 @@ class gil_scoped_acquire { pybind11_fail("scoped_acquire::dec_ref(): internal error!"); } # endif + // Make sure that PyThreadState_Clear is not recursively called by finalizers. + // See issue #5827 + ++tstate->gilstate_counter; PyThreadState_Clear(tstate); + --tstate->gilstate_counter; if (active) { PyThreadState_DeleteCurrent(); } From 056a605a1a11fdb5c9c28cc7eb15b1562e29289b Mon Sep 17 00:00:00 2001 From: Rostan Tabet Date: Mon, 8 Sep 2025 08:56:13 +0000 Subject: [PATCH 3/4] Rename PYBIND11_HAS_BARRIER and move it to common.h Signed-off-by: Rostan Tabet --- include/pybind11/detail/common.h | 4 ++++ tests/test_scoped_critical_section.cpp | 7 +++---- tests/test_thread.cpp | 6 +++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 07c0943006..05d6755896 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -103,6 +103,10 @@ # define PYBIND11_DTOR_CONSTEXPR #endif +#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include() +# define PYBIND11_HAS_STD_BARRIER 1 +#endif + // Compiler version assertions #if defined(__INTEL_COMPILER) # if __INTEL_COMPILER < 1800 diff --git a/tests/test_scoped_critical_section.cpp b/tests/test_scoped_critical_section.cpp index dc9a69e039..7401eb09f8 100644 --- a/tests/test_scoped_critical_section.cpp +++ b/tests/test_scoped_critical_section.cpp @@ -7,8 +7,7 @@ #include #include -#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include() -# define PYBIND11_HAS_BARRIER 1 +#if defined(PYBIND11_HAS_STD_BARRIER) # include #endif @@ -39,7 +38,7 @@ class BoolWrapper { std::atomic_bool value_{false}; }; -#if defined(PYBIND11_HAS_BARRIER) +#if defined(PYBIND11_HAS_STD_BARRIER) // Modifying the C/C++ members of a Python object from multiple threads requires a critical section // to ensure thread safety and data integrity. @@ -259,7 +258,7 @@ TEST_SUBMODULE(scoped_critical_section, m) { (void) BoolWrapperHandle.ptr(); // suppress unused variable warning m.attr("has_barrier") = -#ifdef PYBIND11_HAS_BARRIER +#ifdef PYBIND11_HAS_STD_BARRIER true; #else false; diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp index eb3e4eb586..a86b690ae7 100644 --- a/tests/test_thread.cpp +++ b/tests/test_thread.cpp @@ -16,7 +16,7 @@ #include #if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include() -# define PYBIND11_HAS_BARRIER 1 +# define PYBIND11_HAS_STD_BARRIER 1 # include #endif @@ -71,7 +71,7 @@ TEST_SUBMODULE(thread, m) { py::class_(m, "EmptyStruct") .def_readonly_static("SharedInstance", &SharedInstance); -#if defined(PYBIND11_HAS_BARRIER) +#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. m.def("test_pythread_state_clear_destructor", [](py::type cls) { @@ -97,7 +97,7 @@ TEST_SUBMODULE(thread, m) { #endif m.attr("has_barrier") = -#ifdef PYBIND11_HAS_BARRIER +#ifdef PYBIND11_HAS_STD_BARRIER true; #else false; From d2bd9aaf931348cd468768efc31ba48371de0402 Mon Sep 17 00:00:00 2001 From: Rostan Tabet Date: Thu, 13 Nov 2025 23:19:21 +0000 Subject: [PATCH 4/4] In test_thread.{cpp,py}, rename has_barrier Signed-off-by: Rostan Tabet --- tests/test_thread.cpp | 5 ++--- tests/test_thread.py | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp index a86b690ae7..131bd87710 100644 --- a/tests/test_thread.cpp +++ b/tests/test_thread.cpp @@ -15,8 +15,7 @@ #include #include -#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include() -# define PYBIND11_HAS_STD_BARRIER 1 +#if defined(PYBIND11_HAS_STD_BARRIER) # include #endif @@ -96,7 +95,7 @@ TEST_SUBMODULE(thread, m) { }); #endif - m.attr("has_barrier") = + m.attr("defined_PYBIND11_HAS_STD_BARRIER") = #ifdef PYBIND11_HAS_STD_BARRIER true; #else diff --git a/tests/test_thread.py b/tests/test_thread.py index 034322c6da..d302c382c2 100644 --- a/tests/test_thread.py +++ b/tests/test_thread.py @@ -70,7 +70,7 @@ def access_shared_instance(): @pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads") -@pytest.mark.skipif(not m.has_barrier, reason="no ") +@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") def test_pythread_state_clear_destructor(): class Foo: