diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp index 4ce336d16f15c..9b427994a4048 100644 --- a/services/incremental/IncrementalService.cpp +++ b/services/incremental/IncrementalService.cpp @@ -2660,9 +2660,6 @@ bool IncrementalService::DataLoaderStub::fsmStep() { } 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 @@ bool IncrementalService::DataLoaderStub::fsmStep() { } 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 @@ binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mount << ", 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 @@ binder::Status IncrementalService::DataLoaderStub::onStatusChanged(MountId mount } void IncrementalService::DataLoaderStub::setCurrentStatus(int newStatus) { - int targetStatus, oldStatus; + int oldStatus, oldTargetStatus, newTargetStatus; DataLoaderStatusListener listener; { std::unique_lock lock(mMutex); @@ -2730,22 +2732,31 @@ void IncrementalService::DataLoaderStub::setCurrentStatus(int newStatus) { } 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 6a3d953f74aa1..cdf2e898ed9cb 100644 --- a/services/incremental/test/IncrementalServiceTest.cpp +++ b/services/incremental/test/IncrementalServiceTest.cpp @@ -126,7 +126,7 @@ private: 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 @@ public: binder::Status(int32_t id, const std::vector& addedFiles, const std::vector& 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 @@ public: } return binder::Status::ok(); } + binder::Status bindToDataLoaderOkWithNoDelay(int32_t mountId, + const DataLoaderParamsParcel& params, + int bindDelayMs, + const sp& 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 @@ public: 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 @@ TEST_F(IncrementalServiceTest, testStartDataLoaderRecreateOnPendingReads) { 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();