-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix crash in gil_scoped_acquire
#5828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -15,6 +15,11 @@ | |||
#include <chrono> | ||||
#include <thread> | ||||
|
||||
#if defined(PYBIND11_CPP20) && defined(__has_include) && __has_include(<barrier>) | ||||
# define PYBIND11_HAS_BARRIER 1 | ||||
# include <barrier> | ||||
#endif | ||||
|
||||
Comment on lines
+18
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please move this somewhere around here? pybind11/include/pybind11/detail/common.h Line 97 in 852a4b5 I think |
||||
namespace py = pybind11; | ||||
|
||||
namespace { | ||||
|
@@ -34,7 +39,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 | ||||
|
@@ -67,6 +71,39 @@ TEST_SUBMODULE(thread, m) { | |||
py::class_<EmptyStruct>(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") = | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other places we use a pattern that would be (It makes it a little easier to pin-point all things connected to |
||||
#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. | ||||
} |
There was a problem hiding this comment.
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 andPy_GIL_DISABLED
?There was a problem hiding this comment.
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.