GNOME Bugzilla – Bug 782530
Race condition in property querying
Last modified: 2018-02-21 23:42:13 UTC
Because of the method that the dbus proxies are created there is currently a race condition. When an adapter is added it can have an initial "Powered" state of off which is turned on immediately afterwards. However, the signal connect to get the update only happens later. This can be easily reproduced on thinkpads by running "rfkill block 0" and "rfkill unblock 0" while looking at the gnome-control-center bluetooth panel. Attaching a patch that should fix the issue. I hope I didn't totally misunderstand how the whole DBus proxy stuff is supposed to work.
Created attachment 351663 [details] [review] lib: Rework adapter and device proxy registrations. This fixes a race condition when a property was updated right after the object appeared on the bus. Due to the old implementation the property change would not be registered in that case. This moves the code to use GDBusObjectManagerClient and using the provided GObject properties instead of a separate proxy to avoid the race condition.
Review of attachment 351663 [details] [review]: The patch is unreviewable. Make the hunks bigger and try to split up the changes if at all possible.
Created attachment 351966 [details] [review] lib: Rework adapter and device proxy registrations. This fixes a race condition when a property was updated right after the object appeared on the bus. Due to the old implementation the property change would not be registered in that case. This moves the code to use GDBusObjectManagerClient and using the provided GObject properties instead of a separate proxy to avoid the race condition.
Created attachment 351967 [details] [review] lib: Always send "default-adapter-powered" Fix sending of default-adapter-powered in the case that the default adapter is being disabled.
Created attachment 351968 [details] [review] lib: Force custom icon code override to run The property setter currently contains some magic to modify the icon for certain device types. Send a property change notification to force this code to run after a device has been added.
There, that is all logical changes split out.
I won't have time to review this in the short-term, but a dbusmock regression test which shows the problem before, and shows it fixed afterwards would be really useful. See those in gnome-settings-daemon (though they've been broken for a while for different reasons...): https://git.gnome.org//browse/gnome-settings-daemon/tree/tests/gsdtestcase.py https://git.gnome.org//browse/gnome-settings-daemon/tree/plugins/power/test.py
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.
Created attachment 363618 [details] [review] lib: Remove unneeded include Nothing is using the code in the generated bluetooth-fdo-glue.h header.
Created attachment 363619 [details] [review] lib: Remove never used FDO_PROPERTIES_INTERFACE constant It was added as a constant during the BlueZ 5 port (commit 0b05349) but was never used.
Created attachment 363620 [details] [review] lib: Indent and document gtk_tree_store_new() call Makes it easier to see which column has which type.
Created attachment 363621 [details] [review] lib: Use GDBusObjectManager Instead of our home-made ObjectManager proxy. Note that this is a minimal port and will require more cleaning up to be up to standards. Loosely based on patch by Benjamin Berg <bberg@redhat.com>
Created attachment 363622 [details] [review] lib: Remove creation of intermediate GDBusProxies We already have what we need created by the GDBusObjectManager.
Created attachment 363623 [details] [review] lib: Simplify Properties setting Stop setting properties on adapters or devices by making D-Bus calls ourselves, and rely on wrapped GObject properties instead. For the calls we're making, the only change will be the error message coming from the generated wrapper, rather than being our own custom ones. We weren't really able to recover from errors in the past anyway, so this shouldn't make the overall experience any worse. This also fixes a possible hang on exit in the Bluetooth panel. See https://bugzilla.gnome.org/show_bug.cgi?id=789654
Created attachment 363624 [details] [review] lib: Remove last users of "Properties" D-Bus wrapper As those property objects were not used anywhere anymore.
Created attachment 363625 [details] [review] lib: Remove fd.o Properties API glue As we removed the last users of that code.
Created attachment 363626 [details] [review] lib: Fix possible race when creating adapters or devices When new D-Bus objects appear, listen to changes to their properties as soon as possible to avoid missing out on events. This can happen if a synchronous D-Bus call is made between the device appearing, and we start listening to changes. The window for that race is smaller now that we avoid creating GDBusProxies and fetching properties synchronously when new adapters or devices appear, but this close that window altogether.
Created attachment 363627 [details] [review] lib: Always send "default-adapter-powered" Fix sending of default-adapter-powered in the case that the default adapter is being disabled.
Attachment 363618 [details] pushed as b5a6455 - lib: Remove unneeded include Attachment 363619 [details] pushed as 899eb7d - lib: Remove never used FDO_PROPERTIES_INTERFACE constant Attachment 363620 [details] pushed as 215acf6 - lib: Indent and document gtk_tree_store_new() call Attachment 363621 [details] pushed as 17983ac - lib: Use GDBusObjectManager Attachment 363622 [details] pushed as 9319001 - lib: Remove creation of intermediate GDBusProxies Attachment 363623 [details] pushed as 27e88fc - lib: Simplify Properties setting Attachment 363624 [details] pushed as 61f5ad5 - lib: Remove last users of "Properties" D-Bus wrapper Attachment 363625 [details] pushed as afccb57 - lib: Remove fd.o Properties API glue Attachment 363626 [details] pushed as 9c6d93a - lib: Fix possible race when creating adapters or devices Attachment 363627 [details] pushed as dbe815a - lib: Always send "default-adapter-powered"
Which leaves us with attachment 351968 [details] [review]. I have trouble understanding in which case this could happen now. Can you file a new bug if this is still needed?
Oh, right. Attachment 351968 [details] was just my way of to not duplicate the icon override logic.
*** Bug 775376 has been marked as a duplicate of this bug. ***