GNOME Bugzilla – Bug 656039
race condition between GDBusProxy signals and public API
Last modified: 2011-08-15 17:00:06 UTC
While trying to write a test-case for Bug #651133, I discovered some more race conditions: * the mutable property g-name-owner can be changed by NameOwnerChanged in the proxy's main-context's thread, at the same time that someone's using it (e.g. to cal a method) in a thread * the mutable properties g-default-timeout and g-interface-info can be changed at any time by public API I believe I've fixed both of these, by extending the existing properties_lock to cover them. One thing that I *haven't* fixed is that g_dbus_proxy_get_interface_info returns a borrowed pointer to the g-interface-info, which can be invalidated at any time via g_dbus_proxy_set_interface_info() (which might release the last ref). Thoughts on how to solve that?
Looking at my changes again, they're not working as well as I'd hoped, so no patch yet...
Created attachment 193322 [details] [review] [WIP] GDBusProxy: hold properties_lock while using any mutable property This changes the meaning of "properties_lock" from "lock for D-Bus properties" to "lock for GObject properties". Bug: https://bugzilla.gnome.org/show_bug.cgi?id=656039 Bug-NB: NB#259760 --- This is work-in-progress, and might not be right. /gdbus/codegen/object-manager seems to hang; I haven't checked whether that's a regression.
Created attachment 193323 [details] [review] [WIP] Add a stress-test for GDBusProxy in threads with no default main context Destroying a GDBusProxy in a thread used to race with NameOwnerChanged being delivered to the main context's thread (GNOME #651133). Also, g_dbus_proxy_call_sync in a thread would race with NameOwnerChanged being delivered to the main context's thread and rewriting the name_owner (GNOME #656039). Bug: https://bugzilla.gnome.org/show_bug.cgi?id=656039 Bug-NB: NB#259760 --- The current number of threads (10) and (proxy creation, call, unref) sets (50) are probably both a bit high to be running this by default; it started as an attempt to reproduce Bug #651133, which is a race with quite an unlikely condition.
Created attachment 193435 [details] [review] [PATCH 1/4] g_dbus_proxy_get_property: use accessors for all mutable state These ought to have thread-locking, and having it in the accessor seems better than duplicating it here.
Created attachment 193436 [details] [review] [PATCH 2/4] GDBusProxy: factor out async_init_data_set_name_owner This removes the need for async_init_get_name_owner_cb to cope with being called without a real GAsyncResult, and will simplify the addition of correct thread-locking. In async_init_data_set_name_owner, use the name_owner parameter instead of the corresponding member of GDBusProxyPrivate, partly to reduce pointer-chasing but mainly to avoid needing to hold the lock.
Created attachment 193437 [details] [review] [PATCH 3/4] GDBusProxy: hold properties_lock while using any mutable property This changes the meaning of "properties_lock" from "lock for D-Bus properties" to "lock for GObject properties". The most common problem, and the only one I've reproduced in a regression test, is name_owner, which can be updated by the thread that owns the GDBusProxy's main context (i.e. the thread-default main context of the thread that constructed it) at the same time that a blocking call is made. When a GDBusProxy is constructed in a thread-pool thread for short-term use, the main context will typically be the global default main context (which is actively running in the main thread!), making this extremely problematic. The interface info is perhaps a theoretical concern - one thread could conceivably set it at the same time that another thread uses it, but only in relatively pathological situations. The current API for this does have the problem that it returns a borrowed ref, but interface info is hopefully permanent anyway. The default timeout is probably only a theoretical concern - it's just an int, so writes are indivisible, and there's no worry about whether something has been freed - but to be safe, let's hold the lock for that too. --- Changes since WIP patch: I'm now more careful to avoid holding the lock around calls into GDBusConnection (even asynchronous ones) and calls out to user code (mainly signal emissions), which fixed a deadlock in the gdbus-codegen test.
Created attachment 193438 [details] [review] [PATCH 4/4] Add a stress-test for GDBusProxy in threads with no default main context Destroying a GDBusProxy in a thread used to race with NameOwnerChanged being delivered to the main context's thread (GNOME #651133). Also, g_dbus_proxy_call_sync in a thread would race with NameOwnerChanged being delivered to the main context's thread and rewriting the name_owner (GNOME #656039).
Created attachment 193555 [details] [review] [4/4 v2] improved stress-test for GDBusProxy This version of the stress-test doesn't queue up more than 50 request/release pairs at a time, making it finish in finite time (without calls eventually timing out) on slower CPUs. It also respects "-m slow" to do a more thorough test. In testing on a 1GHz ARM device, the "-m slow" mode crashed with older GLib 5/5 times, and passed with patched GLib 5/5 times, taking about a minute. The default mode takes around 30 seconds. On a 1.8 GHz Core 2 Duo, slow mode takes around 15 seconds and the default mode takes around 9. It could be sped up (at a thoroughness cost) by reducing N_REPEATS and N_RAPID_CYCLES.
Patches, test including, seems OK. No reviewer hat, though.
Review of attachment 193437 [details] [review]: ::: gio/gdbusproxy.c @@ +2346,3 @@ + G_UNLOCK (properties_lock); + /* FIXME: returning a borrowed ref with no guarantee that nobody will + * call g_dbus_proxy_set_interface_info() and make it invalid... */ Pet peeve: I prefer the */ on the next line, lined up with the other *'s
Created attachment 193877 [details] [review] [3/4 v2] GDBusProxy: hold properties_lock while using any mutable property > Pet peeve: I prefer the */ on the next line, lined up with the other *'s If you insist... Any thoughts on the implementation?
Created attachment 193878 [details] [review] [4/4 v3] improved stress-test, with adjusted comments Cosmetic changes only.
Comment on attachment 193435 [details] [review] [PATCH 1/4] g_dbus_proxy_get_property: use accessors for all mutable state Committed, thanks!
Comment on attachment 193436 [details] [review] [PATCH 2/4] GDBusProxy: factor out async_init_data_set_name_owner Committed, thanks!
Comment on attachment 193877 [details] [review] [3/4 v2] GDBusProxy: hold properties_lock while using any mutable property Committed, thanks!
Comment on attachment 193878 [details] [review] [4/4 v3] improved stress-test, with adjusted comments Committed - thanks for writing a test case - your attention to detail is appreciated!