Merge lp:~thomas-voss/dbus-cpp/fix-destruction-of-bus-instances into lp:dbus-cpp

[Thomas Voß]

Fix core::dbus::Bus desctruction and do not call dbus_shutdown unless explicitly requested by setting the env var DBUS_CPP_ENABLE_SHUTDOWN to 1

Fix destruction of core::dbus::Bus instances and do not call dbus_shutdown unless explicitly requested by setting DBUS_CPP_ENABLE_SHUTDOWN to 1 in the env of the process.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:61
http://jenkins.qa.ubuntu.com/job/dbus-cpp-ci/182/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-amd64-ci/22/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-armhf-ci/22/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/dbus-cpp-utopic-i386-ci/22/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/dbus-cpp-ci/182/rebuild

review: Needs Fixing (continuous-integration)

[Manuel de la Peña (mandel)]

review: Approve

[Dimitri John Ledkov (xnox)]

is this going to land?

Unmerged revisions

61. By Thomas Voß on 2014-06-29

Adjust name of env variable to DBUS_CPP_ENABLE_SHUTDOWN.

60. By Thomas Voß on 2014-06-29

Fix destruction of core::dbus::Bus instances.

=== modified file 'src/core/dbus/bus.cpp'
--- src/core/dbus/bus.cpp	2014-04-01 09:14:20 +0000
+++ src/core/dbus/bus.cpp	2014-06-29 04:29:39 +0000
@@ -30,6 +30,11 @@
 
 namespace
 {
+static constexpr const char* dbus_cpp_enable_shutdown_key
+{
+    "DBUS_CPP_ENABLE_SHUTDOWN"
+};
+
 struct VTable
 {
     static void unregister_object_path(DBusConnection*, void* data)
@@ -55,24 +60,20 @@
     std::shared_ptr<core::dbus::Object> object;
 };
 
-DBusHandlerResult static_handle_message(
-        DBusConnection* connection,
-        DBusMessage* message,
-        void* user_data)
-{
-    (void) connection;
-    auto thiz =
-            static_cast<core::dbus::Bus*>(user_data);
-    return static_cast<DBusHandlerResult>(
-                thiz->handle_message(
-                    core::dbus::Message::from_raw_message(
-                        message)));
-}
-
 void init_libdbus_thread_support_and_install_shutdown_handler()
 {
     static std::once_flag once;
-    std::call_once(once, []() { dbus_threads_init_default(); std::atexit(dbus_shutdown); });
+    std::call_once(once, []()
+    {
+        // We initialize the thread-safety support in libdbus by default.
+        dbus_threads_init_default();
+
+        // And we only clean up if explicitly requested in the environment.
+        // We account for the recommendations given in:
+        //   http://dbus.freedesktop.org/doc/api/html/group__DBusMemory.html#ga01912903e39428872920d861ef565bac
+        if (::secure_getenv(dbus_cpp_enable_shutdown_key))
+            ::atexit(dbus_shutdown);
+    });
 }
 }
 
@@ -101,13 +102,117 @@
 
 struct Bus::Private
 {
-    Private()
-        : connection(nullptr),
-          message_factory_impl(new impl::MessageFactory()),
-          message_type_router([](const Message::Ptr& msg) { return msg->type(); }),
-          signal_router([](const Message::Ptr& msg){ return msg->path(); })
-    {
-        init_libdbus_thread_support_and_install_shutdown_handler();
+    static DBusHandlerResult static_handle_message(
+            DBusConnection* connection,
+            DBusMessage* message,
+            void* user_data)
+    {
+        (void) connection;
+        auto thiz =
+                static_cast<Private*>(user_data);
+        return static_cast<DBusHandlerResult>(
+                    thiz->handle_message(
+                        core::dbus::Message::from_raw_message(
+                            message)));
+    }
+
+    static std::shared_ptr<DBusConnection> connect_to_well_known_bus(WellKnownBus bus)
+    {
+        Error se;
+        std::shared_ptr<DBusConnection> result
+        {
+            dbus_bus_get_private(static_cast<DBusBusType>(bus), std::addressof(se.raw())),
+            [](DBusConnection*){}
+        };
+
+        if (se) throw std::runtime_error
+        {
+            se.print()
+        };
+
+        return result;
+    }
+
+    static std::shared_ptr<DBusConnection> connect_to_address(const std::string& address)
+    {
+        Error se;
+        std::shared_ptr<DBusConnection> result
+        {
+            dbus_connection_open_private(address.c_str(), std::addressof(se.raw())),
+            [](DBusConnection*){}
+        };
+
+        if (se) throw std::runtime_error
+        {
+            se.print()
+        };
+
+        return result;
+    }
+
+    Private(const std::string& address)
+        : connection
+          {
+              connect_to_address(address)
+          },
+          message_factory_impl(new impl::MessageFactory()),
+          message_type_router([](const Message::Ptr& msg) { return msg->type(); }),
+          signal_router([](const Message::Ptr& msg){ return msg->path(); })
+    {
+        message_type_router.install_route(
+                    Message::Type::signal,
+                    std::bind(
+                        &Bus::SignalRouter::operator(),
+                        std::ref(signal_router),
+                        std::placeholders::_1));
+
+        dbus_connection_add_filter(
+                    connection.get(),
+                    static_handle_message,
+                    this,
+                    nullptr);
+
+        init_libdbus_thread_support_and_install_shutdown_handler();
+        dbus_connection_set_exit_on_disconnect(connection.get(), FALSE);
+    }
+
+    Private(WellKnownBus bus)
+        : connection
+          {
+              connect_to_well_known_bus(bus)
+          },
+          message_factory_impl(new impl::MessageFactory()),
+          message_type_router([](const Message::Ptr& msg) { return msg->type(); }),
+          signal_router([](const Message::Ptr& msg){ return msg->path(); })
+    {
+        message_type_router.install_route(
+                    Message::Type::signal,
+                    std::bind(
+                        &Bus::SignalRouter::operator(),
+                        std::ref(signal_router),
+                        std::placeholders::_1));
+
+        dbus_connection_add_filter(
+                    connection.get(),
+                    static_handle_message,
+                    this,
+                    nullptr);
+
+        init_libdbus_thread_support_and_install_shutdown_handler();
+        dbus_connection_set_exit_on_disconnect(connection.get(), FALSE);
+    }
+
+    ~Private()
+    {
+        dbus_connection_remove_filter(connection.get(), static_handle_message, this);
+        dbus_connection_close(connection.get());
+        dbus_connection_unref(connection.get());
+    }
+
+    Bus::MessageHandlerResult handle_message(const Message::Ptr& message)
+    {
+        message_type_router(message);
+        return Bus::MessageHandlerResult::not_yet_handled;
     }
 
     std::shared_ptr<DBusConnection> connection;
@@ -124,30 +229,8 @@
 }
 
 Bus::Bus(const std::string& address)
-    : d(new Private())
+    : d(new Private(address))
 {
-    Error se;
-    d->connection.reset(
-                dbus_connection_open_private(address.c_str(), std::addressof(se.raw())),
-                [](DBusConnection*){}
-    );
-
-    if (!d->connection)
-        throw std::runtime_error(se.print());
-
-    d->message_type_router.install_route(
-                Message::Type::signal,
-                std::bind(
-                    &Bus::SignalRouter::operator(),
-                    std::ref(d->signal_router),
-                    std::placeholders::_1));
-
-    dbus_connection_add_filter(
-                d->connection.get(),
-                static_handle_message,
-                this,
-                nullptr);
-
     auto message = dbus::Message::make_method_call(
                 DBus::name(),
                 DBus::path(),
@@ -158,43 +241,15 @@
 
     if (reply->type() == Message::Type::error)
         throw std::runtime_error(reply->error().print());
-
-    dbus_connection_set_exit_on_disconnect(d->connection.get(), FALSE);
 }
 
 Bus::Bus(WellKnownBus bus)
-    : d(new Private())
+    : d(new Private(bus))
 {
-    Error se;
-    d->connection.reset(
-                dbus_bus_get_private(static_cast<DBusBusType>(bus), std::addressof(se.raw())),
-                [](DBusConnection*){}
-    );
-
-    if (!d->connection)
-        throw std::runtime_error(se.print());
-
-    d->message_type_router.install_route(
-                Message::Type::signal,
-                std::bind(
-                    &Bus::SignalRouter::operator(),
-                    std::ref(d->signal_router),
-                    std::placeholders::_1));
-
-    dbus_connection_add_filter(
-                d->connection.get(),
-                static_handle_message,
-                this,
-                nullptr);
-
-    dbus_connection_set_exit_on_disconnect(d->connection.get(), FALSE);
 }
 
 Bus::~Bus() noexcept
 {
-    dbus_connection_remove_filter(d->connection.get(), static_handle_message, this);
-    dbus_connection_close(d->connection.get());
-    dbus_connection_unref(d->connection.get());
 }
 
 const std::shared_ptr<MessageFactory> Bus::message_factory()

=== modified file 'tests/executor_test.cpp'
--- tests/executor_test.cpp	2014-03-05 06:58:15 +0000
+++ tests/executor_test.cpp	2014-06-29 04:29:39 +0000
@@ -211,14 +211,12 @@
                 {
                     auto result = stub->transact_method<test::Service::Method, int64_t>();
 
-                    if (result.is_error())
-                        std::cout << result.error().print() << std::endl;
-                    else
+                    if (not result.is_error())
                         EXPECT_EQ(42, result.value());
 
-                } catch(const std::runtime_error& e)
+                } catch(const std::runtime_error&)
                 {
-                    std::cout << e.what() << std::endl;
+                    // We silently drop this exception.
                 }
             }
         });
@@ -231,14 +229,12 @@
                 {
                     auto result = stub->transact_method<test::Service::Method, int64_t>();
 
-                    if (result.is_error())
-                        std::cout << result.error().print() << std::endl;
-                    else
+                    if (not result.is_error())
                         EXPECT_EQ(42, result.value());
 
-                } catch(const std::runtime_error& e)
+                } catch(const std::runtime_error&)
                 {
-                    std::cout << e.what() << std::endl;
+                    // We silently drop this exception.
                 }
             }
         });