After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 665737 - acquire/release gdk threads lock on incoming dbus
acquire/release gdk threads lock on incoming dbus
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Class: GtkApplication
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 665312
 
 
Reported: 2011-12-07 16:39 UTC by Allison Karlitskaya (desrt)
Modified: 2011-12-17 17:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GDBusActionGroup: add static platform registration (7.68 KB, patch)
2011-12-15 03:23 UTC, Allison Karlitskaya (desrt)
rejected Details | Review
introduce GRemoteActionGroup (30.65 KB, patch)
2011-12-17 17:54 UTC, Allison Karlitskaya (desrt)
committed Details | Review
action group exporter: kill GApplication hackery (3.45 KB, patch)
2011-12-17 17:54 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GApplication: receiving end of GRemoteActionGroup (8.83 KB, patch)
2011-12-17 17:54 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GtkApplication: lock gdk on incoming messages (1.10 KB, patch)
2011-12-17 17:54 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GtkApplicationWindow: deal with remote actions (5.08 KB, patch)
2011-12-17 17:54 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Allison Karlitskaya (desrt) 2011-12-07 16:39:58 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?
Comment 1 Allison Karlitskaya (desrt) 2011-12-07 16:42:49 UTC
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.
Comment 2 Matthias Clasen 2011-12-07 17:34:34 UTC
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.
Comment 3 Emmanuele Bassi (:ebassi) 2011-12-07 18:02:41 UTC
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.
Comment 4 Matthias Clasen 2011-12-10 21:31:07 UTC
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.
Comment 5 Allison Karlitskaya (desrt) 2011-12-12 00:05:07 UTC
This is why I proposed #3: that way actions coming in from the bus would also hold the lock.
Comment 6 Matthias Clasen 2011-12-12 11:56:32 UTC
I've tried this, but it appears that window actions don't get the before/after_emit treatment, currently.
Comment 7 Allison Karlitskaya (desrt) 2011-12-15 03:23:32 UTC
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.
Comment 8 Matthias Clasen 2011-12-15 03:37:29 UTC
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.
Comment 9 Allison Karlitskaya (desrt) 2011-12-15 18:22:35 UTC
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.
Comment 10 Matthias Clasen 2011-12-15 23:06:01 UTC
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 ?
Comment 11 Allison Karlitskaya (desrt) 2011-12-16 06:46:08 UTC
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 12 Allison Karlitskaya (desrt) 2011-12-16 15:55:12 UTC
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 13 Allison Karlitskaya (desrt) 2011-12-17 17:53:07 UTC
Comment on attachment 203548 [details] [review]
GDBusActionGroup: add static platform registration

This ended up being too ugly to tolerate so I reverted it.
Comment 14 Allison Karlitskaya (desrt) 2011-12-17 17:54:03 UTC
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
Comment 15 Allison Karlitskaya (desrt) 2011-12-17 17:54:05 UTC
Created attachment 203736 [details] [review]
action group exporter: kill GApplication hackery

Use the GRemoteActionGroup interface, if available, instead.
Comment 16 Allison Karlitskaya (desrt) 2011-12-17 17:54:07 UTC
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.
Comment 17 Allison Karlitskaya (desrt) 2011-12-17 17:54:27 UTC
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.
Comment 18 Allison Karlitskaya (desrt) 2011-12-17 17:54:29 UTC
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.
Comment 19 Allison Karlitskaya (desrt) 2011-12-17 17:57:12 UTC
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
Comment 20 Allison Karlitskaya (desrt) 2011-12-17 17:57:55 UTC
Attachment 203738 [details] pushed as 12b2669 - GtkApplication: lock gdk on incoming messages
Attachment 203739 [details] pushed as 4589ed9 - GtkApplicationWindow: deal with remote actions