[glib/wip/pwithnall/2092-stupid-dbus-tests-again] tests: Fix remaining race in gdbus-connection filter test

[Philip Withnall <pwithnall src gnome org>]

commit 2f171d0ed4456c7fa3dcc24b38ae80131bd0d59e
Author: Philip Withnall <withnall endlessm com>
Date:   Tue May 5 11:22:26 2020 +0100

    tests: Fix remaining race in gdbus-connection filter test
    
    Commit 721e385 left one remaining race in the filter test, with a
    comment associated with it. Unfortunately, the (seemingly unrelated)
    changes in #1841 to `GCancellable` seem to have made this remaining race
    a lot more likely to fail on FreeBSD than before.
    
    What’s likely to have happened (although I was unable to reproduce the
    failure, due to not having a FreeBSD system; I was only able to
    reproduce the problem as a 3/1000 failure on Linux, which is still worth
    fixing) is that the atomic write of the `FilterData.serial` to be
    expected by the filter function sometimes happened after the filter
    function had executed, so the expected message was dropped and didn’t
    result in an update to the `FilterData` state.
    
    Rework the test so that instead of setting some expectations (on
    `FilterData`) in one thread and then checking them in another thread,
    the worker thread just unconditionally returns messages from the filter
    function to the main thread, and then the main thread checks whether the
    expected one has been filtered.
    
    With this change applied, the `gdbus-connection` test passes 5000 times
    in a row for me, on Linux; and doesn’t seem to fail any more on the
    FreeBSD CI machines over a few runs. (Previously it failed on 4/5 runs.)
    
    Signed-off-by: Philip Withnall <withnall endlessm com>
    
    Fixes: #2092
    Fixes: #1957

 gio/tests/gdbus-connection.c | 62 ++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 31 deletions(-)
---
diff --git a/gio/tests/gdbus-connection.c b/gio/tests/gdbus-connection.c
index 4fec8d0fd..7bd7a02f4 100644
--- a/gio/tests/gdbus-connection.c
+++ b/gio/tests/gdbus-connection.c
@@ -817,9 +817,8 @@ test_connection_signal_match_rules (void)
  * so all accesses must be atomic. */
 typedef struct
 {
-  guint num_handled;  /* (atomic) */
+  GAsyncQueue *incoming_queue;  /* (element-type GDBusMessage) */
   guint num_outgoing;  /* (atomic) */
-  guint32 serial;  /* (atomic) */
 } FilterData;
 
 /* Runs in a worker thread. */
@@ -830,22 +829,31 @@ filter_func (GDBusConnection *connection,
              gpointer         user_data)
 {
   FilterData *data = user_data;
-  guint32 reply_serial;
 
   if (incoming)
-    {
-      reply_serial = g_dbus_message_get_reply_serial (message);
-      if (reply_serial == g_atomic_int_get (&data->serial))
-        g_atomic_int_inc (&data->num_handled);
-    }
+    g_async_queue_push (data->incoming_queue, g_object_ref (message));
   else
-    {
-      g_atomic_int_inc (&data->num_outgoing);
-    }
+    g_atomic_int_inc (&data->num_outgoing);
 
   return message;
 }
 
+static void
+wait_for_filtered_reply (GAsyncQueue *incoming_queue,
+                         guint32      expected_serial)
+{
+  GDBusMessage *popped_message = NULL;
+
+  while ((popped_message = g_async_queue_pop (incoming_queue)) != NULL)
+    {
+      guint32 reply_serial = g_dbus_message_get_reply_serial (popped_message);
+      g_object_unref (popped_message);
+      if (reply_serial == expected_serial)
+        return;
+    }
+
+  g_assert_not_reached ();
+}
 
 typedef struct
 {
@@ -933,7 +941,7 @@ static void
 test_connection_filter (void)
 {
   GDBusConnection *c;
-  FilterData data = { 0, 0, 0 };
+  FilterData data = { NULL, 0 };
   GDBusMessage *m;
   GDBusMessage *m2;
   GDBusMessage *r;
@@ -953,6 +961,8 @@ test_connection_filter (void)
   g_assert_no_error (error);
   g_assert_nonnull (c);
 
+  data.incoming_queue = g_async_queue_new_full (g_object_unref);
+  data.num_outgoing = 0;
   filter_id = g_dbus_connection_add_filter (c,
                                             filter_func,
                                             &data,
@@ -965,21 +975,17 @@ test_connection_filter (void)
   g_dbus_message_set_body (m, g_variant_new ("(s)", "org.freedesktop.DBus"));
   error = NULL;
   g_dbus_connection_send_message (c, m, G_DBUS_SEND_MESSAGE_FLAGS_NONE, &serial_temp, &error);
-  g_atomic_int_set (&data.serial, serial_temp);
   g_assert_no_error (error);
 
-  while (g_atomic_int_get (&data.num_handled) == 0)
-    g_thread_yield ();
+  wait_for_filtered_reply (data.incoming_queue, serial_temp);
 
   m2 = g_dbus_message_copy (m, &error);
   g_assert_no_error (error);
   g_dbus_connection_send_message (c, m2, G_DBUS_SEND_MESSAGE_FLAGS_NONE, &serial_temp, &error);
-  g_atomic_int_set (&data.serial, serial_temp);
   g_object_unref (m2);
   g_assert_no_error (error);
 
-  while (g_atomic_int_get (&data.num_handled) == 1)
-    g_thread_yield ();
+  wait_for_filtered_reply (data.incoming_queue, serial_temp);
 
   m2 = g_dbus_message_copy (m, &error);
   g_assert_no_error (error);
@@ -987,12 +993,10 @@ test_connection_filter (void)
   /* lock the message to test PRESERVE_SERIAL flag. */
   g_dbus_message_lock (m2);
   g_dbus_connection_send_message (c, m2, G_DBUS_SEND_MESSAGE_FLAGS_PRESERVE_SERIAL, &serial_temp, &error);
-  g_atomic_int_set (&data.serial, serial_temp);
   g_object_unref (m2);
   g_assert_no_error (error);
 
-  while (g_atomic_int_get (&data.num_handled) == 2)
-    g_thread_yield ();
+  wait_for_filtered_reply (data.incoming_queue, serial_temp);
 
   m2 = g_dbus_message_copy (m, &error);
   g_assert_no_error (error);
@@ -1000,17 +1004,16 @@ test_connection_filter (void)
                                                       m2,
                                                       G_DBUS_SEND_MESSAGE_FLAGS_NONE,
                                                       -1,
-                                                      /* can't do this write atomically
-                                                       * as filter_func() needs it before
-                                                       * send_message_with_reply_sync() returns */
-                                                      &data.serial,
+                                                      &serial_temp,
                                                       NULL, /* GCancellable */
                                                       &error);
   g_object_unref (m2);
   g_assert_no_error (error);
   g_assert_nonnull (r);
   g_object_unref (r);
-  g_assert_cmpint (g_atomic_int_get (&data.num_handled), ==, 4);
+
+  wait_for_filtered_reply (data.incoming_queue, serial_temp);
+  g_assert_cmpint (g_async_queue_length (data.incoming_queue), ==, 0);
 
   g_dbus_connection_remove_filter (c, filter_id);
 
@@ -1023,12 +1026,11 @@ test_connection_filter (void)
                                                       &serial_temp,
                                                       NULL, /* GCancellable */
                                                       &error);
-  g_atomic_int_set (&data.serial, serial_temp);
   g_object_unref (m2);
   g_assert_no_error (error);
   g_assert_nonnull (r);
   g_object_unref (r);
-  g_assert_cmpint (g_atomic_int_get (&data.num_handled), ==, 4);
+  g_assert_cmpint (g_async_queue_length (data.incoming_queue), ==, 0);
   g_assert_cmpint (g_atomic_int_get (&data.num_outgoing), ==, 4);
 
   /* wait for service to be available */
@@ -1101,6 +1103,7 @@ test_connection_filter (void)
 
   g_object_unref (c);
   g_object_unref (m);
+  g_async_queue_unref (data.incoming_queue);
 
   session_bus_down ();
 }
@@ -1268,9 +1271,6 @@ main (int   argc,
 {
   int ret;
 
-  /* FIXME: Add debug for https://gitlab.gnome.org/GNOME/glib/issues/1957 */
-  g_setenv ("G_DBUS_DEBUG", "all", TRUE);
-
   g_test_init (&argc, &argv, NULL);
 
   /* all the tests rely on a shared main loop */