Re: Changes to the GLib main loop

[Owen Taylor <otaylor redhat com>]

Sebastian Wilhelmi <wilhelmi ira uka de> writes:

> > > There are 3 possibilities:
> > >
> > > 1. Drop GMainWaiter and cond/mutex from g_main_context_wait.
> > > It still is possible to use g_main_context for your own main loops
> > > even after that, I think. That would just be the solution, you
> > > disaproved of in the first place. [I know, why you added that
> > > feature, such that external main loops can use their own condition
> > > variables to be notified, so they can notify themselfs, which
> > > wouldn't be possible otherwise, I'm not sure that is of big
> > > use, but it might be]
> > 
> > Dropping the cond/mutex is, as far as I can see, impossible.
> > 
> > To reliably wait on a condition, you must check the condition
> > with the mutex locked... but we do not, and should not expose the
> > main context's condition through the public API.
> 
> Ah, I see. But actually it might be better to expose the mutex (as in
> GAsyncQueue). Now we have a deadlock problem:
> 
> g_mutex_lock(external_mutex)
> g_main_loop_wait (context, external_cond, external_mutex)
>   /* This does g_mutex_lock(context->mutex) */
> 
> and in another thread
> 
> g_main_loop_release (context)
>   /* This does g_mutex_lock(context->mutex) and then
>      g_mutex_lock(external_mutex) */
> 
> So we could end up in dead-lock. Yes, I know, it would bloat the API, but in
> the end it might be the best solution to add _lock and _unlock calls and
> require the wait call to be done inside those two. We do not need to export
> other _unlocked variants, even though it might make implementing own main
> loops easier.

Ugh, I'm quite opposed to this. If we expose the locking to the outside
world, then we are simply passing off the problem of avoiding deadlocks
to our users. 

And, you can't simply use the g_main_context_lock() function around
where you call g_main_context_wait() -- you also need to use it 
whenever you modify the "state" that you are waiting on with the
condition variable, and expose a g_main_context_broadcast() as well...

I think we need to just bite the complexity bullet and fix this
deadlock ourselves. The basic scheme that comes to mind is something
like:

[ g_main_context_release ]

	  if (loop_internal_waiter)
	    g_cond_signal (waiter->cond);
	  else
	    {
	      waiter->signaling = TRUE;
	      UNLOCK_CONTEXT (context);
	      
	      g_mutex_lock (waiter->mutex);
	      g_cond_signal (waiter->cond);
	      g_mutex_unlock (waiter->mutex);

	      LOCK_CONTEXT (context);
	      waiter->signaling = FALSE;
              g_cond_broadcast (context->waiter_signaling_cond);
	    }

[ g_main_context_wait ]

      if (!loop_internal_waiter)
	UNLOCK_CONTEXT (context);
      g_cond_wait (cond, mutex);
      if (!loop_internal_waiter)
        {
	  LOCK_CONTEXT (context);
          while (waiter->signaling) 
            g_cond_wait (context->waiter_signaling_cond);
        }
 
The reason why we need the signaling flag is that if we don't have it,
we have no guarantee that the caller won't free the condition variable
before we call g_cond_signal() on it.

> > > 2. If you want to retain that, then we should at least special case
> > > the normal case of the context cond/mutex combination by just
> > > setting a flag and not adding that cond to the list multiple times.
> > 
> > I don't see a big win in efficiency here.
> 
> You save a lot, I think, see attached patch. Yes I know, this is not fair
> anymore, because we wake up GLib main loops first.

I don't think it is as much as all that ... all you save is really 
a call to g_slist_delete_link(). The expense of g_cond_signal() is
enough that you don't want to be hitting this code path very
often anways.

Fairness is not a huge deal ... as long as someone is iterating
the loop, it doesn't really matter who. However, I think it is
better to have one mechanism, than two mechanisms.

Regards,
                                          Owen
  
> @@ -1714,19 +1715,15 @@
>      {
>        context->owner = NULL;
>  
> -      if (context->waiters)
> +      if (context->internal_waiters)
>  	{
> +	  g_cond_signal (context->cond);
> +	}
> +      else if (context->waiters)
> +	{
>  	  GMainWaiter *waiter = context->waiters->data;
> -	  gboolean loop_internal_waiter =
> -	    (waiter->mutex == g_static_mutex_get_mutex (&context->mutex));
> -	  context->waiters = g_slist_delete_link (context->waiters,
> -						  context->waiters);
> -	  if (!loop_internal_waiter)
>  	    g_mutex_lock (waiter->mutex);
> -	  
>  	  g_cond_signal (waiter->cond);
> -	  
> -	  if (!loop_internal_waiter)
>  	    g_mutex_unlock (waiter->mutex);
>  	}
>      }