qipython: Fixes an object lifetime issue causing a memory leak. #46735

When an `AnyObject` was exposed to Python as a `py::object`, we
previously created wrappers for its properties and signals, that held
the `AnyObject`. This caused a dependency cycle: the object maintained
the properties/signals alive and the properties/signals maintained the
object alive. This led to a memory leak.

This patch changes the properties/signals wrapper types, so that they
hold a weak reference to the object instead. The resulting dependency
graph is that the `AnyObject` Python object holds its properties/signals
Python objects and not the reverse, thus breaking the cycle.

However, this change in turn caused a regression in `ApplicationSession`.
The `ApplicationSession::session()` member function was exposed as a
Python read-only property of the Python `Application` type. The property
getter C++ implementation transformed the `Session` object into a
temporary `AnyObject`, which was then converted into a Python object.
Every access to the property therefore returned a new temporary Python
object. These objects previously leaked (because of the behavior
described above) and were therefore kept accessible. Because of the leak
fix, they are not held anymore and are immediately destroyed unless
stored into a variable.

Consequently this patch also reworks the `ApplicationSession` type
exposed in Python so that it now holds the Python object that holds the
`AnyObject` which itself holds the `Session`. This ensures that this
`Session` Python object is persistent as long as the `Application`
instance is alive.

Change-Id: Id1d39be6ced9208a23ae53707d1ef4d466029c68
Reviewed-on: http://gerrit2.aldebaran.lan/1784
Reviewed-by: jmonnon <jmonnon@aldebaran.com>
Reviewed-by: philippe.martin <philippe.martin@softbankrobotics.com>
Tested-by: vincent.palancher <vincent.palancher@softbankrobotics.com>

Author: Vincent Palancher

qipython/pyproperty.hpp

@@ -24,10 +24,8 @@ namespace detail
 {
   struct ProxyProperty
   {
-    AnyObject object;
+    AnyWeakObject object;
     unsigned int propertyId;
-
-    ~ProxyProperty();
   };
 }
 

qipython/pysignal.hpp

@@ -25,12 +25,41 @@ using SignalPtr = std::shared_ptr<Signal>;
 namespace detail
 {
 
+/// The destructor of `AnyObject` can block waiting for callbacks to end.
+/// This type ensures that it's always called while the GIL is unlocked.
+struct AnyObjectGILSafe
+{
+  AnyObjectGILSafe() = default;
+
+  /// @throws `std::runtime_error` if the weak object is expired.
+  /// @post With s = AnyObjectGILSafe(weak), s->isValid()
+  explicit AnyObjectGILSafe(AnyWeakObject weak);
+
+  // Copy/Move operations are safe to default because they will never cause the
+  // destruction of the `AnyObject` contained in either the source or the
+  // destination (this) object.
+  // The declaration are still kept to enforce the 3/5/0 rule.
+  AnyObjectGILSafe(const AnyObjectGILSafe& o) = default;
+  AnyObjectGILSafe& operator=(const AnyObjectGILSafe& o) = default;
+  AnyObjectGILSafe(AnyObjectGILSafe&& o) = default;
+  AnyObjectGILSafe& operator=(AnyObjectGILSafe&& o) = default;
+
+  ~AnyObjectGILSafe();
+
+  inline AnyObject& operator*() { return _obj; }
+  inline const AnyObject& operator*() const { return _obj; }
+
+  inline AnyObject* operator->() { return &_obj; }
+  inline const AnyObject* operator->() const { return &_obj; }
+
+private:
+  AnyObject _obj;
+};
+
 struct ProxySignal
 {
-  AnyObject object;
+  AnyWeakObject object;
   unsigned int signalId;
-
-  ~ProxySignal();
 };
 
 pybind11::object signalConnect(SignalBase& sig,
@@ -41,12 +70,12 @@ pybind11::object signalDisconnect(SignalBase& sig, SignalLink id, bool async);
 
 pybind11::object signalDisconnectAll(SignalBase& sig, bool async);
 
-pybind11::object proxySignalConnect(const AnyObject& obj,
+pybind11::object proxySignalConnect(const AnyObjectGILSafe& obj,
                                     unsigned int signalId,
                                     const pybind11::function& callback,
                                     bool async);
 
-pybind11::object proxySignalDisconnect(const AnyObject& obj,
+pybind11::object proxySignalDisconnect(const AnyObjectGILSafe& obj,
                                        SignalLink id,
                                        bool async);
 } // namespace detail

src/pyapplication.cpp

@@ -74,6 +74,45 @@ WithArgcArgv<ka::Decay<F>, ExtraArgs...> withArgcArgv(F&& f)
   return { std::forward<F>(f) };
 }
 
+// Wrapper for the `qi::ApplicationSession` class.
+//
+// Stores the `::py::object` that represents the associated `Session` object.
+//
+// This `::py::object` holds an `AnyObject` that wraps the `Session` object.
+// Maintaining the `AnyObject` is necessary so that any properties or signals
+// wrappers that the `::py::object` also holds for this qi Object don't hold
+// an expired pointer to the `AnyObject`.
+class ApplicationSession : private qi::ApplicationSession
+{
+public:
+  using Base = qi::ApplicationSession;
+  using Config = Base::Config;
+
+  template<typename... Args>
+  explicit ApplicationSession(Args&&... args)
+    : Base(std::forward<Args>(args)...)
+    , _session(py::makeSession(Base::session()))
+  {
+  }
+
+  ~ApplicationSession()
+  {
+    GILAcquire lock;
+    _session.release().dec_ref();
+  }
+
+  using Base::stop;
+  using Base::atRun;
+
+  void run() { return Base::run(); }
+  void startSession() { return Base::startSession(); }
+  std::string url() const { return Base::url().str(); }
+  ::py::object session() const { return _session; }
+
+private:
+  ::py::object _session;
+};
+
 } // namespace
 
 void exportApplication(::py::module& m)
@@ -128,18 +167,12 @@ void exportApplication(::py::module& m)
                 doc(
                   "Add a callback that will be executed when run() is called."))
 
-    .def_property_readonly("url",
-                           [](const ApplicationSession& app) {
-                             return app.url().str();
-                           },
+    .def_property_readonly("url", &ApplicationSession::url,
                            call_guard<GILRelease>(),
                            doc("The url given to the Application. It's the url "
                                "used to connect the session."))
 
-    .def_property_readonly("session",
-                           [](const ApplicationSession& app) {
-                             return makeSession(app.session());
-                           },
+    .def_property_readonly("session", &ApplicationSession::session,
                            doc("The session associated to the application."));
 }
 

src/pyproperty.cpp

@@ -38,18 +38,12 @@ ::py::object proxyPropertyConnect(detail::ProxyProperty& prop,
                                   bool async)
 {
   GILAcquire lock;
-  return detail::proxySignalConnect(prop.object, prop.propertyId, callback, async);
+  detail::AnyObjectGILSafe object(prop.object);
+  return detail::proxySignalConnect(object, prop.propertyId, callback, async);
 }
 
 } // namespace
 
-detail::ProxyProperty::~ProxyProperty()
-{
-  // The destructor can lock when waiting for callbacks to end.
-  GILRelease unlock;
-  object.reset();
-}
-
 bool isProperty(const pybind11::object& obj)
 {
   GILAcquire lock;
@@ -127,7 +121,8 @@ void exportProperty(::py::module& m)
   class_<detail::ProxyProperty>(m, "_ProxyProperty")
     .def("value",
          [](const detail::ProxyProperty& prop, bool async) {
-           const auto fut = prop.object.property(prop.propertyId).async();
+           detail::AnyObjectGILSafe object(prop.object);
+           const auto fut = object->property(prop.propertyId).async();
            GILAcquire lock;
            return resultObject(fut, async);
          },
@@ -136,8 +131,9 @@ void exportProperty(::py::module& m)
          [](detail::ProxyProperty& prop, object pyValue, bool async) {
            AnyValue value(unwrapAsRef(pyValue));
            GILRelease unlock;
+           detail::AnyObjectGILSafe object(prop.object);
            const auto fut =
-             toFuture(prop.object.setProperty(prop.propertyId, std::move(value))
+             toFuture(object->setProperty(prop.propertyId, std::move(value))
                         .async());
            GILAcquire lock;
            return resultObject(fut, async);
@@ -149,7 +145,7 @@ void exportProperty(::py::module& m)
          arg(asyncArgName) = false)
     .def("disconnect",
          [](detail::ProxyProperty& prop, SignalLink id, bool async) {
-           return detail::proxySignalDisconnect(prop.object, id, async);
+           return detail::proxySignalDisconnect(detail::AnyObjectGILSafe(prop.object), id, async);
          },
          "id"_a, arg(asyncArgName) = false);
 }

src/pysignal.cpp

@@ -65,21 +65,28 @@ ::py::object proxySignalConnect(detail::ProxySignal& sig,
                                 bool async)
 {
   GILAcquire lock;
-  return detail::proxySignalConnect(sig.object, sig.signalId, callback, async);
+  return detail::proxySignalConnect(detail::AnyObjectGILSafe(sig.object), sig.signalId, callback, async);
 }
 
 } // namespace
 
 namespace detail
 {
 
-detail::ProxySignal::~ProxySignal()
+static constexpr const auto objectExpiredMsg = "object has expired";
+
+AnyObjectGILSafe::AnyObjectGILSafe(AnyWeakObject weakObj)
 {
-  // The destructor can lock waiting for callbacks to end.
-  GILRelease unlock;
-  object.reset();
+  _obj = weakObj.lock();
+  if (!_obj.isValid())
+    throw std::runtime_error(objectExpiredMsg);
 }
 
+AnyObjectGILSafe::~AnyObjectGILSafe()
+{
+  GILRelease unlock;
+  _obj = {};
+}
 
 ::py::object signalConnect(SignalBase& sig,
                            const ::py::function& callback,
@@ -119,7 +126,7 @@ ::py::object signalDisconnectAll(SignalBase& sig, bool async)
   return resultObject(fut, async);
 }
 
-::py::object proxySignalConnect(const AnyObject& obj,
+::py::object proxySignalConnect(const AnyObjectGILSafe& obj,
                                 unsigned int signalId,
                                 const ::py::function& callback,
                                 bool async)
@@ -128,18 +135,18 @@ ::py::object proxySignalConnect(const AnyObject& obj,
   return connect(
     [&](const SignalSubscriber& sub) {
       GILRelease unlock;
-      return obj.connect(signalId, sub).async();
+      return obj->connect(signalId, sub).async();
     },
     callback, async);
 }
 
-::py::object proxySignalDisconnect(const AnyObject& obj,
+::py::object proxySignalDisconnect(const AnyObjectGILSafe& obj,
                                    SignalLink id,
                                    bool async)
 {
   const auto fut = [&] {
     GILRelease unlock;
-    return toFuture(obj.disconnect(id));
+    return toFuture(obj->disconnect(id));
   }();
   GILAcquire lock;
   return resultObject(fut, async);
@@ -212,15 +219,17 @@ void exportSignal(::py::module& m)
          arg(asyncArgName) = false)
     .def("disconnect",
          [](detail::ProxySignal& sig, SignalLink id, bool async) {
-           return detail::proxySignalDisconnect(sig.object, id, async);
+           auto object = detail::AnyObjectGILSafe(sig.object);
+           return detail::proxySignalDisconnect(object, id, async);
          },
          "id"_a, arg(asyncArgName) = false)
     .def("__call__",
          [](detail::ProxySignal& sig, args pyArgs) {
            const auto args =
              AnyReference::from(pyArgs).content().asTupleValuePtr();
            GILRelease unlock;
-           sig.object.metaPost(sig.signalId, args);
+           auto object = detail::AnyObjectGILSafe(sig.object);
+           object->metaPost(sig.signalId, args);
   });
 }