[Spice-devel] [PATCH 0/2] RFC: handle startup race for monitors config

[Jonathon Jongsma]

On Thu, 2015-04-02 at 11:15 -0500, Jonathon Jongsma wrote:
> On Thu, 2015-04-02 at 16:42 +0200, Marc-André Lureau wrote:
> > 
> > On Thu, Apr 2, 2015 at 4:36 PM, Jonathon Jongsma <jjongsma at redhat.com>
> > wrote:
> >         That would be a pretty drastic change in behavior. So, no, I
> >         have not
> >         considered that. It would also open up a large can of worms.
> >         For
> >         example, what would happen if you re-configured the displays
> >         from within
> >         the guest control panel. What would you do then?
> >         
> > 
> > 
> > I would eventually follow guest configuration, but I wouldn't send
> > back a new configuration (no auto-resize).
> > 
> 
> 
> So, I did a quick test where I used an un-modified spice-server and the
> following diff to virt-viewer:
> 
> ===================
> 
> diff --git a/src/virt-viewer-session.c b/src/virt-viewer-session.c
> index 131a500..5f47d09 100644
> --- a/src/virt-viewer-session.c
> +++ b/src/virt-viewer-session.c
> @@ -412,6 +412,13 @@ virt_viewer_session_on_monitor_geometry_changed(VirtViewerSession* self,
>      if (!klass->apply_monitor_geometry)
>          return;
>  
> +    /* don't send monitor resize if we're in auto-conf fullscreen mode...*/
> +    if (virt_viewer_app_get_fullscreen(self->priv->app)) {
> +        g_debug("%s: not sending new monitors config because app is in fullscreen mode", G_STRFUNC);
> +        return;
> +    }
> +    g_debug("%s: sending", G_STRFUNC);
> +
>      /* find highest monitor ID so we can create the sparse array */
>      for (l = self->priv->displays; l; l = l->next) {
>          VirtViewerDisplay *d = VIRT_VIEWER_DISPLAY(l->data);
> 
> ====================
> 
> 
> This is basically what you suggest. While we're in auto-conf mode, it never
> sends down monitor config changes, but it does respond to display updates from
> the server. When the user exits fullscreen mode, it resumes auto-resize and
> sends new monitor config messages to the server. It improved the situation, but
> didn't fix it. Before applying this patch, when I started virt-viewer in
> --full-screen mode, the extra display was left enabled about 25-50% of the
> time. After this patch, it happens only about 10% of the time.

So here's the current state of this investigation. Just to recap, this
is (as far as I can tell) the source of the problem:

[guest has 2 displays enabled]
      * main channel connects
      * client sends monitor config (only 1 display enabled) to server
      * server sends new monitor config to guest
      * display channel connects
      * display channel sends out monitor config message with old state
        (2 displays)
      * client receives monitor config and enables 2 display windows
      * guest finishes display configuration
      * display channel sends out monitor config message with new state
        (1 display)
      * second display on client becomes "unready" ("waiting for display
        2...")
      * Some future re-allocation / etc causes the client to send down
        new monitor configuration, which re-enables the "unready" second
        display (see rhbz#868970 for why this happens and why it's very
        difficult to solve)

There are two basic approaches that can be taken here. The first is the
one that I originally proposed: handle it on the server side. The
rationale for this approach is pretty straightforward: the server
*knows* that it is sending out an invalid monitor configuration at
startup (since it knows that it has already received a new monitor
configuration from the client), and we should not send out wrong
information knowingly. The change that I originally proposed is not
particularly elegant, I'll admit. It does use a timeout to delay sending
out our initial monitor configuration to give the guest a chance to
reconfigure itself before we send the configuration. Obviously, if the
guest takes longer to reconfigure displays than the timeout, we'll still
see this issue. And a timeout inherently adds more chances for race
conditions, etc. But in my testing (with host and client on different
machines on a local network), I've never yet seen a failure with this
patch applied.

The second approach is to change the client so that it can handle the
server sending us an old configuration followed immediately by a new
configuration. However, this is not trivial, and everything I've tried
so far to handle it has opened up new regressions in other areas that
are hard to predict and require very extensive testing to even find.
It's clear that the client sends out too many unnecessary monitor
reconfiguration messages. But preventing the client from sending out
these messages is risky; it's not always immediately clear which ones
are unnecessary. One of Marc-Andre's proposed patches doing that
introduced 2 new regressions (mentioned in other places in this thread).
It also did not fully solve the bug, though it did reduce the frequency
by half or so. But even if we could manage to prevent all unnecessary
monitor configuration messages, we still have the issue of the extra
display and what to do with it (again, see rhbz#868970 for a discussion
on this issue). Marc-Andre had a promising suggestion that when we're in
fullscreen mode, we should simply follow the display state of the server
and never attempt to modify it from the client (after the very initial
configuration message). He further suggested that we could simply close
displays that were disabled by the server in this case to avoid that
display getting re-enabled accidentally. Unfortunately, after
prototyping this, I realized that displays in fullscreen mode will not
be maintained through a reboot. For example, if you had virt-viewer
fullscreen on two monitors and then rebooted the guest, one of the
monitors would simply disappear during the reboot. So I don't really
think that's a very workable solution (In fact, I think this is the very
reason that we don't automatically close displays that the server
indicates are disabled; see rhbz#868970).

So, that's where I am. The server-side solution basically works but
isn't very pretty. And I can't figure out a client-side solution that
will work for all scenarios. I'd really appreciate additional ideas or
comments.

Jonathon