From 47aec8389587584596c17e5f906022b3e3bb50c5 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 13 Mar 2014 12:06:07 +0000 Subject: [PATCH] Use a better NoReply message for disconnection with reply pending As an implementation detail, dbus-daemon handles this situation by artificially triggering a timeout (even if its configured timeout for method calls is in fact infinite). However, using the same debug message for both is misleading, and can lead people who are debugging a service crash to blame dbus-daemon instead, wasting their time. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=76112 --- bus/connection.c | 7 +- configure.ac | 1 + test/Makefile.am | 1 + .../data/valid-config-files/finite-timeout.conf.in | 19 +++++ test/dbus-daemon.c | 83 ++++++++++++++++++++++ 5 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 test/data/valid-config-files/finite-timeout.conf.in diff --git a/bus/connection.c b/bus/connection.c index ea2d155..af77bf0 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -1554,7 +1554,12 @@ bus_pending_reply_send_no_reply (BusConnections *connections, DBUS_ERROR_NO_REPLY)) goto out; - errmsg = "Message did not receive a reply (timeout by message bus)"; + /* If you change these messages, adjust test/dbus-daemon.c to match */ + if (pending->will_send_reply == NULL) + errmsg = "Message recipient disconnected from message bus without replying"; + else + errmsg = "Message did not receive a reply (timeout by message bus)"; + dbus_message_iter_init_append (message, &iter); if (!dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &errmsg)) goto out; diff --git a/configure.ac b/configure.ac index 13d1c87..2de7240 100644 --- a/configure.ac +++ b/configure.ac @@ -1775,6 +1775,7 @@ dbus-1.pc dbus-1-uninstalled.pc test/data/valid-config-files/debug-allow-all.conf test/data/valid-config-files/debug-allow-all-sha1.conf +test/data/valid-config-files/finite-timeout.conf test/data/valid-config-files/incoming-limit.conf test/data/valid-config-files-system/debug-allow-all-pass.conf test/data/valid-config-files-system/debug-allow-all-fail.conf diff --git a/test/Makefile.am b/test/Makefile.am index cec5cda..7f16414 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -249,6 +249,7 @@ in_data = \ data/valid-config-files-system/debug-allow-all-pass.conf.in \ data/valid-config-files/debug-allow-all-sha1.conf.in \ data/valid-config-files/debug-allow-all.conf.in \ + data/valid-config-files/finite-timeout.conf.in \ data/valid-config-files/incoming-limit.conf.in \ data/invalid-service-files-system/org.freedesktop.DBus.TestSuiteNoExec.service.in \ data/invalid-service-files-system/org.freedesktop.DBus.TestSuiteNoService.service.in \ diff --git a/test/data/valid-config-files/finite-timeout.conf.in b/test/data/valid-config-files/finite-timeout.conf.in new file mode 100644 index 0000000..7d26d71 --- /dev/null +++ b/test/data/valid-config-files/finite-timeout.conf.in @@ -0,0 +1,19 @@ + + + + session + @TEST_LISTEN@ + + + + + + + + + + + + 100 + diff --git a/test/dbus-daemon.c b/test/dbus-daemon.c index c883425..fa5178f 100644 --- a/test/dbus-daemon.c +++ b/test/dbus-daemon.c @@ -160,6 +160,10 @@ echo_filter (DBusConnection *connection, if (dbus_message_get_type (message) != DBUS_MESSAGE_TYPE_METHOD_CALL) return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; + /* WaitForever() never replies, emulating a service that has got stuck */ + if (dbus_message_is_method_call (message, "com.example", "WaitForever")) + return DBUS_HANDLER_RESULT_HANDLED; + reply = dbus_message_new_method_return (message); if (reply == NULL) @@ -341,6 +345,77 @@ pending_call_store_reply (DBusPendingCall *pc, } static void +test_no_reply (Fixture *f, + gconstpointer context) +{ + const Config *config = context; + DBusMessage *m; + DBusPendingCall *pc; + DBusMessage *reply = NULL; + enum { TIMEOUT, DISCONNECT } mode; + gboolean ok; + + if (f->skip) + return; + + g_test_bug ("76112"); + + if (config != NULL && config->config_file != NULL) + mode = TIMEOUT; + else + mode = DISCONNECT; + + m = dbus_message_new_method_call ( + dbus_bus_get_unique_name (f->right_conn), "/", + "com.example", "WaitForever"); + + add_echo_filter (f); + + if (m == NULL) + g_error ("OOM"); + + if (!dbus_connection_send_with_reply (f->left_conn, m, &pc, + DBUS_TIMEOUT_INFINITE) || + pc == NULL) + g_error ("OOM"); + + if (dbus_pending_call_get_completed (pc)) + pending_call_store_reply (pc, &reply); + else if (!dbus_pending_call_set_notify (pc, pending_call_store_reply, &reply, + NULL)) + g_error ("OOM"); + + dbus_pending_call_unref (pc); + dbus_message_unref (m); + + if (mode == DISCONNECT) + { + dbus_connection_remove_filter (f->right_conn, echo_filter, NULL); + dbus_connection_close (f->right_conn); + dbus_connection_unref (f->right_conn); + f->right_conn = NULL; + } + + while (reply == NULL) + test_main_context_iterate (f->ctx, TRUE); + + /* using inefficient string comparison for better assertion message */ + g_assert_cmpstr ( + dbus_message_type_to_string (dbus_message_get_type (reply)), ==, + dbus_message_type_to_string (DBUS_MESSAGE_TYPE_ERROR)); + ok = dbus_set_error_from_message (&f->e, reply); + g_assert (ok); + g_assert_cmpstr (f->e.name, ==, DBUS_ERROR_NO_REPLY); + + if (mode == DISCONNECT) + g_assert_cmpstr (f->e.message, ==, + "Message recipient disconnected from message bus without replying"); + else + g_assert_cmpstr (f->e.message, ==, + "Message did not receive a reply (timeout by message bus)"); +} + +static void test_creds (Fixture *f, gconstpointer context) { @@ -501,6 +576,10 @@ static Config limited_config = { "34393", 10000, "valid-config-files/incoming-limit.conf" }; +static Config finite_timeout_config = { + NULL, 1, "valid-config-files/finite-timeout.conf" +}; + int main (int argc, char **argv) @@ -511,6 +590,10 @@ main (int argc, g_test_add ("/echo/session", Fixture, NULL, setup, test_echo, teardown); g_test_add ("/echo/limited", Fixture, &limited_config, setup, test_echo, teardown); + g_test_add ("/no-reply/disconnect", Fixture, NULL, + setup, test_no_reply, teardown); + g_test_add ("/no-reply/timeout", Fixture, &finite_timeout_config, + setup, test_no_reply, teardown); g_test_add ("/creds", Fixture, NULL, setup, test_creds, teardown); return g_test_run (); -- 1.9.0