Rework TestService using asynchronos API of ExportedObject.

This change is to to exercise asynchronos API of ExportedObject.
The asynchronos API is implemented on top the synchronos API,
hence the synchronos API code is also covered.

Along the way, change EndToEndAsyncTest to use the D-Bus thread.
Simplified the test code per phajdan.jr's comments as well.

TEST=dbus_unittests
BUG=90036


Review URL: http://codereview.chromium.org/7671028

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@97540 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 168 insertions, 75 deletions

diff --git a/dbus/end_to_end_async_unittest.cc b/dbus/end_to_end_async_unittest.cc
index c3689ad..3ce6b6d 100644
--- a/dbus/end_to_end_async_unittest.cc
+++ b/dbus/end_to_end_async_unittest.cc
@@ -29,18 +29,21 @@ class EndToEndAsyncTest : public testing::Test {
     // Make the main thread not to allow IO.
     base::ThreadRestrictions::SetIOAllowed(false);
 
-    // Start the test service;
-    test_service_.reset(new dbus::TestService);
-    test_service_->StartService();
-    test_service_->WaitUntilServiceIsStarted();
-
     // Start the D-Bus thread.
     dbus_thread_.reset(new base::Thread("D-Bus Thread"));
     base::Thread::Options thread_options;
     thread_options.message_loop_type = MessageLoop::TYPE_IO;
-    dbus_thread_->StartWithOptions(thread_options);
+    ASSERT_TRUE(dbus_thread_->StartWithOptions(thread_options));
+
+    // Start the test service, using the D-Bus thread.
+    dbus::TestService::Options options;
+    options.dbus_thread = dbus_thread_.get();
+    test_service_.reset(new dbus::TestService(options));
+    ASSERT_TRUE(test_service_->StartService());
+    ASSERT_TRUE(test_service_->WaitUntilServiceIsStarted());
+    ASSERT_TRUE(test_service_->HasDBusThread());
 
-    // Create the client.
+    // Create the client, using the D-Bus thread.
     dbus::Bus::Options bus_options;
     bus_options.bus_type = dbus::Bus::SESSION;
     bus_options.connection_type = dbus::Bus::PRIVATE;
@@ -48,6 +51,7 @@ class EndToEndAsyncTest : public testing::Test {
     bus_ = new dbus::Bus(bus_options);
     object_proxy_ = bus_->GetObjectProxy("org.chromium.TestService",
                                          "/org/chromium/TestObject");
+    ASSERT_TRUE(bus_->HasDBusThread());
   }
 
   void TearDown() {
@@ -57,6 +61,10 @@ class EndToEndAsyncTest : public testing::Test {
     // mesage_loop_.
     message_loop_.Run();
 
+    // Shut down the service.
+    test_service_->Shutdown();
+    ASSERT_TRUE(test_service_->WaitUntilServiceIsShutdown());
+
     // Reset to the default.
     base::ThreadRestrictions::SetIOAllowed(true);
 
@@ -160,8 +168,8 @@ TEST_F(EndToEndAsyncTest, Timeout) {
   dbus::MessageWriter writer(&method_call);
   writer.AppendString(kHello);
 
-  // Call the method with timeout smaller than TestService::kSlowEchoSleepMs.
-  const int timeout_ms = dbus::TestService::kSlowEchoSleepMs / 10;
+  // Call the method with timeout of 0ms.
+  const int timeout_ms = 0;
   CallMethod(&method_call, timeout_ms);
   WaitForResponses(1);
 
diff --git a/dbus/end_to_end_sync_unittest.cc b/dbus/end_to_end_sync_unittest.cc
index 2f81878..704845e 100644
--- a/dbus/end_to_end_sync_unittest.cc
+++ b/dbus/end_to_end_sync_unittest.cc
@@ -20,9 +20,11 @@ class EndToEndSyncTest : public testing::Test {
 
   void SetUp() {
     // Start the test service;
-    test_service_.reset(new dbus::TestService);
-    test_service_->StartService();
-    test_service_->WaitUntilServiceIsStarted();
+    dbus::TestService::Options options;
+    test_service_.reset(new dbus::TestService(options));
+    ASSERT_TRUE(test_service_->StartService());
+    ASSERT_TRUE(test_service_->WaitUntilServiceIsStarted());
+    ASSERT_FALSE(test_service_->HasDBusThread());
 
     // Create the client.
     dbus::Bus::Options client_bus_options;
@@ -31,9 +33,12 @@ class EndToEndSyncTest : public testing::Test {
     client_bus_ = new dbus::Bus(client_bus_options);
     object_proxy_ = client_bus_->GetObjectProxy("org.chromium.TestService",
                                                 "/org/chromium/TestObject");
+    ASSERT_FALSE(client_bus_->HasDBusThread());
   }
 
   void TearDown() {
+    test_service_->Shutdown();
+    ASSERT_TRUE(test_service_->WaitUntilServiceIsShutdown());
     test_service_->Stop();
     client_bus_->ShutdownAndBlock();
   }
@@ -74,9 +79,9 @@ TEST_F(EndToEndSyncTest, Timeout) {
   dbus::MessageWriter writer(&method_call);
   writer.AppendString(kHello);
 
-  // Call the method with timeout smaller than TestService::kSlowEchoSleepMs.
+  // Call the method with timeout of 0ms.
   dbus::Response response;
-  const int timeout_ms = dbus::TestService::kSlowEchoSleepMs / 10;
+  const int timeout_ms = 0;
   const bool success =
       object_proxy_->CallMethodAndBlock(&method_call, timeout_ms, &response);
   // Should fail because of timeout.
diff --git a/dbus/exported_object.cc b/dbus/exported_object.cc
index 1793ed2..2ac1ff0 100644
--- a/dbus/exported_object.cc
+++ b/dbus/exported_object.cc
@@ -36,9 +36,9 @@ ExportedObject::ExportedObject(Bus* bus,
       service_name_(service_name),
       object_path_(object_path),
       object_is_registered_(false),
-      method_is_called_(false),
       response_from_method_(NULL),
-      on_method_is_called_(&method_is_called_lock_) {
+      on_method_is_called_(false /* manual_reset */,
+                           false /* initially_signaled */) {
 }
 
 ExportedObject::~ExportedObject() {
@@ -180,7 +180,6 @@ DBusHandlerResult ExportedObject::HandleMessage(
   Response* response = NULL;
   if (bus_->HasDBusThread()) {
     response_from_method_ = NULL;
-    method_is_called_ = false;
     // Post a task to run the method in the origin thread.
     bus_->PostTaskToOriginThread(FROM_HERE,
                                  base::Bind(&ExportedObject::RunMethod,
@@ -195,14 +194,11 @@ DBusHandlerResult ExportedObject::HandleMessage(
       const int kTimeoutSecs = 10;
       const base::TimeDelta timeout(
           base::TimeDelta::FromSeconds(kTimeoutSecs));
-      const base::Time start_time = base::Time::Now();
-
-      base::AutoLock auto_lock(method_is_called_lock_);
-      while (!method_is_called_) {
-        on_method_is_called_.TimedWait(timeout);
-        CHECK(base::Time::Now() - start_time < timeout)
-            << "Method " << absolute_method_name << " timed out";
-      }
+
+      const bool signaled = on_method_is_called_.TimedWait(timeout);
+      // Method not called is a fatal error. The method is likely stuck
+      // infinitely in the origin thread. No way to stop it from here.
+      CHECK(signaled) << "Method " << absolute_method_name << " not called";
     }
     response = response_from_method_;
   } else {
@@ -233,9 +229,7 @@ void ExportedObject::RunMethod(MethodCallCallback method_call_callback,
                                MethodCall* method_call) {
   bus_->AssertOnOriginThread();
 
-  base::AutoLock auto_lock(method_is_called_lock_);
   response_from_method_ = method_call_callback.Run(method_call);
-  method_is_called_ = true;
   on_method_is_called_.Signal();
 }
 
diff --git a/dbus/exported_object.h b/dbus/exported_object.h
index 8e03118..1d35c11 100644
--- a/dbus/exported_object.h
+++ b/dbus/exported_object.h
@@ -14,7 +14,7 @@
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
-#include "base/synchronization/condition_variable.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
 
 class MessageLoop;
@@ -128,10 +128,8 @@ class ExportedObject : public base::RefCountedThreadSafe<ExportedObject> {
   std::string service_name_;
   std::string object_path_;
   bool object_is_registered_;
-  bool method_is_called_;
   dbus::Response* response_from_method_;
-  base::Lock method_is_called_lock_;
-  base::ConditionVariable on_method_is_called_;
+  base::WaitableEvent on_method_is_called_;
 
   // The method table where keys are absolute method names (i.e. interface
   // name + method name), and values are the corresponding callbacks.
diff --git a/dbus/test_service.cc b/dbus/test_service.cc
index c25f803..a3bd001 100644
--- a/dbus/test_service.cc
+++ b/dbus/test_service.cc
@@ -5,6 +5,7 @@
 #include "dbus/test_service.h"
 
 #include "base/bind.h"
+#include "base/test/test_timeouts.h"
 #include "base/threading/platform_thread.h"
 #include "dbus/bus.h"
 #include "dbus/exported_object.h"
@@ -12,71 +13,131 @@
 
 namespace dbus {
 
-const int TestService::kSlowEchoSleepMs = 100;  // In milliseconds.
+// Echo, SlowEcho, BrokenMethod.
+const int TestService::kNumMethodsToExport = 3;
 
-TestService::TestService()
+TestService::Options::Options()
+    : dbus_thread(NULL) {
+}
+
+TestService::Options::~Options() {
+}
+
+TestService::TestService(const Options& options)
     : base::Thread("TestService"),
-      service_started_(false),
-      on_service_started_(&service_started_lock_) {
+      dbus_thread_(options.dbus_thread),
+      on_shutdown_(false /* manual_reset */, false /* initially_signaled */),
+      on_all_methods_exported_(false, false),
+      num_exported_methods_(0) {
 }
 
 TestService::~TestService() {
 }
 
-void TestService::StartService() {
+bool TestService::StartService() {
   base::Thread::Options thread_options;
   thread_options.message_loop_type = MessageLoop::TYPE_IO;
-  StartWithOptions(thread_options);
+  return StartWithOptions(thread_options);
+}
+
+bool TestService::WaitUntilServiceIsStarted() {
+  const base::TimeDelta timeout(
+      base::TimeDelta::FromMilliseconds(
+          TestTimeouts::action_max_timeout_ms()));
+  // Wait until all methods are exported.
+  return on_all_methods_exported_.TimedWait(timeout);
 }
 
-void TestService::WaitUntilServiceIsStarted() {
+void TestService::Shutdown() {
   message_loop()->PostTask(
       FROM_HERE,
-      base::Bind(&TestService::OnServiceStarted,
+      base::Bind(&TestService::ShutdownInternal,
                  base::Unretained(this)));
-  base::AutoLock auto_lock(service_started_lock_);
-  while (!service_started_)
-    on_service_started_.Wait();
 }
 
-void TestService::OnServiceStarted() {
-  base::AutoLock auto_lock(service_started_lock_);
-  service_started_ = true;
-  on_service_started_.Signal();
+bool TestService::WaitUntilServiceIsShutdown() {
+  const base::TimeDelta timeout(
+      base::TimeDelta::FromMilliseconds(
+          TestTimeouts::action_max_timeout_ms()));
+  return on_shutdown_.TimedWait(timeout);
+}
+
+bool TestService::HasDBusThread() {
+  return bus_->HasDBusThread();
+}
+
+void TestService::ShutdownInternal() {
+  bus_->Shutdown(base::Bind(&TestService::OnShutdown,
+                            base::Unretained(this)));
+}
+
+void TestService::OnExported(const std::string& interface_name,
+                             const std::string& method_name,
+                             bool success) {
+  if (!success) {
+    LOG(ERROR) << "Failed to export: " << interface_name << "."
+               << method_name;
+    // Returning here will make WaitUntilServiceIsStarted() to time out
+    // and return false.
+    return;
+  }
+
+  ++num_exported_methods_;
+  if (num_exported_methods_ == kNumMethodsToExport)
+    on_all_methods_exported_.Signal();
+}
+
+void TestService::OnShutdown() {
+  on_shutdown_.Signal();
 }
 
 void TestService::Run(MessageLoop* message_loop) {
   Bus::Options bus_options;
   bus_options.bus_type = Bus::SESSION;
   bus_options.connection_type = Bus::PRIVATE;
+  bus_options.dbus_thread = dbus_thread_;
   bus_ = new Bus(bus_options);
 
   exported_object_ = bus_->GetExportedObject(
       "org.chromium.TestService",
       "/org/chromium/TestObject");
-  CHECK(exported_object_->ExportMethodAndBlock(
+
+  int num_methods = 0;
+  exported_object_->ExportMethod(
       "org.chromium.TestInterface",
       "Echo",
       base::Bind(&TestService::Echo,
-                 base::Unretained(this))));
-  CHECK(exported_object_->ExportMethodAndBlock(
+                 base::Unretained(this)),
+      base::Bind(&TestService::OnExported,
+                 base::Unretained(this)));
+  ++num_methods;
+
+  exported_object_->ExportMethod(
       "org.chromium.TestInterface",
       "SlowEcho",
       base::Bind(&TestService::SlowEcho,
-                 base::Unretained(this))));
-  CHECK(exported_object_->ExportMethodAndBlock(
+                 base::Unretained(this)),
+      base::Bind(&TestService::OnExported,
+                 base::Unretained(this)));
+  ++num_methods;
+
+  exported_object_->ExportMethod(
       "org.chromium.TestInterface",
       "BrokenMethod",
       base::Bind(&TestService::BrokenMethod,
-                 base::Unretained(this))));
+                 base::Unretained(this)),
+      base::Bind(&TestService::OnExported,
+                 base::Unretained(this)));
+  ++num_methods;
 
+  // Just print an error message as we don't want to crash tests.
+  // Tests will fail at a call to WaitUntilServiceIsStarted().
+  if (num_methods != kNumMethodsToExport) {
+    LOG(ERROR) << "The number of methods does not match";
+  }
   message_loop->Run();
 }
 
-void TestService::CleanUp() {
-  bus_->ShutdownAndBlock();
-}
-
 Response* TestService::Echo(MethodCall* method_call) {
   MessageReader reader(method_call);
   std::string text_message;
@@ -90,7 +151,7 @@ Response* TestService::Echo(MethodCall* method_call) {
 }
 
 Response* TestService::SlowEcho(MethodCall* method_call) {
-  base::PlatformThread::Sleep(kSlowEchoSleepMs);
+  base::PlatformThread::Sleep(TestTimeouts::tiny_timeout_ms());
   return Echo(method_call);
 }
 
diff --git a/dbus/test_service.h b/dbus/test_service.h
index 362ecb7..7d1abf7 100644
--- a/dbus/test_service.h
+++ b/dbus/test_service.h
@@ -6,10 +6,10 @@
 #define DBUS_TEST_SERVICE_H_
 #pragma once
 
+#include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/thread.h"
-#include "base/synchronization/condition_variable.h"
-#include "base/synchronization/lock.h"
+#include "base/synchronization/waitable_event.h"
 
 namespace dbus {
 
@@ -23,28 +23,53 @@ class Response;
 // the main thread. Methods such as Echo() and SlowEcho() are exported.
 class TestService : public base::Thread {
  public:
-  // SlowEcho() sleeps for this period of time before returns.
-  static const int kSlowEchoSleepMs;
+  // Options for the test service.
+  struct Options {
+    Options();
+    ~Options();
 
-  TestService();
+    // NULL by default (i.e. don't use the D-Bus thread).
+    base::Thread* dbus_thread;
+  };
+
+  // The number of methods we'll export.
+  static const int kNumMethodsToExport;
+
+  TestService(const Options& options);
   virtual ~TestService();
 
   // Starts the service in a separate thread.
-  void StartService();
+  // Returns true if the thread is started successfully.
+  bool StartService();
+
+  // Waits until the service is started (i.e. all methods are exported).
+  // Returns true on success.
+  bool WaitUntilServiceIsStarted() WARN_UNUSED_RESULT;
+
+  // Shuts down the service.
+  void Shutdown();
+
+  // Waits until the service is shut down.
+  // Returns true on success.
+  bool WaitUntilServiceIsShutdown() WARN_UNUSED_RESULT;
 
-  // Waits until the service is started (i.e. methods are exported).
-  void WaitUntilServiceIsStarted();
+  // Returns true if the bus has the D-Bus thread.
+  bool HasDBusThread();
 
  private:
-  // Called when the service is started (i.e. the task is run from the
-  // message loop).
-  void OnServiceStarted();
+  // Helper function used in Shutdown().
+  void ShutdownInternal();
 
-  // base::Thread override.
-  virtual void Run(MessageLoop* message_loop);
+  // Called when a method is exported.
+  void OnExported(const std::string& interface_name,
+                  const std::string& method_name,
+                  bool success);
+
+  // Called when the bus is shut down.
+  void OnShutdown();
 
   // base::Thread override.
-  virtual void CleanUp();
+  virtual void Run(MessageLoop* message_loop);
 
   //
   // Exported methods.
@@ -54,15 +79,17 @@ class TestService : public base::Thread {
   Response* Echo(MethodCall* method_call);
 
   // Echos the text message received from the method call, but sleeps for
-  // kSlowEchoSleepMs before returning the response.
+  // TestTimeouts::tiny_timeout_ms() before returning the response.
   Response* SlowEcho(MethodCall* method_call);
 
   // Returns NULL, instead of a valid Response.
   Response* BrokenMethod(MethodCall* method_call);
 
-  bool service_started_;
-  base::Lock service_started_lock_;
-  base::ConditionVariable on_service_started_;
+  base::Thread* dbus_thread_;
+  base::WaitableEvent on_shutdown_;
+  base::WaitableEvent on_all_methods_exported_;
+  // The number of methods actually exported.
+  int num_exported_methods_;
 
   scoped_refptr<Bus> bus_;
   ExportedObject* exported_object_;