windowManager: Wait for X11 services using systemd

To do this, we now wait for the start/stop job to complete. We also have
two targets in gnome-session to ensure that everything is working as
expected.

In order to start the services, we simply request the
gnome-session-x11-services-ready.target unit, and wait for it to become
available. To stop, we use the gnome-session-x11-services.target unit
which should stop all services in a way that is entirely race free.

This requires both gnome-session and gnome-settings-daemon changes to
work (which are in the corresponding merge requests).

https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/895

js/ui/windowManager.js

@@ -42,6 +42,11 @@ const GsdWacomProxy = Gio.DBusProxy.makeProxyWrapper(GsdWacomIface);
 
 const WINDOW_DIMMER_EFFECT_NAME = "gnome-shell-window-dimmer";
 
+Gio._promisify(Shell,
+    'util_start_systemd_unit', 'util_start_systemd_unit_finish');
+Gio._promisify(Shell,
+    'util_stop_systemd_unit', 'util_stop_systemd_unit_finish');
+
 var DisplayChangeDialog = GObject.registerClass(
 class DisplayChangeDialog extends ModalDialog.ModalDialog {
     _init(wm) {
@@ -901,46 +906,23 @@ var WindowManager = class {
         global.display.connect('init-xserver', (display, task) => {
             IBusManager.getIBusManager().restartDaemon(['--xim']);
 
-            try {
+            /* Timeout waiting for start job completion after 5 seconds */
-                if (!Shell.util_start_systemd_unit('gsd-xsettings.target', 'fail'))
+            let cancellable = new Gio.Cancellable();
-                    log('Not starting gsd-xsettings; waiting for gnome-session to do so');
+            GLib.timeout_add_seconds(GLib.PRIORITY_DEFAULT, 5, () => {
-
+                cancellable.cancel();
-                /* Leave this watchdog timeout so don't block indefinitely here */
-                let timeoutId = GLib.timeout_add_seconds(GLib.PRIORITY_DEFAULT, 5, () => {
-                    Gio.DBus.session.unwatch_name(watchId);
-                    log('Warning: Failed to start gsd-xsettings');
-                    task.return_boolean(true);
-                    timeoutId = 0;
                 return GLib.SOURCE_REMOVE;
             });
 
-                /* When gsd-xsettings daemon is started, we are good to resume */
+            this._startX11Services(task, cancellable);
-                let watchId = Gio.DBus.session.watch_name(
-                    'org.gnome.SettingsDaemon.XSettings',
-                    Gio.BusNameWatcherFlags.NONE,
-                    () => {
-                        Gio.DBus.session.unwatch_name(watchId);
-                        if (timeoutId > 0) {
-                            task.return_boolean(true);
-                            GLib.source_remove(timeoutId);
-                        }
-                    },
-                    null);
-            } catch (e) {
-                log('Error starting gsd-xsettings: %s'.format(e.message));
-                task.return_boolean(true);
-            }
 
             return true;
         });
         global.display.connect('x11-display-closing', () => {
             if (!Meta.is_wayland_compositor())
                 return;
-            try {
+
-                Shell.util_stop_systemd_unit('gsd-xsettings.target', 'fail');
+            this._stopX11Services(null);
-            } catch (e) {
+
-                log('Error stopping gsd-xsettings: %s'.format(e.message));
-            }
             IBusManager.getIBusManager().restartDaemon();
         });
 
@@ -1008,6 +990,36 @@ var WindowManager = class {
         global.stage.add_action(topDragAction);
     }
 
+    async _startX11Services(task, cancellable) {
+        try {
+            await Shell.util_start_systemd_unit(
+                'gnome-session-x11-services-ready.target', 'fail', cancellable);
+        } catch (e) {
+            // Ignore NOT_SUPPORTED error, which indicates we are not systemd
+            // managed and gnome-session will have taken care of everything
+            // already.
+            // Note that we do log cancellation from here.
+            if (!e.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.NOT_SUPPORTED))
+                log('Error starting X11 services: %s'.format(e.message));
+        } finally {
+            task.return_boolean(true);
+        }
+    }
+
+    async _stopX11Services(cancellable) {
+        try {
+            await Shell.util_stop_systemd_unit(
+                'gnome-session-x11-services.target', 'fail', cancellable);
+        } catch (e) {
+            // Ignore NOT_SUPPORTED error, which indicates we are not systemd
+            // managed and gnome-session will have taken care of everything
+            // already.
+            // Note that we do log cancellation from here.
+            if (!e.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.NOT_SUPPORTED))
+                log('Error stopping X11 services: %s'.format(e.message));
+        }
+    }
+
     _showPadOsd(display, device, settings, imagePath, editionMode, monitorIndex) {
         this._currentPadOsd = new PadOsd.PadOsd(device, settings, imagePath, editionMode, monitorIndex);
         this._currentPadOsd.connect('closed', () => (this._currentPadOsd = null));

src/shell-util.c

@@ -570,6 +570,57 @@ shell_util_get_uid (void)
   return getuid ();
 }
 
+typedef struct {
+  GDBusConnection *connection;
+  gchar           *command;
+
+  GCancellable *cancellable;
+  gulong        cancel_id;
+
+  guint    job_watch;
+  gchar   *job;
+} SystemdCall;
+
+static void
+shell_util_systemd_call_data_free (SystemdCall *data)
+{
+  if (data->job_watch)
+    {
+      g_dbus_connection_signal_unsubscribe (data->connection, data->job_watch);
+      data->job_watch = 0;
+    }
+
+  if (data->cancellable)
+    {
+      g_cancellable_disconnect (data->cancellable, data->cancel_id);
+      g_clear_object (&data->cancellable);
+      data->cancel_id = 0;
+    }
+
+  g_clear_object (&data->connection);
+  g_clear_pointer (&data->job, g_free);
+  g_clear_pointer (&data->command, g_free);
+  g_free (data);
+}
+
+static void
+shell_util_systemd_call_cancelled_cb (GCancellable *cancellable,
+                                      GTask        *task)
+{
+  SystemdCall *data = g_task_get_task_data (task);
+
+  /* Task has returned, but data is not yet free'ed, ignore signal. */
+  if (g_task_get_completed (task))
+    return;
+
+  /* We are still in the DBus call; it will return the error. */
+  if (data->job == NULL)
+    return;
+
+  g_task_return_error_if_cancelled (task);
+  g_object_unref (task);
+}
+
 static void
 on_systemd_call_cb (GObject      *source,
                     GAsyncResult *res,
@@ -577,46 +628,143 @@ on_systemd_call_cb (GObject      *source,
 {
   g_autoptr (GVariant) reply = NULL;
   g_autoptr (GError) error = NULL;
-  const gchar *command = user_data;
+  GTask *task = G_TASK (user_data);
+  SystemdCall *data;
 
   reply = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source),
                                          res, &error);
-  if (error)
+
-    g_warning ("Could not issue '%s' systemd call", command);
+  data = g_task_get_task_data (task);
+
+  if (error) {
+    g_warning ("Could not issue '%s' systemd call", data->command);
+    g_task_return_error (task, g_steal_pointer (&error));
+    g_object_unref (task);
+
+    return;
+  }
+
+  g_assert (data->job == NULL);
+  g_variant_get (reply, "(o)", &data->job);
+
+  /* And we wait for the JobRemoved notification. */
 }
 
-static gboolean
+static void
+on_systemd_job_removed_cb (GDBusConnection *connection,
+                           const gchar *sender_name,
+                           const gchar *object_path,
+                           const gchar *interface_name,
+                           const gchar *signal_name,
+                           GVariant *parameters,
+                           gpointer user_data)
+{
+  GTask *task = G_TASK (user_data);
+  SystemdCall *data;
+  guint32 id;
+  const char *path, *unit, *result;
+
+  /* Task has returned, but data is not yet free'ed, ignore signal. */
+  if (g_task_get_completed (task))
+    return;
+
+  data = g_task_get_task_data (task);
+
+  /* No job information yet, ignore. */
+  if (data->job == NULL)
+    return;
+
+  g_variant_get (parameters, "(u&o&s&s)", &id, &path, &unit, &result);
+
+  /* Is it the job we are waiting for? */
+  if (g_strcmp0 (path, data->job) != 0)
+    return;
+
+  /* Task has completed; return the result of the job */
+  if (g_strcmp0 (result, "done") == 0)
+    g_task_return_boolean (task, TRUE);
+  else
+    g_task_return_new_error (task,
+                             G_IO_ERROR,
+                             G_IO_ERROR_FAILED,
+                             "Systemd job completed with status \"%s\"",
+                             result);
+
+  g_object_unref (task);
+}
+
+static void
 shell_util_systemd_call (const char           *command,
                          const char           *unit,
                          const char           *mode,
-                         GError     **error)
+                         GCancellable         *cancellable,
+                         GAsyncReadyCallback   callback,
+                         gpointer              user_data)
 {
+  g_autoptr (GTask) task = g_task_new (NULL, cancellable, callback, user_data);
+
 #ifdef HAVE_SYSTEMD
   g_autoptr (GDBusConnection) connection = NULL;
+  GError *error = NULL;
+  SystemdCall *data;
   g_autofree char *self_unit = NULL;
   int res;
 
+  connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error);
+
+  if (connection == NULL) {
+    g_task_return_error (task, error);
+    return;
+  }
+
+  /* We look up the systemd unit that our own process is running in here.
+   * This way we determine whether the session is managed using systemd.
+   */
   res = sd_pid_get_user_unit (getpid (), &self_unit);
 
   if (res == -ENODATA)
     {
-      g_debug ("Not systemd-managed, not doing '%s' on '%s'", mode, unit);
+      g_task_return_new_error (task,
-      return FALSE;
+                               G_IO_ERROR,
+                               G_IO_ERROR_NOT_SUPPORTED,
+                               "Not systemd managed");
+      return;
     }
   else if (res < 0)
     {
-      g_set_error (error,
+      g_task_return_new_error (task,
                                G_IO_ERROR,
                                g_io_error_from_errno (-res),
-                   "Error trying to start systemd unit '%s': %s",
+                               "Error fetching own systemd unit: %s",
-                   unit, g_strerror (-res));
+                               g_strerror (-res));
-      return FALSE;
+      return;
     }
 
-  connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, error);
+  data = g_new0 (SystemdCall, 1);
+  data->command = g_strdup (command);
+  data->connection = g_object_ref (connection);
+  data->job_watch = g_dbus_connection_signal_subscribe (connection,
+                                                        "org.freedesktop.systemd1",
+                                                        "org.freedesktop.systemd1.Manager",
+                                                        "JobRemoved",
+                                                        "/org/freedesktop/systemd1",
+                                                        NULL,
+                                                        G_DBUS_SIGNAL_FLAGS_NONE,
+                                                        on_systemd_job_removed_cb,
+                                                        task,
+                                                        NULL);
+  g_task_set_task_data (task,
+                        data,
+                        (GDestroyNotify) shell_util_systemd_call_data_free);
 
-  if (connection == NULL)
+  if (cancellable)
-    return FALSE;
+    {
+      data->cancellable = g_object_ref (cancellable);
+      data->cancel_id = g_cancellable_connect (cancellable,
+                                               G_CALLBACK (shell_util_systemd_call_cancelled_cb),
+                                               task,
+                                               NULL);
+    }
 
   g_dbus_connection_call (connection,
                           "org.freedesktop.systemd1",
@@ -625,31 +773,53 @@ shell_util_systemd_call (const char  *command,
                           command,
                           g_variant_new ("(ss)",
                                          unit, mode),
-                          NULL,
+                          G_VARIANT_TYPE ("(o)"),
                           G_DBUS_CALL_FLAGS_NONE,
-                          -1, NULL,
+                          -1, cancellable,
                           on_systemd_call_cb,
-                          (gpointer) command);
+                          g_steal_pointer (&task));
-  return TRUE;
+#else /* HAVE_SYSTEMD */
-#endif /* HAVE_SYSTEMD */
+  g_task_return_new_error (task,
-
+                           G_IO_ERROR,
-  return FALSE;
+                           G_IO_ERROR_NOT_SUPPORTED,
+                           "systemd not supported by gnome-shell");
+#endif /* !HAVE_SYSTEMD */
 }
 
-gboolean
+void
 shell_util_start_systemd_unit (const char           *unit,
                                const char           *mode,
-                               GError     **error)
+                               GCancellable         *cancellable,
+                               GAsyncReadyCallback   callback,
+                               gpointer              user_data)
 {
-  return shell_util_systemd_call ("StartUnit", unit, mode, error);
+  shell_util_systemd_call ("StartUnit", unit, mode,
+                           cancellable, callback, user_data);
 }
 
 gboolean
-shell_util_stop_systemd_unit (const char  *unit,
+shell_util_start_systemd_unit_finish (GAsyncResult  *res,
-                              const char  *mode,
                                       GError       **error)
 {
-  return shell_util_systemd_call ("StopUnit", unit, mode, error);
+  return g_task_propagate_boolean (G_TASK (res), error);
+}
+
+void
+shell_util_stop_systemd_unit (const char           *unit,
+                              const char           *mode,
+                              GCancellable         *cancellable,
+                              GAsyncReadyCallback   callback,
+                              gpointer              user_data)
+{
+  shell_util_systemd_call ("StopUnit", unit, mode,
+                           cancellable, callback, user_data);
+}
+
+gboolean
+shell_util_stop_systemd_unit_finish (GAsyncResult  *res,
+                                     GError       **error)
+{
+  return g_task_propagate_boolean (G_TASK (res), error);
 }
 
 void

src/shell-util.h

@@ -59,11 +59,20 @@ cairo_surface_t * shell_util_composite_capture_images (ClutterCapture  *captures
 
 void shell_util_check_cloexec_fds (void);
 
-gboolean shell_util_start_systemd_unit (const char  *unit,
+void   shell_util_start_systemd_unit          (const char           *unit,
                                                const char           *mode,
+                                               GCancellable         *cancellable,
+                                               GAsyncReadyCallback   callback,
+                                               gpointer              user_data);
+gboolean shell_util_start_systemd_unit_finish (GAsyncResult         *res,
                                                GError              **error);
-gboolean shell_util_stop_systemd_unit  (const char  *unit,
+
+void  shell_util_stop_systemd_unit           (const char           *unit,
                                               const char           *mode,
+                                              GCancellable         *cancellable,
+                                              GAsyncReadyCallback   callback,
+                                              gpointer              user_data);
+gboolean shell_util_stop_systemd_unit_finish (GAsyncResult         *res,
                                               GError              **error);
 
 void shell_util_sd_notify (void);