Fixes a memory leak in native code and a regression. #46735

This patch fixes a leak caused by errors in the native code and a regression
caused by a previous tentative of a fix for another leak.

The leak occurs in qi objects bound functions arguments conversion. It was not
clear whether `qi::StructTypeInterface` subclasses implementations of its `get`
methods should return owned pointers or not (i.e. if the caller of these
methods was expected to destroy these pointers). It seems after research that
they must not, and our `StructuredIterableInterface` subtype implementation was
wrong. These methods are reimplemented by this patch.

The regression was caused by a previous fix. The fix (commit "a072133") aimed
at resolving a leak that concerned qi objects containing properties or signals,
which were kept alive after the Python object holding them was destroyed. This
fix introduced a regression that prevented users to call functions of
properties from temporary objects.
Therefore, this patch reverts that commit. After further research, it was found
that the leak is caused by objects being instantiated on the heap when we
thought `pybind11` took ownership of these, which is not the case. Instead, the
objects are now instantiated on the stack.

Author: Vincent Palancher

qi/test/test_module.py

@@ -42,7 +42,6 @@ def test_module_service():
     cat = session.service("Cat")
     assert cat.meow(3) == 'meow'
 
-
 def test_module_service_object_lifetime():
     session = qi.Session()
     session.listenStandalone("tcp://localhost:0")
@@ -72,3 +71,25 @@ def test_module_service_object_lifetime():
     assert cat.nbPlay() == 1
     del play
     assert cat.nbPlay() == 0
+
+def test_object_bound_functions_arguments_conversion_does_not_leak():
+    session = qi.Session()
+    session.listenStandalone("tcp://localhost:0")
+    session.loadServiceRename("moduletest.Cat", "", "truc")
+    cat = session.service("Cat")
+
+    play = cat.makePlay()
+    assert cat.nbPlay() == 1
+    cat.order(play)
+    assert cat.nbPlay() == 1
+    del play
+    assert cat.nbPlay() == 0
+
+def test_temporary_object_bound_properties_are_usable():
+    session = qi.Session()
+    session.listenStandalone("tcp://localhost:0")
+    session.loadServiceRename("moduletest.Cat", "", "truc")
+    # The `volume` member is a bound property of the `Purr` object.
+    # It should keep the object alive so that setting the property, which
+    # requires accessing the object, does not fail.
+    session.service("Cat").makePurr().volume.setValue(42)

qipython/pyproperty.hpp

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

qipython/pysignal.hpp

@@ -25,41 +25,12 @@ 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
 {
-  AnyWeakObject object;
+  AnyObject object;
   unsigned int signalId;
+
+  ~ProxySignal();
 };
 
 pybind11::object signalConnect(SignalBase& sig,
@@ -70,12 +41,12 @@ pybind11::object signalDisconnect(SignalBase& sig, SignalLink id, bool async);
 
 pybind11::object signalDisconnectAll(SignalBase& sig, bool async);
 
-pybind11::object proxySignalConnect(const AnyObjectGILSafe& obj,
+pybind11::object proxySignalConnect(const AnyObject& obj,
                                     unsigned int signalId,
                                     const pybind11::function& callback,
                                     bool async);
 
-pybind11::object proxySignalDisconnect(const AnyObjectGILSafe& obj,
+pybind11::object proxySignalDisconnect(const AnyObject& obj,
                                        SignalLink id,
                                        bool async);
 } // namespace detail

src/pyapplication.cpp

@@ -74,45 +74,6 @@ 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)
@@ -167,12 +128,18 @@ void exportApplication(::py::module& m)
                 doc(
                   "Add a callback that will be executed when run() is called."))
 
-    .def_property_readonly("url", &ApplicationSession::url,
+    .def_property_readonly("url",
+                           [](const ApplicationSession& app) {
+                             return app.url().str();
+                           },
                            call_guard<GILRelease>(),
                            doc("The url given to the Application. It's the url "
                                "used to connect the session."))
 
-    .def_property_readonly("session", &ApplicationSession::session,
+    .def_property_readonly("session",
+                           [](const ApplicationSession& app) {
+                             return makeSession(app.session());
+                           },
                            doc("The session associated to the application."));
 }
 

src/pyobject.cpp

@@ -121,7 +121,7 @@ void populateSignals(::py::object pyobj, const Object& obj)
       continue;
 
     ::py::setattr(pyobj, signalName.c_str(),
-                  castToPyObject(new detail::ProxySignal{ obj, signal.uid() }));
+                  castToPyObject(detail::ProxySignal{ obj, signal.uid() }));
   }
 }
 
@@ -138,7 +138,7 @@ void populateProperties(::py::object pyobj, const Object& obj)
       continue;
 
     ::py::setattr(pyobj, propName.c_str(),
-                  castToPyObject(new detail::ProxyProperty{ obj, prop.uid() }));
+                  castToPyObject(detail::ProxyProperty{ obj, prop.uid() }));
   }
 }
 

src/pyproperty.cpp

@@ -38,12 +38,18 @@ ::py::object proxyPropertyConnect(detail::ProxyProperty& prop,
                                   bool async)
 {
   GILAcquire lock;
-  detail::AnyObjectGILSafe object(prop.object);
-  return detail::proxySignalConnect(object, prop.propertyId, callback, async);
+  return detail::proxySignalConnect(prop.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;
@@ -121,8 +127,7 @@ void exportProperty(::py::module& m)
   class_<detail::ProxyProperty>(m, "_ProxyProperty")
     .def("value",
          [](const detail::ProxyProperty& prop, bool async) {
-           detail::AnyObjectGILSafe object(prop.object);
-           const auto fut = object->property(prop.propertyId).async();
+           const auto fut = prop.object.property(prop.propertyId).async();
            GILAcquire lock;
            return resultObject(fut, async);
          },
@@ -131,9 +136,8 @@ 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(object->setProperty(prop.propertyId, std::move(value))
+             toFuture(prop.object.setProperty(prop.propertyId, std::move(value))
                         .async());
            GILAcquire lock;
            return resultObject(fut, async);
@@ -145,7 +149,7 @@ void exportProperty(::py::module& m)
          arg(asyncArgName) = false)
     .def("disconnect",
          [](detail::ProxyProperty& prop, SignalLink id, bool async) {
-           return detail::proxySignalDisconnect(detail::AnyObjectGILSafe(prop.object), id, async);
+           return detail::proxySignalDisconnect(prop.object, id, async);
          },
          "id"_a, arg(asyncArgName) = false);
 }

src/pysignal.cpp

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

src/pytypes.cpp

@@ -535,8 +535,10 @@ class StructuredIterableInterface
     res.reserve(_size);
     for (const ::py::handle itemHandle : obj)
     {
-      auto item = ::py::reinterpret_borrow<::py::object>(itemHandle);
-      res.push_back(AnyValue::from(item).release().rawValue());
+      const auto item = ::py::reinterpret_borrow<::py::object>(itemHandle);
+      const auto itemRef = AnyValue::from(item).release();
+      storeDisownedReference(storage, itemRef);
+      res.push_back(itemRef.rawValue());
     }
     return res;
   }
@@ -551,8 +553,10 @@ class StructuredIterableInterface
     // `pybind11::iterator` is not. We use advance instead.
     auto it = obj.begin();
     std::advance(it, index);
-    const auto item = *it;
-    return AnyValue::from(item).release().rawValue();
+    const auto item = ::py::reinterpret_borrow<::py::object>(*it);
+    const auto itemRef = AnyValue::from(item).release();
+    storeDisownedReference(storage, itemRef);
+    return itemRef.rawValue();
   }
 
   void set(void** /*storage*/, const std::vector<void*>&) override

tests/moduletest.cpp

@@ -121,6 +121,11 @@ class Cat
       return boost::make_shared<Play>(playCounter);
     }
 
+    void order(qi::AnyObject /*action*/) const
+    {
+      // Cats do not follow orders, they do nothing.
+    }
+
     int nbPurr()
     {
       return purrCounter->load();
@@ -146,7 +151,8 @@ class Cat
 };
 
 QI_REGISTER_OBJECT(Cat, meow, cloneMe, hunger, boredom, cuteness,
-                   makePurr, makeSleep, makePlay, nbPurr, nbSleep, nbPlay);
+                   makePurr, makeSleep, makePlay, order,
+                   nbPurr, nbSleep, nbPlay);
 
 Cat::Cat()
 {