Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/pybind11/detail/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@
# define PYBIND11_DTOR_CONSTEXPR
#endif

#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include(<barrier>)
# define PYBIND11_HAS_STD_BARRIER 1
#endif

// Compiler version assertions
#if defined(__INTEL_COMPILER)
# if __INTEL_COMPILER < 1800
Expand Down
4 changes: 4 additions & 0 deletions include/pybind11/gil.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally not sure: Is there any point in guarding this with #if defined(...) for certain Python versions and Py_GIL_DISABLED?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I focused on free-threading here but I think that the issue can also occur in destructors of thread-local variables in the regular build.

PyThreadState_Clear(tstate);
--tstate->gilstate_counter;
if (active) {
PyThreadState_DeleteCurrent();
}
Expand Down
7 changes: 3 additions & 4 deletions tests/test_scoped_critical_section.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
#include <thread>
#include <utility>

#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include(<barrier>)
# define PYBIND11_HAS_BARRIER 1
#if defined(PYBIND11_HAS_STD_BARRIER)
# include <barrier>
#endif

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down
38 changes: 37 additions & 1 deletion tests/test_thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
#include <chrono>
#include <thread>

#if defined(PYBIND11_HAS_STD_BARRIER)
# include <barrier>
#endif

namespace py = pybind11;

namespace {
Expand All @@ -34,7 +38,6 @@ EmptyStruct SharedInstance;
} // namespace

TEST_SUBMODULE(thread, m) {

py::class_<IntStruct>(m, "IntStruct").def(py::init([](const int i) { return IntStruct(i); }));

// implicitly_convertible uses loader_life_support when an implicit
Expand Down Expand Up @@ -67,6 +70,39 @@ TEST_SUBMODULE(thread, m) {
py::class_<EmptyStruct>(m, "EmptyStruct")
.def_readonly_static("SharedInstance", &SharedInstance);

#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) {
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("defined_PYBIND11_HAS_STD_BARRIER") =
#ifdef PYBIND11_HAS_STD_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.
}
12 changes: 12 additions & 0 deletions tests/test_thread.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import pytest

import env
from pybind11_tests import thread as m


Expand Down Expand Up @@ -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.defined_PYBIND11_HAS_STD_BARRIER, reason="no <barrier>")
@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)
Loading