Merge pull request #60 from ebertin/fix/python_obj_race

Fix race condition on the destruction of boost::python::object instances

Author: marcschefer

SEImplementation/SEImplementation/Plugin/FlexibleModelFitting/FlexibleModelFittingParameter.h

@@ -13,8 +13,7 @@
 #include <functional>
 #include <vector>
 #include <mutex>
-
-#include <boost/python/object.hpp>
+#include "SEFramework/CoordinateSystem/CoordinateSystem.h"
 
 namespace ModelFitting {
   class BasicParameter;
@@ -93,8 +92,12 @@ class FlexibleModelFittingFreeParameter : public FlexibleModelFittingParameter {
 class FlexibleModelFittingDependentParameter : public FlexibleModelFittingParameter {
 
 public:
+
+  /// The signature of a function that evaluates the dependent parameter. It gets
+  /// as parameters the values of those parameters on which this one depends.
+  using ValueFunc = std::function<double(const std::shared_ptr<CoordinateSystem>&, const std::vector<double>&)>;
 
-  FlexibleModelFittingDependentParameter(int id, boost::python::object value_calculator,
+  FlexibleModelFittingDependentParameter(int id, ValueFunc value_calculator,
                                          std::vector<std::shared_ptr<FlexibleModelFittingParameter>> parameters)
           : FlexibleModelFittingParameter(id),
             m_value_calculator(value_calculator),
@@ -106,8 +109,8 @@ class FlexibleModelFittingDependentParameter : public FlexibleModelFittingParame
                                   const SourceInterface& source) const override;
 
 private:
-  
-  boost::python::object m_value_calculator;
+
+  ValueFunc m_value_calculator;
   std::vector<std::shared_ptr<FlexibleModelFittingParameter>> m_parameters;
 
 };

SEImplementation/src/lib/Configuration/ModelFittingConfig.cpp

@@ -36,7 +36,7 @@ namespace SExtractor {
  *  Elements::Exception if either the call or the extract throw a Python exception
  */
 template <typename R, typename ...T>
-R py_call_wrapper(py::object func, T... args) {
+R py_call_wrapper(const py::object& func, T... args) {
   try {
     return py::extract<R>(func(args...));
   }
@@ -45,13 +45,47 @@ R py_call_wrapper(py::object func, T... args) {
   }
 }
 
+/**
+ * @brief Hold a reference to a Python object
+ * @details
+ *  A boost::python::object contains a pointer to the underlying Python struct, which is
+ *  copied as-is (shared) when copied. When the boost::python::object is destroyed, it checks,
+ *  and then decrements, the reference count. This destruction is *not* thread safe, as the pointer
+ *  is not protected by a mutex or anything.
+ *  This class holds a single reference to the Python object, and relies on the mechanism of
+ *  std::shared_ptr to destroy the object once there is no one using it. std::shared_ptr *is*
+ *  thread safe, unlike boost::python::object.
+ */
+class PyObjectHolder {
+  public:
+    PyObjectHolder(py::object&& obj): m_obj_ptr(std::make_shared<py::object>(obj)) {}
+
+    PyObjectHolder(const PyObjectHolder&) = default;
+    PyObjectHolder(PyObjectHolder&&) = default;
+
+    operator const py::object&() const {
+      return *m_obj_ptr;
+    }
+
+    const py::object& operator *() const {
+      return *m_obj_ptr;
+    }
+
+    py::object attr(const char *name) {
+      return m_obj_ptr->attr(name);
+    }
+
+  private:
+    std::shared_ptr<py::object> m_obj_ptr;
+};
+
 ModelFittingConfig::ModelFittingConfig(long manager_id) : Configuration(manager_id) {
   declareDependency<PythonConfig>();
 }
 
 void ModelFittingConfig::initialize(const UserValues&) {
   for (auto& p : getDependency<PythonConfig>().getInterpreter().getConstantParameters()) {
-    py::object py_value_func = p.second.attr("get_value")();
+    auto py_value_func = PyObjectHolder(p.second.attr("get_value")());
     auto value_func = [py_value_func] (const SourceInterface& o) -> double {
       ObjectInfo oi {o};
       return py_call_wrapper<double>(py_value_func, oi);
@@ -61,25 +95,25 @@ void ModelFittingConfig::initialize(const UserValues&) {
   }
 
   for (auto& p : getDependency<PythonConfig>().getInterpreter().getFreeParameters()) {
-    py::object py_init_value_func = p.second.attr("get_init_value")();
+    auto py_init_value_func = PyObjectHolder(p.second.attr("get_init_value")());
     auto init_value_func = [py_init_value_func] (const SourceInterface& o) -> double {
       ObjectInfo oi {o};
       return py_call_wrapper<double>(py_init_value_func, oi);
     };
 
-    py::object py_range_obj = p.second.attr("get_range")();
+    auto py_range_obj = PyObjectHolder(p.second.attr("get_range")());
 
     std::shared_ptr<FlexibleModelFittingConverterFactory> converter;
     std::string type_string(py::extract<char const*>(py_range_obj.attr("__class__").attr("__name__")));
     if (type_string == "Unbounded") {
-      py::object py_factor_func = py_range_obj.attr("get_normalization_factor")();
+      auto py_factor_func = PyObjectHolder(py_range_obj.attr("get_normalization_factor")());
       auto factor_func = [py_factor_func] (double init, const SourceInterface& o) -> double {
         ObjectInfo oi {o};
         return py_call_wrapper<double>(py_factor_func, init, oi);
       };
       converter = std::make_shared<FlexibleModelFittingUnboundedConverterFactory>(factor_func);
     } else if (type_string == "Range") {
-      py::object py_range_func = py_range_obj.attr("get_limits")();
+      auto py_range_func = PyObjectHolder(py_range_obj.attr("get_limits")());
       auto range_func = [py_range_func] (double init, const SourceInterface& o) -> std::pair<double, double> {
         ObjectInfo oi {o};
         py::tuple range = py_call_wrapper<py::tuple>(py_range_func, init, oi);
@@ -102,15 +136,26 @@ void ModelFittingConfig::initialize(const UserValues&) {
   }
 
   for (auto& p : getDependency<PythonConfig>().getInterpreter().getDependentParameters()) {
-    py::object py_func = p.second.attr("func");
+    auto py_func = PyObjectHolder(p.second.attr("func"));
     std::vector<std::shared_ptr<FlexibleModelFittingParameter>> params {};
     py::list param_ids = py::extract<py::list>(p.second.attr("params"));
     for (int i = 0; i < py::len(param_ids); ++i) {
       int id = py::extract<int>(param_ids[i]);
       params.push_back(m_parameters[id]);
     }
+
+    auto dependent_func = [py_func](const std::shared_ptr<CoordinateSystem> &cs, const std::vector<double> &params) -> double {
+      try {
+        PythonInterpreter::getSingleton().setCoordinateSystem(cs);
+        return py::extract<double>((*py_func)(*py::tuple(params)));
+      }
+      catch (const py::error_already_set&) {
+        throw pyToElementsException(logger);
+      }
+    };
+
     m_parameters[p.first] = std::make_shared<FlexibleModelFittingDependentParameter>(
-                                                      p.first, py_func, params);
+                                                      p.first, dependent_func, params);
   }
 
   for (auto& p : getDependency<PythonConfig>().getInterpreter().getPointSourceModels()) {
@@ -171,12 +216,12 @@ void ModelFittingConfig::initialize(const UserValues&) {
     auto& prior = p.second;
     int param_id = py::extract<int>(prior.attr("param"));
     auto param = m_parameters[param_id];
-    py::object py_value_func = prior.attr("value");
+    auto py_value_func = PyObjectHolder(prior.attr("value"));
     auto value_func = [py_value_func] (const SourceInterface& o) -> double {
       ObjectInfo oi {o};
       return py_call_wrapper<double>(py_value_func, oi);
     };
-    py::object py_sigma_func = prior.attr("sigma");
+    auto py_sigma_func = PyObjectHolder(prior.attr("sigma"));
     auto sigma_func = [py_sigma_func] (const SourceInterface& o) -> double {
       ObjectInfo oi {o};
       return py_call_wrapper<double>(py_sigma_func, oi);

SEImplementation/src/lib/Plugin/FlexibleModelFitting/FlexibleModelFittingParameter.cpp

@@ -6,7 +6,6 @@
  */
 
 #include <iostream>
-#include <boost/python.hpp>
 
 #include "ModelFitting/utils.h"
 
@@ -77,19 +76,14 @@ template<typename ... Parameters>
 std::shared_ptr<ModelFitting::BasicParameter> createDependentParameterHelper(
                                        FlexibleModelFittingParameterManager& parameter_manager,
                                        const SourceInterface& source,
-                                       boost::python::object value_calculator,
+                                       FlexibleModelFittingDependentParameter::ValueFunc value_calculator,
                                        std::shared_ptr<Parameters>... parameters) {
   auto coordinate_system = source.getProperty<DetectionFrame>().getFrame()->getCoordinateSystem();
 
   auto calc = [value_calculator, coordinate_system] (decltype(doubleResolver(std::declval<Parameters>()))... params) -> double {
     std::lock_guard<std::mutex> guard {python_callback_mutex};
-    try {
-      PythonInterpreter::getSingleton().setCoordinateSystem(coordinate_system);
-      return boost::python::extract<double>(value_calculator(params...));
-    }
-    catch (const boost::python::error_already_set &e) {
-      throw pyToElementsException(logger);
-    }
+    std::vector<double> materialized{params...};
+    return value_calculator(coordinate_system, materialized);
   };
   return createDependentParameterPtr(calc, *(parameter_manager.getParameter(source, parameters))...);
 }

SEImplementation/src/lib/Plugin/FlexibleModelFitting/FlexibleModelFittingPrior.cpp

@@ -6,7 +6,6 @@
  */
 
 #include <iostream>
-#include <boost/python.hpp>
 #include <mutex>
 
 #include "ModelFitting/utils.h"

SEImplementation/src/lib/PythonConfig/PythonModule.cpp

@@ -4,6 +4,7 @@
  */
 
 #include <boost/python.hpp>
+#include <boost/python/suite/indexing/vector_indexing_suite.hpp>
 
 #include <SEImplementation/PythonConfig/ObjectInfo.h>
 #include <SEImplementation/PythonConfig/PyMeasurementImage.h>
@@ -61,6 +62,10 @@ BOOST_PYTHON_MODULE(_SExtractorPy) {
       .def(bp::init<double, double>())
       .def_readwrite("x", &ImageCoordinate::m_x)
       .def_readwrite("y", &ImageCoordinate::m_y);
+
+  bp::class_<std::vector<double> >("_DoubleVector")
+    .def(bp::vector_indexing_suite<std::vector<double> >())
+  ;
 }
 
 } // namespace SExtractor