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

@@ -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);
@@ -2207,6 +2210,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,
@@ -2232,7 +2257,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);

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		\
@@ -297,6 +298,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)
 

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;
+}