commit 0b20266c12294bc21d16a0b7fba309cf8526cf23
author Alex Buynytskyy <alexbuy@google.com> Mon Apr 13 09:53:04 2020 -0700
committer Alex Buynytskyy <alexbuy@google.com> Tue Apr 14 09:25:37 2020 -0700
tree bf274b1b98f70a8f9142b3d51766ba47b5d6a03c
parent 159904ef2153a17a3e401e479f2846b78a4b9538

DataLoader lifecycle.

- mark disconnected DataLoaders as non-existent (destroyed),
- remove storage destroy on DataLoader destroy,
- more robust destroy on destruct handling,
- less race-conditions.

Bug: b/153874006
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest
Change-Id: Ib77302aac546d66ce40c5345417c7b424a1d601b

services/incremental/IncrementalService.cpp

diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 149dfa6..8d084cd 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -258,10 +258,6 @@
     mountExistingImages();
 }
 
-FileId IncrementalService::idFromMetadata(std::span<const uint8_t> metadata) {
-    return IncFs_FileIdFromMetadata({(const char*)metadata.data(), metadata.size()});
-}
-
 IncrementalService::~IncrementalService() {
     {
         std::lock_guard lock(mJobMutex);
@@ -1016,7 +1012,7 @@
             return false;
         }
     }
-    return dataLoaderStub->start();
+    return dataLoaderStub->requestStart();
 }
 
 void IncrementalService::mountExistingImages() {
@@ -1475,12 +1471,15 @@
 }
 
 IncrementalService::DataLoaderStub::~DataLoaderStub() {
-    CHECK(mStatus == -1 || mStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED)
-            << "Dataloader has to be destroyed prior to destructor: " << mId
-            << ", status: " << mStatus;
+    waitForDestroy();
 }
 
 bool IncrementalService::DataLoaderStub::create() {
+    {
+        std::unique_lock lock(mStatusMutex);
+        mStartRequested = false;
+        mDestroyRequested = false;
+    }
     bool created = false;
     auto status = mService.mDataLoaderManager->initializeDataLoader(mId, mParams, mControl, this,
                                                                     &created);
@@ -1491,12 +1490,18 @@
     return true;
 }
 
-bool IncrementalService::DataLoaderStub::start() {
-    if (mStatus != IDataLoaderStatusListener::DATA_LOADER_CREATED) {
+bool IncrementalService::DataLoaderStub::requestStart() {
+    {
+        std::unique_lock lock(mStatusMutex);
         mStartRequested = true;
-        return true;
+        if (mStatus != IDataLoaderStatusListener::DATA_LOADER_CREATED) {
+            return true;
+        }
     }
+    return start();
+}
 
+bool IncrementalService::DataLoaderStub::start() {
     sp<IDataLoader> dataloader;
     auto status = mService.mDataLoaderManager->getDataLoader(mId, &dataloader);
     if (!status.isOk()) {
@@ -1513,8 +1518,21 @@
 }
 
 void IncrementalService::DataLoaderStub::destroy() {
-    mDestroyRequested = true;
+    {
+        std::unique_lock lock(mStatusMutex);
+        mDestroyRequested = true;
+    }
     mService.mDataLoaderManager->destroyDataLoader(mId);
+
+    waitForDestroy();
+}
+
+bool IncrementalService::DataLoaderStub::waitForDestroy() {
+    auto now = std::chrono::steady_clock::now();
+    std::unique_lock lock(mStatusMutex);
+    return mStatusCondition.wait_until(lock, now + 60s, [this] {
+        return mStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED;
+    });
 }
 
 binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mountId, int newStatus) {
@@ -1523,34 +1541,36 @@
     }
 
     if (mListener) {
-        // Give an external listener a chance to act before we destroy something.
         mListener->onStatusChanged(mountId, newStatus);
     }
 
+    bool startRequested;
+    bool destroyRequested;
     {
-        std::unique_lock l(mService.mLock);
-        const auto& ifs = mService.getIfsLocked(mountId);
-        if (!ifs) {
-            LOG(WARNING) << "Received data loader status " << int(newStatus)
-                         << " for unknown mount " << mountId;
+        std::unique_lock lock(mStatusMutex);
+        if (mStatus == newStatus) {
             return binder::Status::ok();
         }
-        mStatus = newStatus;
 
-        if (!mDestroyRequested && newStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED) {
-            mService.deleteStorageLocked(*ifs, std::move(l));
-            return binder::Status::ok();
-        }
+        startRequested = mStartRequested;
+        destroyRequested = mDestroyRequested;
+
+        mStatus = newStatus;
     }
 
     switch (newStatus) {
         case IDataLoaderStatusListener::DATA_LOADER_CREATED: {
-            if (mStartRequested) {
+            if (startRequested) {
+                LOG(WARNING) << "Start was requested, triggering, for mount: " << mountId;
                 start();
             }
             break;
         }
         case IDataLoaderStatusListener::DATA_LOADER_DESTROYED: {
+            if (!destroyRequested) {
+                LOG(WARNING) << "DataLoader destroyed, reconnecting, for mount: " << mountId;
+                create();
+            }
             break;
         }
         case IDataLoaderStatusListener::DATA_LOADER_STARTED: {
@@ -1589,4 +1609,8 @@
     return binder::Status::ok();
 }
 
+FileId IncrementalService::idFromMetadata(std::span<const uint8_t> metadata) {
+    return IncFs_FileIdFromMetadata({(const char*)metadata.data(), metadata.size()});
+}
+
 } // namespace android::incremental