commit bf1c0634819a4f3b2cba9c9227d9af2ff027b4c2
author Alex Buynytskyy <alexbuy@google.com> Tue Mar 10 15:49:29 2020 -0700
committer Alex Buynytskyy <alexbuy@google.com> Wed Mar 11 09:22:34 2020 -0700
tree 8e3572040f04d4d681ae72525f4f97f8b6177010
parent 672e0e80ba0f33286d75f98fe4ffae45f5671a66

DataLoader lifecycle cleanup.

Remove 5 second delay in startLoading()
Remove sysprops: incremental.skip_loader

Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest
Bug: b/151178179 b/136132412
Change-Id: Ibbe921c444b7e11fe5482662ca791bd8dfeb8324

services/incremental/IncrementalService.cpp

diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 2426e8f..9c9fe8c 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -917,19 +917,23 @@
 }
 
 bool IncrementalService::startLoading(StorageId storage) const {
-    const auto ifs = getIfs(storage);
-    if (!ifs) {
-        return false;
-    }
-    std::unique_lock l(ifs->lock);
-    if (ifs->dataLoaderStatus != IDataLoaderStatusListener::DATA_LOADER_CREATED) {
-        if (ifs->dataLoaderReady.wait_for(l, Seconds(5)) == std::cv_status::timeout) {
-            LOG(ERROR) << "Timeout waiting for data loader to be ready";
+    {
+        std::unique_lock l(mLock);
+        const auto& ifs = getIfsLocked(storage);
+        if (!ifs) {
             return false;
         }
+        if (ifs->dataLoaderStatus != IDataLoaderStatusListener::DATA_LOADER_CREATED) {
+            ifs->dataLoaderStartRequested = true;
+            return true;
+        }
     }
+    return startDataLoader(storage);
+}
+
+bool IncrementalService::startDataLoader(MountId mountId) const {
     sp<IDataLoader> dataloader;
-    auto status = mDataLoaderManager->getDataLoader(ifs->mountId, &dataloader);
+    auto status = mDataLoaderManager->getDataLoader(mountId, &dataloader);
     if (!status.isOk()) {
         return false;
     }
@@ -1065,15 +1069,9 @@
         }
         return true; // eventually...
     }
-    if (base::GetBoolProperty("incremental.skip_loader", false)) {
-        LOG(INFO) << "Skipped data loader because of incremental.skip_loader property";
-        std::unique_lock l(ifs.lock);
-        ifs.savedDataLoaderParams.reset();
-        return true;
-    }
 
     std::unique_lock l(ifs.lock);
-    if (ifs.dataLoaderStatus == IDataLoaderStatusListener::DATA_LOADER_CREATED) {
+    if (ifs.dataLoaderStatus != -1) {
         LOG(INFO) << "Skipped data loader preparation because it already exists";
         return true;
     }
@@ -1226,30 +1224,42 @@
         externalListener->onStatusChanged(mountId, newStatus);
     }
 
-    std::unique_lock l(incrementalService.mLock);
-    const auto& ifs = incrementalService.getIfsLocked(mountId);
-    if (!ifs) {
-        LOG(WARNING) << "Received data loader status " << int(newStatus) << " for unknown mount "
-                     << mountId;
-        return binder::Status::ok();
+    bool startRequested = false;
+    {
+        std::unique_lock l(incrementalService.mLock);
+        const auto& ifs = incrementalService.getIfsLocked(mountId);
+        if (!ifs) {
+            LOG(WARNING) << "Received data loader status " << int(newStatus) << " for unknown mount "
+                         << mountId;
+            return binder::Status::ok();
+        }
+        ifs->dataLoaderStatus = newStatus;
+
+        if (newStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED) {
+            ifs->dataLoaderStatus = IDataLoaderStatusListener::DATA_LOADER_STOPPED;
+            incrementalService.deleteStorageLocked(*ifs, std::move(l));
+            return binder::Status::ok();
+        }
+
+        startRequested = ifs->dataLoaderStartRequested;
     }
-    ifs->dataLoaderStatus = newStatus;
+
     switch (newStatus) {
         case IDataLoaderStatusListener::DATA_LOADER_NO_CONNECTION: {
             // TODO(b/150411019): handle data loader connection loss
             break;
         }
         case IDataLoaderStatusListener::DATA_LOADER_CONNECTION_OK: {
-            ifs->dataLoaderStatus = IDataLoaderStatusListener::DATA_LOADER_STARTED;
+            // TODO(b/150411019): handle data loader connection loss
             break;
         }
         case IDataLoaderStatusListener::DATA_LOADER_CREATED: {
-            ifs->dataLoaderReady.notify_one();
+            if (startRequested) {
+                incrementalService.startDataLoader(mountId);
+            }
             break;
         }
         case IDataLoaderStatusListener::DATA_LOADER_DESTROYED: {
-            ifs->dataLoaderStatus = IDataLoaderStatusListener::DATA_LOADER_STOPPED;
-            incrementalService.deleteStorageLocked(*ifs, std::move(l));
             break;
         }
         case IDataLoaderStatusListener::DATA_LOADER_STARTED: {

services/incremental/IncrementalService.h

diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index 8ff441b..406b32e 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -170,7 +170,7 @@
         std::optional<DataLoaderParamsParcel> savedDataLoaderParams;
         std::atomic<int> nextStorageDirNo{0};
         std::atomic<int> dataLoaderStatus = -1;
-        std::condition_variable dataLoaderReady;
+        bool dataLoaderStartRequested = false;
         TimePoint connectionLostTime = TimePoint();
         const IncrementalService& incrementalService;
 
@@ -208,6 +208,8 @@
 
     bool prepareDataLoader(IncFsMount& ifs, DataLoaderParamsParcel* params = nullptr,
                            const DataLoaderStatusListener* externalListener = nullptr);
+    bool startDataLoader(MountId mountId) const;
+
     BindPathMap::const_iterator findStorageLocked(std::string_view path) const;
     StorageId findStorageId(std::string_view path) const;