Commit 2d6116b5 by Dean Moldovan

Fix GIL release and acquire when embedding the interpreter

Fixes a race condition when multiple threads try to acquire the GIL
before `detail::internals` have been initialized. `gil_scoped_release`
is now tasked with initializing `internals` (guaranteed single-threaded)
to ensure the safety of subsequent `acquire` calls from multiple threads.
parent f42af24a
...@@ -1661,9 +1661,13 @@ private: ...@@ -1661,9 +1661,13 @@ private:
class gil_scoped_release { class gil_scoped_release {
public: public:
explicit gil_scoped_release(bool disassoc = false) : disassoc(disassoc) { explicit gil_scoped_release(bool disassoc = false) : disassoc(disassoc) {
// `get_internals()` must be called here unconditionally in order to initialize
// `internals.tstate` for subsequent `gil_scoped_acquire` calls. Otherwise, an
// initialization race could occur as multiple threads try `gil_scoped_acquire`.
const auto &internals = detail::get_internals();
tstate = PyEval_SaveThread(); tstate = PyEval_SaveThread();
if (disassoc) { if (disassoc) {
auto key = detail::get_internals().tstate; auto key = internals.tstate;
#if PY_MAJOR_VERSION < 3 #if PY_MAJOR_VERSION < 3
PyThread_delete_key_value(key); PyThread_delete_key_value(key);
#else #else
......
...@@ -26,6 +26,9 @@ else() ...@@ -26,6 +26,9 @@ else()
target_link_libraries(test_embed PRIVATE ${PYTHON_LIBRARIES}) target_link_libraries(test_embed PRIVATE ${PYTHON_LIBRARIES})
endif() endif()
find_package(Threads REQUIRED)
target_link_libraries(test_embed PUBLIC ${CMAKE_THREAD_LIBS_INIT})
add_custom_target(cpptest COMMAND $<TARGET_FILE:test_embed> add_custom_target(cpptest COMMAND $<TARGET_FILE:test_embed>
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}) WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
add_dependencies(check cpptest) add_dependencies(check cpptest)
#include <pybind11/embed.h> #include <pybind11/embed.h>
#include <catch.hpp> #include <catch.hpp>
#include <thread>
namespace py = pybind11; namespace py = pybind11;
using namespace py::literals; using namespace py::literals;
...@@ -185,3 +187,32 @@ TEST_CASE("Execution frame") { ...@@ -185,3 +187,32 @@ TEST_CASE("Execution frame") {
py::exec("var = dict(number=42)"); py::exec("var = dict(number=42)");
REQUIRE(py::globals()["var"]["number"].cast<int>() == 42); REQUIRE(py::globals()["var"]["number"].cast<int>() == 42);
} }
TEST_CASE("Threads") {
// Restart interpreter to ensure threads are not initialized
py::finalize_interpreter();
py::initialize_interpreter();
REQUIRE_FALSE(has_pybind11_internals_static());
constexpr auto num_threads = 10;
auto locals = py::dict("count"_a=0);
{
py::gil_scoped_release gil_release{};
REQUIRE(has_pybind11_internals_static());
auto threads = std::vector<std::thread>();
for (auto i = 0; i < num_threads; ++i) {
threads.emplace_back([&]() {
py::gil_scoped_acquire gil{};
py::exec("count += 1", py::globals(), locals);
});
}
for (auto &thread : threads) {
thread.join();
}
}
REQUIRE(locals["count"].cast<int>() == num_threads);
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment