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

Proposed by Thomas Voß on 2015-11-10
Status: Merged
Approved by: Łukasz Zemczak on 2015-12-04
Approved revision: 109
Merged at revision: 102
Proposed branch: lp:~thomas-voss/dbus-cpp/fix-signal-subscriptions
Merge into: lp:dbus-cpp/15.04
Diff against target: 363 lines (+137/-44)
6 files modified
include/core/dbus/impl/object.h (+95/-34)
include/core/dbus/impl/property.h (+21/-0)
include/core/dbus/impl/signal.h (+4/-2)
include/core/dbus/object.h (+8/-7)
include/core/dbus/property.h (+8/-0)
src/core/dbus/service.cpp (+1/-1)
To merge this branch: bzr merge lp:~thomas-voss/dbus-cpp/fix-signal-subscriptions
Reviewer Review Type Date Requested Status
Alberto Aguirre (community) 2015-11-10 Approve on 2015-11-23
Review via email: mp+277101@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
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) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/core/dbus/impl/object.h'
2--- include/core/dbus/impl/object.h 2014-10-30 13:22:52 +0000
3+++ include/core/dbus/impl/object.h 2015-11-16 15:33:02 +0000
4@@ -19,7 +19,6 @@
5 #define CORE_DBUS_IMPL_OBJECT_H_
6
7 #include <core/dbus/bus.h>
8-#include <core/dbus/lifetime_constrained_cache.h>
9 #include <core/dbus/match_rule.h>
10 #include <core/dbus/message_router.h>
11 #include <core/dbus/message_streaming_operators.h>
12@@ -37,6 +36,7 @@
13
14 #include <functional>
15 #include <future>
16+#include <iostream>
17 #include <map>
18 #include <memory>
19 #include <string>
20@@ -70,17 +70,17 @@
21 object_path.as_string(),
22 traits::Service<typename Method::Interface>::interface_name().c_str(),
23 Method::name());
24-
25+
26 if (!msg)
27 throw std::runtime_error("No memory available to allocate DBus message");
28-
29+
30 auto writer = msg->writer();
31 encode_message(writer, args...);
32-
33+
34 auto reply = parent->get_connection()->send_with_reply_and_block_for_at_most(
35 msg,
36 Method::default_timeout());
37-
38+
39 return Result<ResultType>::from_message(reply);
40 }
41
42@@ -153,40 +153,57 @@
43 inline std::shared_ptr<Property<PropertyDescription>>
44 Object::get_property()
45 {
46- // If this is a proxy object we set up listening for property changes the
47- // first time someone accesses properties.
48- if (parent->is_stub())
49- {
50- if (!signal_properties_changed)
51- {
52- signal_properties_changed
53- = get_signal<interfaces::Properties::Signals::PropertiesChanged>();
54-
55- signal_properties_changed->connect(
56- std::bind(
57- &Object::on_properties_changed,
58- shared_from_this(),
59- std::placeholders::_1));
60- }
61- }
62-
63 typedef Property<PropertyDescription> PropertyType;
64- auto property =
65- PropertyType::make_property(
66- shared_from_this());
67
68+ // Creating a stub property for a remote object/property instance
69+ // requires the following steps:
70+ // [1.] Look up if we already have a property instance available in the cache,
71+ // leveraging the tuple (path, interface, name) as key.
72+ // [1.1] If yes: return the property.
73+ // [1.2] If no: Create a new proeprty instance and:
74+ // [1.2.1] Make it known to the cache.
75+ // [1.2.2] Wire it up for property_changed signal receiving.
76+ // [1.2.3] Communicate a new match rule to the dbus daemon to enable reception.
77 if (parent->is_stub())
78 {
79- auto tuple = std::make_tuple(
80- traits::Service<typename PropertyDescription::Interface>::interface_name(),
81- PropertyDescription::name());
82-
83- property_changed_vtable[tuple] = std::bind(
84- &Property<PropertyDescription>::handle_changed,
85- property,
86- std::placeholders::_1);
87+ auto itf = traits::Service<typename PropertyDescription::Interface>::interface_name();
88+ auto name = PropertyDescription::name();
89+ auto ekey = std::make_tuple(path(), itf, name);
90+
91+ auto property = Object::property_cache<PropertyDescription>().retrieve_value_for_key(ekey);
92+ if (property)
93+ {
94+ return property;
95+ }
96+
97+ auto mr = MatchRule()
98+ .type(Message::Type::signal)
99+ .interface(traits::Service<interfaces::Properties>::interface_name())
100+ .member(interfaces::Properties::Signals::PropertiesChanged::name());
101+
102+ property = PropertyType::make_property(shared_from_this());
103+
104+ Object::property_cache<PropertyDescription>().insert_value_for_key(ekey, property);
105+
106+ // We only ever do this once per object.
107+ std::call_once(add_match_once, [&]()
108+ {
109+ // [1.2.4] Inform the dbus daemon that we would like to receive the respective signals.
110+ add_match(mr);
111+ });
112+
113+ // [1.2.2] Enable dispatching of changes.
114+ std::weak_ptr<PropertyType> wp{property};
115+ property_changed_vtable[std::make_tuple(itf, name)] = [wp](const types::Variant& arg)
116+ {
117+ if (auto sp = wp.lock())
118+ sp->handle_changed(arg);
119+ };
120+
121+ return property;
122 }
123- return property;
124+
125+ return PropertyType::make_property(shared_from_this());
126 }
127
128 template<typename Interface>
129@@ -312,6 +329,23 @@
130 &MessageRouter<PropertyKey>::operator(),
131 std::ref(set_property_router),
132 std::placeholders::_1));
133+ } else
134+ {
135+ // We centrally route org.freedesktop.DBus.Properties.PropertiesChanged
136+ // through the object, which in turn routes via a custom Property cache.
137+ signal_router.install_route(
138+ SignalKey{
139+ traits::Service<interfaces::Properties>::interface_name(),
140+ interfaces::Properties::Signals::PropertiesChanged::name()
141+ },
142+ // Passing 'this' is fine as the lifetime of the signal_router is upper limited
143+ // by the lifetime of 'this'.
144+ [this](const Message::Ptr& msg)
145+ {
146+ interfaces::Properties::Signals::PropertiesChanged::ArgumentType arg;
147+ msg->reader() >> arg;
148+ on_properties_changed(arg);
149+ });
150 }
151 }
152
153@@ -319,6 +353,20 @@
154 {
155 parent->get_connection()->access_signal_router().uninstall_route(object_path);
156 parent->get_connection()->unregister_object_path(object_path);
157+
158+ auto mr = MatchRule()
159+ .type(Message::Type::signal)
160+ .interface(traits::Service<interfaces::Properties>::interface_name())
161+ .member(interfaces::Properties::Signals::PropertiesChanged::name());
162+
163+ try
164+ {
165+ remove_match(mr);
166+ } catch(...)
167+ {
168+ // We consciously drop all possible exceptions here. There is hardly
169+ // anything we can do about the error anyway.
170+ }
171 }
172
173 inline void Object::add_match(const MatchRule& rule)
174@@ -346,6 +394,19 @@
175 }
176 }
177 }
178+
179+template<typename PropertyDescription>
180+inline core::dbus::ThreadSafeLifetimeConstrainedCache<
181+ core::dbus::Object::CacheKey,
182+ core::dbus::Property<PropertyDescription>>&
183+core::dbus::Object::property_cache()
184+{
185+ static core::dbus::ThreadSafeLifetimeConstrainedCache<
186+ core::dbus::Object::CacheKey,
187+ core::dbus::Property<PropertyDescription>
188+ > cache;
189+ return cache;
190+}
191 }
192 }
193
194
195=== modified file 'include/core/dbus/impl/property.h'
196--- include/core/dbus/impl/property.h 2014-06-10 08:44:17 +0000
197+++ include/core/dbus/impl/property.h 2015-11-16 15:33:02 +0000
198@@ -64,6 +64,13 @@
199 }
200
201 template<typename PropertyType>
202+const core::Signal<void>&
203+Property<PropertyType>::about_to_be_destroyed() const
204+{
205+ return signal_about_to_be_destroyed;
206+}
207+
208+template<typename PropertyType>
209 std::shared_ptr<Property<PropertyType>>
210 Property<PropertyType>::make_property(const std::shared_ptr<Object>& parent)
211 {
212@@ -109,6 +116,20 @@
213 }
214
215 template<typename PropertyType>
216+Property<PropertyType>::~Property()
217+{
218+ try
219+ {
220+ signal_about_to_be_destroyed();
221+ } catch(...)
222+ {
223+ // Consciously dropping all exceptions here.
224+ // There is hardly anything we can do about it while
225+ // tearing down the object anyway.
226+ }
227+}
228+
229+template<typename PropertyType>
230 void
231 Property<PropertyType>::handle_get(const Message::Ptr& msg)
232 {
233
234=== modified file 'include/core/dbus/impl/signal.h'
235--- include/core/dbus/impl/signal.h 2015-10-21 20:04:04 +0000
236+++ include/core/dbus/impl/signal.h 2015-11-16 15:33:02 +0000
237@@ -121,8 +121,8 @@
238 &Signal<SignalDescription>::operator(),
239 this,
240 std::placeholders::_1));
241- parent->add_match(
242- rule.type(Message::Type::signal).interface(interface).member(name));
243+ rule = rule.type(Message::Type::signal).interface(interface).member(name);
244+ parent->add_match(rule);
245 }
246
247 template<typename SignalDescription, typename Argument>
248@@ -309,6 +309,8 @@
249 &Signal<SignalDescription, typename SignalDescription::ArgumentType>::operator(),
250 this,
251 std::placeholders::_1));
252+
253+ d->rule = d->rule.type(Message::Type::signal).interface(interface).member(name);
254 }
255
256 template<typename SignalDescription>
257
258=== modified file 'include/core/dbus/object.h'
259--- include/core/dbus/object.h 2014-10-23 21:09:56 +0000
260+++ include/core/dbus/object.h 2015-11-16 15:33:02 +0000
261@@ -19,12 +19,14 @@
262 #define CORE_DBUS_OBJECT_H_
263
264 #include <core/dbus/bus.h>
265+#include <core/dbus/lifetime_constrained_cache.h>
266 #include <core/dbus/service.h>
267
268 #include <functional>
269 #include <future>
270 #include <map>
271 #include <memory>
272+#include <mutex>
273 #include <ostream>
274 #include <string>
275
276@@ -82,10 +84,14 @@
277 class Object : public std::enable_shared_from_this<Object>
278 {
279 private:
280+ typedef std::tuple<types::ObjectPath, std::string, std::string> CacheKey;
281 typedef std::tuple<std::string, std::string> MethodKey;
282 typedef std::tuple<std::string, std::string> PropertyKey;
283 typedef std::tuple<std::string, std::string> SignalKey;
284
285+ template<typename PropertyDescription>
286+ static ThreadSafeLifetimeConstrainedCache<CacheKey, Property<PropertyDescription>>& property_cache();
287+
288 public:
289 typedef std::shared_ptr<Object> Ptr;
290 typedef std::function<void(const Message::Ptr&)> MethodHandler;
291@@ -173,7 +179,7 @@
292 * @param [in] path The path to associate the object with.
293 * @return An object instance or nullptr in case of errors.
294 */
295- std::shared_ptr<Object>
296+ std::shared_ptr<Object>
297 inline add_object_for_path(const types::ObjectPath& path);
298
299 /**
300@@ -227,16 +233,11 @@
301 MessageRouter<MethodKey> method_router;
302 MessageRouter<PropertyKey> get_property_router;
303 MessageRouter<PropertyKey> set_property_router;
304+ std::once_flag add_match_once;
305 std::map<
306 std::tuple<std::string, std::string>,
307 std::function<void(const types::Variant&)>
308 > property_changed_vtable;
309- std::shared_ptr<
310- Signal<
311- interfaces::Properties::Signals::PropertiesChanged,
312- interfaces::Properties::Signals::PropertiesChanged::ArgumentType
313- >
314- > signal_properties_changed;
315 };
316 }
317 }
318
319=== modified file 'include/core/dbus/property.h'
320--- include/core/dbus/property.h 2014-01-20 21:22:02 +0000
321+++ include/core/dbus/property.h 2015-11-16 15:33:02 +0000
322@@ -43,6 +43,8 @@
323 typedef typename PropertyType::ValueType ValueType;
324 typedef core::Property<ValueType> Super;
325
326+ inline ~Property();
327+
328 /**
329 * @brief Non-mutable access to the contained value.
330 * @return Non-mutable reference to the contained value.
331@@ -61,6 +63,11 @@
332 */
333 inline bool is_writable() const;
334
335+ /**
336+ * @brief Emitted during destruction of an object instance.
337+ */
338+ inline const core::Signal<void>& about_to_be_destroyed() const;
339+
340 protected:
341 friend class Object;
342
343@@ -82,6 +89,7 @@
344 std::string interface;
345 std::string name;
346 bool writable;
347+ core::Signal<void> signal_about_to_be_destroyed;
348 };
349 }
350 }
351
352=== modified file 'src/core/dbus/service.cpp'
353--- src/core/dbus/service.cpp 2014-06-30 04:57:08 +0000
354+++ src/core/dbus/service.cpp 2015-11-16 15:33:02 +0000
355@@ -79,7 +79,7 @@
356
357 void Service::remove_match(const MatchRule& rule)
358 {
359- connection->remove_match(rule);
360+ connection->remove_match(rule.sender(name));
361 }
362
363 Service::Service(const Bus::Ptr& connection, const std::string& name)

Subscribers

People subscribed via source and target branches