[Spice-devel] [PATCH 5/9] worker: do not use default context for timers

[Frediano Ziglio]

> 
> > 
> > > 
> > > On Wed, 2015-12-09 at 12:17 +0000, Frediano Ziglio wrote:
> > > > This avoid to use different thread for events.
> > > > All worker event should be attached to worker's context.
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  server/red-worker.c | 27 +++++++++++++++++++--------
> > > >  1 file changed, 19 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > > index a57301c..91a116e 100644
> > > > --- a/server/red-worker.c
> > > > +++ b/server/red-worker.c
> > > > @@ -521,7 +521,7 @@ static int
> > > > common_channel_config_socket(RedChannelClient
> > > > *rcc)
> > > >  typedef struct SpiceTimer {
> > > >      SpiceTimerFunc func;
> > > >      void *opaque;
> > > > -    guint source_id;
> > > > +    GSource *source;
> > > >  } SpiceTimer;
> > > >  
> > > >  static SpiceTimer* worker_timer_add(SpiceTimerFunc func, void *opaque)
> > > > @@ -538,7 +538,6 @@ static gboolean worker_timer_func(gpointer
> > > > user_data)
> > > >  {
> > > >      SpiceTimer *timer = user_data;
> > > >  
> > > > -    timer->source_id = 0;
> > > >      timer->func(timer->opaque);
> > > >      /* timer might be free after func(), don't touch */
> > > >  
> > > > @@ -547,18 +546,30 @@ static gboolean worker_timer_func(gpointer
> > > > user_data)
> > > >  
> > > >  static void worker_timer_cancel(SpiceTimer *timer)
> > > >  {
> > > > -    if (timer->source_id == 0)
> > > > -        return;
> > > > -
> > > > -    g_source_remove(timer->source_id);
> > > > -    timer->source_id = 0;
> > > > +    if (timer->source) {
> > > > +        g_source_destroy(timer->source);
> > > > +        timer->source = NULL;
> > > > +    }
> > > >  }
> > > >  
> > > >  static void worker_timer_start(SpiceTimer *timer, uint32_t ms)
> > > >  {
> > > >      worker_timer_cancel(timer);
> > > >  
> > > > -    timer->source_id = g_timeout_add(ms, worker_timer_func, timer);
> > > > +    RedChannelClient *rcc = timer->opaque;
> > > > +    RedWorker *worker;
> > > > +
> > > > +    spice_assert(rcc != NULL);
> > > > +    /* Since we are called from red_channel_client_create()
> > > > +       CommonChannelClient->worker has not been set yet! */
> > > > +    worker = SPICE_CONTAINEROF(rcc->channel, CommonChannel,
> > > > base)->worker;
> > > > +
> > > > +    timer->source = g_timeout_source_new(ms);
> > > > +    spice_return_if_fail(timer->source != NULL);
> > > > +
> > > > +    g_source_set_callback(timer->source, worker_timer_func, timer,
> > > > NULL);
> > > > +
> > > > +    g_source_attach(timer->source, worker->main_context);
> > > >  }
> > > >  
> > > >  static void worker_timer_remove(SpiceTimer *timer)
> > > 
> > > 
> > > Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> > > 
> > 
> > Probably it makes sense to merge in the loop one
> > 
> > Frediano
> > 
> > Note: this patch is part of changes we'll merge next year
> 
> Some though about this patch.
> 
> I was surprised this problem was not discovered before. It basically make all
> timers run in another thread causing a lot of possible synchronization
> errors.
> I put some debug printf and surprise surprise timers are executed really
> rarely.
> I also noted that our tests use the default loop hiding this issue.
> 
> I would then do:
> - add a unit test for loop code, this is an important part of the code
>   and should be correctly tested; Probably will require some code move;
> - do not use default main loop/context in tests.
> 

Looks like tests have their reasons.
I started removing main context usage from tests and already discovered
some leaks and memory issue on the code.
Also looks like basic_event_loop.c and Glib loop code in new loop code
are very similar. So similar I think is worst having a template.

Frediano