Shill: ShillServiceClient destroys ShillClientHelpers when they are not need.

chromeos/dbus/shill_service_client.cc

Index: chromeos/dbus/shill_service_client.cc
diff --git a/chromeos/dbus/shill_service_client.cc b/chromeos/dbus/shill_service_client.cc
index 8e0ede00f565ea2ecb7d795702c48a3891816029..ccb1ef53743325e57c88f06609298e52e90bc1a6 100644
--- a/chromeos/dbus/shill_service_client.cc
+++ b/chromeos/dbus/shill_service_client.cc
@@ -40,12 +40,81 @@ void OnGetPropertiesError(
   callback.Run(DBUS_METHOD_CALL_FAILURE, empty_dictionary);
 }
 
+// ShillClientHelper callback wrappers.
+// For each one of the different callback types in ShillClientHelper, we define
+// a wrapper that attachs a clousure to the given callback. This allows it to
+// call a second callback (the clousure) after the first callback is called.
+// This is used to get a notification in the ShillServiceClient when a
+// MethodCall returns.
+
+// * Wrapper for DictionaryValueCallback.
+void RunDictionaryValueCallback(
+    const base::Closure& closure,
+    const ShillClientHelper::DictionaryValueCallback& callback,
+    DBusMethodCallStatus call_status,
+    const DictionaryValue& argument) {
+  callback.Run(call_status, argument);
+  closure.Run();
+}
+
+ShillClientHelper::DictionaryValueCallback
+    AttachClosureToDictionaryValueCallback(
+    const ShillClientHelper::DictionaryValueCallback& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunDictionaryValueCallback, closure, callback);
+}
+

[stevenjb, 2013/02/07 01:12:30]

So, I apologize for continuing to iterate on this, I know we want to get this in
for 26, but I still find this really confusing. Couldn't we do something like
this:

void ShillServiceClientImpl::SuccessDictionaryValueCallback(service_path,
callback, argument) {
  callback.Run(DBUS_METHOD_CALL_SUCCESS, argument);
  OnHelperStatusUpdate(service_path);
}

void ShillServiceClientImpl::ErrorDictionaryValueCallback(service_path,
callback, error_name, error_message) {
  OnGetPropertiesError(service_path, callback, error_name, error_message);
  OnHelperStatusUpdate(service_path);
}

// similar for ListValue and Void

Then, the code below would look something like:


  GetHelper(service_path)->CallDictionaryValueMethodWithErrorCallback(
    &method_call,
    base::Bind(&ShillServiceClientImpl::SuccessDictionaryValueCallback,
               weak_ptr_factory_.GetWeakPtr(), service_path, callback),
    base::Bind(&ShillServiceClientImpl::ErrorDictionaryValueCallback,
               weak_ptr_factory_.GetWeakPtr(), service_path, callback));

Then we wouldn't need all of these rather confusing AttachCloserTo* wrappers.

What do you think?

[hashimoto, 2013/02/07 05:20:31]

Wasn't the bug marked as P1/ReleaseBlocker?
I think the fix would be worth merging even after the M26 branch cut.
Or, simply keep helpers alive as we did, unless the size of |helpers_| exceeds a
certain threshold (64, 128, 256, or whatever appropriate)
When it exceeds the threshold, we can delete all unneeded helpers.
(Eventually, this looks like a LRU approach which we have abandoned because it
was too complicated. But the current change looks more complicated than it.)

+// * Wrapper for ListValueCallback
+void RunListValueCallback(
+    const base::Closure& closure,
+    const ShillClientHelper::ListValueCallback& callback,
+    const ListValue& argument) {
+  callback.Run(argument);
+  closure.Run();
+}
+
+ShillClientHelper::ListValueCallback AttachClosureToListValueCallback(
+    const ShillClientHelper::ListValueCallback& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunListValueCallback, closure, callback);
+}
+
+// * Wrapper for ErrorCallback
+void RunErrorCallback(
+    const base::Closure& closure,
+    const ShillClientHelper::ErrorCallback& callback,
+    const std::string& error_name,
+    const std::string& error_message) {
+  callback.Run(error_name, error_message);
+  closure.Run();
+}
+
+ShillClientHelper::ErrorCallback AttachClosureToErrorCallback(
+    const ShillClientHelper::ErrorCallback& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunErrorCallback, closure, callback);
+}
+
+// * Wrapper for Closure
+void RunClosure(const base::Closure& closure,
+                const base::Closure& callback) {
+  callback.Run();
+  closure.Run();
+}
+
+base::Closure AttachClosureToClosure(
+    const base::Closure& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunClosure, closure, callback);
+}
+
 // The ShillServiceClient implementation.
 class ShillServiceClientImpl : public ShillServiceClient {
  public:
   explicit ShillServiceClientImpl(dbus::Bus* bus)
       : bus_(bus),
-        helpers_deleter_(&helpers_) {
+        helpers_deleter_(&helpers_),
+        ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   }
 
   /////////////////////////////////////
@@ -60,6 +129,7 @@ class ShillServiceClientImpl : public ShillServiceClient {
       const dbus::ObjectPath& service_path,
       ShillPropertyChangedObserver* observer) OVERRIDE {
     GetHelper(service_path)->RemovePropertyChangedObserver(observer);
+    OnHelperStatusUpdate(service_path);
   }
 
   virtual void GetProperties(const dbus::ObjectPath& service_path,
@@ -68,8 +138,14 @@ class ShillServiceClientImpl : public ShillServiceClient {
                                  flimflam::kGetPropertiesFunction);
     GetHelper(service_path)->CallDictionaryValueMethodWithErrorCallback(
         &method_call,
-        base::Bind(callback, DBUS_METHOD_CALL_SUCCESS),
-        base::Bind(&OnGetPropertiesError, service_path, callback));
+        base::Bind(
+            AttachClosureToDictionaryValueCallback(callback,
+                base::Bind(&ShillServiceClientImpl::OnHelperStatusUpdate,
+                    weak_ptr_factory_.GetWeakPtr(), service_path)),
+            DBUS_METHOD_CALL_SUCCESS),
+        AttachClosureToErrorCallback(
+            base::Bind(&OnGetPropertiesError, service_path, callback),
+            HelperStatusUpdateCallback(service_path)));
   }
 
   virtual void SetProperty(const dbus::ObjectPath& service_path,
@@ -82,9 +158,12 @@ class ShillServiceClientImpl : public ShillServiceClient {
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(name);
     ShillClientHelper::AppendValueDataAsVariant(&writer, value);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(service_path)));
   }
 
   virtual void ClearProperty(const dbus::ObjectPath& service_path,
@@ -95,9 +174,12 @@ class ShillServiceClientImpl : public ShillServiceClient {
                                  flimflam::kClearPropertyFunction);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(name);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(service_path)));
   }
 
 
@@ -111,8 +193,10 @@ class ShillServiceClientImpl : public ShillServiceClient {
     writer.AppendArrayOfStrings(names);
     GetHelper(service_path)->CallListValueMethodWithErrorCallback(
         &method_call,
-        callback,
-        error_callback);
+        AttachClosureToListValueCallback(callback,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(service_path)));
   }
 
   virtual void Connect(const dbus::ObjectPath& service_path,
@@ -121,7 +205,11 @@ class ShillServiceClientImpl : public ShillServiceClient {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kConnectFunction);
     GetHelper(service_path)->CallVoidMethodWithErrorCallback(
-        &method_call, callback, error_callback);
+        &method_call,
+        AttachClosureToClosure(callback,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(service_path)));
   }
 
   virtual void Disconnect(const dbus::ObjectPath& service_path,
@@ -129,9 +217,12 @@ class ShillServiceClientImpl : public ShillServiceClient {
                           const ErrorCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kDisconnectFunction);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(service_path)));
   }
 
   virtual void Remove(const dbus::ObjectPath& service_path,
@@ -139,9 +230,12 @@ class ShillServiceClientImpl : public ShillServiceClient {
                       const ErrorCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kRemoveServiceFunction);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(service_path)));
   }
 
   virtual void ActivateCellularModem(
@@ -153,9 +247,12 @@ class ShillServiceClientImpl : public ShillServiceClient {
                                  flimflam::kActivateCellularModemFunction);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(carrier);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(service_path)));
   }
 
   virtual bool CallActivateCellularModemAndBlock(
@@ -165,7 +262,9 @@ class ShillServiceClientImpl : public ShillServiceClient {
                                  flimflam::kActivateCellularModemFunction);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(carrier);
-    return GetHelper(service_path)->CallVoidMethodAndBlock(&method_call);
+    bool result = GetHelper(service_path)->CallVoidMethodAndBlock(&method_call);
+    OnHelperStatusUpdate(service_path);
+    return result;
   }
 
   virtual ShillServiceClient::TestInterface* GetTestInterface() OVERRIDE {
@@ -185,15 +284,48 @@ class ShillServiceClientImpl : public ShillServiceClient {
     dbus::ObjectProxy* object_proxy =
         bus_->GetObjectProxy(flimflam::kFlimflamServiceName, service_path);
     ShillClientHelper* helper = new ShillClientHelper(bus_, object_proxy);
+    // TODO(deymo): Delay the MonitorPropertyChanged call until an observer is
+    // added to the helper. If no observer is add and every call finishes we
+    // delete the helper making this AddMatch and RemoveMatch blocking cycle is
+    // useless.

[stevenjb, 2013/02/07 01:12:30]

This doesn't seem too difficult, I think it's worth doing now; I'm a little
concerned about the overhead of adding/removing the Monitoring. It also seems
like code that should be in the Helper anyway; we just need to implement
StartMonitorPropertyChanged() and make it keep track of whether or not it's
already been started.

     helper->MonitorPropertyChanged(flimflam::kFlimflamServiceInterface);
     helpers_.insert(HelperMap::value_type(service_path.value(), helper));
     return helper;
   }
 
+  // Returns a Closure that calls OnHelperStatusUpdate with the given
+  // |service_path| argument.
+  base::Closure HelperStatusUpdateCallback(
+      const dbus::ObjectPath& service_path) {
+    return base::Bind(&ShillServiceClientImpl::OnHelperStatusUpdate,
+        weak_ptr_factory_.GetWeakPtr(), service_path);
+  }
+
+  // This function is called every time the IsObserved or the IsWaitingResponse
+  // helper's properties may have changed. If both are false, the cached helper
+  // can be safely removed.
+  void OnHelperStatusUpdate(const dbus::ObjectPath& service_path) {
+    HelperMap::iterator it = helpers_.find(service_path.value());
+    if (it == helpers_.end())
+      return;
+
+    ShillClientHelper* const helper = it->second;
+    if (!helper->IsObserved() && !helper->IsWaitingResponse()) {
+      helpers_.erase(it);
+      delete helper;
+      bus_->RemoveObjectProxy(flimflam::kFlimflamServiceName, service_path,
+          base::Bind(&base::DoNothing));
+    }
+  }
+
   dbus::Bus* bus_;
   HelperMap helpers_;
   STLValueDeleter<HelperMap> helpers_deleter_;
 
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate its weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<ShillServiceClientImpl> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ShillServiceClientImpl);
 };
 
@@ -201,7 +333,8 @@ class ShillServiceClientImpl : public ShillServiceClient {
 class ShillServiceClientStubImpl : public ShillServiceClient,
                                    public ShillServiceClient::TestInterface {
  public:
-  ShillServiceClientStubImpl() : weak_ptr_factory_(this) {
+  ShillServiceClientStubImpl() :
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
     SetDefaultProperties();
   }