The patch might be hard to read as it changes all the property accessors, but really it all boils down to the two questions:
* Is it correct to use GDBusObjectManagerClient to keep track of the objects?
* Is it good to use GObject properties on the Adapter1/Device1 proxies (instead of manually driving a DBus properties interface proxy)?
I agree that dbusmock test would be nice in here. But I personally think adding that testing infrastructure is out of the scope for me here. In this particular case I would even say that it would be an over specific test to catch a case of synchronous API abuse.
The race is actually surprisingly obvious. The calls to *_proxy_new_for_bus_sync cause the mainloop to run which means DBus messages are processed too early:
* InterfacesAdded is fired by bluez
* Properties change notification for "Powered" is fired by bluez
* adapter_added runs mainloop for adapter1_proxy_new_for_bus_sync
- at least dbus calls for name resolving happen
* adapter_added runs mainloop for properties_proxy_new_for_bus_sync
- at least dbus calls for name resolving happen
* Properties notification has been lost while running those mainloops
* adapter_added uses cached property values from InterfacesAdded method call
* adapter_added connects to "g-properties-changed" on the proxy
The improvements from bug #701399 made this race condition worse.
With the patch, the code becomes:
* InterfaceAdded is fired on bluez
* Properties change notification for "Powered" is fired by bluez
* GLib creates the Adapter1 proxy object
* glib sends notification about the new object
* connect to Adapter1 happens
* Properties are queried from Adapter1 object through GObject
There simply is no scope for a race condition as connecting/querying happens in an atomic fashion.
--
Note that there is quite a lot of cleanup potential. In particular BLUETOOTH_COLUMN_PROPERTIES and creation of the properties proxy can be removed if this doesn't constitute an API/ABI break.
The patch might be hard to read as it changes all the property accessors, but really it all boils down to the two questions: gerClient to keep track of the objects?
* Is it correct to use GDBusObjectMana
* Is it good to use GObject properties on the Adapter1/Device1 proxies (instead of manually driving a DBus properties interface proxy)?
I agree that dbusmock test would be nice in here. But I personally think adding that testing infrastructure is out of the scope for me here. In this particular case I would even say that it would be an over specific test to catch a case of synchronous API abuse.
The race is actually surprisingly obvious. The calls to *_proxy_ new_for_ bus_sync cause the mainloop to run which means DBus messages are processed too early: proxy_new_ for_bus_ sync proxy_new_ for_bus_ sync changed" on the proxy
* InterfacesAdded is fired by bluez
* Properties change notification for "Powered" is fired by bluez
* adapter_added runs mainloop for adapter1_
- at least dbus calls for name resolving happen
* adapter_added runs mainloop for properties_
- at least dbus calls for name resolving happen
* Properties notification has been lost while running those mainloops
* adapter_added uses cached property values from InterfacesAdded method call
* adapter_added connects to "g-properties-
The improvements from bug #701399 made this race condition worse.
With the patch, the code becomes:
* InterfaceAdded is fired on bluez
* Properties change notification for "Powered" is fired by bluez
* GLib creates the Adapter1 proxy object
* glib sends notification about the new object
* connect to Adapter1 happens
* Properties are queried from Adapter1 object through GObject
There simply is no scope for a race condition as connecting/querying happens in an atomic fashion.
--
Note that there is quite a lot of cleanup potential. In particular BLUETOOTH_ COLUMN_ PROPERTIES and creation of the properties proxy can be removed if this doesn't constitute an API/ABI break.