GNOME Bugzilla – Bug 665737
acquire/release gdk threads lock on incoming dbus
Last modified: 2011-12-17 17:58:01 UTC
GtkApplication has a rather sordid relationship with threads-enabled GDK. The next instalment in the story comes when we consider action invocations in gdk-threads-enabled programs. Probably most people would expect to be able to invoke actions within their own program without thinking about the gdk lock. Similarly, most people would probably expect to write action invocation handlers without acquiring the gdk lock before touching gtk. Indeed, we have quite a lot of code like this already (for binding GActionGroup into GtkMenus, etc). On the other side of things, we sometimes have action invocations arriving over GDBus which takes a very different view of concurrency -- one based on main contexts. "Who is allowed to touch GDK" and "the default main context" are pretty much the same thing in non-gdk-threads-enabled programs, so we're OK. In gdk-threads-enabled programs, however, we can have people expecting to be able to touch GDK from outside of the default main context (ie: from another thread, after acquiring the lock). There are a couple of possible ways of dealing with this issue: 1) kill the entire concept of gdk threads 2) tell people that if they want to use gdk threads then they have to acquire the gdk thread lock when handling any action invocations. We would probably also need to take care to do this in our own internal use of actions for binding to Gtk widgetry. 3) use the before_emit() and after_emit() vfuncs in GtkApplication to acquire and release the gdk threads lock around all incoming actions -- probably leading to excessive locking 4) something else?
Note that aligning gdkthreads to the GDBus view of the world (ie: contexts are king) would simply involve following a rather well-established existing pattern/best-practice: modifications of the UI must be dispatched back to the default main context.
It is 2) if you ask me; for backwards compatibility, if nothing else. 'aligning gdk to the gdbus view of the world' sounds like a gtk4 topic.
just to note that being able to touch GDK/GTK data structures from different threads by acquiring the Big GDK Lock™ in the critical sections is something that is really supported on X11 only — trying to do the same on Windows (and I assume on Quartz as well) will yield undefined results, i.e. most likely blow up in your face. I'd assume for gtk4 we ought to really kill the whole concept of GDK threads, and just say that we don't support using GDK/GTK from anything that is not the default main context.
Looking into this in some more detail, the concrete problem here is that we emit the e.g. GAction::activate sometimes outside the gdk lock (when the activation comes in via the bus) and sometimes inside (when clicking on a menuitem inside the app). So, there is no way to do the right thing here, locking-wise for application authors.
This is why I proposed #3: that way actions coming in from the bus would also hold the lock.
I've tried this, but it appears that window actions don't get the before/after_emit treatment, currently.
Created attachment 203548 [details] [review] GDBusActionGroup: add static platform registration We provide a mechanism by which a 'platform' (eg: Gtk) can register some hook functions to be called to collect platform-data at the point of sending an outgoing action activation request and also to inform the platform of this data on incoming requests (before and after dispatching the actual request). This can be used for forwarding timestamp and startup-notification information (as is presently done in GApplication) but the before/after hook could also be used for acquiring/releasing the Gdk lock or other similar things.
Not a beauty, but I guess it does the job. I find it a little disconcerting that we have the same three functions as GApplication vfuncs and duplicate them here as hooks.
I have the same reservations. The alternative, for the record, would be to introduce: gdk_threads_dbus_export_action_group() - would deal with timestamps on incoming messages - would acquire lock around action invocations and GdkThreadsGBusActionGroup - would send timestamp platform-data on action invocation we're getting back to talking about something like GContextual again if we go this way, though. either that or: g_action_group_activate_action_with_platform_data To be honest, I hate both options. The alternative would be to introduce some sort of grand per-thread globally-relevant-contexual-data scheme in glib, and that's pretty far out of scope here. I think I therefore prefer going with the less-visible kludge.
given that this is only going to be used by gtkapplication and the like, not by end users, we can probably live with the kludge. One improvement might be to give gapplication default implmentations of the vfuncs that call the hooks - do you think that would work ?
Almost. GApplication also invokes the hooks on 'open' and 'activate', which are not actions. Of course, GApplication itself is a GActionGroup, and the specific name of the action that is being invoked is not mentioned to the hook, so we could just pretend we're doing an action invocation in this case...
Comment on attachment 203548 [details] [review] GDBusActionGroup: add static platform registration Attachment 203548 [details] pushed as fcc9902 - GDBusActionGroup: add static platform registration GApplication and Gtk patches forthcoming.
Comment on attachment 203548 [details] [review] GDBusActionGroup: add static platform registration This ended up being too ugly to tolerate so I reverted it.
Created attachment 203735 [details] [review] introduce GRemoteActionGroup This interfaceifies the extra functions that were on GDBusActionGroup for dealing with platform data. The two main benefits of doing this: - no longer have to do a silly song and dance in GApplication to avoid calling GDBusActionGroup API from non-dbus-aware code - the interface can be reused by the action group exporter to avoid ugly and unbindable hook callbacks
Created attachment 203736 [details] [review] action group exporter: kill GApplication hackery Use the GRemoteActionGroup interface, if available, instead.
Created attachment 203737 [details] [review] GApplication: receiving end of GRemoteActionGroup Use the fact that the action group exporter now speaks to GRemoteActionGroup to get the platform data into GApplicion without hacks.
Created attachment 203738 [details] [review] GtkApplication: lock gdk on incoming messages When we have incoming activations or action invocations we should acquire the GDK lock, just in case the program in question is using gdk threads.
Created attachment 203739 [details] [review] GtkApplicationWindow: deal with remote actions Deal with remote action invocations correctly by implementing GRemoteActionGroup in the same way that GApplication does and pushing remote activations through the before/after_emit functions of the GApplication associated with the window. This is the last part of getting the threading situation right.
Attachment 203735 [details] pushed as eefd089 - introduce GRemoteActionGroup Attachment 203736 [details] pushed as 1807ef3 - action group exporter: kill GApplication hackery Attachment 203737 [details] pushed as 0971d36 - GApplication: receiving end of GRemoteActionGroup
Attachment 203738 [details] pushed as 12b2669 - GtkApplication: lock gdk on incoming messages Attachment 203739 [details] pushed as 4589ed9 - GtkApplicationWindow: deal with remote actions