[vd_agent_linux,v2] vdagent: Stop trying to connect to the daemon after a while

Submitted by Frediano Ziglio on Nov. 19, 2018, 10:23 a.m.

Details

Message ID 20181119102322.7252-1-fziglio@redhat.com
State New
Headers show
Series "vdagent: Stop trying to connect to the daemon after a while" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Nov. 19, 2018, 10:23 a.m.
Do not try  indefinitely to connect to the daemon, should not
take long to activate.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 src/vdagent/vdagent.c | 6 ++++++
 1 file changed, 6 insertions(+)

Changes since v1:
- log error when we reach the limit

Patch hide | download patch | download mbox

diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
index f7c8b72..7a58150 100644
--- a/src/vdagent/vdagent.c
+++ b/src/vdagent/vdagent.c
@@ -53,6 +53,7 @@  typedef struct VDAgent {
     struct vdagent_file_xfers *xfers;
     struct udscs_connection *conn;
     GIOChannel *x11_channel;
+    guint connection_attempts;
 
     GMainLoop *loop;
 } VDAgent;
@@ -370,6 +371,11 @@  static gboolean vdagent_init_async_cb(gpointer user_data)
                                 daemon_read_complete, daemon_disconnect_cb,
                                 debug);
     if (agent->conn == NULL) {
+        // limit connection attempts, this will try for 5 minutes
+        if (++agent->connection_attempts > 5 * 60) {
+            syslog(LOG_ERR, "Attempted to contact daemon for 5 minutes, giving up");
+            goto err_init;
+        }
         g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
         return G_SOURCE_REMOVE;
     }

Comments

On Mon, 2018-11-19 at 10:23 +0000, Frediano Ziglio wrote:
> Do not try  indefinitely to connect to the daemon, should not
> take long to activate.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  src/vdagent/vdagent.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Changes since v1:
> - log error when we reach the limit
> 
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index f7c8b72..7a58150 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -53,6 +53,7 @@ typedef struct VDAgent {
>      struct vdagent_file_xfers *xfers;
>      struct udscs_connection *conn;
>      GIOChannel *x11_channel;
> +    guint connection_attempts;
>  
>      GMainLoop *loop;
>  } VDAgent;
> @@ -370,6 +371,11 @@ static gboolean vdagent_init_async_cb(gpointer user_data)
>                                  daemon_read_complete, daemon_disconnect_cb,
>                                  debug);
>      if (agent->conn == NULL) {
> +        // limit connection attempts, this will try for 5 minutes
> +        if (++agent->connection_attempts > 5 * 60) {
> +            syslog(LOG_ERR, "Attempted to contact daemon for 5 minutes, giving up");
> +            goto err_init;
> +        }

(in reference to trying to keep it simple from v1 thread:)
I think using 60 instead of 1 in g_timeout_add_seconds() wouldn't be
much of a complication... unless there's something else I'm missing.
I'm also thinking this is being launched from a .desktop file so AFAIK
there is no easy way for a user to restart the service, a relogin is
needed?

I also just realized you should reset connection_attempts to 0 after a
successful connect?

Cheers,
Lukas

>          g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
>          return G_SOURCE_REMOVE;
>      }
Hi,

On Mon, Nov 19, 2018 at 10:23:22AM +0000, Frediano Ziglio wrote:
> Do not try  indefinitely to connect to the daemon, should not
> take long to activate.

Why?

> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  src/vdagent/vdagent.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Changes since v1:
> - log error when we reach the limit
> 
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index f7c8b72..7a58150 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -53,6 +53,7 @@ typedef struct VDAgent {
>      struct vdagent_file_xfers *xfers;
>      struct udscs_connection *conn;
>      GIOChannel *x11_channel;
> +    guint connection_attempts;
>  
>      GMainLoop *loop;
>  } VDAgent;
> @@ -370,6 +371,11 @@ static gboolean vdagent_init_async_cb(gpointer user_data)
>                                  daemon_read_complete, daemon_disconnect_cb,
>                                  debug);
>      if (agent->conn == NULL) {
> +        // limit connection attempts, this will try for 5 minutes
> +        if (++agent->connection_attempts > 5 * 60) {
> +            syslog(LOG_ERR, "Attempted to contact daemon for 5 minutes, giving up");
> +            goto err_init;
> +        }
>          g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
>          return G_SOURCE_REMOVE;
>      }
> -- 
> 2.17.2
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> On Mon, 2018-11-19 at 10:23 +0000, Frediano Ziglio wrote:
> > Do not try  indefinitely to connect to the daemon, should not
> > take long to activate.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  src/vdagent/vdagent.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > Changes since v1:
> > - log error when we reach the limit
> > 
> > diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> > index f7c8b72..7a58150 100644
> > --- a/src/vdagent/vdagent.c
> > +++ b/src/vdagent/vdagent.c
> > @@ -53,6 +53,7 @@ typedef struct VDAgent {
> >      struct vdagent_file_xfers *xfers;
> >      struct udscs_connection *conn;
> >      GIOChannel *x11_channel;
> > +    guint connection_attempts;
> >  
> >      GMainLoop *loop;
> >  } VDAgent;
> > @@ -370,6 +371,11 @@ static gboolean vdagent_init_async_cb(gpointer
> > user_data)
> >                                  daemon_read_complete,
> >                                  daemon_disconnect_cb,
> >                                  debug);
> >      if (agent->conn == NULL) {
> > +        // limit connection attempts, this will try for 5 minutes
> > +        if (++agent->connection_attempts > 5 * 60) {
> > +            syslog(LOG_ERR, "Attempted to contact daemon for 5 minutes,
> > giving up");
> > +            goto err_init;
> > +        }
> 
> (in reference to trying to keep it simple from v1 thread:)
> I think using 60 instead of 1 in g_timeout_add_seconds() wouldn't be
> much of a complication... unless there's something else I'm missing.
> I'm also thinking this is being launched from a .desktop file so AFAIK
> there is no easy way for a user to restart the service, a relogin is
> needed?
> 

It reconnects to the new daemon after detecting the disconnection.
Not sure what will happen if the agent is updated... I think nothing
or possibly the agent will end if not compatible with the new daemon.
If you retry every minute is possible that the user has to wait 1
minute instead of 1 second.
Probably the optimal way would be to use inotify on the socket to
wait for the new daemon, so something like:
- try 10 times with 1 second delay
- setup inotify
- try once to avoid races
- wait inotify
- attempt connection again (repeat from first step)
This way if the daemon is restored after even hours you don't have to
keep polling.

> I also just realized you should reset connection_attempts to 0 after a
> successful connect?
> 

The object is created again so will be 0 again (not that would hurt
to reset on a successful connection).

> Cheers,
> Lukas
> 
> >          g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
> >          return G_SOURCE_REMOVE;
> >      }
>
On Mon, 2018-11-19 at 11:42 -0500, Frediano Ziglio wrote:
> > 
> > On Mon, 2018-11-19 at 10:23 +0000, Frediano Ziglio wrote:
> > > Do not try  indefinitely to connect to the daemon, should not
> > > take long to activate.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > ---
> > >  src/vdagent/vdagent.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > Changes since v1:
> > > - log error when we reach the limit
> > > 
> > > diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> > > index f7c8b72..7a58150 100644
> > > --- a/src/vdagent/vdagent.c
> > > +++ b/src/vdagent/vdagent.c
> > > @@ -53,6 +53,7 @@ typedef struct VDAgent {
> > >      struct vdagent_file_xfers *xfers;
> > >      struct udscs_connection *conn;
> > >      GIOChannel *x11_channel;
> > > +    guint connection_attempts;
> > >  
> > >      GMainLoop *loop;
> > >  } VDAgent;
> > > @@ -370,6 +371,11 @@ static gboolean vdagent_init_async_cb(gpointer
> > > user_data)
> > >                                  daemon_read_complete,
> > >                                  daemon_disconnect_cb,
> > >                                  debug);
> > >      if (agent->conn == NULL) {
> > > +        // limit connection attempts, this will try for 5 minutes
> > > +        if (++agent->connection_attempts > 5 * 60) {
> > > +            syslog(LOG_ERR, "Attempted to contact daemon for 5 minutes,
> > > giving up");
> > > +            goto err_init;
> > > +        }
> > 
> > (in reference to trying to keep it simple from v1 thread:)
> > I think using 60 instead of 1 in g_timeout_add_seconds() wouldn't be
> > much of a complication... unless there's something else I'm missing.
> > I'm also thinking this is being launched from a .desktop file so AFAIK
> > there is no easy way for a user to restart the service, a relogin is
> > needed?
> > 
> 
> It reconnects to the new daemon after detecting the disconnection.
> Not sure what will happen if the agent is updated... I think nothing
> or possibly the agent will end if not compatible with the new daemon.
> If you retry every minute is possible that the user has to wait 1
> minute instead of 1 second.
> Probably the optimal way would be to use inotify on the socket to
> wait for the new daemon, so something like:
> - try 10 times with 1 second delay
> - setup inotify
> - try once to avoid races
> - wait inotify
> - attempt connection again (repeat from first step)
> This way if the daemon is restored after even hours you don't have to
> keep polling.

Well, you wanted a simple solution and I agree with that. I suggested
something that keeps the agent running and doesn't really make it more
complicated. Now you're suggesting a much more complex solution... :)

> > I also just realized you should reset connection_attempts to 0 after a
> > successful connect?
> > 
> 
> The object is created again so will be 0 again (not that would hurt
> to reset on a successful connection).

Right, didn't notice that...

Cheers,
Lukas

> > Cheers,
> > Lukas
> > 
> > >          g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
> > >          return G_SOURCE_REMOVE;
> > >      }
> On Mon, 2018-11-19 at 11:42 -0500, Frediano Ziglio wrote:
> > > 
> > > On Mon, 2018-11-19 at 10:23 +0000, Frediano Ziglio wrote:
> > > > Do not try  indefinitely to connect to the daemon, should not
> > > > take long to activate.
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > > ---
> > > >  src/vdagent/vdagent.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > Changes since v1:
> > > > - log error when we reach the limit
> > > > 
> > > > diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> > > > index f7c8b72..7a58150 100644
> > > > --- a/src/vdagent/vdagent.c
> > > > +++ b/src/vdagent/vdagent.c
> > > > @@ -53,6 +53,7 @@ typedef struct VDAgent {
> > > >      struct vdagent_file_xfers *xfers;
> > > >      struct udscs_connection *conn;
> > > >      GIOChannel *x11_channel;
> > > > +    guint connection_attempts;
> > > >  
> > > >      GMainLoop *loop;
> > > >  } VDAgent;
> > > > @@ -370,6 +371,11 @@ static gboolean vdagent_init_async_cb(gpointer
> > > > user_data)
> > > >                                  daemon_read_complete,
> > > >                                  daemon_disconnect_cb,
> > > >                                  debug);
> > > >      if (agent->conn == NULL) {
> > > > +        // limit connection attempts, this will try for 5 minutes
> > > > +        if (++agent->connection_attempts > 5 * 60) {
> > > > +            syslog(LOG_ERR, "Attempted to contact daemon for 5
> > > > minutes,
> > > > giving up");
> > > > +            goto err_init;
> > > > +        }
> > > 
> > > (in reference to trying to keep it simple from v1 thread:)
> > > I think using 60 instead of 1 in g_timeout_add_seconds() wouldn't be
> > > much of a complication... unless there's something else I'm missing.
> > > I'm also thinking this is being launched from a .desktop file so AFAIK
> > > there is no easy way for a user to restart the service, a relogin is
> > > needed?
> > > 
> > 
> > It reconnects to the new daemon after detecting the disconnection.
> > Not sure what will happen if the agent is updated... I think nothing
> > or possibly the agent will end if not compatible with the new daemon.
> > If you retry every minute is possible that the user has to wait 1
> > minute instead of 1 second.
> > Probably the optimal way would be to use inotify on the socket to
> > wait for the new daemon, so something like:
> > - try 10 times with 1 second delay
> > - setup inotify
> > - try once to avoid races
> > - wait inotify
> > - attempt connection again (repeat from first step)
> > This way if the daemon is restored after even hours you don't have to
> > keep polling.
> 
> Well, you wanted a simple solution and I agree with that. I suggested
> something that keeps the agent running and doesn't really make it more
> complicated. Now you're suggesting a much more complex solution... :)
> 

Well... it all started with Victor comment "Why?". The feature that if the
daemon is resurrected the agent will be active in a second could be
valuable. Not that I like processes keeping polling all the time.
I though systemd or whatever program doing socket activation would reuse
the same socket, just keeping looking for new connections if the daemon
is not active but it seems that (at least systemd) create a new socket
(so the i-node and the directory changes and can be monitored). In case
the daemon is launched stand-alone (not with socket activation) it has
to recreate the socket. I'm not aware or other possible socket activator
so I assume is safe to check the i-node change.

On the other hand people can ask "why keeping the agents polling/monitoring
forever?". Maybe is just my paranoia that I don't like processes polling
for nothing and daemon should just be running and people fix that condition.

> > > I also just realized you should reset connection_attempts to 0 after a
> > > successful connect?
> > > 
> > 
> > The object is created again so will be 0 again (not that would hurt
> > to reset on a successful connection).
> 
> Right, didn't notice that...
> 
> Cheers,
> Lukas
> 
> > > Cheers,
> > > Lukas
> > > 
> > > >          g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
> > > >          return G_SOURCE_REMOVE;
> > > >      }
>