Add Bus::ShutdownOnDBusThreadAndBlock() and remove bus::Shutdown()
This function is intended to use at the the very end of the browser shutdown,
where it it makes more sense to shut down the bus synchronously, than trying
to make it asynchronous.
Remove Bus::Shutdown() as we are unlikely to need it for the production code.
BUG=chromium:90036
TEST=dbus_unittests
Review URL: http://codereview.chromium.org/7830009
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@99331 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: e20d39fe8164f6633d8224030133a1c6ad18a289
diff --git a/dbus/bus.cc b/dbus/bus.cc
index 1b766ac..3651332 100644
--- a/dbus/bus.cc
+++ b/dbus/bus.cc
@@ -14,6 +14,7 @@
#include "base/stl_util.h"
#include "base/threading/thread.h"
#include "base/threading/thread_restrictions.h"
+#include "base/time.h"
#include "dbus/exported_object.h"
#include "dbus/object_proxy.h"
#include "dbus/scoped_dbus_error.h"
@@ -178,11 +179,13 @@
: bus_type_(options.bus_type),
connection_type_(options.connection_type),
dbus_thread_(options.dbus_thread),
+ on_shutdown_(false /* manual_reset */, false /* initially_signaled */),
connection_(NULL),
origin_loop_(MessageLoop::current()),
origin_thread_id_(base::PlatformThread::CurrentId()),
dbus_thread_id_(base::kInvalidThreadId),
async_operations_set_up_(false),
+ shutdown_completed_(false),
num_pending_watches_(0),
num_pending_timeouts_(0) {
if (dbus_thread_) {
@@ -299,21 +302,31 @@
}
// Private connection should be closed.
- if (connection_ && connection_type_ == PRIVATE) {
- dbus_connection_close(connection_);
+ if (connection_) {
+ if (connection_type_ == PRIVATE)
+ dbus_connection_close(connection_);
+ // dbus_connection_close() won't unref.
+ dbus_connection_unref(connection_);
}
- // dbus_connection_close() won't unref.
- dbus_connection_unref(connection_);
connection_ = NULL;
+ shutdown_completed_ = true;
}
-void Bus::Shutdown(OnShutdownCallback callback) {
+void Bus::ShutdownOnDBusThreadAndBlock() {
AssertOnOriginThread();
+ DCHECK(dbus_thread_);
- PostTaskToDBusThread(FROM_HERE, base::Bind(&Bus::ShutdownInternal,
- this,
- callback));
+ PostTaskToDBusThread(FROM_HERE, base::Bind(
+ &Bus::ShutdownOnDBusThreadAndBlockInternal,
+ this));
+
+ // Wait until the shutdown is complete on the D-Bus thread.
+ // The shutdown should not hang, but set timeout just in case.
+ const int kTimeoutSecs = 3;
+ const base::TimeDelta timeout(base::TimeDelta::FromSeconds(kTimeoutSecs));
+ const bool signaled = on_shutdown_.TimedWait(timeout);
+ LOG_IF(ERROR, !signaled) << "Failed to shutdown the bus";
}
bool Bus::RequestOwnership(const std::string& service_name) {
@@ -535,11 +548,11 @@
registered_object_paths_.erase(object_path);
}
-void Bus::ShutdownInternal(OnShutdownCallback callback) {
+void Bus::ShutdownOnDBusThreadAndBlockInternal() {
AssertOnDBusThread();
ShutdownAndBlock();
- PostTaskToOriginThread(FROM_HERE, callback);
+ on_shutdown_.Signal();
}
void Bus::ProcessAllIncomingDataIfAny() {
diff --git a/dbus/bus.h b/dbus/bus.h
index 792eb83..5be6ddc 100644
--- a/dbus/bus.h
+++ b/dbus/bus.h
@@ -14,6 +14,7 @@
#include "base/callback.h"
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/waitable_event.h"
#include "base/threading/platform_thread.h"
#include "base/tracked_objects.h"
@@ -61,9 +62,9 @@
//
// SHUTDOWN
//
-// The Bus object must be shut down manually by Shutdown() or
-// ShutdownAndBlock(). We require the manual shutdown as we should not
-// issue blocking calls in the destructor.
+// The Bus object must be shut down manually by ShutdownAndBlock() and
+// friends. We require the manual shutdown to make the operation explicit
+// rather than doing it silently in the destructor.
//
// EXAMPLE USAGE:
//
@@ -119,8 +120,8 @@
// WHY IS THIS A REF COUNTED OBJECT?
//
// Bus is a ref counted object, to ensure that |this| of the object is
-// alive when callbacks referencing |this| are called. However, after
-// Shutdown() is called, |connection_| can be NULL. Hence, callbacks should
+// alive when callbacks referencing |this| are called. However, after the
+// bus is shut down, |connection_| can be NULL. Hence, callbacks should
// not rely on that |connection_| is alive.
class Bus : public base::RefCountedThreadSafe<Bus> {
public:
@@ -163,9 +164,6 @@
base::Thread* dbus_thread; // NULL by default.
};
- // Called when shutdown is done. Used for Shutdown().
- typedef base::Callback<void ()> OnShutdownCallback;
-
// Creates a Bus object. The actual connection will be established when
// Connect() is called.
explicit Bus(const Options& options);
@@ -219,11 +217,17 @@
// BLOCKING CALL.
virtual void ShutdownAndBlock();
- // Shuts down the bus in the D-Bus thread. |callback| will be called in
- // the origin thread.
+ // Similar to ShutdownAndBlock(), but this function is used to
+ // synchronously shut down the bus that uses the D-Bus thread. This
+ // function is intended to be used at the very end of the browser
+ // shutdown, where it makes more sense to shut down the bus
+ // synchronously, than trying to make it asynchronous.
//
- // Must be called in the origin thread.
- virtual void Shutdown(OnShutdownCallback callback);
+ // BLOCKING CALL, but must be called in the origin thread.
+ virtual void ShutdownOnDBusThreadAndBlock();
+
+ // Returns true if the shutdown has been completed.
+ bool shutdown_completed() { return shutdown_completed_; }
//
// The public functions below are not intended to be used in client
@@ -381,8 +385,8 @@
private:
friend class base::RefCountedThreadSafe<Bus>;
- // Helper function used for Shutdown().
- void ShutdownInternal(OnShutdownCallback callback);
+ // Helper function used for ShutdownOnDBusThreadAndBlock().
+ void ShutdownOnDBusThreadAndBlockInternal();
// Processes the all incoming data to the connection, if any.
//
@@ -427,6 +431,7 @@
const BusType bus_type_;
const ConnectionType connection_type_;
base::Thread* dbus_thread_;
+ base::WaitableEvent on_shutdown_;
DBusConnection* connection_;
MessageLoop* origin_loop_;
@@ -455,6 +460,7 @@
ExportedObjectTable exported_object_table_;
bool async_operations_set_up_;
+ bool shutdown_completed_;
// Counters to make sure that OnAddWatch()/OnRemoveWatch() and
// OnAddTimeout()/OnRemoveTimeou() are balanced.
diff --git a/dbus/bus_unittest.cc b/dbus/bus_unittest.cc
index 5cd198d..7d477e2 100644
--- a/dbus/bus_unittest.cc
+++ b/dbus/bus_unittest.cc
@@ -4,7 +4,10 @@
#include "dbus/bus.h"
+#include "base/bind.h"
+#include "base/message_loop.h"
#include "base/memory/ref_counted.h"
+#include "base/threading/thread.h"
#include "dbus/exported_object.h"
#include "dbus/object_proxy.h"
@@ -57,3 +60,32 @@
ASSERT_TRUE(object_proxy3);
EXPECT_NE(object_proxy1, object_proxy3);
}
+
+TEST(BusTest, ShutdownAndBlock) {
+ dbus::Bus::Options options;
+ scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
+ ASSERT_FALSE(bus->shutdown_completed());
+
+ // Shut down synchronously.
+ bus->ShutdownAndBlock();
+ EXPECT_TRUE(bus->shutdown_completed());
+}
+
+TEST(BusTest, ShutdownAndBlockWithDBusThread) {
+ // Start the D-Bus thread.
+ base::Thread::Options thread_options;
+ thread_options.message_loop_type = MessageLoop::TYPE_IO;
+ base::Thread dbus_thread("D-Bus thread");
+ dbus_thread.StartWithOptions(thread_options);
+
+ // Create the bus.
+ dbus::Bus::Options options;
+ options.dbus_thread = &dbus_thread;
+ scoped_refptr<dbus::Bus> bus = new dbus::Bus(options);
+ ASSERT_FALSE(bus->shutdown_completed());
+
+ // Shut down synchronously.
+ bus->ShutdownOnDBusThreadAndBlock();
+ EXPECT_TRUE(bus->shutdown_completed());
+ dbus_thread.Stop();
+}
diff --git a/dbus/end_to_end_async_unittest.cc b/dbus/end_to_end_async_unittest.cc
index 41b06b6..cea37a8 100644
--- a/dbus/end_to_end_async_unittest.cc
+++ b/dbus/end_to_end_async_unittest.cc
@@ -66,15 +66,10 @@
}
virtual void TearDown() {
- bus_->Shutdown(base::Bind(&EndToEndAsyncTest::OnShutdown,
- base::Unretained(this)));
- // Wait until the bus is shutdown. OnShutdown() will be called in
- // message_loop_.
- message_loop_.Run();
+ bus_->ShutdownOnDBusThreadAndBlock();
// Shut down the service.
- test_service_->Shutdown();
- ASSERT_TRUE(test_service_->WaitUntilServiceIsShutdown());
+ test_service_->ShutdownAndBlock();
// Reset to the default.
base::ThreadRestrictions::SetIOAllowed(true);
@@ -117,11 +112,6 @@
message_loop_.Quit();
};
- // Called when the shutdown is complete.
- void OnShutdown() {
- message_loop_.Quit();
- }
-
// Called when the "Test" signal is received, in the main thread.
// Copy the string payload to |test_signal_string_|.
void OnTestSignal(dbus::Signal* signal) {
diff --git a/dbus/end_to_end_sync_unittest.cc b/dbus/end_to_end_sync_unittest.cc
index 8968985..c338809 100644
--- a/dbus/end_to_end_sync_unittest.cc
+++ b/dbus/end_to_end_sync_unittest.cc
@@ -37,8 +37,7 @@
}
virtual void TearDown() {
- test_service_->Shutdown();
- ASSERT_TRUE(test_service_->WaitUntilServiceIsShutdown());
+ test_service_->ShutdownAndBlock();
test_service_->Stop();
client_bus_->ShutdownAndBlock();
}
diff --git a/dbus/mock_bus.h b/dbus/mock_bus.h
index f1c5b4e..7b949cb 100644
--- a/dbus/mock_bus.h
+++ b/dbus/mock_bus.h
@@ -25,7 +25,7 @@
const std::string& service_name,
const std::string& object_path));
MOCK_METHOD0(ShutdownAndBlock, void());
- MOCK_METHOD1(Shutdown, void(OnShutdownCallback callback));
+ MOCK_METHOD0(ShutdownOnDBusThreadAndBlock, void());
MOCK_METHOD0(Connect, bool());
MOCK_METHOD1(RequestOwnership, bool(const std::string& service_name));
MOCK_METHOD1(ReleaseOwnership, bool(const std::string& service_name));