Merge lp:~ted/libindicator/upstart-no-dbus into lp:libindicator/13.10

[Ted Gould]

When running under Upstart session let Upstart manage the service.

When we're running under upstart we want upstart to manage the process lifecycle for the service, not the panel-service. This patch makes it so that when we have an upstart session there isn't any dbus activation attempted. Long term we should be able to assume Upstart user session (default on Saucy) and remove the checking code, but let's keep things usable on Raring for now.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:491
http://jenkins.qa.ubuntu.com/job/libindicator-ci/21/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/8/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/7/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/21/rebuild

review: Needs Fixing (continuous-integration)

[Lars Karlitski (larsu)]

I think having the panel (or anything in the session) being aware of the init daemon is inside-out architecture.

Will every service now have to ship with an upstart script in addition to the dbus service file it already has? That adds an unnecessary dependency, and makes it harder to write services.

Why don't we make upstart aware of dbus activation? (I believe I heard it already is, or will be in the near future?) This will bring us all the benefits of upstart managing the services without any extra work.

Are there any advantages to using upstart explicitly?

[Ted Gould (ted)]

On Wed, 2013-05-29 at 15:51 +0000, Lars Uebernickel wrote:

> I think having the panel (or anything in the session) being aware of
> the init daemon is inside-out architecture.

It's not the init daemon in this case. It's the session manager. It's
like being aware that there is a gnome-session.

> Will every service now have to ship with an upstart script in addition
> to the dbus service file it already has? That adds an unnecessary
> dependency, and makes it harder to write services.

No, there'll be no reason to ship a dbus service file.

> Why don't we make upstart aware of dbus activation? (I believe I heard
> it already is, or will be in the near future?) This will bring us all
> the benefits of upstart managing the services without any extra work.
>
> Are there any advantages to using upstart explicitly?

There was a patch for that, it was rejected upstream, we may put it back
in Ubuntu. But, even with that using Upstart jobs is better for
indicators. It keeps the log files in a single file that is
automatically picked up by apport and attached to bugs. Upstart can
restart the service including retry intervals. And for developers they
can manage when the service stops and starts with external tools that
already exist.

[Lars Karlitski (larsu)]

> On Wed, 2013-05-29 at 15:51 +0000, Lars Uebernickel wrote:
>
> > I think having the panel (or anything in the session) being aware of
> > the init daemon is inside-out architecture.
>
> It's not the init daemon in this case. It's the session manager. It's
> like being aware that there is a gnome-session.

Right. But I don't see why it should be aware of the session service, either.

> > Will every service now have to ship with an upstart script in addition
> > to the dbus service file it already has? That adds an unnecessary
> > dependency, and makes it harder to write services.
>
> No, there'll be no reason to ship a dbus service file.
>
> > Why don't we make upstart aware of dbus activation? (I believe I heard
> > it already is, or will be in the near future?) This will bring us all
> > the benefits of upstart managing the services without any extra work.
> >
> > Are there any advantages to using upstart explicitly?
>
> There was a patch for that, it was rejected upstream, we may put it back
> in Ubuntu. But, even with that using Upstart jobs is better for
> indicators. It keeps the log files in a single file that is
> automatically picked up by apport and attached to bugs. Upstart can
> restart the service including retry intervals. And for developers they
> can manage when the service stops and starts with external tools that
> already exist.

All of these things will be true anyway, because upstart *is* managing the services.

[Ted Gould (ted)]

On Wed, 2013-05-29 at 16:18 +0000, Lars Uebernickel wrote:

> All of these things will be true anyway, because upstart *is* managing
> the services.

I guess, but I'm not sure why you're concerned over the job
configuration having:

  start on dbus-activation com.canonical.indicator.network

versus

  start on indicators-loaded

It seems like the second one is clearer and can be managed more easily
as we'll be able to introspect the startup with things like initctl2dot.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:491
http://jenkins.qa.ubuntu.com/job/libindicator-ci/23/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/9

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/23/rebuild

review: Approve (continuous-integration)

[lp:~ted/libindicator/upstart-no-dbus]

492. By Ted Gould on 2013-05-29

Don't make the service manager do DBus activation in the Upstart session case.

493. By Ted Gould on 2013-05-29

Merging trunk

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:493
http://jenkins.qa.ubuntu.com/job/libindicator-ci/24/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/11
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/10

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/24/rebuild

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

> On Wed, 2013-05-29 at 16:18 +0000, Lars Uebernickel wrote:
>
> > All of these things will be true anyway, because upstart *is* managing
> > the services.
>
>
> I guess, but I'm not sure why you're concerned over the job
> configuration having:
>
> start on dbus-activation com.canonical.indicator.network
>
> versus
>
> start on indicators-loaded
>
> It seems like the second one is clearer and can be managed more easily
> as we'll be able to introspect the startup with things like initctl2dot.

My point is that the first one won't be needed. Upstart should just take control over all dbus-activated things in the session.

[Ted Gould (ted)]

On Thu, 2013-05-30 at 12:17 +0000, Lars Uebernickel wrote:

> My point is that the first one won't be needed. Upstart should just
> take control over all dbus-activated things in the session.

I don't think that it'll "take over" all of them because my
understanding is that for Upstart, dbus activation is an event type not
a job.

[Ted Gould (ted)]

At the end of the day, it comes down to upstart being a process manager. Sure, we could use dbus activation, but then the panel service becomes the process manager. Let's leave process management to a tool that is designed to do that job (and does it well).

[lp:~ted/libindicator/upstart-no-dbus]

494. By Ted Gould on 2013-06-06

Only build the calcelable if we've got a name

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:494
http://jenkins.qa.ubuntu.com/job/libindicator-ci/26/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/13
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/12

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/26/rebuild

review: Approve (continuous-integration)

[lp:~ted/libindicator/upstart-no-dbus]

495. By Ted Gould on 2013-08-05

Update to trunk

496. By Ted Gould on 2013-08-05

Kill the restart code

497. By Ted Gould on 2013-08-05

Remove the callback

498. By Ted Gould on 2013-08-05

Assume Upstart in the manager code

499. By Ted Gould on 2013-08-05

Drop the restartarting the service code

500. By Ted Gould on 2013-08-05

Don't unwatch on error, we still want to watch for a new one starting

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:500
http://jenkins.qa.ubuntu.com/job/libindicator-ci/38/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/25
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/24

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/38/rebuild

review: Approve (continuous-integration)

[Dimitri John Ledkov (xnox)]

I object to this. upstart is not mandatory to manage desktop session on ubuntu. And indicators are used on the desktop sessions which are not managed by upstart. At the moment upstart as a user session manager, is opt-in, not opt-out on per session type as defined in /etc/upstart-xsessions. Thus one should preserve dbus activation files.

I understand the desire to have dbus services be managed/respawned by upstart the session manager, but forcing that to happen is not the right solution.

Unmerged revisions

500. By Ted Gould on 2013-08-05

Don't unwatch on error, we still want to watch for a new one starting

499. By Ted Gould on 2013-08-05

Drop the restartarting the service code

498. By Ted Gould on 2013-08-05

Assume Upstart in the manager code

497. By Ted Gould on 2013-08-05

Remove the callback

496. By Ted Gould on 2013-08-05

Kill the restart code

495. By Ted Gould on 2013-08-05

Update to trunk

494. By Ted Gould on 2013-06-06

Only build the calcelable if we've got a name

493. By Ted Gould on 2013-05-29

Merging trunk

492. By Ted Gould on 2013-05-29

Don't make the service manager do DBus activation in the Upstart session case.

491. By Ted Gould on 2013-05-29

Don't start the service when we're under the Upstart session

=== modified file 'libindicator/indicator-ng.c'
--- libindicator/indicator-ng.c	2013-06-28 18:30:33 +0000
+++ libindicator/indicator-ng.c	2013-08-05 14:54:23 +0000
@@ -390,44 +390,9 @@
   indicator_ng_update_entry (self);
 }
 
-static void
-indicator_ng_service_started (GObject      *source_object,
-                              GAsyncResult *res,
-                              gpointer      user_data)
-{
-  IndicatorNg *self = user_data;
-  GError *error = NULL;
-  GVariant *result;
-  guint32 start_service_reply;
-
-  result = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source_object), res, &error);
-  if (!result)
-    {
-      g_warning ("Could not activate service '%s': %s", self->name, error->message);
-      indicator_object_set_visible (INDICATOR_OBJECT (self), FALSE);
-      g_error_free (error);
-      return;
-    }
-
-  start_service_reply = 0;
-  g_variant_get (result, "(u)", &start_service_reply);
-
-  switch (start_service_reply)
-    {
-    case 1: /* DBUS_START_REPLY_SUCCESS */
-      break;
-
-    case 2: /* DBUS_START_REPLY_ALREADY_RUNNING */
-      g_warning ("could not start service '%s': it is already running", self->name);
-      break;
-
-    default:
-      g_assert_not_reached ();
-    }
-
-  g_variant_unref (result);
-}
-
+/* This function gets called everytime a service matching an
+   indicator file vanishes from dbus.  It cleans up and sets
+   it up again to be ready when it comes back. */
 static void
 indicator_ng_service_vanished (GDBusConnection *connection,
                                const gchar     *name,
@@ -437,34 +402,7 @@
 
   indicator_ng_free_actions_and_menu (self);
 
-  /* Names may vanish because the service decided it doesn't need to
-   * show its indicator anymore, or because it crashed.  Let's assume it
-   * crashes and restart it unless it explicitly hid its indicator. */
-
-  if (indicator_object_entry_is_visible (INDICATOR_OBJECT (self), &self->entry))
-    {
-      gint64 now;
-
-      /* take care not to start it if it repeatedly crashes */
-      now = g_get_monotonic_time ();
-      if (now - self->last_service_restart < 1 * G_USEC_PER_SEC)
-        return;
-
-      self->last_service_restart = now;
-
-      g_dbus_connection_call (self->session_bus,
-                              "org.freedesktop.DBus",
-                              "/",
-                              "org.freedesktop.DBus",
-                              "StartServiceByName",
-                              g_variant_new ("(su)", self->bus_name, 0),
-                              G_VARIANT_TYPE ("(u)"),
-                              G_DBUS_CALL_FLAGS_NONE,
-                              -1,
-                              NULL,
-                              indicator_ng_service_started,
-                              self);
-    }
+  return;
 }
 
 /* Get an integer from a keyfile. Returns @default_value if the key
@@ -534,6 +472,7 @@
   self->bus_name = g_path_get_basename (self->service_file);
 
   keyfile = g_key_file_new ();
+
   if (g_key_file_load_from_file (keyfile, self->service_file, G_KEY_FILE_NONE, error) &&
       indicator_ng_load_from_keyfile (self, keyfile, error))
     {
@@ -544,7 +483,7 @@
         {
           self->name_watch_id = g_bus_watch_name (G_BUS_TYPE_SESSION,
                                                   self->bus_name,
-                                                  G_BUS_NAME_WATCHER_FLAGS_AUTO_START,
+                                                  G_BUS_NAME_WATCHER_FLAGS_NONE,
                                                   indicator_ng_service_appeared,
                                                   indicator_ng_service_vanished,
                                                   self, NULL);

=== modified file 'libindicator/indicator-service-manager.c'
--- libindicator/indicator-service-manager.c	2012-07-12 15:44:39 +0000
+++ libindicator/indicator-service-manager.c	2013-08-05 14:54:23 +0000
@@ -100,7 +100,6 @@
 static void set_property (GObject * object, guint prop_id, const GValue * value, GParamSpec * pspec);
 static void get_property (GObject * object, guint prop_id, GValue * value, GParamSpec * pspec);
 static void start_service (IndicatorServiceManager * service);
-static void start_service_again (IndicatorServiceManager * manager);
 static void unwatch (GDBusProxy * proxy);
 static void service_proxy_cb (GObject * object, GAsyncResult * res, gpointer user_data);
 static void service_proxy_name_changed (GDBusConnection * connection, const gchar * sender_name, const gchar * object_path, const gchar * interface_name, const gchar * signal_name, GVariant * parameters, gpointer user_data);
@@ -370,7 +369,6 @@
 	if (error != NULL) {
 		g_warning("Unable to set watch on '%s': '%s'", priv->name, error->message);
 		g_error_free(error);
-		start_service_again(INDICATOR_SERVICE_MANAGER(user_data));
 		return;
 	}
 
@@ -388,21 +386,11 @@
 
 	if (service_api_version != INDICATOR_SERVICE_VERSION) {
 		g_warning("Service is using a different version of the service interface.  Expecting %d and got %d.", INDICATOR_SERVICE_VERSION, service_api_version);
-		unwatch(priv->service_proxy);
-
-		/* Let's make us wait a little while, then try again */
-		priv->restart_count = TIMEOUT_A_LITTLE_WHILE;
-		start_service_again(INDICATOR_SERVICE_MANAGER(user_data));
 		return;
 	}
 
 	if (this_service_version != priv->this_service_version) {
 		g_warning("Service is using a different API version than the manager.  Expecting %d and got %d.", priv->this_service_version, this_service_version);
-		unwatch(priv->service_proxy);
-
-		/* Let's make us wait a little while, then try again */
-		priv->restart_count = TIMEOUT_A_LITTLE_WHILE;
-		start_service_again(INDICATOR_SERVICE_MANAGER(user_data));
 		return;
 	}
 
@@ -437,8 +425,11 @@
 
 	priv->service_proxy_cancel = g_cancellable_new();
 
+	/* Ugly code.  Basically if Upstart is managing the lifecycle of the
+	   service we shouldn't do it for it.  This should go away as soon as
+	   we can assume that we're always in an Upstart desktop session.  (Saucy) */
 	g_dbus_proxy_new_for_bus(G_BUS_TYPE_SESSION,
-	                         G_DBUS_PROXY_FLAGS_NONE,
+	                         G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START,
 	                         interface_info,
 	                         priv->name,
 	                         INDICATOR_SERVICE_OBJECT,
@@ -474,20 +465,10 @@
 		/* Unable to create the proxy, eh, let's try again
 		   in a bit */
 		g_error_free(error);
-		start_service_again(service);
 		return;
 	}
 
 	gchar * name = g_dbus_proxy_get_name_owner(proxy);
-	if (name == NULL) {
-		/* Hmm, since creating the proxy should start it, it seems very
-		   odd that it wouldn't have an owner at this point.  But, all
-		   we can do is try again. */
-		g_object_unref(proxy);
-		start_service_again(service);
-		return;
-	}
-	g_free(name);
 
 	/* Okay, we're good to grab the proxy at this point, we're
 	   sure that it's ours. */
@@ -506,20 +487,24 @@
 	                                   user_data,
 	                                   NULL);
 
-	/* Build cancelable if we need it */
-	if (priv->watch_cancel == NULL) {
-		priv->watch_cancel = g_cancellable_new();
+	/* Send watch if someone is there */
+	if (name != NULL) {
+		/* Build cancelable if we need it */
+		if (priv->watch_cancel == NULL) {
+			priv->watch_cancel = g_cancellable_new();
+		}
+
+		g_dbus_proxy_call(priv->service_proxy,
+		                  "Watch",
+		                  NULL, /* params */
+		                  G_DBUS_CALL_FLAGS_NONE,
+		                  -1,
+		                  priv->watch_cancel,
+		                  watch_cb,
+		                  user_data);
 	}
 
-	/* Send watch */
-	g_dbus_proxy_call(priv->service_proxy,
-	                  "Watch",
-	                  NULL, /* params */
-	                  G_DBUS_CALL_FLAGS_NONE,
-	                  -1,
-	                  priv->watch_cancel,
-	                  watch_cb,
-	                  user_data);
+	g_free(name);
 
 	return;
 }
@@ -545,8 +530,6 @@
 			priv->connected = FALSE;
 			g_signal_emit(G_OBJECT(user_data), signals[CONNECTION_CHANGE], 0, FALSE, TRUE);
 		}
-
-		start_service_again(INDICATOR_SERVICE_MANAGER(user_data));
 	} else {
 		/* If we weren't connected before, we are now.  Let's tell the
 		   world! */
@@ -555,10 +538,12 @@
 			g_signal_emit(G_OBJECT(user_data), signals[CONNECTION_CHANGE], 0, TRUE, TRUE);
 		}
 
+		/* First check to see if this is a new service getting the name, if that's
+		   the case, watch it. */
 		/* If the names are both valid, and they're not the same, it means that
 		   we've actually changed.  So we need to tell the new guy that we're
 		   watching them */
-		if (new_name != NULL && prev_name != NULL && new_name[0] != 0 && prev_name != 0 && g_strcmp0(prev_name, new_name) != 0) {
+		if (prev_name == NULL || (new_name != NULL && prev_name != NULL && new_name[0] != 0 && prev_name != 0 && g_strcmp0(prev_name, new_name) != 0)) {
 			/* Send watch */
 			g_dbus_proxy_call(priv->service_proxy,
 			                  "Watch",
@@ -574,53 +559,6 @@
 	return;
 }
 
-/* The callback that starts the service for real after
-   the timeout as determined in 'start_service_again'.
-   This could be in the idle or a timer. */
-static gboolean
-start_service_again_cb (gpointer data)
-{
-	IndicatorServiceManagerPrivate * priv = INDICATOR_SERVICE_MANAGER_GET_PRIVATE(data);
-	priv->restart_count++;
-	g_debug("Restarting service '%s' count %d", priv->name, priv->restart_count);
-	start_service(INDICATOR_SERVICE_MANAGER(data));
-	priv->restart_source = 0;
-	return FALSE;
-}
-
-/* This function tries to start a new service, perhaps
-   after a timeout that it determines.  The real issue
-   here is that it throttles restarting if we're not
-   being successful. */
-static void
-start_service_again (IndicatorServiceManager * manager)
-{
-	IndicatorServiceManagerPrivate * priv = INDICATOR_SERVICE_MANAGER_GET_PRIVATE(manager);
-
-	/* If we've already got a restart source running then
-	   let's not do this again. */
-	if (priv->restart_source != 0) {
-		return;
-	}
-
-	/* Allow the restarting to be disabled */
-	if (g_getenv(TIMEOUT_ENV_NAME)) {
-		return;
-	}
-
-	if (priv->restart_count == 0) {
-		/* First time, do it in idle */
-		g_idle_add(start_service_again_cb, manager);
-	} else {
-		/* Not our first time 'round the block.  Let's slow this down. */
-		if (priv->restart_count > 16)
-			priv->restart_count = 16; /* Not more than 1024x */
-		priv->restart_source = g_timeout_add((1 << priv->restart_count) * TIMEOUT_MULTIPLIER, start_service_again_cb, manager);
-	}
-
-	return;
-}
-
 /* API */
 
 /**