Merge commit 'refs/integration/team/platform/merge-309'

Author: opennao

cmake/add_tests.cmake

@@ -49,18 +49,32 @@ target_link_libraries(test_qipython
           qi.interface
           cxx11
           gmock)
-# Unfortunately, in some of our toolchains, gtest/gmock headers are found in the qi-framework
-# package, which comes first in the include paths order given to the compiler. This causes the
-# compiler to use those headers instead of the ones we got from a `FetchContent` of the googletest
-# repository.
-target_include_directories(test_qipython
-  BEFORE # Force this path to come first in the list of include paths.
-  PRIVATE $<TARGET_PROPERTY:gmock,INTERFACE_INCLUDE_DIRECTORIES>)
-enable_warnings(test_qipython)
-set_build_rpath_to_qipython_dependencies(test_qipython)
+
+add_executable(test_qipython_local_interpreter)
+target_sources(test_qipython_local_interpreter
+  PRIVATE tests/test_qipython_local_interpreter.cpp)
+target_link_libraries(test_qipython_local_interpreter
+  PRIVATE Python::Python
+          pybind11
+          qi_python_objects
+          qi.interface
+          cxx11
+          gmock)
 
 set(_sdk_prefix "${CMAKE_BINARY_DIR}/sdk")
-gtest_discover_tests(test_qipython EXTRA_ARGS --qi-sdk-prefix ${_sdk_prefix})
+foreach(test_target IN ITEMS test_qipython
+                             test_qipython_local_interpreter)
+  # Unfortunately, in some of our toolchains, gtest/gmock headers are found in the qi-framework
+  # package, which comes first in the include paths order given to the compiler. This causes the
+  # compiler to use those headers instead of the ones we got from a `FetchContent` of the googletest
+  # repository.
+  target_include_directories(${test_target}
+    BEFORE # Force this path to come first in the list of include paths.
+    PRIVATE $<TARGET_PROPERTY:gmock,INTERFACE_INCLUDE_DIRECTORIES>)
+  enable_warnings(${test_target})
+  set_build_rpath_to_qipython_dependencies(${test_target})
+  gtest_discover_tests(${test_target} EXTRA_ARGS --qi-sdk-prefix ${_sdk_prefix})
+endforeach()
 
 if(NOT Python_Interpreter_FOUND)
   message(WARNING "tests: a compatible Python Interpreter was NOT found, Python tests are DISABLED.")

cmake/set_libqi_dependency.cmake

@@ -1,4 +1,4 @@
-overridable_variable(LIBQI_VERSION 2.1.0)
+overridable_variable(LIBQI_VERSION 3.0.0)
 
 # Our github clone is sometimes late or is missing tags, so we enable
 # customisation of the URL at configuration time, so users can use another clone.

qi/_version.py

@@ -1 +1 @@
-__version__ = '3.1.0'
+__version__ = '3.1.1'

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/pyguard.hpp

@@ -143,59 +143,57 @@ void pybind11GuardDisarm(G& guard)
 /// RAII utility type that guarantees that the GIL is locked for the scope of
 /// the lifetime of the object.
 ///
+/// Objects of this type (or objects composed of them) must not be kept alive
+/// after the hand is given back to the interpreter.
+///
 /// This type is re-entrant.
 ///
-/// postcondition: `GILAcquire acq;` establishes `currentThreadHoldsGil()`
+/// postcondition: `GILAcquire acq;` establishes
+/// `(interpreterIsFinalizing() && *interpreterIsFinalizing()) || currentThreadHoldsGil()`
 struct GILAcquire
 {
   inline GILAcquire()
   {
+    const auto optIsFinalizing = interpreterIsFinalizing();
+    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
     // `gil_scoped_acquire` is re-entrant by itself, so we don't need to check
     // whether or not the GIL is already held by the current thread.
-    QI_ASSERT(currentThreadHoldsGil());
+    if (!definitelyFinalizing)
+      _acq.emplace();
+    QI_ASSERT(definitelyFinalizing || currentThreadHoldsGil());
   }
 
   GILAcquire(const GILAcquire&) = delete;
   GILAcquire& operator=(const GILAcquire&) = delete;
 
-  inline ~GILAcquire()
-  {
-    const auto optIsFinalizing = interpreterIsFinalizing();
-    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
-    if (definitelyFinalizing)
-      detail::pybind11GuardDisarm(_acq);
-  }
-
 private:
-  pybind11::gil_scoped_acquire _acq;
+  boost::optional<pybind11::gil_scoped_acquire> _acq;
 };
 
 /// RAII utility type that guarantees that the GIL is unlocked for the scope of
 /// the lifetime of the object.
 ///
+/// Objects of this type (or objects composed of them) must not be kept alive
+/// after the hand is given back to the interpreter.
+///
 /// This type is re-entrant.
 ///
-/// postcondition: `GILRelease rel;` establishes `!currentThreadHoldsGil()`
+/// postcondition: `GILRelease rel;` establishes
+/// `(interpreterIsFinalizing() && *interpreterIsFinalizing()) || !currentThreadHoldsGil()`
 struct GILRelease
 {
   inline GILRelease()
   {
-    if (currentThreadHoldsGil())
+    const auto optIsFinalizing = interpreterIsFinalizing();
+    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
+    if (!definitelyFinalizing && currentThreadHoldsGil())
       _release.emplace();
-    QI_ASSERT(!currentThreadHoldsGil());
+    QI_ASSERT(definitelyFinalizing || !currentThreadHoldsGil());
   }
 
   GILRelease(const GILRelease&) = delete;
   GILRelease& operator=(const GILRelease&) = delete;
 
-  inline ~GILRelease()
-  {
-    const auto optIsFinalizing = interpreterIsFinalizing();
-    const auto definitelyFinalizing = optIsFinalizing && *optIsFinalizing;
-    if (_release && definitelyFinalizing)
-      detail::pybind11GuardDisarm(*_release);
-  }
-
 private:
   boost::optional<pybind11::gil_scoped_release> _release;
 };

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);
 }