Fix python segfaults

include/routing/Routing_Py_Adapter.hpp

@@ -107,6 +107,14 @@ namespace routing_py_adapter {
 
     private:
 
+
+        /** Handle to the interperter util.
+         * 
+         * Order is important, must be constructed before anything depending on it
+         * and destructed after all dependent members.
+        */
+        std::shared_ptr<utils::ngenPy::InterpreterUtil> interperter;
+
         /** A binding to the Python numpy package/module. */
         py::object np;
 

include/utilities/python/InterpreterUtil.hpp

@@ -30,11 +30,55 @@ namespace utils {
         class InterpreterUtil {
 
         public:
-            static InterpreterUtil &getInstance() {
-                static InterpreterUtil instance;
+            static std::shared_ptr<InterpreterUtil> getInstance() {
+                /**
+                 * @brief Singleton instance of embdedded python interpreter.
+                 * 
+                 * Note that if no client holds a currernt reference to this singleton, it will get destroyed
+                 * and the next call to getInstance will create and initialize a new scoped interpreter
+                 * 
+                 * This is required for the simple reason that a traditional static singleton instance has no guarantee
+                 * about the destrutction order of resources across multiple compliation units,
+                 * and this will cause seg faults if the python interpreter is torn down before the destruction of bound modules
+                 * (such as this class' Path module).  If the interpreter is not available when those destructors are called,
+                 * a seg fault will occur.  With this implementaiton, the interpreter is guarnateed to exist as long as anything 
+                 * referencing it needs it, and then can cleanly clean up internal references before the `py::scoped_interpreter`
+                 * guard is destroyed, removing the python interpreter.
+                 * 
+                 * See the following issues and links for reference:
+                 * 
+                 * https://cplusplus.com/forum/general/37113/
+                 * https://stackoverflow.com/questions/60100922/keeping-python-interpreter-alive-only-during-the-life-of-an-object-instance
+                 * https://github.com/pybind/pybind11/issues/1598
+                 * 
+                 */
+                //Functionally, a global instance variable
+                //This can be made a class attribute, but would need to find a place to put a single definition
+                //e.g. make a .cpp file
+                static std::weak_ptr<InterpreterUtil> _instance;
+                std::shared_ptr<InterpreterUtil> instance = _instance.lock();
+                if(!instance){
+                    //instance is null
+                    InterpreterUtil* pt = new InterpreterUtil();
+                    instance = std::shared_ptr<InterpreterUtil>(pt, Deleter{});
+                    //update the weak ref
+                    _instance = instance;
+                }
                 return instance;
             }
 
+            struct Deleter {
+                /**
+                 * @brief Custom deleter functor to provide access to private destructor of singleton
+                 * 
+                 * In theory, could probably safely move the destructor to public scope as well...
+                 * 
+                 * @param ptr 
+                 */
+                void operator()(InterpreterUtil* ptr){delete ptr;}
+            };
+            friend Deleter;
+
         private:
             InterpreterUtil() {
                 guardPtr = std::make_shared<py::scoped_interpreter>();
@@ -65,7 +109,7 @@ namespace utils {
              * @param directoryPath The desired directory to add to the Python system path.
              */
             static void addToPyPath(const std::string &directoryPath) {
-                getInstance().addToPath(directoryPath);
+                getInstance()->addToPath(directoryPath);
             }
 
             /**
@@ -101,7 +145,7 @@ namespace utils {
              * @return Handle to the desired top level Python module.
              */
             static py::object getPyModule(const std::string &name) {
-                return getInstance().getModule(name);
+                return getInstance()->getModule(name);
             }
 
             /**
@@ -119,7 +163,7 @@ namespace utils {
              * @return Handle to the desired Python module or type.
              */
             static py::object getPyModule(const std::vector<std::string> &moduleLevelNames) {
-                return getInstance().getModule(moduleLevelNames);
+                return getInstance()->getModule(moduleLevelNames);
             }
 
             /**
@@ -252,6 +296,8 @@ namespace utils {
             }
 
         private:
+            //List this FIRST, to ensure it isn't destroyed before anything else (e.g. Path) that might
+            //need a valid interpreter in its destructor calls.
             // This is required for the Python interpreter and must be kept alive
             std::shared_ptr<py::scoped_interpreter> guardPtr;
 

src/NGen.cpp

@@ -71,7 +71,11 @@ int main(int argc, char *argv[]) {
 
     #ifdef ACTIVATE_PYTHON
     // Start Python interpreter via the manager singleton
-    utils::ngenPy::InterpreterUtil::getInstance();
+    // Need to bind to a variable so that the underlying reference count
+    // is incremented, this essentially becomes the global reference to keep
+    // the interpreter alive till the end of `main`
+    auto _interp = utils::ngenPy::InterpreterUtil::getInstance();
+    //utils::ngenPy::InterpreterUtil::getInstance();
     #endif // ACTIVATE_PYTHON
 
     //Pull a few "options" form the cli input, this is a temporary solution to CLI parsing!

src/routing/Routing_Py_Adapter.cpp

@@ -9,6 +9,8 @@ using namespace routing_py_adapter;
 
 Routing_Py_Adapter::Routing_Py_Adapter(std::string t_route_config_file_with_path):
   t_route_config_path(t_route_config_file_with_path){
+  //hold a reference to the interpreter, ensures an interperter exists as long as the reference is held
+  interperter = utils::ngenPy::InterpreterUtil::getInstance();
   //Import ngen_main.  Will throw error if module isn't available
   //in the embedded interperters PYTHON_PATH
   this->t_route_module = utils::ngenPy::InterpreterUtil::getPyModule("ngen_routing.ngen_main");

test/realizations/catchments/Bmi_Multi_Formulation_Test.cpp

@@ -26,6 +26,8 @@ using namespace utils::ngenPy;
 using namespace realization;
 
 class Bmi_Multi_Formulation_Test : public ::testing::Test {
+private:
+    static std::shared_ptr<InterpreterUtil> interperter;
 protected:
 
     static std::string find_file(std::vector<std::string> dir_opts, const std::string& basename) {
@@ -445,6 +447,8 @@ class Bmi_Multi_Formulation_Test : public ::testing::Test {
 
 
 };
+//Make sure the interperter is instansiated and lives throught the test class
+std::shared_ptr<InterpreterUtil> Bmi_Multi_Formulation_Test::interperter = InterpreterUtil::getInstance();
 
 void Bmi_Multi_Formulation_Test::SetUpTestSuite() {
     #ifdef ACTIVATE_PYTHON

test/realizations/catchments/Bmi_Py_Adapter_Test.cpp

@@ -26,7 +26,8 @@ typedef struct example_scenario {
 } example_scenario;
 
 class Bmi_Py_Adapter_Test : public ::testing::Test {
-
+private:
+    static std::shared_ptr<InterpreterUtil> interperter;
 protected:
 
     static std::shared_ptr<py::object> friend_get_raw_model(Bmi_Py_Adapter *adapter) {
@@ -120,7 +121,8 @@ class Bmi_Py_Adapter_Test : public ::testing::Test {
     int expected_var_nbytes = 8; //type double
 
 };
-
+//Make sure the interperter is instansiated and lives throught the test class
+std::shared_ptr<InterpreterUtil> Bmi_Py_Adapter_Test::interperter = InterpreterUtil::getInstance();
 py::object Bmi_Py_Adapter_Test::Path = InterpreterUtil::getPyModule(std::vector<std::string> {"pathlib", "Path"});
 
 void Bmi_Py_Adapter_Test::SetUp() {

test/realizations/catchments/Bmi_Py_Formulation_Test.cpp

@@ -34,7 +34,8 @@ typedef struct py_formulation_example_scenario {
 } py_formulation_example_scenario;
 
 class Bmi_Py_Formulation_Test : public ::testing::Test {
-
+private:
+    static std::shared_ptr<InterpreterUtil> interperter;
 protected:
 
     py::object Path;
@@ -175,6 +176,8 @@ class Bmi_Py_Formulation_Test : public ::testing::Test {
                                    const std::vector<std::string> &file_basenames);
 
 };
+//Make sure the interperter is instansiated and lives throught the test class
+std::shared_ptr<InterpreterUtil> Bmi_Py_Formulation_Test::interperter = InterpreterUtil::getInstance();
 
 void Bmi_Py_Formulation_Test::SetUp() {
     Path = InterpreterUtil::getPyModule(std::vector<std::string> {"pathlib", "Path"});

test/routing/Routing_Py_Bind_Test.cpp

@@ -10,6 +10,8 @@ namespace py = pybind11;
 
 
 class RoutingPyBindTest : public ::testing::Test {
+private:
+  static std::shared_ptr<utils::ngenPy::InterpreterUtil> interperter;
 
 protected:
 
@@ -21,6 +23,8 @@ class RoutingPyBindTest : public ::testing::Test {
 
     }
 };
+//Make sure the interperter is instansiated and lives throught the test class
+std::shared_ptr<utils::ngenPy::InterpreterUtil> RoutingPyBindTest::interperter = utils::ngenPy::InterpreterUtil::getInstance();
 
 TEST_F(RoutingPyBindTest, TestRoutingPyBind)
 {