From 758fbd7aac9d49e0abe20ba184c939c112bddf68 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Thu, 18 Mar 2021 10:12:16 +0100
Subject: [PATCH] glib-aux: use g_cancellable_connection() in
 nm_utils_invoke_on_idle() and cleanup

Use g_cancellable_connect(). That approach better handles the case where the
cancellable is already cancelled. Theoretically, we could extend that
approach further and make it thread-safe, but in the current form is
nm_utils_invoke_on_idle() not thread-safe.

While at it, also cleanup duplicate code during completion.
---
 src/libnm-glib-aux/nm-shared-utils.c | 57 +++++++++++++++-------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c
index fe9994459f..a33b84be32 100644
--- a/src/libnm-glib-aux/nm-shared-utils.c
+++ b/src/libnm-glib-aux/nm-shared-utils.c
@@ -4293,11 +4293,9 @@ typedef struct {
     gulong                      cancelled_id;
 } InvokeOnIdleData;
 
-static gboolean
-_nm_utils_invoke_on_idle_cb_idle(gpointer user_data)
+static void
+_nm_utils_invoke_on_idle_complete(InvokeOnIdleData *data)
 {
-    InvokeOnIdleData *data = user_data;
-
     nm_clear_g_signal_handler(data->cancellable, &data->cancelled_id);
 
     data->callback(data->callback_user_data, data->cancellable);
@@ -4305,18 +4303,31 @@ _nm_utils_invoke_on_idle_cb_idle(gpointer user_data)
     nm_g_object_unref(data->cancellable);
     g_source_destroy(data->source);
     nm_g_slice_free(data);
+}
+
+static gboolean
+_nm_utils_invoke_on_idle_cb_idle(gpointer user_data)
+{
+    _nm_utils_invoke_on_idle_complete(user_data);
     return G_SOURCE_REMOVE;
 }
 
 static void
 _nm_utils_invoke_on_idle_cb_cancelled(GCancellable *cancellable, InvokeOnIdleData *data)
 {
-    /* on cancellation, we invoke the callback synchronously. */
-    nm_clear_g_signal_handler(data->cancellable, &data->cancelled_id);
-    nm_clear_g_source_inst(&data->source);
-    data->callback(data->callback_user_data, data->cancellable);
-    nm_g_object_unref(data->cancellable);
-    nm_g_slice_free(data);
+    if (data->cancelled_id == 0) {
+        /* this can only happen during _nm_utils_invoke_on_idle_start(). Don't do anything,
+         * we still schedule an idle action. */
+        return;
+    }
+
+    /* On cancellation, we invoke the callback synchronously.
+     *
+     * Note that this is not thread-safe, meaning: you can only cancel the cancellable
+     * while not iterating the GMainContext (that has the idle/timeout source attached).
+     * Making this thread safe would be complicated, and it's simply not used by our
+     * callers. */
+    _nm_utils_invoke_on_idle_complete(data);
 }
 
 static void
@@ -4340,23 +4351,17 @@ _nm_utils_invoke_on_idle_start(gboolean                    use_timeout,
     };
 
     if (cancellable) {
-        if (g_cancellable_is_cancelled(cancellable)) {
-            /* the cancellable is already cancelled. We ignore the timeout
-             * and always schedule an idle action. */
+        gulong cancelled_id;
+
+        cancelled_id = g_cancellable_connect(cancellable,
+                                             G_CALLBACK(_nm_utils_invoke_on_idle_cb_cancelled),
+                                             data,
+                                             NULL);
+        if (cancelled_id == 0) {
+            /* the cancellable is already cancelled. We still schedule an idle action. */
             use_timeout = FALSE;
-        } else {
-            /* if we are passed a non-cancelled cancellable, we register to the "cancelled"
-             * signal an invoke the callback synchronously (from the signal handler).
-             *
-             * We don't do that,
-             *  - if the cancellable is already cancelled (because we don't want to invoke
-             *    the callback synchronously from the caller).
-             *  - if we have no cancellable at hand. */
-            data->cancelled_id = g_signal_connect(cancellable,
-                                                  "cancelled",
-                                                  G_CALLBACK(_nm_utils_invoke_on_idle_cb_cancelled),
-                                                  data);
-        }
+        } else
+            data->cancelled_id = cancelled_id;
     }
 
     if (use_timeout) {