Proper retrying DL installation sessions.
Plus more robust handling of broken DLs.
Bug: 190012477
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest com.google.android.packageinstallerv2proxy.host.gts.IncrementalInstallerHostTest
Change-Id: I5cb037d49cd2b140bed1045c99f072112495acfc
diff --git a/core/java/android/os/incremental/IncrementalFileStorages.java b/core/java/android/os/incremental/IncrementalFileStorages.java
index 6e25968..8ebc081 100644
--- a/core/java/android/os/incremental/IncrementalFileStorages.java
+++ b/core/java/android/os/incremental/IncrementalFileStorages.java
@@ -160,7 +160,7 @@
/**
* Starts or re-starts loading of data.
*/
- void startLoading(
+ public void startLoading(
@NonNull DataLoaderParams dataLoaderParams,
@Nullable IDataLoaderStatusListener statusListener,
@Nullable StorageHealthCheckParams healthCheckParams,
diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java
index 8202587..4b0eb65 100644
--- a/services/core/java/com/android/server/pm/PackageInstallerSession.java
+++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java
@@ -3760,11 +3760,6 @@
return true;
}
- // Retrying commit.
- if (mIncrementalFileStorages != null) {
- return false;
- }
-
final List<InstallationFileParcel> addedFiles = new ArrayList<>();
final List<String> removedFiles = new ArrayList<>();
@@ -3925,18 +3920,24 @@
(pkgInfo != null && pkgInfo.applicationInfo != null) ? new File(
pkgInfo.applicationInfo.getCodePath()).getParentFile() : null;
- mIncrementalFileStorages = IncrementalFileStorages.initialize(mContext, stageDir,
- inheritedDir, params, statusListener, healthCheckParams, healthListener,
- addedFiles, perUidReadTimeouts,
- new IPackageLoadingProgressCallback.Stub() {
- @Override
- public void onPackageLoadingProgressChanged(float progress) {
- synchronized (mProgressLock) {
- mIncrementalProgress = progress;
- computeProgressLocked(true);
+ if (mIncrementalFileStorages == null) {
+ mIncrementalFileStorages = IncrementalFileStorages.initialize(mContext,
+ stageDir, inheritedDir, params, statusListener, healthCheckParams,
+ healthListener, addedFiles, perUidReadTimeouts,
+ new IPackageLoadingProgressCallback.Stub() {
+ @Override
+ public void onPackageLoadingProgressChanged(float progress) {
+ synchronized (mProgressLock) {
+ mIncrementalProgress = progress;
+ computeProgressLocked(true);
+ }
}
- }
- });
+ });
+ } else {
+ // Retrying commit.
+ mIncrementalFileStorages.startLoading(params, statusListener, healthCheckParams,
+ healthListener, perUidReadTimeouts);
+ }
return false;
} catch (IOException e) {
throw new PackageManagerException(INSTALL_FAILED_MEDIA_UNAVAILABLE, e.getMessage(),
diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 47d59b2..757c9de 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -89,6 +89,11 @@
// Max interval after system invoked the DL when readlog collection can be enabled.
static constexpr auto readLogsMaxInterval = 2h;
+
+ // How long should we wait till dataLoader reports destroyed.
+ static constexpr auto destroyTimeout = 60s;
+
+ static constexpr auto anyStatus = INT_MIN;
};
static const Constants& constants() {
@@ -2554,7 +2559,7 @@
mControl = {};
mHealthControl = {};
mHealthListener = {};
- mStatusCondition.wait_until(lock, now + 60s, [this] {
+ mStatusCondition.wait_until(lock, now + Constants::destroyTimeout, [this] {
return mCurrentStatus == IDataLoaderStatusListener::DATA_LOADER_DESTROYED;
});
mStatusListener = {};
@@ -2754,8 +2759,16 @@
switch (targetStatus) {
case IDataLoaderStatusListener::DATA_LOADER_DESTROYED: {
switch (currentStatus) {
+ case IDataLoaderStatusListener::DATA_LOADER_UNAVAILABLE:
+ case IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE:
+ destroy();
+ // DataLoader is broken, just assume it's destroyed.
+ compareAndSetCurrentStatus(currentStatus,
+ IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+ return true;
case IDataLoaderStatusListener::DATA_LOADER_BINDING:
- setCurrentStatus(IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+ compareAndSetCurrentStatus(currentStatus,
+ IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
return true;
default:
return destroy();
@@ -2776,7 +2789,11 @@
case IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE:
// Before binding need to make sure we are unbound.
// Otherwise we'll get stuck binding.
- return destroy();
+ destroy();
+ // DataLoader is broken, just assume it's destroyed.
+ compareAndSetCurrentStatus(currentStatus,
+ IDataLoaderStatusListener::DATA_LOADER_DESTROYED);
+ return true;
case IDataLoaderStatusListener::DATA_LOADER_DESTROYED:
case IDataLoaderStatusListener::DATA_LOADER_BINDING:
return bind();
@@ -2815,6 +2832,11 @@
}
void IncrementalService::DataLoaderStub::setCurrentStatus(int newStatus) {
+ compareAndSetCurrentStatus(Constants::anyStatus, newStatus);
+}
+
+void IncrementalService::DataLoaderStub::compareAndSetCurrentStatus(int expectedStatus,
+ int newStatus) {
int oldStatus, oldTargetStatus, newTargetStatus;
DataLoaderStatusListener listener;
{
@@ -2822,6 +2844,9 @@
if (mCurrentStatus == newStatus) {
return;
}
+ if (expectedStatus != Constants::anyStatus && expectedStatus != mCurrentStatus) {
+ return;
+ }
oldStatus = mCurrentStatus;
oldTargetStatus = mTargetStatus;
diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index 5de7325..b81e1b1 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -255,6 +255,7 @@
binder::Status onStatusChanged(MountId mount, int newStatus) final;
void setCurrentStatus(int newStatus);
+ void compareAndSetCurrentStatus(int expectedStatus, int newStatus);
sp<content::pm::IDataLoader> getDataLoader();
diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp
index 094c5fb..59d96d2 100644
--- a/services/incremental/test/IncrementalServiceTest.cpp
+++ b/services/incremental/test/IncrementalServiceTest.cpp
@@ -808,7 +808,9 @@
METRICS_MILLIS_SINCE_OLDEST_PENDING_READ()
.c_str()),
&millisSinceOldestPendingRead));
- ASSERT_EQ(expectedMillisSinceOldestPendingRead, millisSinceOldestPendingRead);
+ // Allow 10ms.
+ ASSERT_LE(expectedMillisSinceOldestPendingRead, millisSinceOldestPendingRead);
+ ASSERT_GE(expectedMillisSinceOldestPendingRead + 10, millisSinceOldestPendingRead);
int storageHealthStatusCode = -1;
ASSERT_TRUE(
result.getInt(String16(BnIncrementalService::METRICS_STORAGE_HEALTH_STATUS_CODE()