chrome: dbus: support asynchronous method replies

BUG=chromium-os:23241
TEST=Unit tests and manual testing on device.

Change-Id: Iab009ddbd12dea1e12299ae0ddccd4e430d9cf97


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

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

chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc

@@ -224,8 +224,9 @@ bool ProxyResolutionServiceProvider::OnOriginThread() {
   return base::PlatformThread::CurrentId() == origin_thread_id_;
 }
 
-dbus::Response* ProxyResolutionServiceProvider::ResolveProxyHandler(
-    dbus::MethodCall* method_call) {
+void ProxyResolutionServiceProvider::ResolveProxyHandler(
+    dbus::MethodCall* method_call,
+    dbus::ExportedObject::ResponseSender response_sender) {
   DCHECK(OnOriginThread());
   VLOG(1) << "Handing method call: " << method_call->ToString();
   // The method call should contain the three string parameters.
@@ -237,29 +238,32 @@ dbus::Response* ProxyResolutionServiceProvider::ResolveProxyHandler(
       !reader.PopString(&signal_interface) ||
       !reader.PopString(&signal_name)) {
     LOG(ERROR) << "Unexpected method call: " << method_call->ToString();
-    return NULL;
+    response_sender.Run(NULL);
+    return;
   }
 
   resolver_->ResolveProxy(source_url,
                           signal_interface,
                           signal_name,
                           exported_object_);
 
-  // Return an empty response for now. We'll send a signal once the
-  // network proxy resolution is completed.
+  // Send an empty response for now. We'll send a signal once the network proxy
+  // resolution is completed.
   dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-  return response;
+  response_sender.Run(response);
 }
 
 // static
-dbus::Response* ProxyResolutionServiceProvider::CallResolveProxyHandler(
+void ProxyResolutionServiceProvider::CallResolveProxyHandler(
     base::WeakPtr<ProxyResolutionServiceProvider> provider_weak_ptr,
-    dbus::MethodCall* method_call) {
+    dbus::MethodCall* method_call,
+    dbus::ExportedObject::ResponseSender response_sender) {
   if (!provider_weak_ptr) {
     LOG(WARNING) << "Called after the object is deleted";
-    return NULL;
+    response_sender.Run(NULL);
+    return;
   }
-  return provider_weak_ptr->ResolveProxyHandler(method_call);
+  provider_weak_ptr->ResolveProxyHandler(method_call, response_sender);
 }
 
 ProxyResolutionServiceProvider* ProxyResolutionServiceProvider::Create() {

chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h

@@ -15,9 +15,9 @@
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
 #include "chrome/browser/chromeos/dbus/cros_dbus_service.h"
+#include "dbus/exported_object.h"
 
 namespace dbus {
-class ExportedObject;
 class MethodCall;
 class Response;
 }
@@ -104,13 +104,15 @@ class ProxyResolutionServiceProvider
   // Callback to be invoked when ChromeOS clients send network proxy
   // resolution requests to the service running in chrome executable.
   // Called on UI thread from dbus request.
-  dbus::Response* ResolveProxyHandler(dbus::MethodCall* method_call);
+  void ResolveProxyHandler(dbus::MethodCall* method_call,
+      dbus::ExportedObject::ResponseSender response_sender);
 
   // Calls ResolveProxyHandler() if weak_ptr is not NULL. Used to ensure a
   // safe shutdown.
-  static dbus::Response* CallResolveProxyHandler(
+  static void CallResolveProxyHandler(
       base::WeakPtr<ProxyResolutionServiceProvider> weak_ptr,
-      dbus::MethodCall* method_call);
+      dbus::MethodCall* method_call,
+      dbus::ExportedObject::ResponseSender response_sender);
 
   // Returns true if the current thread is on the origin thread.
   bool OnOriginThread();

chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc

@@ -17,6 +17,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
+#include "base/message_loop.h"
 #include "dbus/message.h"
 #include "dbus/mock_bus.h"
 #include "dbus/mock_exported_object.h"
@@ -61,7 +62,8 @@ class ProxyResolutionServiceProviderTest : public testing::Test {
  public:
   ProxyResolutionServiceProviderTest()
       : signal_received_successfully_(false),
-        mock_resolver_(NULL) {
+        mock_resolver_(NULL),
+        response_received_(false) {
   }
 
   virtual void SetUp() {
@@ -102,12 +104,12 @@ class ProxyResolutionServiceProviderTest : public testing::Test {
                                   kLibCrosServiceName,
                                   kLibCrosServicePath);
     // |mock_object_proxy_|'s CallMethodAndBlock() will use
-    // CreateResponse() to return responses.
+    // MockCallMethodAndBlock() to return responses.
     EXPECT_CALL(*mock_object_proxy_,
                 CallMethodAndBlock(_, _))
         .WillOnce(Invoke(
             this,
-            &ProxyResolutionServiceProviderTest::CreateResponse));
+            &ProxyResolutionServiceProviderTest::MockCallMethodAndBlock));
     // |mock_object_proxy_|'s ConnectToSignal will use
     // MockConnectToSignal().
     EXPECT_CALL(*mock_object_proxy_,
@@ -185,6 +187,9 @@ class ProxyResolutionServiceProviderTest : public testing::Test {
   scoped_ptr<ProxyResolutionServiceProvider> proxy_resolution_service_;
   dbus::ExportedObject::MethodCallCallback resolve_network_proxy_;
   dbus::ObjectProxy::SignalCallback on_signal_callback_;
+  MessageLoop message_loop_;
+  bool response_received_;
+  scoped_ptr<dbus::Response> response_;
 
  private:
   // Behaves as |mock_exported_object_|'s ExportMethod().
@@ -237,24 +242,40 @@ class ProxyResolutionServiceProviderTest : public testing::Test {
     LOG(ERROR) << "Unexpected source URL: " << source_url;
   }
 
-  // Creates a response for |mock_object_proxy_|.
-  dbus::Response* CreateResponse(
+  // Calls exported method and waits for a response for |mock_object_proxy_|.
+  dbus::Response* MockCallMethodAndBlock(
       dbus::MethodCall* method_call,
       Unused) {
-    if (method_call->GetInterface() ==
-        kLibCrosServiceInterface &&
-        method_call->GetMember() == kResolveNetworkProxy) {
-      // Set the serial number to non-zero, so
-      // dbus_message_new_method_return() won't emit a warning.
-      method_call->SetSerial(1);
-      // Run the callback captured in MockExportMethod(). This will send
-      // a signal, which will be received by |on_signal_callback_|.
-      dbus::Response* response = resolve_network_proxy_.Run(method_call);
-      return response;
+    if (method_call->GetInterface() != kLibCrosServiceInterface ||
+        method_call->GetMember() != kResolveNetworkProxy) {
+      LOG(ERROR) << "Unexpected method call: " << method_call->ToString();
+      return NULL;
     }
+    // Set the serial number to non-zero, so
+    // dbus_message_new_method_return() won't emit a warning.
+    method_call->SetSerial(1);
+    // Run the callback captured in MockExportMethod(). In addition to returning
+    // a response that the caller will ignore, this will send a signal, which
+    // will be received by |on_signal_callback_|.
+    resolve_network_proxy_.Run(
+        method_call,
+        base::Bind(&ProxyResolutionServiceProviderTest::OnResponse,
+                   base::Unretained(this)));
+    // Wait for a response.
+    while (!response_received_) {
+      message_loop_.Run();
+    }
+    // Return response.
+    return response_.release();
+  }
 
-    LOG(ERROR) << "Unexpected method call: " << method_call->ToString();
-    return NULL;
+  // Receives a response and makes it available to MockCallMethodAndBlock().
+  void OnResponse(dbus::Response* response) {
+    response_.reset(response);
+    response_received_ = true;
+    if (message_loop_.is_running()) {
+        message_loop_.Quit();
+    }
   }
 
   // Behaves as |mock_object_proxy_|'s ConnectToSignal().

dbus/bus.h

@@ -100,11 +100,15 @@ class ObjectProxy;
 //
 // Exporting a method:
 //
-//   Response* Echo(dbus::MethodCall* method_call) {
+//   void Echo(dbus::MethodCall* method_call,
+//             dbus::ExportedObject::ResponseSender response_sender) {
 //     // Do something with method_call.
 //     Response* response = Response::FromMethodCall(method_call);
 //     // Build response here.
-//     return response;
+//     // Can send an immediate response here to implement a synchronous service
+//     // or store the response_sender and send a response later to implement an
+//     // asynchronous service.
+//     response_sender.Run(response);
 //   }
 //
 //   void OnExported(const std::string& interface_name,

dbus/end_to_end_async_unittest.cc

@@ -223,6 +223,24 @@ TEST_F(EndToEndAsyncTest, Timeout) {
   ASSERT_EQ("", response_strings_[0]);
 }
 
+// Tests calling a method that sends its reply asynchronously.
+TEST_F(EndToEndAsyncTest, AsyncEcho) {
+  const char* kHello = "hello";
+
+  // Create the method call.
+  dbus::MethodCall method_call("org.chromium.TestInterface", "AsyncEcho");
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendString(kHello);
+
+  // Call the method.
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethod(&method_call, timeout_ms);
+
+  // Check the response.
+  WaitForResponses(1);
+  EXPECT_EQ(kHello, response_strings_[0]);
+}
+
 TEST_F(EndToEndAsyncTest, NonexistentMethod) {
   dbus::MethodCall method_call("org.chromium.TestInterface", "Nonexistent");
 

dbus/exported_object.cc

@@ -224,12 +224,13 @@ DBusHandlerResult ExportedObject::HandleMessage(
                                             method_call.release(),
                                             start_time));
   } else {
-    // If the D-Bus thread is not used, just call the method directly. We
-    // don't need the complicated logic to wait for the method call to be
-    // complete.
-    // |response| will be deleted in OnMethodCompleted().
-    Response* response = iter->second.Run(method_call.get());
-    OnMethodCompleted(method_call.release(), response, start_time);
+    // If the D-Bus thread is not used, just call the method directly.
+    MethodCall* released_method_call = method_call.release();
+    iter->second.Run(released_method_call,
+                     base::Bind(&ExportedObject::SendResponse,
+                                this,
+                                start_time,
+                                released_method_call));
   }
 
   // It's valid to say HANDLED here, and send a method response at a later
@@ -241,14 +242,27 @@ void ExportedObject::RunMethod(MethodCallCallback method_call_callback,
                                MethodCall* method_call,
                                base::TimeTicks start_time) {
   bus_->AssertOnOriginThread();
+  method_call_callback.Run(method_call,
+                           base::Bind(&ExportedObject::SendResponse,
+                                      this,
+                                      start_time,
+                                      method_call));
+}
 
-  Response* response = method_call_callback.Run(method_call);
-  bus_->PostTaskToDBusThread(FROM_HERE,
-                             base::Bind(&ExportedObject::OnMethodCompleted,
-                                        this,
-                                        method_call,
-                                        response,
-                                        start_time));
+void ExportedObject::SendResponse(base::TimeTicks start_time,
+                                  MethodCall* method_call,
+                                  Response* response) {
+  DCHECK(method_call);
+  if (bus_->HasDBusThread()) {
+    bus_->PostTaskToDBusThread(FROM_HERE,
+                               base::Bind(&ExportedObject::OnMethodCompleted,
+                                          this,
+                                          method_call,
+                                          response,
+                                          start_time));
+  } else {
+    OnMethodCompleted(method_call, response, start_time);
+  }
 }
 
 void ExportedObject::OnMethodCompleted(MethodCall* method_call,

dbus/exported_object.h

@@ -38,9 +38,15 @@ class ExportedObject : public base::RefCountedThreadSafe<ExportedObject> {
                  const std::string& service_name,
                  const std::string& object_path);
 
+  // Called to send a response from an exported method. Response* is the
+  // response message. Callers should pass a NULL Response* in the event
+  // of an error that prevents the sending of a response.
+  typedef base::Callback<void (Response*)> ResponseSender;
+
   // Called when an exported method is called. MethodCall* is the request
-  // message.
-  typedef base::Callback<Response* (MethodCall*)> MethodCallCallback;
+  // message. ResponseSender is the callback that should be used to send a
+  // response.
+  typedef base::Callback<void (MethodCall*, ResponseSender)> MethodCallCallback;
 
   // Called when method exporting is done.
   // Parameters:
@@ -124,7 +130,14 @@ class ExportedObject : public base::RefCountedThreadSafe<ExportedObject> {
                  MethodCall* method_call,
                  base::TimeTicks start_time);
 
-  // Called on completion of the method run from RunMethod().
+  // Callback invoked by service provider to send a response to a method call.
+  // Can be called immediately from a MethodCallCallback to implement a
+  // synchronous service or called later to implement an asynchronous service.
+  void SendResponse(base::TimeTicks start_time,
+                    MethodCall* method_call,
+                    Response* response);
+
+  // Called on completion of the method run from SendResponse().
   // Takes ownership of |method_call| and |response|.
   void OnMethodCompleted(MethodCall* method_call,
                          Response* response,

dbus/test_service.cc

@@ -13,8 +13,8 @@
 
 namespace dbus {
 
-// Echo, SlowEcho, BrokenMethod.
-const int TestService::kNumMethodsToExport = 3;
+// Echo, SlowEcho, AsyncEcho, BrokenMethod.
+const int TestService::kNumMethodsToExport = 4;
 
 TestService::Options::Options() {
 }
@@ -146,6 +146,15 @@ void TestService::Run(MessageLoop* message_loop) {
                  base::Unretained(this)));
   ++num_methods;
 
+  exported_object_->ExportMethod(
+      "org.chromium.TestInterface",
+      "AsyncEcho",
+      base::Bind(&TestService::AsyncEcho,
+                 base::Unretained(this)),
+      base::Bind(&TestService::OnExported,
+                 base::Unretained(this)));
+  ++num_methods;
+
   exported_object_->ExportMethod(
       "org.chromium.TestInterface",
       "BrokenMethod",
@@ -163,25 +172,44 @@ void TestService::Run(MessageLoop* message_loop) {
   message_loop->Run();
 }
 
-Response* TestService::Echo(MethodCall* method_call) {
+void TestService::Echo(MethodCall* method_call,
+                       dbus::ExportedObject::ResponseSender response_sender) {
   MessageReader reader(method_call);
   std::string text_message;
-  if (!reader.PopString(&text_message))
-    return NULL;
+  if (!reader.PopString(&text_message)) {
+    response_sender.Run(NULL);
+    return;
+  }
 
   Response* response = Response::FromMethodCall(method_call);
   MessageWriter writer(response);
   writer.AppendString(text_message);
-  return response;
+  response_sender.Run(response);
 }
 
-Response* TestService::SlowEcho(MethodCall* method_call) {
+void TestService::SlowEcho(
+    MethodCall* method_call,
+    dbus::ExportedObject::ResponseSender response_sender) {
   base::PlatformThread::Sleep(TestTimeouts::tiny_timeout_ms());
-  return Echo(method_call);
+  Echo(method_call, response_sender);
+}
+
+void TestService::AsyncEcho(
+    MethodCall* method_call,
+    dbus::ExportedObject::ResponseSender response_sender) {
+  // Schedule a call to Echo() to send an asynchronous response after we return.
+  message_loop()->PostDelayedTask(FROM_HERE,
+                                  base::Bind(&TestService::Echo,
+                                             base::Unretained(this),
+                                             method_call,
+                                             response_sender),
+                                  TestTimeouts::tiny_timeout_ms());
 }
 
-Response* TestService::BrokenMethod(MethodCall* method_call) {
-  return NULL;
+void TestService::BrokenMethod(
+    MethodCall* method_call,
+    dbus::ExportedObject::ResponseSender response_sender) {
+  response_sender.Run(NULL);
 }
 
 }  // namespace dbus