Re: nasty bug in multithreaded g_main_loop...

[Owen Taylor <otaylor redhat com>]

Tim Janik <timj@gtk.org> writes:

> On 16 Jan 1999, Owen Taylor wrote:
> 
> > b) We destroy sources within the main loop lock. This
> >    is nasty because we will end up calling the destroy
> >    notifies for the user data within the main lock
> >    too.
> > 
> >    So, considering how that destroy-notify is used
> >    in many language bindings (reference counting)
> >    you could easily end up with the destructor for
> >    some Perl object (for example) being called within 
> >    the main loop lock. 
> > 
> >    And by that time, expecting the application programmer
> >    to understand the consequences is a pretty slim
> >    chance. (Note that making the main loop lock recursive 
> >    does not remove all possible problems here)
> > 
> >    But avoiding this is not easy, since you first
> >    have to disentagle the GHook out of the GHookList,
> >    unlock, then destroy the hook. This definitely
> >    would require modifications to the GHook API's.
> > 
> >    (Hmmm, I suppose the cheat is to do all destroy
> >    notifications from an idle function... (not really
> >    a serious suggestion))
> 
> i think the answer to this problem is pretty obvious, you need to call
> the hook destroy notifiers without the main loop lock being held.

Well, the idea had occured to me ... but it's not completely
trivial. (See below)

> the only thing that's lacking in the g_hook API to achive this is a
> destroy function marshaller, which would actually be pretty consistent
> to add to GHookList, since we already feature marshallers for every
> other operational bit of the g_hook API.
> unfortunately this will require a binary incompatible change ala:
> 
>  struct _GHookList
>  {
>    guint          seq_id;
>    guint          hook_size;
>    guint          is_setup : 1;
>    GHook         *hooks;
>    GMemChunk     *hook_memchunk; 
>    GHookFreeFunc  hook_free; /* virtual function */
> +  GHookFreeFunc  hook_destroy; /* virtual function */
>  };
> 
> but i think that this is reasonably justified and we need to increase
> GLib's version number anyways.

I recently moved the call to the source's destroy function
into the hook_free function. This was done to deal with
a problem with the timeout dispatch function:

====
static gboolean
g_timeout_dispatch (gpointer source_data, 
		    GTimeVal *current_time,
		    gpointer user_data)
{
  GTimeoutData *data = source_data;

  if (data->callback(user_data))
    {
      guint seconds = data->interval / 1000;
      guint msecs = data->interval - seconds * 1000;

      data->expiration.tv_sec = current_time->tv_sec + seconds;
      data->expiration.tv_usec = current_time->tv_usec + msecs * 1000;
      if (data->expiration.tv_usec >= 1000000)
	{
	  data->expiration.tv_usec -= 1000000;
	  data->expiration.tv_sec++;
	}
      return TRUE;
    }
  else
    return FALSE;
}
====

If, during the callback, the source is destroyed and
that calls the source's destroy function, then by
the time we get down to setting up the next timeout
we are accesing freed data. To move things back
so that we destroy the source when g_hook_destroy_link() 
is called, we need to change the GSource API so
that the dispatch function can check if it was
destroyed in the callback - we need to change:

  gboolean (*dispatch) (gpointer  source_data, 
			GTimeVal *current_time,
			gpointer  user_data);

to:

  gboolean (*dispatch) (GSource  *source,
			GTimeVal *current_time,
			gpointer  user_data);

and probably make GSource non-opaque.


We could also maintain the status quo and destroy
the source in the free function. But if we do it that
way, then we have to account for the fact that
any call to g_hook_unref() can also cause the
main loop lock to be released temporarily.


In either case, the thread-safety of the main
loop algorithm becomes rather harder to analyze
since we are potentially unlocking at more places.
But it is a worthwhile goal, since holding locks 
while calling user code is unpleasant.

Regards,
                                        Owen