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

Submitted by Jonathon Jongsma on April 2, 2015, 4:15 p.m.

Details

Message ID 1427991351.3967.110.camel@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Jonathon Jongsma April 2, 2015, 4:15 p.m.
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@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:


====================


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.

But it also seems to introduce a regression that I haven't investigated yet:
when things work properly (e.g. only a single fullscreen display is shown) and
I follow these steps:
 - exit fullscreen
 - enable a second display (View > Displays > Display 2)

The second window appears, but it simply displays "Waiting for display 2..."
forever. The display only shows starts working after you resize one of the
windows.

Patch hide | download patch | download mbox

===================

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);

Comments

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@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
On Thu, 2015-04-09 at 14:06 -0500, Jonathon Jongsma wrote:
> 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@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


So, I've got another potential solution for this issue that's entirely
done on the client side. See this thread on virt-tools-list:
https://www.redhat.com/archives/virt-tools-list/2015-April/msg00128.html

Comments appreciated, but should probably be sent to virt-tools-list.

Jonathon