GNOME Bugzilla – Bug 661992
GDBusConnection should be thread-safe, and documented as such
Last modified: 2011-10-24 10:12:33 UTC
+++ This bug was initially created as a clone of Bug #661689 +++ On the original bug, I wrote: > This is non-trivial to fix because the locking discipline is a bit unclear. Am > I right in thinking that, as a consequence of the g_bus_get() family returning > a singleton, GDBusConnection is expected to be fully thread-safe (all public > methods may be called from arbitrary threads), an exception to the usual rule > that you can only use a given GObject from one thread at a time? davidz replied: > That is correct, GDBusConnection is supposed to be 100% thread-safe. Its thread-safety should be documented and verified, with annotations on methods to indicate which methods are called from which threads and need which locks. I'm working on some patches.
Depends on (at least part of) Bug #661689 (currently the first 2 commits), which make the initialized flag atomic, and use it as a memory barrier to access some other stuff.
Created attachment 199653 [details] [review] Annotate GDBusConnection private functions with thread/lock status The thread shared between all GDBusWorker instances was variously called the "worker thread" or "message handler thread", which I mostly changed to "the GDBusWorker thread" to avoid ambiguity.
Created attachment 199654 [details] [review] GDBusConnection: access the exit-on-close flag atomically This isn't strictly necessary, because in every location where it's checked, if the reading thread misses an update from another thread, it's indistinguishable from the reading thread having been scheduled before the writing thread, which is an unavoidable race condition that callers need to cope with anyway. On the other hand, merging exit_on_close into atomic_flags gives the least astonishing semantics to library users and saves 4 bytes of struct, and if you're accessing exit-on-close often enough for it to be a performance concern, you're probably doing it wrong.
Created attachment 199655 [details] [review] GDBusConnection: document which properties are protected by @lock Also, a few that don't need to be.
Created attachment 199657 [details] [review] GDBusConnection: make the closed flag atomic (but still lock to write) Strictly speaking, neither of the two uses that aren't under the lock *needs* to be atomic, but it seems better to be obviously correct (and we save another 4 bytes of struct). One of these uses is in g_dbus_connection_is_closed(), any use of which is inherently a race condition anyway. The other is g_dbus_connection_flush_sync, which as far as I can tell just needs a best-effort check, to not waste effort on a connection that has been closed for a while (but I could be wrong). I removed the check for the closed flag altogether in g_dbus_connection_send_message_with_reply_unlocked, because it turns out to be redundant with one in g_dbus_connection_send_message_unlocked, which is called immediately after. g_dbus_connection_close_sync held the lock to check the closed flag, which is no longer needed. As far as I can tell, the only reason why the lock is still desirable when setting the closed flag is so that remove_match_rule can't fail by racing with close notification from the worker thread - but on_worker_closed needs to hold the lock anyway, to deal with other data structures, so there's no point in trying to eliminate the requirement to hold the lock.
Created attachment 199658 [details] [review] GDBusConnection: document that this object is (meant to be) thread-safe
As far as I can tell, I did the important bits of this over on Bug #661689, so none of this needs to go to stable branches. (The patches here are against 2.30, but they apply fine to master, with a trivial conflict fix in the first one.)
Review of attachment 199653 [details] [review]: Looks good to me. Thanks.
Review of attachment 199654 [details] [review]: OK.
Review of attachment 199655 [details] [review]: Yup.
Review of attachment 199657 [details] [review]: Good point about the g_critical() if RemoveMatch() fails.
Review of attachment 199658 [details] [review]: LGTM
Fixed in master for 2.31.1, commits 3a0b60647d to a124562d1 inclusive. Thanks!