From 045ed0e023ae2d1a567b3426ef2b48980a851b7e Mon Sep 17 00:00:00 2001 From: John Spray Date: Mon, 16 Oct 2017 10:51:34 -0400 Subject: [PATCH] mgr: update for SafeThreadState A bunch of the previous commits were done before this class existed, so updating in one go instead of trying to edit history in fine detail. Signed-off-by: John Spray (cherry picked from commit 29193a47e6cf8297d9b1ceecc7695f2c85434999) --- src/mgr/ActivePyModule.cc | 8 ++++---- src/mgr/ActivePyModule.h | 2 +- src/mgr/ActivePyModules.cc | 2 +- src/mgr/ActivePyModules.h | 2 +- src/mgr/PyModuleRegistry.cc | 19 +++++++++++++++---- src/mgr/PyModuleRegistry.h | 6 ++++-- src/mgr/PyModuleRunner.cc | 2 +- src/mgr/PyModuleRunner.h | 4 ++-- src/mgr/StandbyPyModules.cc | 4 ++-- src/mgr/StandbyPyModules.h | 5 +++-- 10 files changed, 34 insertions(+), 20 deletions(-) diff --git a/src/mgr/ActivePyModule.cc b/src/mgr/ActivePyModule.cc index 46d6b9f8aef85..90040af930d5a 100644 --- a/src/mgr/ActivePyModule.cc +++ b/src/mgr/ActivePyModule.cc @@ -54,7 +54,7 @@ std::string handle_pyerror() int ActivePyModule::load(ActivePyModules *py_modules) { assert(py_modules); - Gil gil(pMyThreadState); + Gil gil(pMyThreadState, true); // We tell the module how we name it, so that it can be consistent // with us in logging etc. @@ -82,7 +82,7 @@ void ActivePyModule::notify(const std::string ¬ify_type, const std::string &n { assert(pClassInstance != nullptr); - Gil gil(pMyThreadState); + Gil gil(pMyThreadState, true); // Execute auto pValue = PyObject_CallMethod(pClassInstance, @@ -105,7 +105,7 @@ void ActivePyModule::notify_clog(const LogEntry &log_entry) { assert(pClassInstance != nullptr); - Gil gil(pMyThreadState); + Gil gil(pMyThreadState, true); // Construct python-ized LogEntry PyFormatter f; @@ -187,7 +187,7 @@ int ActivePyModule::handle_command( assert(ss != nullptr); assert(ds != nullptr); - Gil gil(pMyThreadState); + Gil gil(pMyThreadState, true); PyFormatter f; cmdmap_dump(cmdmap, &f); diff --git a/src/mgr/ActivePyModule.h b/src/mgr/ActivePyModule.h index 2a3e0c8bfe092..0c2ee12f34e10 100644 --- a/src/mgr/ActivePyModule.h +++ b/src/mgr/ActivePyModule.h @@ -60,7 +60,7 @@ private: public: ActivePyModule(const std::string &module_name_, PyObject *pClass_, - PyThreadState *my_ts_) + const SafeThreadState &my_ts_) : PyModuleRunner(module_name_, pClass_, my_ts_) {} diff --git a/src/mgr/ActivePyModules.cc b/src/mgr/ActivePyModules.cc index 412d7824c29a0..6025e62a4f04f 100644 --- a/src/mgr/ActivePyModules.cc +++ b/src/mgr/ActivePyModules.cc @@ -322,7 +322,7 @@ PyObject *ActivePyModules::get_python(const std::string &what) } int ActivePyModules::start_one(std::string const &module_name, - PyObject *pClass, PyThreadState *pMyThreadState) + PyObject *pClass, const SafeThreadState &pMyThreadState) { Mutex::Locker l(lock); diff --git a/src/mgr/ActivePyModules.h b/src/mgr/ActivePyModules.h index 688469f8c135e..21e6529a93343 100644 --- a/src/mgr/ActivePyModules.h +++ b/src/mgr/ActivePyModules.h @@ -110,7 +110,7 @@ public: int start_one(std::string const &module_name, PyObject *pClass, - PyThreadState *pMyThreadState); + const SafeThreadState &pMyThreadState); void dump_server(const std::string &hostname, const DaemonStateCollection &dmc, diff --git a/src/mgr/PyModuleRegistry.cc b/src/mgr/PyModuleRegistry.cc index 3f4e60b86d223..bab057c6c2846 100644 --- a/src/mgr/PyModuleRegistry.cc +++ b/src/mgr/PyModuleRegistry.cc @@ -155,6 +155,7 @@ int PyModuleRegistry::init(const MgrMap &map) // Drop the GIL and remember the main thread state (current // thread state becomes NULL) pMainThreadState = PyEval_SaveThread(); + assert(pMainThreadState != nullptr); std::list failed_modules; @@ -191,14 +192,15 @@ int PyModule::load(PyThreadState *pMainThreadState) // Configure sub-interpreter and construct C++-generated python classes { - Gil gil(pMainThreadState); - - pMyThreadState = Py_NewInterpreter(); + SafeThreadState sts(pMainThreadState); + Gil gil(sts); - if (pMyThreadState == nullptr) { + auto thread_state = Py_NewInterpreter(); + if (thread_state == nullptr) { derr << "Failed to create python sub-interpreter for '" << module_name << '"' << dendl; return -EINVAL; } else { + pMyThreadState.set(thread_state); // Some python modules do not cope with an unpopulated argv, so lets // fake one. This step also picks up site-packages into sys.path. const char *argv[] = {"ceph-mgr"}; @@ -289,6 +291,15 @@ int PyModule::load(PyThreadState *pMainThreadState) return 0; } +PyModule::~PyModule() +{ + if (pMyThreadState.ts != nullptr) { + Gil gil(pMyThreadState, true); + Py_XDECREF(pClass); + Py_XDECREF(pStandbyClass); + } +} + void PyModuleRegistry::standby_start(MonClient *monc) { Mutex::Locker l(lock); diff --git a/src/mgr/PyModuleRegistry.h b/src/mgr/PyModuleRegistry.h index 8249fc9e0d200..5564e7f1fa5de 100644 --- a/src/mgr/PyModuleRegistry.h +++ b/src/mgr/PyModuleRegistry.h @@ -33,7 +33,7 @@ private: std::string get_site_packages(); public: - PyThreadState *pMyThreadState = nullptr; + SafeThreadState pMyThreadState; PyObject *pClass = nullptr; PyObject *pStandbyClass = nullptr; @@ -42,6 +42,8 @@ public: { } + ~PyModule(); + int load(PyThreadState *pMainThreadState); std::string get_name() const { @@ -68,7 +70,7 @@ private: std::unique_ptr active_modules; std::unique_ptr standby_modules; - PyThreadState *pMainThreadState = nullptr; + PyThreadState *pMainThreadState; // We have our own copy of MgrMap, because we are constructed // before ClusterState exists. diff --git a/src/mgr/PyModuleRunner.cc b/src/mgr/PyModuleRunner.cc index 59d86a8bede0a..5e04e3da27c71 100644 --- a/src/mgr/PyModuleRunner.cc +++ b/src/mgr/PyModuleRunner.cc @@ -66,7 +66,7 @@ void PyModuleRunner::shutdown() { assert(pClassInstance != nullptr); - Gil gil(pMyThreadState); + Gil gil(pMyThreadState, true); auto pValue = PyObject_CallMethod(pClassInstance, const_cast("shutdown"), nullptr); diff --git a/src/mgr/PyModuleRunner.h b/src/mgr/PyModuleRunner.h index 1cb0506054ec7..cb51df3eaaff1 100644 --- a/src/mgr/PyModuleRunner.h +++ b/src/mgr/PyModuleRunner.h @@ -55,14 +55,14 @@ public: PyModuleRunner( const std::string &module_name_, PyObject *pClass_, - PyThreadState *pMyThreadState_) + const SafeThreadState &pMyThreadState_) : module_name(module_name_), pClass(pClass_), pMyThreadState(pMyThreadState_), thread(this) { assert(pClass != nullptr); - assert(pMyThreadState != nullptr); + assert(pMyThreadState.ts != nullptr); assert(!module_name.empty()); } diff --git a/src/mgr/StandbyPyModules.cc b/src/mgr/StandbyPyModules.cc index 2b442d5ef9d29..e567269a3c75c 100644 --- a/src/mgr/StandbyPyModules.cc +++ b/src/mgr/StandbyPyModules.cc @@ -77,7 +77,7 @@ void StandbyPyModules::shutdown() } int StandbyPyModules::start_one(std::string const &module_name, - PyObject *pClass, PyThreadState *pMyThreadState) + PyObject *pClass, const SafeThreadState &pMyThreadState) { Mutex::Locker l(lock); @@ -108,7 +108,7 @@ int StandbyPyModules::start_one(std::string const &module_name, int StandbyPyModule::load() { - Gil gil(pMyThreadState); + Gil gil(pMyThreadState, true); // We tell the module how we name it, so that it can be consistent // with us in logging etc. diff --git a/src/mgr/StandbyPyModules.h b/src/mgr/StandbyPyModules.h index e5896e05ce291..4f011464a1372 100644 --- a/src/mgr/StandbyPyModules.h +++ b/src/mgr/StandbyPyModules.h @@ -21,6 +21,7 @@ #include "common/Thread.h" #include "common/Mutex.h" +#include "mgr/Gil.h" #include "mon/MonClient.h" #include "mon/MgrMap.h" #include "mgr/PyModuleRunner.h" @@ -90,7 +91,7 @@ class StandbyPyModule : public PyModuleRunner StandbyPyModuleState &state_, const std::string &module_name_, PyObject *pClass_, - PyThreadState *pMyThreadState_) + const SafeThreadState &pMyThreadState_) : PyModuleRunner(module_name_, pClass_, pMyThreadState_), state(state_) @@ -136,7 +137,7 @@ public: int start_one(std::string const &module_name, PyObject *pClass, - PyThreadState *pMyThreadState); + const SafeThreadState &pMyThreadState); void shutdown(); -- 2.39.5