Re: race condition in idle handling (#321886)

[Matthias Clasen <mclasen redhat com>]

On Tue, 2005-11-22 at 14:22 -0500, Owen Taylor wrote:
> On Tue, 2005-11-22 at 12:02 -0500, Matthias Clasen wrote:
> > The meeting is intended for the GTK+ team, but everybody is 
> > welcome to come and listen. The meeting logs will be posted 
> > on the GTK+ website (http://www.gtk.org/plan/meetings).
> > 
> >  Place: irc.gnome.org:#gtk-devel
> >  Time: 21:00 UTC (16:00 EDT), Tuesday, November 22
> > 
> > Possible agenda items:
> > - race condition in idle handling (#321886)
> 
> The second patch on the bug is buggy because it checks the destroyed
> flag on the source *without holding the corresponding lock*.
> 
> It cannot be fixed up be moving the LOCK_CONTEXT()/UNLOCK_CONTEXT(),
> because you must not run any user code, and in particular must
> not acquire the GTK+ lock while holding the main loop lock. 
> 
> You could fix it by *relocking* the context in order to check the
> destroyed flag on the source. I think the idea of 
> g_source_set_lock_funcs() is pretty ugly though ... 
> 
> You could see this bug as being a symptom of a larger problem with
> using GObject from a threaded program: the weak reference facility
> provided by g_object_weak_ref() and g_object_add_weak_pointer() 
> is inherently not thread safe - you need the operation to
> atomically:
> 
>  Checking whether the pointer is null
>  If not null, reference the object
> 
> This isn't really a big problem for this bug because we can make
> weak references (implemented here in finalize) thread safe by just
> surrounding them by the global GTK+ lock. But it might be worth
> taking a look at whether it would make sense to look at the general
> problem.
> 
> In other words, if you fixed the thread safety of 
> g_cclosure_new_object(), then you could use that and 
> g_source_set_closure() and not have to worry about removing the 
> sources in finalize.
> 
> I don't know how hard that would be ... it might be a significant
> challenge.
> 
> Regards,
> 						Owen
> 
> To throw out another random idea, you could use thread-local-data
> and have:
> 
>  g_source_is_current_destroyed()
> 

Thanks for jumping in this discussion, Owen.

Something like that is what I had in mind, actually. Add some way
for idle callbacks to check if they have already been destroyed.

> or some better name for that. Or alternatively, and somewhat 
> horribly, you could invoke idles and timeouts with a hidden extra
> parameter which was the GSource and have g_source_is_destroyed().
> 
>