Merge lp:~thomas-voss/dbus-cpp/fix-signal-subscriptions into lp:dbus-cpp/15.04

[Thomas Voß]

Ensure that Signal with non-void argument types correctly narrow their match rules.

Ensure that Signal with non-void argument types correctly narrow their match rules.

[lp:~thomas-voss/dbus-cpp/fix-signal-subscriptions]

103. By Thomas Voß on 2015-11-14

Fix disconnect behavior for stub signals and lifetime issues for Signal<> instances.

104. By Thomas Voß on 2015-11-14

Handle o.f.dbus.Properties.PropertiesChanged manually to fix circular lifetime dependency.

105. By Thomas Voß on 2015-11-16

Cache properties on stub object instances.

106. By Thomas Voß on 2015-11-16

Ensure that Property::about_to_be_destroyed() is triggered in dtor.

107. By Thomas Voß on 2015-11-16

Ensure that match rules are correctly removed on property destruction.

108. By Thomas Voß on 2015-11-16

Only subscribe to PropertiesChanged once per dbus::Object instance (on demand).

109. By Thomas Voß on 2015-11-16

Remove match for PropertiesChanged in Object::~Object.

[Alberto Aguirre (albaguirre)]

LGTM

review: Approve

=== modified file 'include/core/dbus/impl/object.h'
--- include/core/dbus/impl/object.h	2014-10-30 13:22:52 +0000
+++ include/core/dbus/impl/object.h	2015-11-16 15:33:02 +0000
@@ -19,7 +19,6 @@
 #define CORE_DBUS_IMPL_OBJECT_H_
 
 #include <core/dbus/bus.h>
-#include <core/dbus/lifetime_constrained_cache.h>
 #include <core/dbus/match_rule.h>
 #include <core/dbus/message_router.h>
 #include <core/dbus/message_streaming_operators.h>
@@ -37,6 +36,7 @@
 
 #include <functional>
 #include <future>
+#include <iostream>
 #include <map>
 #include <memory>
 #include <string>
@@ -70,17 +70,17 @@
         object_path.as_string(),
         traits::Service<typename Method::Interface>::interface_name().c_str(),
         Method::name());
-    
+
     if (!msg)
         throw std::runtime_error("No memory available to allocate DBus message");
-    
+ 
     auto writer = msg->writer();
     encode_message(writer, args...);
-    
+
     auto reply = parent->get_connection()->send_with_reply_and_block_for_at_most(
                 msg,
                 Method::default_timeout());
-    
+
     return Result<ResultType>::from_message(reply);
 }
 
@@ -153,40 +153,57 @@
 inline std::shared_ptr<Property<PropertyDescription>>
 Object::get_property()
 {
-    // If this is a proxy object we set up listening for property changes the
-    // first time someone accesses properties.
-    if (parent->is_stub())
-    {
-        if (!signal_properties_changed)
-        {
-            signal_properties_changed
-                = get_signal<interfaces::Properties::Signals::PropertiesChanged>();
-
-            signal_properties_changed->connect(
-                std::bind(
-                    &Object::on_properties_changed,
-                    shared_from_this(),
-                    std::placeholders::_1));
-        }
-    }
-
     typedef Property<PropertyDescription> PropertyType;
-    auto property =
-            PropertyType::make_property(
-                shared_from_this());
 
+    // Creating a stub property for a remote object/property instance
+    // requires the following steps:
+    //   [1.] Look up if we already have a property instance available in the cache, 
+    //        leveraging the tuple (path, interface, name) as key.
+    //     [1.1] If yes: return the property.
+    //     [1.2] If no: Create a new proeprty instance and:
+    //       [1.2.1] Make it known to the cache.
+    //       [1.2.2] Wire it up for property_changed signal receiving.
+    //       [1.2.3] Communicate a new match rule to the dbus daemon to enable reception.
     if (parent->is_stub())
     {
-        auto tuple = std::make_tuple(
-                traits::Service<typename PropertyDescription::Interface>::interface_name(),
-                PropertyDescription::name());
-
-        property_changed_vtable[tuple] = std::bind(
-                &Property<PropertyDescription>::handle_changed,
-                property,
-                std::placeholders::_1);
+        auto itf = traits::Service<typename PropertyDescription::Interface>::interface_name();
+        auto name = PropertyDescription::name(); 
+        auto ekey = std::make_tuple(path(), itf, name);
+
+        auto property = Object::property_cache<PropertyDescription>().retrieve_value_for_key(ekey);
+        if (property)
+        {
+            return property;
+        }
+
+        auto mr = MatchRule()
+            .type(Message::Type::signal)
+            .interface(traits::Service<interfaces::Properties>::interface_name())
+            .member(interfaces::Properties::Signals::PropertiesChanged::name());
+
+        property = PropertyType::make_property(shared_from_this());
+
+        Object::property_cache<PropertyDescription>().insert_value_for_key(ekey, property);
+
+        // We only ever do this once per object.
+        std::call_once(add_match_once, [&]()
+        {
+            // [1.2.4] Inform the dbus daemon that we would like to receive the respective signals.
+            add_match(mr);
+        });
+
+        // [1.2.2] Enable dispatching of changes.
+        std::weak_ptr<PropertyType> wp{property};
+        property_changed_vtable[std::make_tuple(itf, name)] = [wp](const types::Variant& arg)
+        {
+            if (auto sp = wp.lock())
+                sp->handle_changed(arg);
+        };
+
+        return property;
     }
-    return property;
+
+    return PropertyType::make_property(shared_from_this());
 }
 
 template<typename Interface>
@@ -312,6 +329,23 @@
                 &MessageRouter<PropertyKey>::operator(), 
                 std::ref(set_property_router), 
                 std::placeholders::_1));
+    } else
+    {
+        // We centrally route org.freedesktop.DBus.Properties.PropertiesChanged
+        // through the object, which in turn routes via a custom Property cache.
+        signal_router.install_route(
+            SignalKey{
+                traits::Service<interfaces::Properties>::interface_name(), 
+                interfaces::Properties::Signals::PropertiesChanged::name()
+            },
+            // Passing 'this' is fine as the lifetime of the signal_router is upper limited
+            // by the lifetime of 'this'.
+            [this](const Message::Ptr& msg)
+            {
+                interfaces::Properties::Signals::PropertiesChanged::ArgumentType arg;
+                msg->reader() >> arg;
+                on_properties_changed(arg);
+            });
     }
 }
 
@@ -319,6 +353,20 @@
 {
     parent->get_connection()->access_signal_router().uninstall_route(object_path);
     parent->get_connection()->unregister_object_path(object_path);
+
+    auto mr = MatchRule()
+        .type(Message::Type::signal)
+        .interface(traits::Service<interfaces::Properties>::interface_name())
+        .member(interfaces::Properties::Signals::PropertiesChanged::name());
+
+    try
+    {
+        remove_match(mr);
+    } catch(...)
+   {
+        // We consciously drop all possible exceptions here. There is hardly 
+        // anything we can do about the error anyway.
+   }
 }
 
 inline void Object::add_match(const MatchRule& rule)
@@ -346,6 +394,19 @@
         }
     }
 }
+
+template<typename PropertyDescription>
+inline core::dbus::ThreadSafeLifetimeConstrainedCache<
+    core::dbus::Object::CacheKey,
+    core::dbus::Property<PropertyDescription>>&
+core::dbus::Object::property_cache()
+{
+    static core::dbus::ThreadSafeLifetimeConstrainedCache<
+        core::dbus::Object::CacheKey,
+        core::dbus::Property<PropertyDescription>
+    > cache;
+    return cache;
+}
 }
 }
 

=== modified file 'include/core/dbus/impl/property.h'
--- include/core/dbus/impl/property.h	2014-06-10 08:44:17 +0000
+++ include/core/dbus/impl/property.h	2015-11-16 15:33:02 +0000
@@ -64,6 +64,13 @@
 }
 
 template<typename PropertyType>
+const core::Signal<void>&
+Property<PropertyType>::about_to_be_destroyed() const
+{
+    return signal_about_to_be_destroyed;
+}
+
+template<typename PropertyType>
 std::shared_ptr<Property<PropertyType>>
 Property<PropertyType>::make_property(const std::shared_ptr<Object>& parent)
 {
@@ -109,6 +116,20 @@
 }
 
 template<typename PropertyType>
+Property<PropertyType>::~Property()
+{
+    try
+    {
+        signal_about_to_be_destroyed();
+    } catch(...)
+    {
+        // Consciously dropping all exceptions here.
+        // There is hardly anything we can do about it while
+        // tearing down the object anyway.
+    }
+}
+
+template<typename PropertyType>
 void
 Property<PropertyType>::handle_get(const Message::Ptr& msg)
 {

=== modified file 'include/core/dbus/impl/signal.h'
--- include/core/dbus/impl/signal.h	2015-10-21 20:04:04 +0000
+++ include/core/dbus/impl/signal.h	2015-11-16 15:33:02 +0000
@@ -121,8 +121,8 @@
             &Signal<SignalDescription>::operator(),
             this,
             std::placeholders::_1));
-    parent->add_match(
-        rule.type(Message::Type::signal).interface(interface).member(name));
+    rule = rule.type(Message::Type::signal).interface(interface).member(name);
+    parent->add_match(rule);
 }
 
 template<typename SignalDescription, typename Argument>
@@ -309,6 +309,8 @@
             &Signal<SignalDescription, typename SignalDescription::ArgumentType>::operator(),
             this,
             std::placeholders::_1));
+
+    d->rule = d->rule.type(Message::Type::signal).interface(interface).member(name);
 }
 
 template<typename SignalDescription>

=== modified file 'include/core/dbus/object.h'
--- include/core/dbus/object.h	2014-10-23 21:09:56 +0000
+++ include/core/dbus/object.h	2015-11-16 15:33:02 +0000
@@ -19,12 +19,14 @@
 #define CORE_DBUS_OBJECT_H_
 
 #include <core/dbus/bus.h>
+#include <core/dbus/lifetime_constrained_cache.h>
 #include <core/dbus/service.h>
 
 #include <functional>
 #include <future>
 #include <map>
 #include <memory>
+#include <mutex>
 #include <ostream>
 #include <string>
 
@@ -82,10 +84,14 @@
 class Object : public std::enable_shared_from_this<Object>
 {
   private:
+    typedef std::tuple<types::ObjectPath, std::string, std::string> CacheKey;
     typedef std::tuple<std::string, std::string> MethodKey;
     typedef std::tuple<std::string, std::string> PropertyKey;
     typedef std::tuple<std::string, std::string> SignalKey;
 
+    template<typename PropertyDescription>
+    static ThreadSafeLifetimeConstrainedCache<CacheKey, Property<PropertyDescription>>& property_cache();
+
   public:
     typedef std::shared_ptr<Object> Ptr;
     typedef std::function<void(const Message::Ptr&)> MethodHandler;
@@ -173,7 +179,7 @@
      * @param [in] path The path to associate the object with.
      * @return An object instance or nullptr in case of errors.
      */
-    std::shared_ptr<Object> 
+    std::shared_ptr<Object>
     inline add_object_for_path(const types::ObjectPath& path);
 
     /**
@@ -227,16 +233,11 @@
     MessageRouter<MethodKey> method_router;
     MessageRouter<PropertyKey> get_property_router;
     MessageRouter<PropertyKey> set_property_router;
+    std::once_flag add_match_once;
     std::map<
         std::tuple<std::string, std::string>,
         std::function<void(const types::Variant&)>
     > property_changed_vtable;
-    std::shared_ptr<
-        Signal<
-            interfaces::Properties::Signals::PropertiesChanged,
-            interfaces::Properties::Signals::PropertiesChanged::ArgumentType
-        >
-    > signal_properties_changed;
 };
 }
 }

=== modified file 'include/core/dbus/property.h'
--- include/core/dbus/property.h	2014-01-20 21:22:02 +0000
+++ include/core/dbus/property.h	2015-11-16 15:33:02 +0000
@@ -43,6 +43,8 @@
     typedef typename PropertyType::ValueType ValueType;
     typedef core::Property<ValueType> Super;
 
+    inline ~Property();
+
     /**
      * @brief Non-mutable access to the contained value.
      * @return Non-mutable reference to the contained value.
@@ -61,6 +63,11 @@
      */
     inline bool is_writable() const;
 
+    /**
+     * @brief Emitted during destruction of an object instance.
+     */
+    inline const core::Signal<void>& about_to_be_destroyed() const;
+
 protected:
     friend class Object;
 
@@ -82,6 +89,7 @@
     std::string interface;
     std::string name;
     bool writable;
+    core::Signal<void> signal_about_to_be_destroyed;
 };
 }
 }

=== modified file 'src/core/dbus/service.cpp'
--- src/core/dbus/service.cpp	2014-06-30 04:57:08 +0000
+++ src/core/dbus/service.cpp	2015-11-16 15:33:02 +0000
@@ -79,7 +79,7 @@
 
 void Service::remove_match(const MatchRule& rule)
 {
-    connection->remove_match(rule);
+    connection->remove_match(rule.sender(name));
 }
 
 Service::Service(const Bus::Ptr& connection, const std::string& name)