Retry on unavailable.
Bug: 182214420
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest PackageManagerServiceTest ChecksumsTest
Change-Id: Iaf61b6825ced45ffdc7e9c87dfea830e50633476
diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 4ce336d..9b42799 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -2660,9 +2660,6 @@
}
switch (targetStatus) {
- case IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE:
- // Do nothing, this is a reset state.
- break;
case IDataLoaderStatusListener::DATA_LOADER_DESTROYED: {
switch (currentStatus) {
case IDataLoaderStatusListener::DATA_LOADER_BINDING:
@@ -2683,8 +2680,12 @@
}
case IDataLoaderStatusListener::DATA_LOADER_CREATED:
switch (currentStatus) {
- case IDataLoaderStatusListener::DATA_LOADER_DESTROYED:
case IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE:
+ case IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE:
+ // Before binding need to make sure we are unbound.
+ // Otherwise we'll get stuck binding.
+ return destroy();
+ case IDataLoaderStatusListener::DATA_LOADER_DESTROYED:
case IDataLoaderStatusListener::DATA_LOADER_BINDING:
return bind();
case IDataLoaderStatusListener::DATA_LOADER_BOUND:
@@ -2709,7 +2710,8 @@
<< ", but got: " << mountId;
return binder::Status::fromServiceSpecificError(-EPERM, "Mount ID mismatch.");
}
- if (newStatus == IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE) {
+ if (newStatus == IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE ||
+ newStatus == IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE) {
// User-provided status, let's postpone the handling to avoid possible deadlocks.
mService.addTimedJob(*mService.mTimedQueue, id(), Constants::userStatusDelay,
[this, newStatus]() { setCurrentStatus(newStatus); });
@@ -2721,7 +2723,7 @@
}
void IncrementalService::DataLoaderStub::setCurrentStatus(int newStatus) {
- int targetStatus, oldStatus;
+ int oldStatus, oldTargetStatus, newTargetStatus;
DataLoaderStatusListener listener;
{
std::unique_lock lock(mMutex);
@@ -2730,22 +2732,31 @@
}
oldStatus = mCurrentStatus;
- targetStatus = mTargetStatus;
+ oldTargetStatus = mTargetStatus;
listener = mStatusListener;
// Change the status.
mCurrentStatus = newStatus;
mCurrentStatusTs = mService.mClock->now();
- if (mCurrentStatus == IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE ||
- mCurrentStatus == IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE) {
- // For unavailable, unbind from DataLoader to ensure proper re-commit.
- setTargetStatusLocked(IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+ switch (mCurrentStatus) {
+ case IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE:
+ // Unavailable, retry.
+ setTargetStatusLocked(IDataLoaderStatusListener::DATA_LOADER_STARTED);
+ break;
+ case IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE:
+ // Unrecoverable, just unbind.
+ setTargetStatusLocked(IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+ break;
+ default:
+ break;
}
+
+ newTargetStatus = mTargetStatus;
}
LOG(DEBUG) << "Current status update for DataLoader " << id() << ": " << oldStatus << " -> "
- << newStatus << " (target " << targetStatus << ")";
+ << newStatus << " (target " << oldTargetStatus << " -> " << newTargetStatus << ")";
if (listener) {
listener->onStatusChanged(id(), newStatus);
diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp
index 6a3d953..cdf2e89 100644
--- a/services/incremental/test/IncrementalServiceTest.cpp
+++ b/services/incremental/test/IncrementalServiceTest.cpp
@@ -126,7 +126,7 @@
class MockDataLoader : public IDataLoader {
public:
MockDataLoader() {
- ON_CALL(*this, create(_, _, _, _)).WillByDefault(Invoke(this, &MockDataLoader::createOk));
+ initializeCreateOk();
ON_CALL(*this, start(_)).WillByDefault(Invoke(this, &MockDataLoader::startOk));
ON_CALL(*this, stop(_)).WillByDefault(Invoke(this, &MockDataLoader::stopOk));
ON_CALL(*this, destroy(_)).WillByDefault(Invoke(this, &MockDataLoader::destroyOk));
@@ -145,6 +145,10 @@
binder::Status(int32_t id, const std::vector<InstallationFileParcel>& addedFiles,
const std::vector<std::string>& removedFiles));
+ void initializeCreateOk() {
+ ON_CALL(*this, create(_, _, _, _)).WillByDefault(Invoke(this, &MockDataLoader::createOk));
+ }
+
void initializeCreateOkNoStatus() {
ON_CALL(*this, create(_, _, _, _))
.WillByDefault(Invoke(this, &MockDataLoader::createOkNoStatus));
@@ -275,6 +279,14 @@
}
return binder::Status::ok();
}
+ binder::Status bindToDataLoaderOkWithNoDelay(int32_t mountId,
+ const DataLoaderParamsParcel& params,
+ int bindDelayMs,
+ const sp<IDataLoaderStatusListener>& listener,
+ bool* _aidl_return) {
+ CHECK(bindDelayMs == 0) << bindDelayMs;
+ return bindToDataLoaderOk(mountId, params, bindDelayMs, listener, _aidl_return);
+ }
binder::Status bindToDataLoaderOkWith1sDelay(int32_t mountId,
const DataLoaderParamsParcel& params,
int bindDelayMs,
@@ -338,14 +350,13 @@
mListener->onStatusChanged(mId, IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE);
}
binder::Status unbindFromDataLoaderOk(int32_t id) {
+ mBindDelayMs = -1;
if (mDataLoader) {
if (auto status = mDataLoader->destroy(id); !status.isOk()) {
return status;
}
mDataLoader = nullptr;
- }
- mBindDelayMs = -1;
- if (mListener) {
+ } else if (mListener) {
mListener->onStatusChanged(id, IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
}
return binder::Status::ok();
@@ -1156,12 +1167,81 @@
ASSERT_GE(storageId, 0);
ASSERT_TRUE(mIncrementalService->startLoading(storageId, std::move(mDataLoaderParcel), {}, {},
{}, {}));
- mDataLoaderManager->setDataLoaderStatusUnavailable();
+ mDataLoaderManager->setDataLoaderStatusUnrecoverable();
+
+ // Timed callback present.
+ ASSERT_EQ(storageId, mTimedQueue->mId);
+ ASSERT_GE(mTimedQueue->mAfter, 10ms);
+ auto timedCallback = mTimedQueue->mWhat;
+ mTimedQueue->clearJob(storageId);
+
+ // First callback call to propagate unrecoverable.
+ timedCallback();
+
+ // And second call to trigger recreation.
ASSERT_NE(nullptr, mLooper->mCallback);
ASSERT_NE(nullptr, mLooper->mCallbackData);
mLooper->mCallback(-1, -1, mLooper->mCallbackData);
}
+TEST_F(IncrementalServiceTest, testStartDataLoaderUnavailable) {
+ mIncFs->openMountSuccess();
+ mDataLoader->initializeCreateOkNoStatus();
+
+ EXPECT_CALL(*mDataLoaderManager, bindToDataLoader(_, _, _, _, _)).Times(3);
+ EXPECT_CALL(*mDataLoaderManager, unbindFromDataLoader(_)).Times(3);
+ EXPECT_CALL(*mDataLoader, create(_, _, _, _)).Times(3);
+ EXPECT_CALL(*mDataLoader, start(_)).Times(1);
+ EXPECT_CALL(*mDataLoader, destroy(_)).Times(2);
+ EXPECT_CALL(*mVold, unmountIncFs(_)).Times(2);
+ EXPECT_CALL(*mLooper, addFd(MockIncFs::kPendingReadsFd, _, _, _, _)).Times(1);
+ EXPECT_CALL(*mLooper, removeFd(MockIncFs::kPendingReadsFd)).Times(1);
+ TemporaryDir tempDir;
+ int storageId =
+ mIncrementalService->createStorage(tempDir.path, mDataLoaderParcel,
+ IncrementalService::CreateOptions::CreateNew);
+ ASSERT_GE(storageId, 0);
+
+ ON_CALL(*mDataLoaderManager, bindToDataLoader(_, _, _, _, _))
+ .WillByDefault(Invoke(mDataLoaderManager,
+ &MockDataLoaderManager::bindToDataLoaderOkWithNoDelay));
+
+ ASSERT_TRUE(mIncrementalService->startLoading(storageId, std::move(mDataLoaderParcel), {}, {},
+ {}, {}));
+
+ // Unavailable.
+ mDataLoaderManager->setDataLoaderStatusUnavailable();
+
+ // Timed callback present.
+ ASSERT_EQ(storageId, mTimedQueue->mId);
+ ASSERT_GE(mTimedQueue->mAfter, 10ms);
+ auto timedCallback = mTimedQueue->mWhat;
+ mTimedQueue->clearJob(storageId);
+
+ // Propagating unavailable and expecting it to trigger rebind with 1s retry delay.
+ ON_CALL(*mDataLoaderManager, bindToDataLoader(_, _, _, _, _))
+ .WillByDefault(Invoke(mDataLoaderManager,
+ &MockDataLoaderManager::bindToDataLoaderOkWith1sDelay));
+ timedCallback();
+
+ // Unavailable #2.
+ mDataLoaderManager->setDataLoaderStatusUnavailable();
+
+ // Timed callback present.
+ ASSERT_EQ(storageId, mTimedQueue->mId);
+ ASSERT_GE(mTimedQueue->mAfter, 10ms);
+ timedCallback = mTimedQueue->mWhat;
+ mTimedQueue->clearJob(storageId);
+
+ // Propagating unavailable and expecting it to trigger rebind with 10s retry delay.
+ // This time succeed.
+ mDataLoader->initializeCreateOk();
+ ON_CALL(*mDataLoaderManager, bindToDataLoader(_, _, _, _, _))
+ .WillByDefault(Invoke(mDataLoaderManager,
+ &MockDataLoaderManager::bindToDataLoaderOkWith10sDelay));
+ timedCallback();
+}
+
TEST_F(IncrementalServiceTest, testStartDataLoaderUnhealthyStorage) {
mIncFs->openMountSuccess();
