From 2106ab4f019b86a9b38422c6a5326986931a90e3 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sat, 4 Jan 2020 12:37:00 +0000 Subject: [PATCH] Fix Python GIL causing a hang when running the logging callback pybind11 holds the GIL whenever a C++ function is called from Python, so the logging callback can't then execute Python code. To avoid this problem, release the GIL for every function bound by pybind11 (in case log statements are added to those that don't currently have any), and wrap the logging callback in a lambda function that first aquires the GIL lock for the function's scope. --- src/main.cpp | 76 +++++++++++++++++++++++++++++++++++++++------------- test/test.py | 2 +- 2 files changed, 58 insertions(+), 20 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 36ecc14..c8559e7 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -58,6 +58,14 @@ bool UpdateMasterlist(std::shared_ptr db, std::string masterl MasterlistInfo GetMasterlistRevision(std::shared_ptr db, std::string masterlistPath, bool getShortId) { return db->GetMasterlistRevision(u8path(masterlistPath), getShortId); } + +void SetLoggingCallback(std::function callback) { + callback = [callback](LogLevel level, const char* message) { + pybind11::gil_scoped_acquire acquire; + callback(level, message); + }; + loot::SetLoggingCallback(callback); +} } void bindEnums(pybind11::module& module) { @@ -109,7 +117,9 @@ void bindMetadataClasses(pybind11::module& module) { .def_readwrite("userlist_modified", &PluginTags::userlist_modified); class_(module, "PluginMetadata") - .def("get_simple_messages", &PluginMetadata::GetSimpleMessages); + .def("get_simple_messages", + &PluginMetadata::GetSimpleMessages, + pybind11::call_guard()); } void bindVersionClasses(pybind11::module& module) { @@ -118,36 +128,60 @@ void bindVersionClasses(pybind11::module& module) { .def_readonly_static("minor", &LootVersion::minor) .def_readonly_static("patch", &LootVersion::patch) .def_readonly_static("revision", &LootVersion::revision) - .def_static("string", LootVersion::GetVersionString); + .def_static("string", + LootVersion::GetVersionString, + pybind11::call_guard()); class_(module, "WrapperVersion") .def_readonly_static("major", &WrapperVersion::major) .def_readonly_static("minor", &WrapperVersion::minor) .def_readonly_static("patch", &WrapperVersion::patch) .def_readonly_static("revision", &WrapperVersion::revision) - .def_static("string", WrapperVersion::string); + .def_static("string", + WrapperVersion::string, + pybind11::call_guard()); } void bindInterfaceClasses(pybind11::module& module) { class_>(module, "GameInterface") - .def("load_current_load_order_state", &GameInterface::LoadCurrentLoadOrderState) - .def("get_database", &GameInterface::GetDatabase); + .def("load_current_load_order_state", + &GameInterface::LoadCurrentLoadOrderState, + pybind11::call_guard()) + .def("get_database", + &GameInterface::GetDatabase, + pybind11::call_guard()); class_>(module, "DatabaseInterface") - .def("load_lists", &py::LoadLists, arg("masterlist_path"), arg("userlist_path") = "") - .def("update_masterlist", &py::UpdateMasterlist) - .def("get_masterlist_revision", &py::GetMasterlistRevision) - .def("get_plugin_metadata", &DatabaseInterface::GetPluginMetadata, + .def("load_lists", + &py::LoadLists, + arg("masterlist_path"), + arg("userlist_path") = "", + pybind11::call_guard()) + .def("update_masterlist", + &py::UpdateMasterlist, + pybind11::call_guard()) + .def("get_masterlist_revision", + &py::GetMasterlistRevision, + pybind11::call_guard()) + .def("get_plugin_metadata", + &DatabaseInterface::GetPluginMetadata, arg("plugin"), arg("includeUserMetadata") = true, - arg("evaluateConditions") = false) - .def("get_plugin_tags", &GetPluginTags, + arg("evaluateConditions") = false, + pybind11::call_guard()) + .def("get_plugin_tags", + &GetPluginTags, arg("plugin"), - arg("evaluateConditions") = false) - .def("get_plugin_cleanliness", &GetPluginCleanliness, + arg("evaluateConditions") = false, + pybind11::call_guard()) + .def("get_plugin_cleanliness", + &GetPluginCleanliness, arg("plugin"), - arg("evaluateConditions") = false) - .def("write_minimal_list", &py::WriteMinimalList); + arg("evaluateConditions") = false, + pybind11::call_guard()) + .def("write_minimal_list", + &py::WriteMinimalList, + pybind11::call_guard()); } void bindClasses(pybind11::module& module) { @@ -157,7 +191,7 @@ void bindClasses(pybind11::module& module) { } void bindFunctions(pybind11::module& module) { - module.def("set_logging_callback", &SetLoggingCallback); + module.def("set_logging_callback", &py::SetLoggingCallback); // Need to clear the stored logging callback when exiting, or Python will // hang because the callback pointer is still stored by libloot. @@ -166,12 +200,16 @@ void bindFunctions(pybind11::module& module) { SetLoggingCallback(nullptr); })); - module.def("is_compatible", &IsCompatible); + module.def("is_compatible", + &IsCompatible, + pybind11::call_guard()); - module.def("create_game_handle", &py::CreateGameHandle, + module.def("create_game_handle", + &py::CreateGameHandle, arg("game"), arg("game_path"), - arg("game_local_path") = ""); + arg("game_local_path") = "", + pybind11::call_guard()); } } diff --git a/test/test.py b/test/test.py index 335d171..be8d614 100644 --- a/test/test.py +++ b/test/test.py @@ -20,7 +20,7 @@ from loot import is_compatible from loot import set_logging_callback def logging_callback(level, message): - pass + print(level, message) set_logging_callback(logging_callback)