Re: strange deadlock ...

[Owen Taylor <otaylor redhat com>]

Michael Meeks <michael ximian com> writes:

> On 17 Jul 2001, Owen Taylor wrote:
> > > 	On the contrary, it will cripple anyone using threads and Gtk+ eg.
> > > try adding a g_thread_init to gtk-demo, and then pressing "Dialog and
> > > Message Boxes" and then any button [ to cause a re-entering of the glib
> > > mainloop through gtk_dialog_run ].
> > 
> > But this is just a bug in gtk_dialog_run. gtk_dialog_run() knows that
> > if g_thread_init() has called an the application isn't buggy, that it
> > holds the GTK+ lock, so it should do:
> 
> 	I beg to differ.
> 
> >   ri.loop = g_main_new (FALSE);
> > 
> > +  GDK_THREADS_LEAVE ()
> > 
> >   g_main_loop_run (ri.loop);
> > 
> > -  GDK_THREADS_ENTER ()
> 
> 	Assuming you meant '+' not '-' here, or it's truly broken.

Yes, of course.
 
> > The problem is that in the case of Bonobo usage, you have know idea
> > whether the GDK lock is held or not.
> 
> 	No - both cases are the same. You can't be sure what is on the
> stack when you are called, it is true that in the case I mention that the
> dialog_run is hit via. the handling of a gdk_event [1] and thus the lock
> is held.
> 
> 	But, there is no reason why this should be so, if we put the
> dialog on a timeout, or if we do gtk_dialog_run inside our 'main'
> function, then we will not have the GDK lock when we hit here, and then we
> will try and drop the lock ... wow, amazing you can do:

Look at the GTK+ FAQ. The rules are pretty simple, if not
perhaps obvious:

If you call g_thread_init(), then:

 - All calls to GTK+ or GDK must be within a 
   gdK_threads_enter() / gdK_thread_leaves() pair.

 - The GDK lock is held on entry for signals, but not for timeout or
   idle functions, which are run directly from the GLib
   main loop.

So, if you are going to call GTK+ functions from a timout, you
must put a gdk_threads_enter()/leave() pair around it.

> 	So - of course, perhaps throwing up dialogs on a timeout is
> broken, perhaps in fact the solution to all locking problem is:
> 
> 	if (!(was_locked = peek_lock_locked))
> 		lock_lock;
> 
> 	do magic
> 	
> 	if (!was_locked)
> 		unlock_lock;

This type of thing makes me quite uncomfortable in a general
sense:

 - Locks must have a proper (partial) ordering.
 - If you don't know what locks you have at any given time,
   you can't tell if your locks are ordered or not.

Plus, you add a lot a significant amount of complexity and
expense to locking and unlocking. (I'm not sure even 
how to properly implement peek_lock_locked(), though I suspect
it is probably possible.) 

For GTK+ I'd much rather stick to the current set of deterministic
rules. The one obvious improvement would be to separate out
whether GDK/GTK+ is in multi-threaded mode from whether 
g_thread_init() has been called to make something like
gnome-vfs easier to use.

I gave someone (Ettore?) a hack to do this for GTK+-1.2 
which should still work for GTK+-2.0, but perhaps we need
to have a clean way to do this.

> 	Alternatively - perhaps the GDK_THREADS_ENTER / LEAVE scoping
> needs re-thinking. Whilst I know nothing about the issues whatsoever I
> can't resist suggesting a solution:
> 
> 	Move the GTK_THREADS locks down to where they are actualy useful,
> and ensure that they are not held for long periods of time as a matter of
> course. So, eg.
> 
> static gboolean  
> gdk_event_dispatch (GSource    *source,
> 		    GSourceFunc callback,
> 		    gpointer    user_data)
> {
>   GdkEvent *event;
>  
>   GDK_THREADS_ENTER ();
> 
>   gdk_events_queue();
>   event = gdk_event_unqueue();
> 
> +  GDK_THREADS_LEAVE ();
> 
>   if (event)
>     {
>       if (gdk_event_func)
> 	(*gdk_event_func) (event, gdk_event_data);
>       
> +      GDK_THREADS_ENTER ();
> 
>       gdk_event_free (event);
> 
> +      GDK_THREADS_LEAVE ();
>     }
>   
> -      GDK_THREADS_LEAVE ();
> 
>   return TRUE;
> }
> 
> 	Then in gtk_main_do_event we can do some more lock taking and
> releasing - of course this will not have a pleasant performance impact. So
> perhaps instead - just release the lock before we do the switch
> event->type here, and take it again afterwards.

Unless you are saying that we should go completely to never holding
the GTK+ lock over user code, I don't see what that buys us at
all. 

And trying to move all locking internal to GTK+ is Hard. 

 * Because so much goes through signals, you wouldn't be
   able to hold the lock for more than 5 lines of code or
   so at a time. And 

 * There is no clear distinction between the public and private
   APIs - every time a public function called another public
   function it would  

In fact, I think it is infeasible. What probably is feasible
is moving to the GLib model -- you could have multiple threads
using multiple toplevels, but if you wanted to have two threads
working on the same toplevel, you have to do your own locking. 

But feasible there means "6 months of work for someone."
 
> 	And again, I know little about good practice with locking, but
> things like this:
> 
>   if (g_main_is_running (main_loops->data))
>     {
>       GDK_THREADS_LEAVE ();
>       g_main_run (loop);
>       GDK_THREADS_ENTER ();
>       gdk_flush ();
>     }
> 
> 	Looks like a recepie for disaster. 

Why? 
 
> 	Then again, it is entirely possible that I've totaly misunderstood
> any or all of Gtk+, Gdk, threads, locking, desirable granularity, the
> penalty of lock taking, what a deadlock is etc. etc. so please be patient
> with me.

Perhaps :-) I'm not exceedingly happy with the way things work, but it
was thought out, and if you follow the rules, there is good evidence
that things do work.

                                        Owen