Concurrent DLC installation requests [pre platform batching/queueing]

It's possible now to handle concurrent installation requests for the
same DLC ID, this can be extended to any concurrent DLC requests as soon
as platform dlcservice can batch/queue installation requests.

This also simplifies the complex "holder" pattern used previously and
allows for it to scale once platform dlcservice starts batching/queueing
installation requests.

Bug: 1095715, 1052183
Test: ./out/Default/unit_tests
Change-Id: Id4e8dc025537d7e8fe81333cfefc09c349513829
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2261480
Auto-Submit: Jae Hoon Kim <kimjae@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781983}

chromeos/dbus/dlcservice/dlcservice_client.cc

@@ -8,6 +8,7 @@
 
 #include <algorithm>
 #include <deque>
+#include <map>
 #include <string>
 #include <unordered_set>
 #include <utility>
@@ -106,6 +107,13 @@ class DlcserviceClientImpl : public DlcserviceClient {
   void Install(const std::string& dlc_id,
                InstallCallback install_callback,
                ProgressCallback progress_callback) override {
+    // If another installation for the same DLC ID was already called, go ahead
+    // and hold the installation fields.
+    if (installation_holder_.find(dlc_id) != installation_holder_.end()) {
+      HoldInstallation(dlc_id, std::move(install_callback),
+                       std::move(progress_callback));
+      return;
+    }
     if (installing_) {
       EnqueueTask(base::BindOnce(&DlcserviceClientImpl::Install,
                                  weak_ptr_factory_.GetWeakPtr(),
@@ -120,15 +128,12 @@ class DlcserviceClientImpl : public DlcserviceClient {
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(dlc_id);
 
-    progress_callback_holder_ = std::move(progress_callback);
-    install_callback_holder_ = std::move(install_callback);
-    dlc_id_holder_ = dlc_id;
-
     VLOG(1) << "Requesting to install DLC(s).";
     dlcservice_proxy_->CallMethodWithErrorResponse(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&DlcserviceClientImpl::OnInstall,
-                       weak_ptr_factory_.GetWeakPtr()));
+        base::BindOnce(
+            &DlcserviceClientImpl::OnInstall, weak_ptr_factory_.GetWeakPtr(),
+            dlc_id, std::move(install_callback), std::move(progress_callback)));
   }
 
   void Uninstall(const std::string& dlc_id,
@@ -196,17 +201,35 @@ class DlcserviceClientImpl : public DlcserviceClient {
   }
 
  private:
+  // Fields related to an installation allowing for multiple installations to be
+  // in flight concurrently and handled by this dlcservice client. The callbacks
+  // are used to report progress and the final installation.
+  struct InstallationCallbacks {
+    InstallCallback install_callback;
+    ProgressCallback progress_callback;
+
+    InstallationCallbacks(InstallCallback install_callback,
+                          ProgressCallback progress_callback)
+        : install_callback(std::move(install_callback)),
+          progress_callback(std::move(progress_callback)) {}
+  };
+
   // Set the indication that an install is being performed which was requested
   // from this client (Chrome specifically).
   void TaskStarted() { installing_ = true; }
 
   // Clears any state an installation had setup while being performed.
-  void TaskEnded() {
-    installing_ = false;
-    // |Install()|
-    install_callback_holder_.reset();
-    progress_callback_holder_.reset();
-    dlc_id_holder_.reset();
+  void TaskEnded() { installing_ = false; }
+
+  void HoldInstallation(const std::string& id,
+                        InstallCallback install_callback,
+                        ProgressCallback progress_callback) {
+    installation_holder_[id].emplace_back(std::move(install_callback),
+                                          std::move(progress_callback));
+  }
+
+  void ReleaseInstallation(const std::string& id) {
+    installation_holder_.erase(id);
   }
 
   void EnqueueTask(base::OnceClosure task) {
@@ -221,21 +244,22 @@ class DlcserviceClientImpl : public DlcserviceClient {
     }
   }
 
-  void SendProgress(double progress) {
-    VLOG(2) << "Install in progress: " << progress;
-    DCHECK(progress_callback_holder_.has_value());
-    progress_callback_holder_.value().Run(progress);
+  void SendProgress(const dlcservice::DlcState& dlc_state) {
+    auto id = dlc_state.id();
+    auto progress = dlc_state.progress();
+    VLOG(2) << "Installation for DLC " << id << " in progress: " << progress;
+    for (auto& installation_state : installation_holder_[id])
+      installation_state.progress_callback.Run(progress);
   }
 
   void SendCompleted(const dlcservice::DlcState& dlc_state) {
-    DCHECK(install_callback_holder_.has_value());
-
+    auto id = dlc_state.id();
     if (dlc_state.state() == dlcservice::DlcState::NOT_INSTALLED) {
-      LOG(ERROR) << "Failed to install DLC " << dlc_state.id()
+      LOG(ERROR) << "Failed to install DLC " << id
                  << " with error code: " << dlc_state.last_error_code();
 
     } else {
-      VLOG(1) << "DLC " << dlc_state.id() << " installed successfully.";
+      VLOG(1) << "DLC " << id << " installed successfully.";
       if (dlc_state.last_error_code() != dlcservice::kErrorNone) {
         LOG(WARNING) << "DLC installation was sucessful but non-success "
                      << "error code: " << dlc_state.last_error_code();
@@ -244,10 +268,12 @@ class DlcserviceClientImpl : public DlcserviceClient {
 
     InstallResult result = {
         .error = dlc_state.last_error_code(),
-        .dlc_id = dlc_state.id(),
+        .dlc_id = id,
         .root_path = dlc_state.root_path(),
     };
-    std::move(install_callback_holder_.value()).Run(result);
+    for (auto& installation_state : installation_holder_[id])
+      std::move(installation_state.install_callback).Run(result);
+    ReleaseInstallation(id);
   }
 
   void DlcStateChanged(dbus::Signal* signal) {
@@ -262,9 +288,8 @@ class DlcserviceClientImpl : public DlcserviceClient {
       observer.OnDlcStateChanged(dlc_state);
     }
 
-    // We are not interested in this DLC. Some other process might be
-    // installing it.
-    if (!dlc_id_holder_.has_value() || dlc_id_holder_ != dlc_state.id())
+    // Skip DLCs not installing from this dlcservice client.
+    if (installation_holder_.find(dlc_state.id()) == installation_holder_.end())
       return;
 
     switch (dlc_state.state()) {
@@ -273,7 +298,7 @@ class DlcserviceClientImpl : public DlcserviceClient {
         SendCompleted(dlc_state);
         break;
       case dlcservice::DlcState::INSTALLING:
-        SendProgress(dlc_state.progress());
+        SendProgress(dlc_state);
         // Need to return here since we don't want to try starting another
         // pending install from the queue (would waste time checking).
         return;
@@ -293,26 +318,24 @@ class DlcserviceClientImpl : public DlcserviceClient {
     LOG_IF(ERROR, !success) << "Failed to connect to DlcStateChanged signal.";
   }
 
-  void OnInstall(dbus::Response* response, dbus::ErrorResponse* err_response) {
+  void OnInstall(const std::string& dlc_id,
+                 InstallCallback install_callback,
+                 ProgressCallback progress_callback,
+                 dbus::Response* response,
+                 dbus::ErrorResponse* err_response) {
+    HoldInstallation(dlc_id, std::move(install_callback),
+                     std::move(progress_callback));
     if (response)
       return;
 
-    // Perform DCHECKs only when an error occurs, platform dlcservice currently
-    // sends a signal prior to DBus method callback on quick install scenarios.
-    DCHECK(dlc_id_holder_.has_value());
-    DCHECK(install_callback_holder_.has_value());
-    DCHECK(progress_callback_holder_.has_value());
-
     const auto err = DlcserviceErrorResponseHandler(err_response).get_err();
     if (err == dlcservice::kErrorBusy) {
-      EnqueueTask(base::BindOnce(&DlcserviceClientImpl::Install,
-                                 weak_ptr_factory_.GetWeakPtr(),
-                                 std::move(dlc_id_holder_.value()),
-                                 std::move(install_callback_holder_.value()),
-                                 std::move(progress_callback_holder_.value())));
+      EnqueueTask(base::BindOnce(
+          &DlcserviceClientImpl::Install, weak_ptr_factory_.GetWeakPtr(),
+          dlc_id, std::move(install_callback), std::move(progress_callback)));
     } else {
       dlcservice::DlcState dlc_state;
-      dlc_state.set_id(dlc_id_holder_.value());
+      dlc_state.set_id(dlc_id);
       dlc_state.set_last_error_code(err);
       SendCompleted(dlc_state);
     }
@@ -349,21 +372,18 @@ class DlcserviceClientImpl : public DlcserviceClient {
     }
   }
 
+  // DLC ID to |InstallationCallbacks| mapping.
+  std::map<std::string, std::vector<InstallationCallbacks>>
+      installation_holder_;
+
   dbus::ObjectProxy* dlcservice_proxy_;
 
+  // TODO(crbug.com/928805): Once platform dlcservice batches, can be removed.
+  // Specifically when platform dlcservice doesn't return a busy status.
   // Whether an install is currently in progress. Can be used to decide whether
   // to queue up incoming install requests.
   bool installing_ = false;
 
-  // The cached callback to call on a finished |Install()|.
-  base::Optional<InstallCallback> install_callback_holder_;
-
-  // The cached callback to call on during progress of |Install()|.
-  base::Optional<ProgressCallback> progress_callback_holder_;
-
-  // The cached DLC ID for retrying call to install.
-  base::Optional<std::string> dlc_id_holder_;
-
   // A list of postponed installs to dlcservice.
   std::deque<base::OnceClosure> pending_tasks_;
 

chromeos/dbus/dlcservice/dlcservice_client_unittest.cc

@@ -307,13 +307,15 @@ TEST_F(DlcserviceClientTest, InstallFailureTest) {
 
 TEST_F(DlcserviceClientTest, InstallProgressTest) {
   EXPECT_CALL(*mock_proxy_.get(), DoCallMethodWithErrorResponse(_, _, _))
-      .WillOnce(Return());
+      .WillOnce(
+          Invoke(this, &DlcserviceClientTest::CallMethodWithErrorResponse));
   std::atomic<size_t> counter{0};
   DlcserviceClient::InstallCallback install_callback = base::BindOnce(
       [](const DlcserviceClient::InstallResult& install_result) {});
   DlcserviceClient::ProgressCallback progress_callback = base::BindRepeating(
       [](decltype(counter)* counter, double) { ++*counter; }, &counter);
 
+  responses_.push_back(dbus::Response::CreateEmpty());
   client_->Install({}, std::move(install_callback),
                    std::move(progress_callback));
   base::RunLoop().RunUntilIdle();
@@ -378,7 +380,8 @@ TEST_F(DlcserviceClientTest, PendingTaskTest) {
   const size_t kLoopCount = 3;
   EXPECT_CALL(*mock_proxy_.get(), DoCallMethodWithErrorResponse(_, _, _))
       .Times(kLoopCount)
-      .WillRepeatedly(Return());
+      .WillRepeatedly(
+          Invoke(this, &DlcserviceClientTest::CallMethodWithErrorResponse));
   std::atomic<size_t> counter{0};
 
   // All |Install()| request after the first should be queued.
@@ -389,6 +392,7 @@ TEST_F(DlcserviceClientTest, PendingTaskTest) {
           ++*counter;
         },
         &counter);
+    responses_.push_back(dbus::Response::CreateEmpty());
     client_->Install({}, std::move(install_callback),
                      DlcserviceClient::IgnoreProgress);
   }