[glib/glib-2-30] GDBusConnection: Make pending calls error when the connection is lost

[David Zeuthen <davidz src gnome org>]

commit 3dcde986bff5b869da13ba7f3abd815ad34c1c45
Author: David Zeuthen <davidz redhat com>
Date:   Fri Oct 7 14:20:03 2011 -0400

    GDBusConnection: Make pending calls error when the connection is lost
    
    If the connection to the bus is lost while a method call is ongoing,
    the method call does not get cancelled. Instead it just sits around
    until it times out.
    
    This is visible here on XO laptops when stopping the display manager
    during shutdown. imsettings starts sending a sync message to give up
    its bus name (via g_bus_unown_name()), then systemd terminates the
    session bus at approximately the same time. imsettings then hangs for
    about 20 seconds before timing out the message.
    
     http://lists.freedesktop.org/archives/dbus/2011-September/014717.html
    
    imsettings behaviour could be improved as described in that thread,
    but I think this is a glib bug. I've also come up with the attached
    patch which fixes it.
    
    Credits for the bug-fix goes to Daniel Drake <dsd laptop org>. The test
    case was written by David Zeuthen <zeuthen gmail com>.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=660637
    
    Signed-off-by: David Zeuthen <davidz redhat com>

 gio/gdbusconnection.c             |   42 ++++++++--
 gio/tests/Makefile.am             |    4 +
 gio/tests/gdbus-connection-loss.c |  155 +++++++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+), 7 deletions(-)
---
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index a54972b..35d6cf6 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -1580,7 +1580,7 @@ send_message_data_unref (SendMessageData *data)
 
 /* can be called from any thread with lock held - caller must have prepared GSimpleAsyncResult already */
 static void
-send_message_with_reply_deliver (SendMessageData *data)
+send_message_with_reply_deliver (SendMessageData *data, gboolean remove)
 {
   CONNECTION_ENSURE_LOCK (data->connection);
 
@@ -1603,8 +1603,11 @@ send_message_with_reply_deliver (SendMessageData *data)
       data->cancellable_handler_id = 0;
     }
 
-  g_warn_if_fail (g_hash_table_remove (data->connection->map_method_serial_to_send_message_data,
-                                       GUINT_TO_POINTER (data->serial)));
+  if (remove)
+    {
+      g_warn_if_fail (g_hash_table_remove (data->connection->map_method_serial_to_send_message_data,
+                                           GUINT_TO_POINTER (data->serial)));
+    }
 
   send_message_data_unref (data);
 }
@@ -1623,7 +1626,7 @@ send_message_data_deliver_reply_unlocked (SendMessageData *data,
                                              g_object_ref (reply),
                                              g_object_unref);
 
-  send_message_with_reply_deliver (data);
+  send_message_with_reply_deliver (data, TRUE);
 
  out:
   ;
@@ -1645,7 +1648,7 @@ send_message_with_reply_cancelled_idle_cb (gpointer user_data)
                                    G_IO_ERROR_CANCELLED,
                                    _("Operation was cancelled"));
 
-  send_message_with_reply_deliver (data);
+  send_message_with_reply_deliver (data, TRUE);
 
  out:
   CONNECTION_UNLOCK (data->connection);
@@ -1689,7 +1692,7 @@ send_message_with_reply_timeout_cb (gpointer user_data)
                                    G_IO_ERROR_TIMED_OUT,
                                    _("Timeout was reached"));
 
-  send_message_with_reply_deliver (data);
+  send_message_with_reply_deliver (data, TRUE);
 
  out:
   CONNECTION_UNLOCK (data->connection);
@@ -2209,6 +2212,28 @@ on_worker_message_about_to_be_sent (GDBusWorker  *worker,
   return message;
 }
 
+/* called with connection lock held */
+static gboolean
+cancel_method_on_close (gpointer key, gpointer value, gpointer user_data)
+{
+  SendMessageData *data = value;
+
+  if (data->delivered)
+    return FALSE;
+
+  g_simple_async_result_set_error (data->simple,
+                                   G_IO_ERROR,
+                                   G_IO_ERROR_CLOSED,
+                                   _("The connection is closed"));
+
+  /* Ask send_message_with_reply_deliver not to remove the element from the
+   * hash table - we're in the middle of a foreach; that would be unsafe.
+   * Instead, return TRUE from this function so that it gets removed safely.
+   */
+  send_message_with_reply_deliver (data, FALSE);
+  return TRUE;
+}
+
 /* Called in worker's thread - we must not block */
 static void
 on_worker_closed (GDBusWorker *worker,
@@ -2234,7 +2259,10 @@ on_worker_closed (GDBusWorker *worker,
 
   CONNECTION_LOCK (connection);
   if (!connection->closed)
-    set_closed_unlocked (connection, remote_peer_vanished, error);
+    {
+      g_hash_table_foreach_remove (connection->map_method_serial_to_send_message_data, cancel_method_on_close, NULL);
+      set_closed_unlocked (connection, remote_peer_vanished, error);
+    }
   CONNECTION_UNLOCK (connection);
 
   g_object_unref (connection);
diff --git a/gio/tests/Makefile.am b/gio/tests/Makefile.am
index 0c0c932..2493e40 100644
--- a/gio/tests/Makefile.am
+++ b/gio/tests/Makefile.am
@@ -61,6 +61,7 @@ if OS_UNIX
 TEST_PROGS +=			\
 	gdbus-close-pending	\
 	gdbus-connection	\
+	gdbus-connection-loss	\
 	gdbus-connection-slow	\
 	gdbus-names		\
 	gdbus-proxy		\
@@ -295,6 +296,9 @@ endif # OS_UNIX
 gdbus_connection_SOURCES = gdbus-connection.c gdbus-sessionbus.c gdbus-sessionbus.h gdbus-tests.h gdbus-tests.c
 gdbus_connection_LDADD = $(progs_ldadd)
 
+gdbus_connection_loss_SOURCES = gdbus-connection-loss.c gdbus-sessionbus.c gdbus-sessionbus.h gdbus-tests.h gdbus-tests.c
+gdbus_connection_loss_LDADD = $(progs_ldadd)
+
 gdbus_connection_slow_SOURCES = gdbus-connection-slow.c gdbus-sessionbus.c gdbus-sessionbus.h gdbus-tests.h gdbus-tests.c
 gdbus_connection_slow_LDADD = $(progs_ldadd)
 
diff --git a/gio/tests/gdbus-connection-loss.c b/gio/tests/gdbus-connection-loss.c
new file mode 100644
index 0000000..44e97ae
--- /dev/null
+++ b/gio/tests/gdbus-connection-loss.c
@@ -0,0 +1,155 @@
+/* GLib testing framework examples and tests
+ *
+ * Copyright (C) 2008-2010 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General
+ * Public License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place, Suite 330,
+ * Boston, MA 02111-1307, USA.
+ *
+ * Author: David Zeuthen <davidz redhat com>
+ */
+
+#include <gio/gio.h>
+#include <unistd.h>
+#include <string.h>
+
+#include "gdbus-tests.h"
+
+/* all tests rely on a global connection */
+static GDBusConnection *c = NULL;
+
+/* all tests rely on a shared mainloop */
+static GMainLoop *loop = NULL;
+
+/* ---------------------------------------------------------------------------------------------------- */
+/* Check that pending calls fail with G_IO_ERROR_CLOSED if the connection is closed  */
+/* See https://bugzilla.gnome.org/show_bug.cgi?id=660637 */
+/* ---------------------------------------------------------------------------------------------------- */
+
+static void
+sleep_cb (GObject      *source_object,
+          GAsyncResult *res,
+          gpointer      user_data)
+{
+  GError **error = user_data;
+  GVariant *result;
+
+  result = g_dbus_proxy_call_finish (G_DBUS_PROXY (source_object), res, error);
+  g_assert (result == NULL);
+  g_main_loop_quit (loop);
+}
+
+static gboolean
+on_timeout (gpointer user_data)
+{
+  /* tear down bus */
+  session_bus_down ();
+  return FALSE; /* remove source */
+}
+
+static void
+test_connection_loss (void)
+{
+  GDBusProxy *proxy;
+  GError *error;
+
+  error = NULL;
+  proxy = g_dbus_proxy_new_sync (c,
+                                 G_DBUS_PROXY_FLAGS_NONE,
+                                 NULL,                      /* GDBusInterfaceInfo */
+                                 "com.example.TestService", /* name */
+                                 "/com/example/TestObject", /* object path */
+                                 "com.example.Frob",        /* interface */
+                                 NULL, /* GCancellable */
+                                 &error);
+  g_assert_no_error (error);
+
+  error = NULL;
+  g_dbus_proxy_call (proxy,
+                     "Sleep",
+                     g_variant_new ("(i)", 100 * 1000 /* msec */),
+                     G_DBUS_CALL_FLAGS_NONE,
+                     10 * 1000 /* msec */,
+                     NULL, /* GCancellable */
+                     sleep_cb,
+                     &error);
+
+  /* Make sure we don't exit when the bus goes away */
+  g_dbus_connection_set_exit_on_close (c, FALSE);
+
+  /* Nuke the connection to the bus */
+  g_timeout_add (100 /* ms */, on_timeout, NULL);
+
+  g_main_loop_run (loop);
+
+  /* If we didn't act on connection-loss we'd be getting G_IO_ERROR_TIMEOUT
+   * generated locally. So if we get G_IO_ERROR_CLOSED it means that we
+   * are acting correctly on connection loss.
+   */
+  g_assert_error (error, G_IO_ERROR, G_IO_ERROR_CLOSED);
+  g_assert (!g_dbus_error_is_remote_error (error));
+  g_clear_error (&error);
+
+  g_object_unref (proxy);
+}
+
+/* ---------------------------------------------------------------------------------------------------- */
+
+int
+main (int   argc,
+      char *argv[])
+{
+  GError *error;
+  gint ret;
+
+  g_type_init ();
+  g_thread_init (NULL);
+  g_test_init (&argc, &argv, NULL);
+
+  /* all the tests rely on a shared main loop */
+  loop = g_main_loop_new (NULL, FALSE);
+
+  /* all the tests use a session bus with a well-known address that we can bring up and down
+   * using session_bus_up() and session_bus_down().
+   */
+  g_unsetenv ("DISPLAY");
+  g_setenv ("DBUS_SESSION_BUS_ADDRESS", session_bus_get_temporary_address (), TRUE);
+
+  session_bus_up ();
+
+  /* TODO: wait a bit for the bus to come up.. ideally session_bus_up() won't return
+   * until one can connect to the bus but that's not how things work right now
+   */
+  usleep (500 * 1000);
+
+  /* this is safe; testserver will exit once the bus goes away */
+  g_assert (g_spawn_command_line_async (SRCDIR "/gdbus-testserver.py", NULL));
+
+  /* wait for the service to come up */
+  usleep (500 * 1000);
+
+  /* Create the connection in the main thread */
+  error = NULL;
+  c = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (c != NULL);
+
+  g_test_add_func ("/gdbus/connection-loss", test_connection_loss);
+
+  ret = g_test_run();
+
+  g_object_unref (c);
+
+  return ret;
+}