Comment 17 for bug 1738838

Revision history for this message
In , BenjaminBerg (benjamin-sipsolutions) wrote :

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.