[PATCH] dbus: Flush pending object manager signals in send_message

[PATCH] dbus: Flush pending object manager signals in send_message

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 5897 bytes --]

Make sure the PropertiesChanged and InterfacesAdded/Removed signals
related to one object path are sent to the dbus socket in the order
that they are generated in.
---
 ell/dbus-private.h |  2 ++
 ell/dbus-service.c | 85 +++++++++++++++++++++++++++++++++++++++---------------
 ell/dbus.c         |  5 ++++
 3 files changed, 68 insertions(+), 24 deletions(-)

diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 441e668..9ffedb2 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -229,6 +229,8 @@ bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
 					const char *interface_name,
 					const char *property_name);
 
+void _dbus_object_tree_signals_flush(struct l_dbus *dbus, const char *path);
+
 typedef void (*_dbus_name_owner_change_func_t)(const char *name,
 						uint64_t old_owner,
 						uint64_t new_owner,
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 1df8f1b..7212340 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -1024,9 +1024,29 @@ done:
 	return signal;
 }
 
-static void emit_signals(struct l_idle *idle, void *user_data)
+static bool match_interfaces_added_object(const void *a, const void *b)
+{
+	const struct interface_add_record *rec = a;
+
+	return rec->object == b || !b;
+}
+
+static bool match_interfaces_removed_object(const void *a, const void *b)
+{
+	const struct interface_remove_record *rec = a;
+
+	return rec->object == b || !b;
+}
+
+static bool match_properties_changed_object(const void *a, const void *b)
+{
+	const struct property_change_record *rec = a;
+
+	return rec->object == b || !b;
+}
+
+void _dbus_object_tree_signals_flush(struct l_dbus *dbus, const char *path)
 {
-	struct l_dbus *dbus = user_data;
 	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
 	struct interface_add_record *interfaces_added_rec;
 	struct interface_remove_record *interfaces_removed_rec;
@@ -1034,16 +1054,23 @@ static void emit_signals(struct l_idle *idle, void *user_data)
 	const struct l_queue_entry *entry;
 	struct object_manager *manager;
 	struct l_dbus_message *signal;
+	struct object_node *node = NULL;
+	bool all_done = true;
 
-	l_idle_remove(tree->emit_signals_work);
-	tree->emit_signals_work = NULL;
+	if (!tree->emit_signals_work)
+		return;
+
+	if (path)
+		node = _dbus_object_tree_lookup(tree, path);
 
 	for (entry = l_queue_get_entries(tree->object_managers); entry;
 			entry = entry->next) {
 		manager = entry->data;
 
-		while ((interfaces_removed_rec =
-				l_queue_pop_head(manager->announce_removed))) {
+		while ((interfaces_removed_rec = l_queue_remove_if(
+						manager->announce_removed,
+						match_interfaces_removed_object,
+						node))) {
 			signal = build_interfaces_removed_signal(manager,
 							interfaces_removed_rec);
 			interface_removed_record_free(interfaces_removed_rec);
@@ -1052,8 +1079,13 @@ static void emit_signals(struct l_idle *idle, void *user_data)
 				l_dbus_send(manager->dbus, signal);
 		}
 
-		while ((interfaces_added_rec =
-				l_queue_pop_head(manager->announce_added))) {
+		if (!l_queue_isempty(manager->announce_removed))
+			all_done = false;
+
+		while ((interfaces_added_rec = l_queue_remove_if(
+						manager->announce_added,
+						match_interfaces_added_object,
+						node))) {
 			signal = build_interfaces_added_signal(manager,
 							interfaces_added_rec);
 			interface_add_record_free(interfaces_added_rec);
@@ -1061,10 +1093,14 @@ static void emit_signals(struct l_idle *idle, void *user_data)
 			if (signal)
 				l_dbus_send(manager->dbus, signal);
 		}
+
+		if (!l_queue_isempty(manager->announce_added))
+			all_done = false;
 	}
 
-	while ((property_changes_rec =
-			l_queue_pop_head(tree->property_changes))) {
+	while ((property_changes_rec = l_queue_remove_if(tree->property_changes,
+						match_properties_changed_object,
+						node))) {
 		if (property_changes_rec->instance->interface->
 				handle_old_style_properties)
 			for (entry = l_queue_get_entries(property_changes_rec->
@@ -1089,6 +1125,21 @@ static void emit_signals(struct l_idle *idle, void *user_data)
 
 		property_change_record_free(property_changes_rec);
 	}
+
+	if (!l_queue_isempty(tree->property_changes))
+		all_done = false;
+
+	if (all_done) {
+		l_idle_remove(tree->emit_signals_work);
+		tree->emit_signals_work = NULL;
+	}
+}
+
+static void emit_signals(struct l_idle *idle, void *user_data)
+{
+	struct l_dbus *dbus = user_data;
+
+	_dbus_object_tree_signals_flush(dbus, NULL);
 }
 
 static void schedule_emit_signals(struct l_dbus *dbus)
@@ -1347,20 +1398,6 @@ bool _dbus_object_tree_unregister_interface(struct _dbus_object_tree *tree,
 	return true;
 }
 
-static bool match_interfaces_added_object(const void *a, const void *b)
-{
-	const struct interface_add_record *rec = a;
-
-	return rec->object == b;
-}
-
-static bool match_interfaces_removed_object(const void *a, const void *b)
-{
-	const struct interface_add_record *rec = a;
-
-	return rec->object == b;
-}
-
 bool _dbus_object_tree_add_interface(struct _dbus_object_tree *tree,
 					const char *path, const char *interface,
 					void *user_data)
diff --git a/ell/dbus.c b/ell/dbus.c
index db69c0d..59795a7 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -306,6 +306,7 @@ static uint32_t send_message(struct l_dbus *dbus, bool priority,
 {
 	struct message_callback *callback;
 	enum dbus_message_type type;
+	const char *path;
 
 	type = _dbus_message_get_type(message);
 
@@ -338,6 +339,10 @@ static uint32_t send_message(struct l_dbus *dbus, bool priority,
 		return callback->serial;
 	}
 
+	path = l_dbus_message_get_path(message);
+	if (path)
+		_dbus_object_tree_signals_flush(dbus, path);
+
 	l_queue_push_tail(dbus->message_queue, callback);
 
 	if (dbus->is_ready)
-- 
2.9.3

Re: [PATCH] dbus: Flush pending object manager signals in send_message

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

Hi Andrew,

On 03/23/2017 10:12 AM, Andrew Zaborowski wrote:
> Make sure the PropertiesChanged and InterfacesAdded/Removed signals
> related to one object path are sent to the dbus socket in the order
> that they are generated in.
> ---
>  ell/dbus-private.h |  2 ++
>  ell/dbus-service.c | 85 +++++++++++++++++++++++++++++++++++++++---------------
>  ell/dbus.c         |  5 ++++
>  3 files changed, 68 insertions(+), 24 deletions(-)
>

<snip>

>  	for (entry = l_queue_get_entries(tree->object_managers); entry;
>  			entry = entry->next) {
>  		manager = entry->data;
>
> -		while ((interfaces_removed_rec =
> -				l_queue_pop_head(manager->announce_removed))) {
> +		while ((interfaces_removed_rec = l_queue_remove_if(
> +						manager->announce_removed,
> +						match_interfaces_removed_object,
> +						node))) {

So we're going from O(n) loop here to something like O(n^2) loop.  Can 
this be made a bit smarter?  Same goes for the other loop changes below. 
  Otherwise I'm fine with the general approach.

Regards,
-Denis

Re: [PATCH] dbus: Flush pending object manager signals in send_message

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

 Hi Denis,

On 27 March 2017 at 07:07, Denis Kenzior <denkenz@gmail.com> wrote:
> On 03/23/2017 10:12 AM, Andrew Zaborowski wrote:
>>         for (entry = l_queue_get_entries(tree->object_managers); entry;
>>                         entry = entry->next) {
>>                 manager = entry->data;
>>
>> -               while ((interfaces_removed_rec =
>> -
>> l_queue_pop_head(manager->announce_removed))) {
>> +               while ((interfaces_removed_rec = l_queue_remove_if(
>> +                                               manager->announce_removed,
>> +
>> match_interfaces_removed_object,
>> +                                               node))) {
>
>
> So we're going from O(n) loop here to something like O(n^2) loop.  Can this
> be made a bit smarter?  Same goes for the other loop changes below.
> Otherwise I'm fine with the general approach.

Ok, I pondered using l_queue_foreach_remove but had opted for a smaller patch.

Best regards

[PATCH] dbus: Flush pending object manager signals in send_message

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 7581 bytes --]

Make sure the PropertiesChanged and InterfacesAdded/Removed signals
related to one object path are sent to the dbus socket in the order
that they are generated in.
---
 ell/dbus-private.h |   2 +
 ell/dbus-service.c | 160 ++++++++++++++++++++++++++++++++++++-----------------
 ell/dbus.c         |   5 ++
 3 files changed, 117 insertions(+), 50 deletions(-)

diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index 441e668..9ffedb2 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -229,6 +229,8 @@ bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
 					const char *interface_name,
 					const char *property_name);
 
+void _dbus_object_tree_signals_flush(struct l_dbus *dbus, const char *path);
+
 typedef void (*_dbus_name_owner_change_func_t)(const char *name,
 						uint64_t old_owner,
 						uint64_t new_owner,
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 1df8f1b..5ee7d27 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -132,6 +132,7 @@ struct _dbus_object_tree {
 	struct l_queue *object_managers;
 	struct l_queue *property_changes;
 	struct l_idle *emit_signals_work;
+	bool flushing;
 };
 
 void _dbus_method_introspection(struct _dbus_method *info,
@@ -1024,71 +1025,130 @@ done:
 	return signal;
 }
 
-static void emit_signals(struct l_idle *idle, void *user_data)
-{
-	struct l_dbus *dbus = user_data;
-	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
-	struct interface_add_record *interfaces_added_rec;
-	struct interface_remove_record *interfaces_removed_rec;
-	struct property_change_record *property_changes_rec;
-	const struct l_queue_entry *entry;
+struct emit_signals_data {
+	struct l_dbus *dbus;
 	struct object_manager *manager;
+	struct object_node *node;
+};
+
+static bool emit_interfaces_removed(void *data, void *user_data)
+{
+	struct interface_remove_record *rec = data;
+	struct emit_signals_data *es = user_data;
 	struct l_dbus_message *signal;
 
-	l_idle_remove(tree->emit_signals_work);
-	tree->emit_signals_work = NULL;
+	if (es->node && rec->object != es->node)
+		return false;
 
-	for (entry = l_queue_get_entries(tree->object_managers); entry;
-			entry = entry->next) {
-		manager = entry->data;
+	signal = build_interfaces_removed_signal(es->manager, rec);
+	interface_removed_record_free(rec);
 
-		while ((interfaces_removed_rec =
-				l_queue_pop_head(manager->announce_removed))) {
-			signal = build_interfaces_removed_signal(manager,
-							interfaces_removed_rec);
-			interface_removed_record_free(interfaces_removed_rec);
+	if (signal)
+		l_dbus_send(es->manager->dbus, signal);
 
-			if (signal)
-				l_dbus_send(manager->dbus, signal);
-		}
+	return true;
+}
 
-		while ((interfaces_added_rec =
-				l_queue_pop_head(manager->announce_added))) {
-			signal = build_interfaces_added_signal(manager,
-							interfaces_added_rec);
-			interface_add_record_free(interfaces_added_rec);
+static bool emit_interfaces_added(void *data, void *user_data)
+{
+	struct interface_add_record *rec = data;
+	struct emit_signals_data *es = user_data;
+	struct l_dbus_message *signal;
+
+	if (es->node && rec->object != es->node)
+		return false;
 
+	signal = build_interfaces_added_signal(es->manager, rec);
+	interface_add_record_free(rec);
+
+	if (signal)
+		l_dbus_send(es->manager->dbus, signal);
+
+	return true;
+}
+
+static bool emit_properties_changed(void *data, void *user_data)
+{
+	struct property_change_record *rec = data;
+	struct emit_signals_data *es = user_data;
+	struct l_dbus_message *signal;
+	const struct l_queue_entry *entry;
+
+	if (es->node && rec->object != es->node)
+		return false;
+
+	if (rec->instance->interface->handle_old_style_properties)
+		for (entry = l_queue_get_entries(rec->properties);
+				entry; entry = entry->next) {
+			signal = build_old_property_changed_signal(es->dbus,
+							rec, entry->data);
 			if (signal)
-				l_dbus_send(manager->dbus, signal);
+				l_dbus_send(es->dbus, signal);
 		}
+
+	if (l_queue_find(rec->object->instances, match_interface_instance,
+				L_DBUS_INTERFACE_PROPERTIES)) {
+		signal = build_properties_changed_signal(es->dbus, rec);
+		if (signal)
+			l_dbus_send(es->dbus, signal);
 	}
 
-	while ((property_changes_rec =
-			l_queue_pop_head(tree->property_changes))) {
-		if (property_changes_rec->instance->interface->
-				handle_old_style_properties)
-			for (entry = l_queue_get_entries(property_changes_rec->
-								properties);
-					entry; entry = entry->next) {
-				signal = build_old_property_changed_signal(dbus,
-							property_changes_rec,
-							entry->data);
-				if (signal)
-					l_dbus_send(dbus, signal);
-			}
+	property_change_record_free(rec);
 
+	return true;
+}
 
-		if (l_queue_find(property_changes_rec->object->instances,
-					match_interface_instance,
-					L_DBUS_INTERFACE_PROPERTIES)) {
-			signal = build_properties_changed_signal(dbus,
-							property_changes_rec);
-			if (signal)
-				l_dbus_send(dbus, signal);
-		}
+void _dbus_object_tree_signals_flush(struct l_dbus *dbus, const char *path)
+{
+	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+	const struct l_queue_entry *entry;
+	struct emit_signals_data data;
+	bool all_done = true;
+
+	if (!tree->emit_signals_work || tree->flushing)
+		return;
 
-		property_change_record_free(property_changes_rec);
+	tree->flushing = true;
+
+	data.dbus = dbus;
+	data.node = path ? _dbus_object_tree_lookup(tree, path) : NULL;
+
+	for (entry = l_queue_get_entries(tree->object_managers); entry;
+			entry = entry->next) {
+		data.manager = entry->data;
+
+		l_queue_foreach_remove(data.manager->announce_removed,
+					emit_interfaces_removed, &data);
+
+		if (!l_queue_isempty(data.manager->announce_removed))
+			all_done = false;
+
+		l_queue_foreach_remove(data.manager->announce_added,
+					emit_interfaces_added, &data);
+
+		if (!l_queue_isempty(data.manager->announce_added))
+			all_done = false;
+	}
+
+	l_queue_foreach_remove(tree->property_changes,
+				emit_properties_changed, &data);
+
+	if (!l_queue_isempty(tree->property_changes))
+		all_done = false;
+
+	if (all_done) {
+		l_idle_remove(tree->emit_signals_work);
+		tree->emit_signals_work = NULL;
 	}
+
+	tree->flushing = false;
+}
+
+static void emit_signals(struct l_idle *idle, void *user_data)
+{
+	struct l_dbus *dbus = user_data;
+
+	_dbus_object_tree_signals_flush(dbus, NULL);
 }
 
 static void schedule_emit_signals(struct l_dbus *dbus)
@@ -1356,7 +1416,7 @@ static bool match_interfaces_added_object(const void *a, const void *b)
 
 static bool match_interfaces_removed_object(const void *a, const void *b)
 {
-	const struct interface_add_record *rec = a;
+	const struct interface_remove_record *rec = a;
 
 	return rec->object == b;
 }
diff --git a/ell/dbus.c b/ell/dbus.c
index db69c0d..59795a7 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -306,6 +306,7 @@ static uint32_t send_message(struct l_dbus *dbus, bool priority,
 {
 	struct message_callback *callback;
 	enum dbus_message_type type;
+	const char *path;
 
 	type = _dbus_message_get_type(message);
 
@@ -338,6 +339,10 @@ static uint32_t send_message(struct l_dbus *dbus, bool priority,
 		return callback->serial;
 	}
 
+	path = l_dbus_message_get_path(message);
+	if (path)
+		_dbus_object_tree_signals_flush(dbus, path);
+
 	l_queue_push_tail(dbus->message_queue, callback);
 
 	if (dbus->is_ready)
-- 
2.9.3

Re: [PATCH] dbus: Flush pending object manager signals in send_message

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

Hi Andrew,

On 03/27/2017 06:23 PM, Andrew Zaborowski wrote:
> Make sure the PropertiesChanged and InterfacesAdded/Removed signals
> related to one object path are sent to the dbus socket in the order
> that they are generated in.
> ---
>  ell/dbus-private.h |   2 +
>  ell/dbus-service.c | 160 ++++++++++++++++++++++++++++++++++++-----------------
>  ell/dbus.c         |   5 ++
>  3 files changed, 117 insertions(+), 50 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH] dbus: Flush pending object manager signals in send_message

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3238 bytes --]

Hi Denis,

On 20 March 2017 at 19:17, Denis Kenzior <denkenz@gmail.com> wrote:
>> In all cases it can be worked around by clients like in the Scan case.
>> We can fix it on the iwd side just for this one case but I think
>> instead we should have a policy that tells clients if they can rely on
>> the order of the signals or not.  If not then the test utilities
>> should deal with this.
>
>
> In general the stated policy is that the client cannot rely on the order of
> signals.  However, in certain cases it is extremely advantageous for us to
> implement a logical order and stick to it.  This has the effect that a
> client with decent D-Bus bindings will 'just work'.

Then doing the same thing in all the cases can't hurt either ;)

>
> Scan() is one such example.

Ok, I sent a patch trying a yet different approach, if you think that
is not what we want then I'll try just adding a public function for
Scan() to call before returning like you suggested earlier.

>
>>>
>>> I detect hand-waving :)  Can you describe specific situations where this
>>> is
>>> a concern?
>>
>>
>> The Scanning case is one but really it's same as other situations
>> related to State and other properties, where the client code
>> (including agent methods) need to take some decision based on the
>> properties.
>
>
> My original comment was about limiting the scope to a single object. E.g. if
> /foo emits a property change and then sends a method return, only property
> changed signals for /foo should be processed.  The rest can be still
> coalesced.

Ok, I sent a patch attempting this, not 100% sure if this is what you
meant though.

With this patch the MFP-Options test fails for me.  I spent some time
trying to figure out why it seemed to work with the previous patch I
sent, where all signals were flushed on any message being sent.  I
think there was still a race and it works by pure luck (but works all
the times I tried).  Either the test code needs to call Scan() again
before GetOrderedNetworks() or the scan abort callback in iwd needs to
avoid forgetting the list of networks, I'll send a patch for this.

>
> Can you see a situation where a signal order can be guaranteed across
> objects?

Do you mean where it would be needed by the client?  I guess in cases
similar to the MFP-Options test except where the code relied on the
state of the Network objects instead of calling a method like
GetOrderedNetworks.  In this case the Scan() method is on one object
but the properties that the client needs are on other objects.

>
>>
>> Fixing it just for the Scanning case probably costs us about a line
>> more or the same amount code as actually fixing it in general.
>> Dropping the coalescing code would simplify the code even more though.
>>
>
> So that's the question.  My feeling is that if we don't limit the scope,
> then the coalescing logic is more trouble than it is worth and we should
> just send the signals directly.

I think it's a little costly but it's also a nice feature because the
spec seems to expect you to send related property changes and
interface changes in one signal so we'e a better citizen by doing
this.

Best regards

Re: [PATCH] dbus: Flush pending object manager signals in send_message

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

Hi Andrew,

>>
>> errors should not affect properties.
>
> I don't see why not, a Connect error will cause State to change for
> example, could also affect some KnownNetwork properties, but there
> will be some more obvious cases.

I think in our case we emit signals after returning from the Connect 
method call and KnownNetworks doesn't have properties.  But fair enough.

>
>>
>> Your signal + InterfacesAdded example is valid, though probably never occurs
>> in practice.  I can't think of any other situation where an in-order signal
>> reception was useful.
>
> In all cases it can be worked around by clients like in the Scan case.
> We can fix it on the iwd side just for this one case but I think
> instead we should have a policy that tells clients if they can rely on
> the order of the signals or not.  If not then the test utilities
> should deal with this.

In general the stated policy is that the client cannot rely on the order 
of signals.  However, in certain cases it is extremely advantageous for 
us to implement a logical order and stick to it.  This has the effect 
that a client with decent D-Bus bindings will 'just work'.

Scan() is one such example.

>>
>> I detect hand-waving :)  Can you describe specific situations where this is
>> a concern?
>
> The Scanning case is one but really it's same as other situations
> related to State and other properties, where the client code
> (including agent methods) need to take some decision based on the
> properties.

My original comment was about limiting the scope to a single object. 
E.g. if /foo emits a property change and then sends a method return, 
only property changed signals for /foo should be processed.  The rest 
can be still coalesced.

Can you see a situation where a signal order can be guaranteed across 
objects?

>
> Fixing it just for the Scanning case probably costs us about a line
> more or the same amount code as actually fixing it in general.
> Dropping the coalescing code would simplify the code even more though.
>

So that's the question.  My feeling is that if we don't limit the scope, 
then the coalescing logic is more trouble than it is worth and we should 
just send the signals directly.

Regards,
-Denis

Re: [PATCH] dbus: Flush pending object manager signals in send_message

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2535 bytes --]

Hi Denis,

On 20 March 2017 at 15:53, Denis Kenzior <denkenz@gmail.com> wrote:
>> Not sure, I thought it would be better to guarantee the ordering on
>> the dbus socket generally for all messages and not have to worry if
>> the issue may possibly come back in the future.  I think it should
>
>
> If you're worried about every possibility that you're being this heavy
> handed, it might be simpler to just get rid of the coalescing code and
> simply always emit the property changed signal right away.  We're not saving
> much on average by having it.
>
>> affect at least method returns, signals and errors, if we want to do
>> it.  Method calls I'm not sure because services don't make any except
>> to agents, but it may be worth doing for the agent calls.
>
>
> errors should not affect properties.

I don't see why not, a Connect error will cause State to change for
example, could also affect some KnownNetwork properties, but there
will be some more obvious cases.

>
> Your signal + InterfacesAdded example is valid, though probably never occurs
> in practice.  I can't think of any other situation where an in-order signal
> reception was useful.

In all cases it can be worked around by clients like in the Scan case.
We can fix it on the iwd side just for this one case but I think
instead we should have a policy that tells clients if they can rely on
the order of the signals or not.  If not then the test utilities
should deal with this.

>
> Method calls I think we can ignore completely.
>
>>
>>>
>>> Also, this causes (potentially lots) of getter calls to be made.  Would
>>> it
>>> not be better (and safer) to only send the properties for the object
>>> which
>>> is generating the method return?
>>
>>
>> Again if we do that there will be some situations where the clients
>> will have to work around issues.  If the getter calls are an issue we
>> can probably move the other messages to idle callbacks too, relatively
>> easily.
>
>
> I detect hand-waving :)  Can you describe specific situations where this is
> a concern?

The Scanning case is one but really it's same as other situations
related to State and other properties, where the client code
(including agent methods) need to take some decision based on the
properties.

Fixing it just for the Scanning case probably costs us about a line
more or the same amount code as actually fixing it in general.
Dropping the coalescing code would simplify the code even more though.

Best regards

Re: [PATCH] dbus: Flush pending object manager signals in send_message

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1757 bytes --]

Hi Andrew,

>>> +       _dbus_object_tree_signals_flush(dbus);
>>> +
>>
>>
>> Should this only affect method returns?
>
> Not sure, I thought it would be better to guarantee the ordering on
> the dbus socket generally for all messages and not have to worry if
> the issue may possibly come back in the future.  I think it should

If you're worried about every possibility that you're being this heavy 
handed, it might be simpler to just get rid of the coalescing code and 
simply always emit the property changed signal right away.  We're not 
saving much on average by having it.

> affect at least method returns, signals and errors, if we want to do
> it.  Method calls I'm not sure because services don't make any except
> to agents, but it may be worth doing for the agent calls.

errors should not affect properties.

Your signal + InterfacesAdded example is valid, though probably never 
occurs in practice.  I can't think of any other situation where an 
in-order signal reception was useful.

Method calls I think we can ignore completely.

>
>>
>> Also, this causes (potentially lots) of getter calls to be made.  Would it
>> not be better (and safer) to only send the properties for the object which
>> is generating the method return?
>
> Again if we do that there will be some situations where the clients
> will have to work around issues.  If the getter calls are an issue we
> can probably move the other messages to idle callbacks too, relatively
> easily.

I detect hand-waving :)  Can you describe specific situations where this 
is a concern?

Again, my argument above applies.  If we're being this heavy handed, 
maybe coalescing needs to not happen in the first place.

Regards,
-Denis

Re: [PATCH] dbus: Flush pending object manager signals in send_message

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 4641 bytes --]

Hi Denis,

On 18 March 2017 at 05:05, Denis Kenzior <denkenz@gmail.com> wrote:
> On 03/17/2017 08:30 PM, Andrew Zaborowski wrote:
>>
>> Try to warrant the original ordering of PropertiesChanged and
>> InterfacesAdded/Removed signals in relation to other messages for
>> clients that rely on this ordering.  One case where this may matter
>> is when a method call causes a property value change followed by the
>> method return message in the same main loop iteration as the property
>> change.  In this case the property change would always be seen by
>> dbus after the method return because of the signals being send from
>> an idle callback.  Set a barrier for the object manager signals
>> before queuing other messages like method returns.
>
>
> Does this fix our MFP autotest failure?

It does for me, didn't expect that.

>
>
>> ---
>>  ell/dbus-private.h | 2 ++
>>  ell/dbus-service.c | 8 ++++++++
>>  ell/dbus.c         | 2 ++
>>  3 files changed, 12 insertions(+)
>>
>> diff --git a/ell/dbus-private.h b/ell/dbus-private.h
>> index a4118c6..c22b95a 100644
>> --- a/ell/dbus-private.h
>> +++ b/ell/dbus-private.h
>> @@ -234,6 +234,8 @@ bool _dbus_object_tree_property_changed(struct l_dbus
>> *dbus,
>>                                         const char *interface_name,
>>                                         const char *property_name);
>>
>> +void _dbus_object_tree_signals_flush(struct l_dbus *dbus);
>> +
>>  void _dbus_kernel_bloom_add(uint64_t filter[], size_t size, uint8_t
>> num_hash,
>>                                 const char *prefix, const char *str);
>>  void _dbus_kernel_bloom_add_parents(uint64_t filter[], size_t size,
>> diff --git a/ell/dbus-service.c b/ell/dbus-service.c
>> index 1df8f1b..f54d2dd 100644
>> --- a/ell/dbus-service.c
>> +++ b/ell/dbus-service.c
>> @@ -1101,6 +1101,14 @@ static void schedule_emit_signals(struct l_dbus
>> *dbus)
>>         tree->emit_signals_work = l_idle_create(emit_signals, dbus, NULL);
>>  }
>>
>> +void _dbus_object_tree_signals_flush(struct l_dbus *dbus)
>> +{
>> +       struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
>> +
>> +       if (tree->emit_signals_work)
>> +               emit_signals(tree->emit_signals_work, dbus);
>> +}
>> +
>>  static bool match_property_changes_instance(const void *a, const void *b)
>>  {
>>         const struct property_change_record *rec = a;
>> diff --git a/ell/dbus.c b/ell/dbus.c
>> index ddc51aa..274e4ae 100644
>> --- a/ell/dbus.c
>> +++ b/ell/dbus.c
>> @@ -346,6 +346,8 @@ static uint32_t send_message(struct l_dbus *dbus, bool
>> priority,
>>                 return callback->serial;
>>         }
>>
>> +       _dbus_object_tree_signals_flush(dbus);
>> +
>
>
> Should this only affect method returns?

Not sure, I thought it would be better to guarantee the ordering on
the dbus socket generally for all messages and not have to worry if
the issue may possibly come back in the future.  I think it should
affect at least method returns, signals and errors, if we want to do
it.  Method calls I'm not sure because services don't make any except
to agents, but it may be worth doing for the agent calls.

>
> Also, this causes (potentially lots) of getter calls to be made.  Would it
> not be better (and safer) to only send the properties for the object which
> is generating the method return?

Again if we do that there will be some situations where the clients
will have to work around issues.  If the getter calls are an issue we
can probably move the other messages to idle callbacks too, relatively
easily.

>
>>         l_queue_push_tail(dbus->message_queue, callback);
>>
>>         if (dbus->is_ready)
>>
>
> I still wonder if something like l_dbus_property_changed_immediate would be
> better?

Not sure.  Imagine a situation where an interface is added and it
emits a custom signal in the same cycle.  Due to the coalescing logic
I think we will emit the InterfaceAdded after the signal.  I think we
should try a general fix.

>
> Also, it looks like our property setters generate the method return, then
> generate the property changed.  These are probably okay as they are.  At
> least less critical since the caller should assume the property change has
> occurred, but just thought I'd mention it in case you think they need to be
> affected by this change.

Good point, I think we should actually switch the order in
properties_set_complete / set_property_complete if we want to let
clients rely on things like the order of the Scaning change and the
Scan() call.

Best regards

Re: [PATCH] dbus: Flush pending object manager signals in send_message

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3150 bytes --]

On 03/17/2017 08:30 PM, Andrew Zaborowski wrote:
> Try to warrant the original ordering of PropertiesChanged and
> InterfacesAdded/Removed signals in relation to other messages for
> clients that rely on this ordering.  One case where this may matter
> is when a method call causes a property value change followed by the
> method return message in the same main loop iteration as the property
> change.  In this case the property change would always be seen by
> dbus after the method return because of the signals being send from
> an idle callback.  Set a barrier for the object manager signals
> before queuing other messages like method returns.

Does this fix our MFP autotest failure?

> ---
>  ell/dbus-private.h | 2 ++
>  ell/dbus-service.c | 8 ++++++++
>  ell/dbus.c         | 2 ++
>  3 files changed, 12 insertions(+)
>
> diff --git a/ell/dbus-private.h b/ell/dbus-private.h
> index a4118c6..c22b95a 100644
> --- a/ell/dbus-private.h
> +++ b/ell/dbus-private.h
> @@ -234,6 +234,8 @@ bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
>  					const char *interface_name,
>  					const char *property_name);
>
> +void _dbus_object_tree_signals_flush(struct l_dbus *dbus);
> +
>  void _dbus_kernel_bloom_add(uint64_t filter[], size_t size, uint8_t num_hash,
>  				const char *prefix, const char *str);
>  void _dbus_kernel_bloom_add_parents(uint64_t filter[], size_t size,
> diff --git a/ell/dbus-service.c b/ell/dbus-service.c
> index 1df8f1b..f54d2dd 100644
> --- a/ell/dbus-service.c
> +++ b/ell/dbus-service.c
> @@ -1101,6 +1101,14 @@ static void schedule_emit_signals(struct l_dbus *dbus)
>  	tree->emit_signals_work = l_idle_create(emit_signals, dbus, NULL);
>  }
>
> +void _dbus_object_tree_signals_flush(struct l_dbus *dbus)
> +{
> +	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
> +
> +	if (tree->emit_signals_work)
> +		emit_signals(tree->emit_signals_work, dbus);
> +}
> +
>  static bool match_property_changes_instance(const void *a, const void *b)
>  {
>  	const struct property_change_record *rec = a;
> diff --git a/ell/dbus.c b/ell/dbus.c
> index ddc51aa..274e4ae 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
> @@ -346,6 +346,8 @@ static uint32_t send_message(struct l_dbus *dbus, bool priority,
>  		return callback->serial;
>  	}
>
> +	_dbus_object_tree_signals_flush(dbus);
> +

Should this only affect method returns?

Also, this causes (potentially lots) of getter calls to be made.  Would 
it not be better (and safer) to only send the properties for the object 
which is generating the method return?

>  	l_queue_push_tail(dbus->message_queue, callback);
>
>  	if (dbus->is_ready)
>

I still wonder if something like l_dbus_property_changed_immediate would 
be better?

Also, it looks like our property setters generate the method return, 
then generate the property changed.  These are probably okay as they 
are.  At least less critical since the caller should assume the property 
change has occurred, but just thought I'd mention it in case you think 
they need to be affected by this change.

Regards,
-Denis

[PATCH] dbus: Flush pending object manager signals in send_message

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

Try to warrant the original ordering of PropertiesChanged and
InterfacesAdded/Removed signals in relation to other messages for
clients that rely on this ordering.  One case where this may matter
is when a method call causes a property value change followed by the
method return message in the same main loop iteration as the property
change.  In this case the property change would always be seen by
dbus after the method return because of the signals being send from
an idle callback.  Set a barrier for the object manager signals
before queuing other messages like method returns.
---
 ell/dbus-private.h | 2 ++
 ell/dbus-service.c | 8 ++++++++
 ell/dbus.c         | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/ell/dbus-private.h b/ell/dbus-private.h
index a4118c6..c22b95a 100644
--- a/ell/dbus-private.h
+++ b/ell/dbus-private.h
@@ -234,6 +234,8 @@ bool _dbus_object_tree_property_changed(struct l_dbus *dbus,
 					const char *interface_name,
 					const char *property_name);
 
+void _dbus_object_tree_signals_flush(struct l_dbus *dbus);
+
 void _dbus_kernel_bloom_add(uint64_t filter[], size_t size, uint8_t num_hash,
 				const char *prefix, const char *str);
 void _dbus_kernel_bloom_add_parents(uint64_t filter[], size_t size,
diff --git a/ell/dbus-service.c b/ell/dbus-service.c
index 1df8f1b..f54d2dd 100644
--- a/ell/dbus-service.c
+++ b/ell/dbus-service.c
@@ -1101,6 +1101,14 @@ static void schedule_emit_signals(struct l_dbus *dbus)
 	tree->emit_signals_work = l_idle_create(emit_signals, dbus, NULL);
 }
 
+void _dbus_object_tree_signals_flush(struct l_dbus *dbus)
+{
+	struct _dbus_object_tree *tree = _dbus_get_tree(dbus);
+
+	if (tree->emit_signals_work)
+		emit_signals(tree->emit_signals_work, dbus);
+}
+
 static bool match_property_changes_instance(const void *a, const void *b)
 {
 	const struct property_change_record *rec = a;
diff --git a/ell/dbus.c b/ell/dbus.c
index ddc51aa..274e4ae 100644
--- a/ell/dbus.c
+++ b/ell/dbus.c
@@ -346,6 +346,8 @@ static uint32_t send_message(struct l_dbus *dbus, bool priority,
 		return callback->serial;
 	}
 
+	_dbus_object_tree_signals_flush(dbus);
+
 	l_queue_push_tail(dbus->message_queue, callback);
 
 	if (dbus->is_ready)
-- 
2.9.3