[Spice-devel] [PATCH spice-server 16/16] red-worker: Start processing commands as soon as possible

[Christophe Fergeau]

On Tue, Sep 12, 2017 at 03:37:43AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Sep 11, 2017 at 04:03:18AM -0400, Frediano Ziglio wrote:
> > > ping
> > > 
> > > > 
> > > > When the worker is started it could take a while to start
> > > > processing commands.
> > > > The reason is that the dispatcher handler is called after
> > > > the worker so GLib will receive a FALSE answer to both
> > > > prepare and check callbacks of the RedWorkerSource causing
> > > > GLib to wait till another event is received.
> > 
> > So we may have messages from the main thread to the worker thread
> > delayed until another (non-dispatching related) event wakes up the glib
> > main loop? Or do you mean something else?
> > 
> 
> Time to write some documentation on terminology.
> Both shortlog and message refers to commands, the main thread talk
> to the worker thread through messages, not commands.
> 
> By the way, yes, quite complicated, I'll add
> 
> "Commands (from QXL interface for cursor and display) are processed
> during the RedWorkerSource dispatch so if they are not processed
> just when the VM is started they will be processed on next event
> which could be from dispatcher (main thread requests), from 
> existing connections or from pending timers. However in the case there
> are no clients connected and no other requests from main thread the
> worker thread won't process them."

Better with the expanded log :)
Acked-by: Christophe Fergeau <cfergeau at redhat.com>

> 
> > If that's correct, maybe makes this a bit more explicit in the log, and
> > Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> > 
> > 
> > 
> > > > This is a regression since the introduction of GLib event
> > > > loop, before the command processing was always attempted
> > > > after any events.
> > > > Setting the event_timeout to 0 cause the prepare callback
> > > > to return TRUE so GLib will dispatch the RedWorkerSource.
> > > > This was discovered attempting to use the tests in server/tests
> > > > directory to reproduce a leak in RedWorker.
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  server/red-worker.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > > index 594dec55..c02674ba 100644
> > > > --- a/server/red-worker.c
> > > > +++ b/server/red-worker.c
> > > > @@ -635,6 +635,7 @@ static void handle_dev_start(void *opaque, void
> > > > *payload)
> > > >          display_channel_wait_for_migrate_data(worker->display_channel);
> > > >      }
> > > >      worker->running = TRUE;
> > > > +    worker->event_timeout = 0;
> > > >      guest_set_client_capabilities(worker);
> > > >  }
> > > >