-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: change PYBIND11_EMBEDDED_MODULE to multiphase init #5665
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
Changes from all commits
79c3dec
733d248
8e5e26d
d7946ab
64f7a0a
8160c8a
7cbc3fc
ca5ea1e
f0e6ac2
8c99bfb
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 |
---|---|---|
|
@@ -55,7 +55,7 @@ class test_override_cache_helper_trampoline : public test_override_cache_helper | |
int func() override { PYBIND11_OVERRIDE(int, test_override_cache_helper, func); } | ||
}; | ||
|
||
PYBIND11_EMBEDDED_MODULE(widget_module, m) { | ||
PYBIND11_EMBEDDED_MODULE(widget_module, m, py::multiple_interpreters::per_interpreter_gil()) { | ||
py::class_<Widget, PyWidget>(m, "Widget") | ||
.def(py::init<std::string>()) | ||
.def_property_readonly("the_message", &Widget::the_message); | ||
|
@@ -336,6 +336,7 @@ TEST_CASE("Restart the interpreter") { | |
REQUIRE(py_widget.attr("the_message").cast<std::string>() == "Hello after restart"); | ||
} | ||
|
||
#if defined(PYBIND11_SUBINTERPRETER_SUPPORT) | ||
TEST_CASE("Subinterpreter") { | ||
py::module_::import("external_module"); // in the main interpreter | ||
|
||
|
@@ -347,6 +348,10 @@ TEST_CASE("Subinterpreter") { | |
|
||
REQUIRE(m.attr("add")(1, 2).cast<int>() == 3); | ||
} | ||
|
||
auto main_int | ||
= py::module_::import("external_module").attr("internals_at")().cast<uintptr_t>(); | ||
|
||
REQUIRE(has_state_dict_internals_obj()); | ||
REQUIRE(has_pybind11_internals_static()); | ||
|
||
|
@@ -359,7 +364,6 @@ TEST_CASE("Subinterpreter") { | |
// Subinterpreters get their own copy of builtins. | ||
REQUIRE_FALSE(has_state_dict_internals_obj()); | ||
|
||
#if defined(PYBIND11_SUBINTERPRETER_SUPPORT) && PY_VERSION_HEX >= 0x030C0000 | ||
// internals hasn't been populated yet, but will be different for the subinterpreter | ||
REQUIRE_FALSE(has_pybind11_internals_static()); | ||
|
||
|
@@ -369,14 +373,12 @@ TEST_CASE("Subinterpreter") { | |
py::detail::get_internals(); | ||
REQUIRE(has_pybind11_internals_static()); | ||
REQUIRE(get_details_as_uintptr() == ext_int); | ||
#else | ||
// This static is still defined | ||
REQUIRE(has_pybind11_internals_static()); | ||
#endif | ||
REQUIRE(main_int != ext_int); | ||
|
||
// Modules tags should be gone. | ||
REQUIRE_FALSE(py::hasattr(py::module_::import("__main__"), "tag")); | ||
{ | ||
REQUIRE_NOTHROW(py::module_::import("widget_module")); | ||
auto m = py::module_::import("widget_module"); | ||
REQUIRE_FALSE(py::hasattr(m, "extension_module_tag")); | ||
|
||
|
@@ -397,7 +399,6 @@ TEST_CASE("Subinterpreter") { | |
REQUIRE(has_state_dict_internals_obj()); | ||
} | ||
|
||
#if defined(PYBIND11_SUBINTERPRETER_SUPPORT) | ||
TEST_CASE("Multiple Subinterpreters") { | ||
// Make sure the module is in the main interpreter and save its pointer | ||
auto *main_ext = py::module_::import("external_module").ptr(); | ||
|
@@ -512,10 +513,11 @@ TEST_CASE("Per-Subinterpreter GIL") { | |
|
||
// we have switched to the new interpreter and released the main gil | ||
|
||
// widget_module did not provide the mod_per_interpreter_gil tag, so it cannot be imported | ||
// trampoline_module did not provide the per_interpreter_gil tag, so it cannot be | ||
// imported | ||
bool caught = false; | ||
try { | ||
py::module_::import("widget_module"); | ||
py::module_::import("trampoline_module"); | ||
} catch (pybind11::error_already_set &pe) { | ||
T_REQUIRE(pe.matches(PyExc_ImportError)); | ||
std::string msg(pe.what()); | ||
|
@@ -525,6 +527,9 @@ TEST_CASE("Per-Subinterpreter GIL") { | |
} | ||
T_REQUIRE(caught); | ||
|
||
// widget_module did provide the per_interpreter_gil tag, so it this does not throw | ||
py::module_::import("widget_module"); | ||
|
||
T_REQUIRE(!py::hasattr(py::module_::import("external_module"), "multi_interp")); | ||
py::module_::import("external_module").attr("multi_interp") = std::to_string(num); | ||
|
||
|
@@ -547,8 +552,8 @@ TEST_CASE("Per-Subinterpreter GIL") { | |
|
||
Py_EndInterpreter(sub); | ||
|
||
PyThreadState_Swap( | ||
main_tstate); // switch back so the scoped_acquire can release the GIL properly | ||
// switch back so the scoped_acquire can release the GIL properly | ||
PyThreadState_Swap(main_tstate); | ||
}; | ||
|
||
std::thread t1(thread_main, 1); | ||
|
@@ -622,12 +627,26 @@ TEST_CASE("Threads") { | |
|
||
{ | ||
py::gil_scoped_release gil_release{}; | ||
#if defined(Py_GIL_DISABLED) && PY_VERSION_HEX < 0x030E0000 | ||
std::mutex mutex; | ||
#endif | ||
|
||
auto threads = std::vector<std::thread>(); | ||
for (auto i = 0; i < num_threads; ++i) { | ||
threads.emplace_back([&]() { | ||
py::gil_scoped_acquire gil{}; | ||
#ifdef Py_GIL_DISABLED | ||
# if PY_VERSION_HEX < 0x030E0000 | ||
std::lock_guard<std::mutex> lock(mutex); | ||
locals["count"] = locals["count"].cast<int>() + 1; | ||
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. The double-locking here (GIL in line 637, C++ mutex in line 640) has the potential to deadlock, because the code in line 641 calls into the Python C API, which may release (and reacquire) the GIL (see docs/advanced/deadlock.md). — I can believe that this will be extremely rare, but we should leave a comment here to explain that we're aware of the risk, and that we're knowingly accepting it. See also: https://chatgpt.com/share/682a07ec-dd70-8008-92ea-e722ff92a055 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. I don't think this can deadlock. These specific concrete types and calls will not release/reacquire the GIL (PyDict_Get/SetItem with string keys and long object values). 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.
I believe that, but examples and tests are often copy-pasted, and then something else is done here. The comment I'm suggesting is to make people aware of the pitfall. This is not a safe pattern in general. |
||
# else | ||
Py_BEGIN_CRITICAL_SECTION(locals.ptr()); | ||
locals["count"] = locals["count"].cast<int>() + 1; | ||
Py_END_CRITICAL_SECTION(); | ||
# endif | ||
#else | ||
locals["count"] = locals["count"].cast<int>() + 1; | ||
#endif | ||
}); | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.