Re: Changes to the GLib main loop

[Sebastian Wilhelmi <wilhelmi ira uka de>]

Owen Taylor wrote:
> 
> Sebastian Wilhelmi <wilhelmi ira uka de> writes:
>
> > > 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.

Ok, will you do it, then?

Bye,
Sebastian
-- 
Sebastian Wilhelmi
mailto:wilhelmi ira uka de
http://goethe.ira.uka.de/~wilhelmi