From fd9076dee6b2312e8a441b863a90150214668983 Mon Sep 17 00:00:00 2001 From: "Markus Kitsinger (SwooshyCueb)" Date: Tue, 20 Aug 2024 15:23:21 -0500 Subject: [PATCH 1/7] [???] Use std:: when calling strcmp --- include/irods/private/re/python.hpp | 17 +++++++++-------- src/main.cpp | 17 +++++++++-------- src/types/irods/objInfo.cpp | 4 +++- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/include/irods/private/re/python.hpp b/include/irods/private/re/python.hpp index fcc84c7..ae998b3 100644 --- a/include/irods/private/re/python.hpp +++ b/include/irods/private/re/python.hpp @@ -1,6 +1,7 @@ #ifndef IRODS_RE_PYTHON_HPP #define IRODS_RE_PYTHON_HPP +#include #include #include @@ -109,28 +110,28 @@ namespace else if (!msParam.type) { THROW(SYS_NOT_SUPPORTED, "msParam type is null"); } - else if (strcmp(msParam.type, GenQueryInp_MS_T) == 0) { + else if (std::strcmp(msParam.type, GenQueryInp_MS_T) == 0) { return boost::python::object{*static_cast(msParam.inOutStruct)}; } - else if (strcmp(msParam.type, GenQueryOut_MS_T) == 0) { + else if (std::strcmp(msParam.type, GenQueryOut_MS_T) == 0) { return boost::python::object{*static_cast(msParam.inOutStruct)}; } - else if (strcmp(msParam.type, KeyValPair_MS_T) == 0) { + else if (std::strcmp(msParam.type, KeyValPair_MS_T) == 0) { return boost::python::object{*static_cast(msParam.inOutStruct)}; } - else if (strcmp(msParam.type, DataObjLseekOut_MS_T) == 0) { + else if (std::strcmp(msParam.type, DataObjLseekOut_MS_T) == 0) { return boost::python::object{*static_cast(msParam.inOutStruct)}; } - else if (strcmp(msParam.type, RodsObjStat_MS_T) == 0) { + else if (std::strcmp(msParam.type, RodsObjStat_MS_T) == 0) { return boost::python::object{*static_cast(msParam.inOutStruct)}; } - else if (strcmp(msParam.type, INT_MS_T) == 0) { + else if (std::strcmp(msParam.type, INT_MS_T) == 0) { return boost::python::object{*static_cast(msParam.inOutStruct)}; } - else if (strcmp(msParam.type, FLOAT_MS_T) == 0) { + else if (std::strcmp(msParam.type, FLOAT_MS_T) == 0) { return boost::python::object{*static_cast(msParam.inOutStruct)}; } - else if (strcmp(msParam.type, BUF_LEN_MS_T) == 0) { + else if (std::strcmp(msParam.type, BUF_LEN_MS_T) == 0) { // clang-format off return msParam.inpOutBuf ? boost::python::object{bytesBuf_t{msParam.inpOutBuf->len, msParam.inpOutBuf->buf}} diff --git a/src/main.cpp b/src/main.cpp index f36f4b7..dc43e47 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -750,20 +751,20 @@ static irods::error exec_rule_text(const irods::default_re_ctx&, if (mp->type == NULL) { rule_vars_python[label] = NULL; } - else if (strcmp(mp->type, DOUBLE_MS_T) == 0) { + else if (std::strcmp(mp->type, DOUBLE_MS_T) == 0) { double* tmpDouble = (double*) mp->inOutStruct; rule_vars_python[label] = tmpDouble; } - else if (strcmp(mp->type, INT_MS_T) == 0) { + else if (std::strcmp(mp->type, INT_MS_T) == 0) { int* tmpInt = (int*) mp->inOutStruct; rule_vars_python[label] = tmpInt; } - else if (strcmp(mp->type, STR_MS_T) == 0) { + else if (std::strcmp(mp->type, STR_MS_T) == 0) { char* tmpChar = (char*) mp->inOutStruct; std::string tmpStr(tmpChar); rule_vars_python[label] = tmpStr; } - else if (strcmp(mp->type, DATETIME_MS_T) == 0) { + else if (std::strcmp(mp->type, DATETIME_MS_T) == 0) { rodsLong_t* tmpRodsLong = (rodsLong_t*) mp->inOutStruct; rule_vars_python[label] = tmpRodsLong; } @@ -899,20 +900,20 @@ static irods::error exec_rule_expression(irods::default_re_ctx&, if (mp->type == NULL) { rule_vars_python[label] = boost::python::object{}; } - else if (strcmp(mp->type, DOUBLE_MS_T) == 0) { + else if (std::strcmp(mp->type, DOUBLE_MS_T) == 0) { double* tmpDouble = (double*) mp->inOutStruct; rule_vars_python[label] = tmpDouble; } - else if (strcmp(mp->type, INT_MS_T) == 0) { + else if (std::strcmp(mp->type, INT_MS_T) == 0) { int* tmpInt = (int*) mp->inOutStruct; rule_vars_python[label] = tmpInt; } - else if (strcmp(mp->type, STR_MS_T) == 0) { + else if (std::strcmp(mp->type, STR_MS_T) == 0) { char* tmpChar = (char*) mp->inOutStruct; std::string tmpStr(tmpChar); rule_vars_python[label] = tmpStr; } - else if (strcmp(mp->type, DATETIME_MS_T) == 0) { + else if (std::strcmp(mp->type, DATETIME_MS_T) == 0) { rodsLong_t* tmpRodsLong = (rodsLong_t*) mp->inOutStruct; rule_vars_python[label] = tmpRodsLong; } diff --git a/src/types/irods/objInfo.cpp b/src/types/irods/objInfo.cpp index 36f279a..b5c3257 100644 --- a/src/types/irods/objInfo.cpp +++ b/src/types/irods/objInfo.cpp @@ -3,6 +3,8 @@ #include "irods/private/re/python/types/irods/objInfo.hpp" +#include + #include #include "irods/private/re/python/types/array_ref.hpp" @@ -92,7 +94,7 @@ namespace irods::re::python::types .add_property("value", +[](keyValPair_t *s) { return array_ref>{s->value, static_cast(s->len)}; }) .def("__getitem__", +[](keyValPair_t* s, const std::string key) { for (int i = 0; i < s->len; ++i) { - if (strcmp(key.c_str(), s->keyWord[i]) == 0) { + if (std::strcmp(key.c_str(), s->keyWord[i]) == 0) { return std::string{s->value[i]}; } } From b0c72e9ca4fed451fe294a0a35050e737df88885 Mon Sep 17 00:00:00 2001 From: "Markus Kitsinger (SwooshyCueb)" Date: Mon, 12 Aug 2024 17:36:07 -0500 Subject: [PATCH 2/7] [???] Replace usage of boost::format with fmt --- include/irods/private/re/python.hpp | 13 +++++++------ src/types/irods/objInfo.cpp | 5 +++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/include/irods/private/re/python.hpp b/include/irods/private/re/python.hpp index ae998b3..a8233a6 100644 --- a/include/irods/private/re/python.hpp +++ b/include/irods/private/re/python.hpp @@ -7,7 +7,6 @@ #include #include -#include #include #include @@ -31,6 +30,8 @@ #include #pragma GCC diagnostic pop +#include + #include "irods/private/re/python/irods_types.hpp" #include "irods/private/re/python/irods_errors.hpp" #include "irods/private/re/python/types/array_ref.hpp" @@ -139,7 +140,7 @@ namespace // clang-format on } else { - THROW(SYS_NOT_SUPPORTED, boost::format("Unknown type in msParam: [%s]") % msParam.type); + THROW(SYS_NOT_SUPPORTED, fmt::format("Unknown type in msParam: [{0}]", msParam.type)); } } @@ -404,8 +405,8 @@ void update_argument(boost::any& cpp_arg, boost::python::object& py_arg) } catch (const std::out_of_range&) { //THROW(SYS_NOT_SUPPORTED, - // boost::format("Attempted to extract from a boost::python::object containing an " - // "unsupported type: %s") % boost::core::scoped_demangled_name{cpp_arg.type().name()}.get()); + // fmt::format("Attempted to extract from a boost::python::object containing an " + // "unsupported type: {0}", boost::core::scoped_demangled_name{cpp_arg.type().name()}.get())); } catch (const boost::bad_any_cast&) { THROW(SYS_NOT_SUPPORTED, "Failed any_cast when updating boost::any from boost:python::object"); @@ -430,8 +431,8 @@ boost::python::object object_from_any(boost::any& arg) } else { THROW(SYS_NOT_SUPPORTED, - boost::format("Attempted to create a boost::python::object from a boost::any containing an " - "unsupported type: %s") % boost::core::scoped_demangled_name{arg.type().name()}.get()); + fmt::format("Attempted to create a boost::python::object from a boost::any containing an " + "unsupported type: {0}", boost::core::scoped_demangled_name{arg.type().name()}.get())); } } catch (const boost::bad_any_cast&) { diff --git a/src/types/irods/objInfo.cpp b/src/types/irods/objInfo.cpp index b5c3257..c656393 100644 --- a/src/types/irods/objInfo.cpp +++ b/src/types/irods/objInfo.cpp @@ -22,7 +22,8 @@ #include #include #pragma GCC diagnostic pop -#include + +#include namespace bp = boost::python; @@ -98,7 +99,7 @@ namespace irods::re::python::types return std::string{s->value[i]}; } } - PyErr_SetString(PyExc_KeyError, (boost::format{"%s was not found in the list of keys."} % key).str().c_str()); + PyErr_SetString(PyExc_KeyError, fmt::format("{0} was not found in the list of keys.", key).c_str()); bp::throw_error_already_set(); return std::string{}; }) From 5fe1ced7973ea7512666b134f690edf7b79b3e4d Mon Sep 17 00:00:00 2001 From: "Markus Kitsinger (SwooshyCueb)" Date: Tue, 20 Aug 2024 16:15:38 -0500 Subject: [PATCH 3/7] [???] Use std:: when referencing size_t --- src/main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index dc43e47..832519a 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -553,7 +553,7 @@ static irods::error list_rules(const irods::default_re_ctx&, std::vector(core_namespace["function_names"]); - size_t len_names = bp::extract(function_names.attr("__len__")()); + std::size_t len_names = bp::extract(function_names.attr("__len__")()); for (std::size_t i = 0; i < len_names; ++i) { rule_vec.push_back(bp::extract(function_names[i])); std::string tmp = bp::extract(function_names[i]); From a648780f3a1f793b4d300bb35badc10676f19427 Mon Sep 17 00:00:00 2001 From: "Markus Kitsinger (SwooshyCueb)" Date: Tue, 20 Aug 2024 17:58:14 -0500 Subject: [PATCH 4/7] [WIP] [211] Initialize module search paths via PyConfig for 3.8+ --- src/main.cpp | 94 ++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 84 insertions(+), 10 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 832519a..f49981f 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -103,7 +103,7 @@ static std::recursive_mutex python_mutex; namespace { - const char* const rule_engine_name = "python"; + static inline constexpr const char* const rule_engine_name = "python"; using log_re = irods::experimental::log::rule_engine; // Python thread states and other data from the interpreter @@ -118,6 +118,13 @@ namespace static thread_local PyThreadState* ts_thread_old; // Reference counter for nested python operations static thread_local uint64_t ts_thread_refct = 0; + +#if PY_VERSION_HEX >= 0x03080000 + // List of default module search paths + static std::vector default_module_search_paths; + // Whether or not default_module_search_paths is populated + static bool default_module_search_paths_set = false; +#endif } //namespace python_state } @@ -378,6 +385,52 @@ namespace bp::class_("CallbackWrapper", bp::no_init) .def("__getattribute__", &CallbackWrapper::getAttribute); } + + static inline void initialize_python(const std::string& _instance_name) + { + auto etc_irods_path = irods::get_irods_config_directory(); + +#if PY_VERSION_HEX >= 0x03080000 + if (python_state::default_module_search_paths_set) { + PyConfig py_config; + PyConfig_InitPythonConfig(&py_config); + + py_config.install_signal_handlers = 0; + + for (auto&& module_search_path : python_state::default_module_search_paths) { + PyWideStringList_Append(&py_config.module_search_paths, module_search_path.c_str()); + } + PyWideStringList_Append(&py_config.module_search_paths, etc_irods_path.generic_wstring().c_str()); + py_config.module_search_paths_set = 1; + + PyStatus py_status = Py_InitializeFromConfig(&py_config); + + if (!PyStatus_Exception(py_status)) { + return; + } + + // clang-format off + log_re::error({ + {"rule_engine_plugin", rule_engine_name}, + {"instance_name", _instance_name}, + {"log_message", "failed to initialize interpreter with PyConfig; falling back to legacy initialization"}, + {"PyStatus.exitcode", fmt::to_string(py_status.exitcode)}, + {"PyStatus.err_msg", py_status.err_msg}, + {"PyStatus.func", py_status.func}, + }); + // clang-format on + } +#endif + + Py_InitializeEx(0); +#if PY_VERSION_HEX < 0x03070000 + PyEval_InitThreads(); +#endif + bp::object mod_sys = bp::import("sys"); + bp::list sys_path = bp::extract(mod_sys.attr("path")); + sys_path.append(bp::str(etc_irods_path.generic_string().c_str())); + } + } // anonymous namespace static irods::error start(irods::default_re_ctx&, const std::string& _instance_name) @@ -391,16 +444,8 @@ static irods::error start(irods::default_re_ctx&, const std::string& _instance_n PyImport_AppendInittab("plugin_wrappers", &PyInit_plugin_wrappers); PyImport_AppendInittab("irods_types", &PyInit_irods_types); PyImport_AppendInittab("irods_errors", &PyInit_irods_errors); - Py_InitializeEx(0); -#if PY_VERSION_HEX < 0x03070000 - PyEval_InitThreads(); -#endif - boost::filesystem::path etc_irods_path = irods::get_irods_config_directory(); - std::string exec_str = "import sys\nsys.path.append('" + etc_irods_path.generic_string() + "')"; - bp::object main_module = bp::import("__main__"); - bp::object main_namespace = main_module.attr("__dict__"); - bp::exec(exec_str.c_str(), main_namespace); + initialize_python(_instance_name); bp::object plugin_wrappers = bp::import("plugin_wrappers"); bp::object irods_types = bp::import("irods_types"); @@ -1022,5 +1067,34 @@ extern "C" irods::pluggable_rule_engine* plugin_factory(c std::function( exec_rule_expression)); +#if PY_VERSION_HEX >= 0x03080000 + Py_InitializeEx(0); + try { + bp::object mod_sys = bp::import("sys"); + bp::list sys_path = bp::extract(mod_sys.attr("path")); + std::size_t sys_path_len = bp::extract(sys_path.attr("__len__")()); + python_state::default_module_search_paths.reserve(sys_path_len); + for (std::size_t i = 0; i < sys_path_len; ++i) { + python_state::default_module_search_paths.push_back(bp::extract(sys_path[i])); + } + python_state::default_module_search_paths_set = true; + } + catch (const bp::error_already_set&) { + const std::string formatted_python_exception = extract_python_exception(); + // clang-format off + log_re::error({ + {"rule_engine_plugin", rule_engine_name}, + {"instance_name", _inst_name}, + {"log_message", "caught python exception in plugin factory; will use legacy module search path initialization"}, + {"python_exception", formatted_python_exception}, + }); + // clang-format on + python_state::default_module_search_paths.clear(); + python_state::default_module_search_paths.shrink_to_fit(); + python_state::default_module_search_paths_set = false; + } + Py_Finalize(); // We're not supposed to do this, but let's try it anyway. +#endif + return re; } From faca3532d940a376d39de6c3421fe59548fe2591 Mon Sep 17 00:00:00 2001 From: "Markus Kitsinger (SwooshyCueb)" Date: Wed, 21 Aug 2024 14:09:38 -0500 Subject: [PATCH 5/7] [225] Use PyGILState_* API for thread handling --- src/main.cpp | 64 ++++++++++++++++++++-------------------------------- 1 file changed, 25 insertions(+), 39 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index f49981f..b4ad7b2 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -106,19 +106,12 @@ namespace static inline constexpr const char* const rule_engine_name = "python"; using log_re = irods::experimental::log::rule_engine; - // Python thread states and other data from the interpreter + // Data we hang onto for the interpreter namespace python_state { // Thread state object for main Python interpreter static PyThreadState* ts_main; - // Thread state object for current thread - static thread_local PyThreadState* ts_thread; - // Pre-initialize thread state object for current thread - static thread_local PyThreadState* ts_thread_old; - // Reference counter for nested python operations - static thread_local uint64_t ts_thread_refct = 0; - #if PY_VERSION_HEX >= 0x03080000 // List of default module search paths static std::vector default_module_search_paths; @@ -253,34 +246,27 @@ namespace return rei; } - // Helper struct for managing python thread state - struct python_thread_state_scope + // Helper class that acquires the GIL while in scope + class python_gil_lock { - python_thread_state_scope() - { - if (0 == python_state::ts_thread_refct) { - python_state::ts_thread = PyThreadState_New(python_state::ts_main->interp); - PyEval_RestoreThread(python_state::ts_thread); - python_state::ts_thread_old = PyThreadState_Swap(python_state::ts_thread); - } - python_state::ts_thread_refct++; - } + public: + python_gil_lock() + : _previous_gil_state(PyGILState_Ensure()) + { } - ~python_thread_state_scope() + ~python_gil_lock() { - if (1 == python_state::ts_thread_refct) { - PyThreadState_Swap(python_state::ts_thread_old); - PyThreadState_Clear(python_state::ts_thread); - PyThreadState_DeleteCurrent(); - } - python_state::ts_thread_refct--; + PyGILState_Release(_previous_gil_state); } - python_thread_state_scope(const python_thread_state_scope&) = delete; + python_gil_lock(const python_gil_lock&) = delete; + + python_gil_lock& operator=(const python_gil_lock&) = delete; - python_thread_state_scope& operator=(const python_thread_state_scope&) = delete; + private: + PyGILState_STATE _previous_gil_state; // GIL state prior to calling PyGILState_Ensure - }; //struct python_thread_state_scope + }; //class python_gil_lock struct RuleCallWrapper { @@ -552,7 +538,7 @@ static irods::error rule_exists(const irods::default_re_ctx&, const std::string& { _return = false; std::lock_guard lock{python_mutex}; - python_thread_state_scope tstate; + python_gil_lock gil_lock; try { // TODO Enable non core.py Python rulebases bp::object core_module = bp::import("core"); @@ -572,7 +558,7 @@ static irods::error rule_exists(const irods::default_re_ctx&, const std::string& return ERROR(RULE_ENGINE_ERROR, err_msg); } // NOTE: If adding more catch blocks, nest this try/catch in another try block - // along with the lock and tstate definitions. They need to stay in-scope for extract_python_exception, + // along with the lock and gil_lock definitions. They need to stay in-scope for extract_python_exception, // but should be out of scope for other exception handlers. return SUCCESS(); @@ -582,8 +568,8 @@ static irods::error list_rules(const irods::default_re_ctx&, std::vector lock{python_mutex}; - python_thread_state_scope tstate; - // tstate (and therefore also lock) needs to stay in scope for extract_python_exception + python_gil_lock gil_lock; + // gil_lock (and therefore also lock) needs to stay in scope for extract_python_exception // hence the nested exception handling try { bp::object core_module = bp::import("core"); @@ -649,8 +635,8 @@ static irods::error exec_rule(const irods::default_re_ctx&, { try { std::lock_guard lock{python_mutex}; - python_thread_state_scope tstate; - // tstate (and therefore also lock) needs to stay in scope for extract_python_exception + python_gil_lock gil_lock; + // gil_lock (and therefore also lock) needs to stay in scope for extract_python_exception // hence the nested exception handling try { // TODO Enable non core.py Python rulebases @@ -776,8 +762,8 @@ static irods::error exec_rule_text(const irods::default_re_ctx&, try { std::lock_guard lock{python_mutex}; - python_thread_state_scope tstate; - // tstate (and therefore also lock) needs to stay in scope for extract_python_exception + python_gil_lock gil_lock; + // gil_lock (and therefore also lock) needs to stay in scope for extract_python_exception // hence the nested exception handling try { execCmdOut_t* myExecCmdOut = (execCmdOut_t*) malloc(sizeof(*myExecCmdOut)); @@ -930,8 +916,8 @@ static irods::error exec_rule_expression(irods::default_re_ctx&, { try { std::lock_guard lock{python_mutex}; - python_thread_state_scope tstate; - // tstate (and therefore also lock) needs to stay in scope for extract_python_exception + python_gil_lock gil_lock; + // gil_lock (and therefore also lock) needs to stay in scope for extract_python_exception // hence the nested exception handling try { bp::dict rule_vars_python; From b66ade03970c417a11cde4f3e863da627633071e Mon Sep 17 00:00:00 2001 From: "Markus Kitsinger (SwooshyCueb)" Date: Wed, 21 Aug 2024 16:28:48 -0500 Subject: [PATCH 6/7] [WIP] [225] Remove python_mutex --- src/main.cpp | 74 ++++++++++++++++++++++------------------------------ 1 file changed, 31 insertions(+), 43 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index b4ad7b2..a8c5bc9 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -99,8 +99,6 @@ const std::string DYNAMIC_PEP_RULE_REGEX = "[^ ]*pep_[^ ]*_(pre|post)"; namespace bp = boost::python; -static std::recursive_mutex python_mutex; - namespace { static inline constexpr const char* const rule_engine_name = "python"; @@ -422,42 +420,37 @@ namespace static irods::error start(irods::default_re_ctx&, const std::string& _instance_name) { python_state::ts_main = nullptr; - { - std::lock_guard lock{python_mutex}; - // lock needs to stay in scope for extract_python_exception - // hence the weird nesting here - try { - PyImport_AppendInittab("plugin_wrappers", &PyInit_plugin_wrappers); - PyImport_AppendInittab("irods_types", &PyInit_irods_types); - PyImport_AppendInittab("irods_errors", &PyInit_irods_errors); + try { + PyImport_AppendInittab("plugin_wrappers", &PyInit_plugin_wrappers); + PyImport_AppendInittab("irods_types", &PyInit_irods_types); + PyImport_AppendInittab("irods_errors", &PyInit_irods_errors); - initialize_python(_instance_name); + initialize_python(_instance_name); - bp::object plugin_wrappers = bp::import("plugin_wrappers"); - bp::object irods_types = bp::import("irods_types"); - bp::object irods_errors = bp::import("irods_errors"); - - StringFromPythonUnicode::register_converter(); - } - catch (const bp::error_already_set&) { - const std::string formatted_python_exception = extract_python_exception(); - // clang-format off - log_re::error({ - {"rule_engine_plugin", rule_engine_name}, - {"instance_name", _instance_name}, - {"log_message", "caught python exception"}, - {"python_exception", formatted_python_exception}, - }); - // clang-format on - std::string err_msg = std::string("irods_rule_engine_plugin_python::") + __PRETTY_FUNCTION__ + - " Caught Python exception.\n" + formatted_python_exception; - return ERROR(RULE_ENGINE_ERROR, err_msg); - } + bp::object plugin_wrappers = bp::import("plugin_wrappers"); + bp::object irods_types = bp::import("irods_types"); + bp::object irods_errors = bp::import("irods_errors"); - python_state::ts_main = PyEval_SaveThread(); - // NO MORE PYTHON IN THIS FUNCTION PAST THIS POINT + StringFromPythonUnicode::register_converter(); + } + catch (const bp::error_already_set&) { + const std::string formatted_python_exception = extract_python_exception(); + // clang-format off + log_re::error({ + {"rule_engine_plugin", rule_engine_name}, + {"instance_name", _instance_name}, + {"log_message", "caught python exception"}, + {"python_exception", formatted_python_exception}, + }); + // clang-format on + std::string err_msg = std::string("irods_rule_engine_plugin_python::") + __PRETTY_FUNCTION__ + + " Caught Python exception.\n" + formatted_python_exception; + return ERROR(RULE_ENGINE_ERROR, err_msg); } + python_state::ts_main = PyEval_SaveThread(); + // NO MORE PYTHON IN THIS FUNCTION PAST THIS POINT + // Initialize microservice table irods::ms_table& ms_table = get_microservice_table(); // writeLine is not in the microservice table in 4.2.0 - #3408 @@ -537,7 +530,6 @@ static irods::error stop(irods::default_re_ctx&, const std::string&) static irods::error rule_exists(const irods::default_re_ctx&, const std::string& rule_name, bool& _return) { _return = false; - std::lock_guard lock{python_mutex}; python_gil_lock gil_lock; try { // TODO Enable non core.py Python rulebases @@ -558,7 +550,7 @@ static irods::error rule_exists(const irods::default_re_ctx&, const std::string& return ERROR(RULE_ENGINE_ERROR, err_msg); } // NOTE: If adding more catch blocks, nest this try/catch in another try block - // along with the lock and gil_lock definitions. They need to stay in-scope for extract_python_exception, + // along with the gil_lock definition. They need to stay in-scope for extract_python_exception, // but should be out of scope for other exception handlers. return SUCCESS(); @@ -567,9 +559,8 @@ static irods::error rule_exists(const irods::default_re_ctx&, const std::string& static irods::error list_rules(const irods::default_re_ctx&, std::vector& rule_vec) { try { - std::lock_guard lock{python_mutex}; python_gil_lock gil_lock; - // gil_lock (and therefore also lock) needs to stay in scope for extract_python_exception + // gil_lock needs to stay in scope for extract_python_exception // hence the nested exception handling try { bp::object core_module = bp::import("core"); @@ -634,9 +625,8 @@ static irods::error exec_rule(const irods::default_re_ctx&, irods::callback effect_handler) { try { - std::lock_guard lock{python_mutex}; python_gil_lock gil_lock; - // gil_lock (and therefore also lock) needs to stay in scope for extract_python_exception + // gil_lock needs to stay in scope for extract_python_exception // hence the nested exception handling try { // TODO Enable non core.py Python rulebases @@ -761,9 +751,8 @@ static irods::error exec_rule_text(const irods::default_re_ctx&, } try { - std::lock_guard lock{python_mutex}; python_gil_lock gil_lock; - // gil_lock (and therefore also lock) needs to stay in scope for extract_python_exception + // gil_lock needs to stay in scope for extract_python_exception // hence the nested exception handling try { execCmdOut_t* myExecCmdOut = (execCmdOut_t*) malloc(sizeof(*myExecCmdOut)); @@ -915,9 +904,8 @@ static irods::error exec_rule_expression(irods::default_re_ctx&, irods::callback effect_handler) { try { - std::lock_guard lock{python_mutex}; python_gil_lock gil_lock; - // gil_lock (and therefore also lock) needs to stay in scope for extract_python_exception + // gil_lock needs to stay in scope for extract_python_exception // hence the nested exception handling try { bp::dict rule_vars_python; From eb6fe2dc44d461549b7bb86a9c7955a4cf6b76cd Mon Sep 17 00:00:00 2001 From: "Markus Kitsinger (SwooshyCueb)" Date: Wed, 21 Aug 2024 16:26:15 -0500 Subject: [PATCH 7/7] [WIP] [225] Implement early/temporary GIL releasing/unlocking --- src/main.cpp | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index a8c5bc9..6527f09 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1,6 +1,7 @@ // include this first to fix macro redef warnings #include +#include #include #include #include @@ -27,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -254,18 +256,100 @@ namespace ~python_gil_lock() { - PyGILState_Release(_previous_gil_state); + if (!_released) { + PyGILState_Release(_previous_gil_state); + } } python_gil_lock(const python_gil_lock&) = delete; python_gil_lock& operator=(const python_gil_lock&) = delete; + inline bool released() + { + return _released; + } + + // restores python state to prior to last PyGILState_Ensure call + // returns active PyThreadState + // doesn't necessarily release the GIL, only this instance's hold on it. + // if GIL was acquired further up the stack, it will still be locked. + inline PyThreadState* release() + { + if (_released) { + THROW(RULE_ENGINE_ERROR, "release called on already-released python_gil_lock"); + } + + PyThreadState* current_thread_state = PyThreadState_Get(); + if (current_thread_state->gilstate_counter <= 1) { + current_thread_state = nullptr; + } + + PyGILState_Release(_previous_gil_state); + + _released = true; + return current_thread_state; + } + + // reacquires GIL after a call to release + inline void reacquire() + { + if (_released) { + THROW(RULE_ENGINE_ERROR, "reacquire called on non-released python_gil_lock"); + } + + _previous_gil_state = PyGILState_Ensure(); + _released = false; + } + private: PyGILState_STATE _previous_gil_state; // GIL state prior to calling PyGILState_Ensure + bool _released = false; // whether or not PyGILState_Release has already been called }; //class python_gil_lock + // Helper class that unlocks GIL while in scope + class python_gil_unlock + { + public: + // Saves the current thread state, releasing the GIL + // Does not otherwise alter thread state in any way + python_gil_unlock() + : _previous_thread_state(PyEval_SaveThread()) + { } + + // Calls release on gil_lock and, if necessary, saves the current thread state + // if should_reacquire is true, reacquire is called on gil_lock during destruction + python_gil_unlock(python_gil_lock* const gil_lock, bool should_reacquire) + : _gil_lock(gil_lock), _should_reacquire(should_reacquire) + { + if (_gil_lock->release() != nullptr) { + _previous_thread_state = PyEval_SaveThread(); + } + } + + ~python_gil_unlock() + { + if (_previous_thread_state != nullptr) { + PyEval_RestoreThread(_previous_thread_state); + } + if (_should_reacquire) { + assert(_gil_lock != nullptr); + _gil_lock->reacquire(); + } + } + + python_gil_unlock(const python_gil_unlock&) = delete; + + python_gil_unlock& operator=(const python_gil_unlock&) = delete; + + private: + PyThreadState* _previous_thread_state = nullptr; + python_gil_lock* const _gil_lock = nullptr; + const bool _should_reacquire = false; + + }; //class python_gil_unlock + struct RuleCallWrapper { RuleCallWrapper(irods::callback& effect_handler, std::string rule_name) @@ -538,6 +622,7 @@ static irods::error rule_exists(const irods::default_re_ctx&, const std::string& } catch (const bp::error_already_set&) { const std::string formatted_python_exception = extract_python_exception(); + python_gil_unlock gil_release(&gil_lock, false); // clang-format off log_re::error({ {"rule_engine_plugin", rule_engine_name}, @@ -583,6 +668,7 @@ static irods::error list_rules(const irods::default_re_ctx&, std::vector