From e78c034bbbaa7979eaf179816a9807ab59e58d49 Mon Sep 17 00:00:00 2001 From: Keun young Park Date: Wed, 4 May 2022 17:13:03 -0700 Subject: [PATCH] Continue dexopt for other packages when a package reports an error - The job itelf will report an error if any error happens. - The failed APK will be added to failed list so that it is not tried agai until the package is updated. - Updated the unit test to test this logic. Bug: 230869482 Test: atest com.android.server.pm.BackgroundDexOptServiceUnitTest atest android.compilation.cts.BackgroundDexOptimizationTest Change-Id: I3a55b07b0f88d94fdf8a70f3e365be743dc1782e --- .../server/pm/BackgroundDexOptService.java | 53 +++-- .../pm/BackgroundDexOptServiceUnitTest.java | 201 ++++++++++++++++-- 2 files changed, 216 insertions(+), 38 deletions(-) diff --git a/services/core/java/com/android/server/pm/BackgroundDexOptService.java b/services/core/java/com/android/server/pm/BackgroundDexOptService.java index d26a1ac4fba07..9ea0192eaab52 100644 --- a/services/core/java/com/android/server/pm/BackgroundDexOptService.java +++ b/services/core/java/com/android/server/pm/BackgroundDexOptService.java @@ -91,17 +91,21 @@ public final class BackgroundDexOptService { // Possible return codes of individual optimization steps. /** Ok status: Optimizations finished, All packages were processed, can continue */ - private static final int STATUS_OK = 0; + /* package */ static final int STATUS_OK = 0; /** Optimizations should be aborted. Job scheduler requested it. */ - private static final int STATUS_ABORT_BY_CANCELLATION = 1; + /* package */ static final int STATUS_ABORT_BY_CANCELLATION = 1; /** Optimizations should be aborted. No space left on device. */ - private static final int STATUS_ABORT_NO_SPACE_LEFT = 2; + /* package */ static final int STATUS_ABORT_NO_SPACE_LEFT = 2; /** Optimizations should be aborted. Thermal throttling level too high. */ - private static final int STATUS_ABORT_THERMAL = 3; + /* package */ static final int STATUS_ABORT_THERMAL = 3; /** Battery level too low */ - private static final int STATUS_ABORT_BATTERY = 4; - /** {@link PackageDexOptimizer#DEX_OPT_FAILED} case */ - private static final int STATUS_DEX_OPT_FAILED = 5; + /* package */ static final int STATUS_ABORT_BATTERY = 4; + /** + * {@link PackageDexOptimizer#DEX_OPT_FAILED} case. This state means some packages have failed + * compilation during the job. Note that the failure will not be permanent as the next dexopt + * job will exclude those failed packages. + */ + /* package */ static final int STATUS_DEX_OPT_FAILED = 5; @IntDef(prefix = {"STATUS_"}, value = { STATUS_OK, @@ -525,7 +529,10 @@ public final class BackgroundDexOptService { } } - /** Returns true if completed */ + /** + * Returns whether we've successfully run the job. Note that it will return true even if some + * packages may have failed compiling. + */ private boolean runIdleOptimization(PackageManagerService pm, List pkgs, boolean isPostBootUpdate) { synchronized (mLock) { @@ -541,7 +548,7 @@ public final class BackgroundDexOptService { mLastExecutionDurationMs = SystemClock.elapsedRealtime() - mLastExecutionStartTimeMs; } - return status == STATUS_OK; + return status == STATUS_OK || status == STATUS_DEX_OPT_FAILED; } /** Gets the size of the directory. It uses recursion to go over all files. */ @@ -661,6 +668,11 @@ public final class BackgroundDexOptService { ArraySet updatedPackages, boolean isPostBootUpdate) { boolean supportSecondaryDex = mInjector.supportSecondaryDex(); + // Keep the error if there is any error from any package. + @Status int status = STATUS_OK; + + // Other than cancellation, all packages will be processed even if an error happens + // in a package. for (String pkg : pkgs) { int abortCode = abortIdleOptimizations(lowStorageThreshold); if (abortCode != STATUS_OK) { @@ -670,10 +682,13 @@ public final class BackgroundDexOptService { @DexOptResult int primaryResult = optimizePackage(pkg, true /* isForPrimaryDex */, isPostBootUpdate); + if (primaryResult == PackageDexOptimizer.DEX_OPT_CANCELLED) { + return STATUS_ABORT_BY_CANCELLATION; + } if (primaryResult == PackageDexOptimizer.DEX_OPT_PERFORMED) { updatedPackages.add(pkg); - } else if (primaryResult != PackageDexOptimizer.DEX_OPT_SKIPPED) { - return convertPackageDexOptimizerStatusToInternal(primaryResult); + } else if (primaryResult == PackageDexOptimizer.DEX_OPT_FAILED) { + status = convertPackageDexOptimizerStatusToInternal(primaryResult); } if (!supportSecondaryDex) { @@ -682,12 +697,14 @@ public final class BackgroundDexOptService { @DexOptResult int secondaryResult = optimizePackage(pkg, false /* isForPrimaryDex */, isPostBootUpdate); - if (secondaryResult != PackageDexOptimizer.DEX_OPT_PERFORMED - && secondaryResult != PackageDexOptimizer.DEX_OPT_SKIPPED) { - return convertPackageDexOptimizerStatusToInternal(secondaryResult); + if (secondaryResult == PackageDexOptimizer.DEX_OPT_CANCELLED) { + return STATUS_ABORT_BY_CANCELLATION; + } + if (secondaryResult == PackageDexOptimizer.DEX_OPT_FAILED) { + status = convertPackageDexOptimizerStatusToInternal(secondaryResult); } } - return STATUS_OK; + return status; } /** @@ -779,9 +796,9 @@ public final class BackgroundDexOptService { @DexOptResult private int performDexOptPrimary(String pkg, int reason, int dexoptFlags) { + DexoptOptions dexoptOptions = new DexoptOptions(pkg, reason, dexoptFlags); return trackPerformDexOpt(pkg, /*isForPrimaryDex=*/ true, - () -> mDexOptHelper.performDexOptWithStatus( - new DexoptOptions(pkg, reason, dexoptFlags))); + () -> mDexOptHelper.performDexOptWithStatus(dexoptOptions)); } @DexOptResult @@ -791,7 +808,7 @@ public final class BackgroundDexOptService { dexoptFlags | DexoptOptions.DEXOPT_ONLY_SECONDARY_DEX); return trackPerformDexOpt(pkg, /*isForPrimaryDex=*/ false, () -> mDexOptHelper.performDexOpt(dexoptOptions) - ? PackageDexOptimizer.DEX_OPT_PERFORMED : PackageDexOptimizer.DEX_OPT_FAILED + ? PackageDexOptimizer.DEX_OPT_PERFORMED : PackageDexOptimizer.DEX_OPT_FAILED ); } diff --git a/services/tests/mockingservicestests/src/com/android/server/pm/BackgroundDexOptServiceUnitTest.java b/services/tests/mockingservicestests/src/com/android/server/pm/BackgroundDexOptServiceUnitTest.java index 444db9128662f..da5c8f06bc86b 100644 --- a/services/tests/mockingservicestests/src/com/android/server/pm/BackgroundDexOptServiceUnitTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/pm/BackgroundDexOptServiceUnitTest.java @@ -16,6 +16,9 @@ package com.android.server.pm; +import static com.android.server.pm.BackgroundDexOptService.STATUS_DEX_OPT_FAILED; +import static com.android.server.pm.BackgroundDexOptService.STATUS_OK; + import static com.google.common.truth.Truth.assertThat; import static org.mockito.ArgumentMatchers.any; @@ -23,12 +26,14 @@ import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.testng.Assert.assertThrows; +import android.annotation.Nullable; import android.app.job.JobInfo; import android.app.job.JobParameters; import android.app.job.JobScheduler; @@ -38,10 +43,13 @@ import android.content.Intent; import android.content.IntentFilter; import android.os.HandlerThread; import android.os.PowerManager; +import android.util.Log; +import com.android.internal.util.IndentingPrintWriter; import com.android.server.LocalServices; import com.android.server.PinnerService; import com.android.server.pm.dex.DexManager; +import com.android.server.pm.dex.DexoptOptions; import org.junit.After; import org.junit.Before; @@ -52,7 +60,11 @@ import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; +import java.io.ByteArrayOutputStream; +import java.io.PrintWriter; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.stream.Collectors; @@ -66,8 +78,12 @@ public final class BackgroundDexOptServiceUnitTest { private static final long TEST_WAIT_TIMEOUT_MS = 10_000; - private static final List DEFAULT_PACKAGE_LIST = List.of("aaa", "bbb"); - private static final List EMPTY_PACKAGE_LIST = List.of(); + private static final String PACKAGE_AAA = "aaa"; + private static final List DEFAULT_PACKAGE_LIST = List.of(PACKAGE_AAA, "bbb"); + private int mDexOptResultForPackageAAA = PackageDexOptimizer.DEX_OPT_PERFORMED; + + // Store expected dexopt sequence for verification. + private ArrayList mDexInfoSequence = new ArrayList<>(); @Mock private Context mContext; @@ -116,14 +132,23 @@ public final class BackgroundDexOptServiceUnitTest { when(mInjector.getDexOptThermalCutoff()).thenReturn(PowerManager.THERMAL_STATUS_CRITICAL); when(mInjector.getCurrentThermalStatus()).thenReturn(PowerManager.THERMAL_STATUS_NONE); when(mInjector.supportSecondaryDex()).thenReturn(true); - when(mDexOptHelper.getOptimizablePackages(any())).thenReturn(DEFAULT_PACKAGE_LIST); - when(mDexOptHelper.performDexOptWithStatus(any())).thenReturn( - PackageDexOptimizer.DEX_OPT_PERFORMED); - when(mDexOptHelper.performDexOpt(any())).thenReturn(true); + setupDexOptHelper(); mService = new BackgroundDexOptService(mInjector); } + private void setupDexOptHelper() { + when(mDexOptHelper.getOptimizablePackages(any())).thenReturn(DEFAULT_PACKAGE_LIST); + when(mDexOptHelper.performDexOptWithStatus(any())).thenAnswer(inv -> { + DexoptOptions opt = inv.getArgument(0); + if (opt.getPackageName().equals(PACKAGE_AAA)) { + return mDexOptResultForPackageAAA; + } + return PackageDexOptimizer.DEX_OPT_PERFORMED; + }); + when(mDexOptHelper.performDexOpt(any())).thenReturn(true); + } + @After public void tearDown() throws Exception { LocalServices.removeServiceForTest(BackgroundDexOptService.class); @@ -159,7 +184,7 @@ public final class BackgroundDexOptServiceUnitTest { @Test public void testNoExecutionForNoOptimizablePackages() { initUntilBootCompleted(); - when(mDexOptHelper.getOptimizablePackages(any())).thenReturn(EMPTY_PACKAGE_LIST); + when(mDexOptHelper.getOptimizablePackages(any())).thenReturn(Collections.emptyList()); assertThat(mService.onStartJob(mJobServiceForPostBoot, mJobParametersForPostBoot)).isFalse(); @@ -170,15 +195,70 @@ public final class BackgroundDexOptServiceUnitTest { public void testPostBootUpdateFullRun() { initUntilBootCompleted(); - runFullJob(mJobServiceForPostBoot, mJobParametersForPostBoot, false, 1); + runFullJob(mJobServiceForPostBoot, mJobParametersForPostBoot, + /* expectedReschedule= */ false, /* expectedStatus= */ STATUS_OK, + /* totalJobFinishedWithParams= */ 1, /* expectedSkippedPackage= */ null); + } + + @Test + public void testPostBootUpdateFullRunWithPackageFailure() { + mDexOptResultForPackageAAA = PackageDexOptimizer.DEX_OPT_FAILED; + + initUntilBootCompleted(); + + runFullJob(mJobServiceForPostBoot, mJobParametersForPostBoot, + /* expectedReschedule= */ false, /* expectedStatus= */ STATUS_DEX_OPT_FAILED, + /* totalJobFinishedWithParams= */ 1, /* expectedSkippedPackage= */ PACKAGE_AAA); + + assertThat(getFailedPackageNamesPrimary()).containsExactly(PACKAGE_AAA); + assertThat(getFailedPackageNamesSecondary()).isEmpty(); } @Test public void testIdleJobFullRun() { initUntilBootCompleted(); + runFullJob(mJobServiceForPostBoot, mJobParametersForPostBoot, + /* expectedReschedule= */ false, /* expectedStatus= */ STATUS_OK, + /* totalJobFinishedWithParams= */ 1, /* expectedSkippedPackage= */ null); + runFullJob(mJobServiceForIdle, mJobParametersForIdle, + /* expectedReschedule= */ true, /* expectedStatus= */ STATUS_OK, + /* totalJobFinishedWithParams= */ 1, /* expectedSkippedPackage= */ null); + } - runFullJob(mJobServiceForPostBoot, mJobParametersForPostBoot, false, 1); - runFullJob(mJobServiceForIdle, mJobParametersForIdle, true, 2); + @Test + public void testIdleJobFullRunWithFailureOnceAndSuccessAfterUpdate() { + mDexOptResultForPackageAAA = PackageDexOptimizer.DEX_OPT_FAILED; + + initUntilBootCompleted(); + + runFullJob(mJobServiceForPostBoot, mJobParametersForPostBoot, + /* expectedReschedule= */ false, /* expectedStatus= */ STATUS_DEX_OPT_FAILED, + /* totalJobFinishedWithParams= */ 1, /* expectedSkippedPackage= */ PACKAGE_AAA); + + assertThat(getFailedPackageNamesPrimary()).containsExactly(PACKAGE_AAA); + assertThat(getFailedPackageNamesSecondary()).isEmpty(); + + runFullJob(mJobServiceForIdle, mJobParametersForIdle, + /* expectedReschedule= */ true, /* expectedStatus= */ STATUS_OK, + /* totalJobFinishedWithParams= */ 1, /* expectedSkippedPackage= */ PACKAGE_AAA); + + assertThat(getFailedPackageNamesPrimary()).containsExactly(PACKAGE_AAA); + assertThat(getFailedPackageNamesSecondary()).isEmpty(); + + mService.notifyPackageChanged(PACKAGE_AAA); + + assertThat(getFailedPackageNamesPrimary()).isEmpty(); + assertThat(getFailedPackageNamesSecondary()).isEmpty(); + + // Succeed this time. + mDexOptResultForPackageAAA = PackageDexOptimizer.DEX_OPT_PERFORMED; + + runFullJob(mJobServiceForIdle, mJobParametersForIdle, + /* expectedReschedule= */ true, /* expectedStatus= */ STATUS_OK, + /* totalJobFinishedWithParams= */ 2, /* expectedSkippedPackage= */ null); + + assertThat(getFailedPackageNamesPrimary()).isEmpty(); + assertThat(getFailedPackageNamesSecondary()).isEmpty(); } @Test @@ -404,8 +484,10 @@ public final class BackgroundDexOptServiceUnitTest { } private void runFullJob(BackgroundDexOptJobService jobService, JobParameters params, - boolean expectedReschedule, int totalJobRuns) { + boolean expectedReschedule, int expectedStatus, int totalJobFinishedWithParams, + @Nullable String expectedSkippedPackage) { when(mInjector.createAndStartThread(any(), any())).thenReturn(Thread.currentThread()); + addFullRunSequence(expectedSkippedPackage); assertThat(mService.onStartJob(jobService, params)).isTrue(); ArgumentCaptor argThreadRunnable = ArgumentCaptor.forClass(Runnable.class); @@ -413,20 +495,99 @@ public final class BackgroundDexOptServiceUnitTest { argThreadRunnable.getValue().run(); - verify(jobService).jobFinished(params, expectedReschedule); + verify(jobService, times(totalJobFinishedWithParams)).jobFinished(params, + expectedReschedule); // Never block verify(mDexOptHelper, never()).controlDexOptBlocking(true); - verifyPerformDexOpt(DEFAULT_PACKAGE_LIST, totalJobRuns); + verifyPerformDexOpt(); + assertThat(getLastExecutionStatus()).isEqualTo(expectedStatus); } - private void verifyPerformDexOpt(List pkgs, int expectedRuns) { + private void verifyPerformDexOpt() { InOrder inOrder = inOrder(mDexOptHelper); - for (int i = 0; i < expectedRuns; i++) { - for (String pkg : pkgs) { - inOrder.verify(mDexOptHelper, times(1)).performDexOptWithStatus(argThat((option) -> - option.getPackageName().equals(pkg) && !option.isDexoptOnlySecondaryDex())); - inOrder.verify(mDexOptHelper, times(1)).performDexOpt(argThat((option) -> - option.getPackageName().equals(pkg) && option.isDexoptOnlySecondaryDex())); + inOrder.verify(mDexOptHelper).getOptimizablePackages(any()); + for (DexOptInfo info : mDexInfoSequence) { + if (info.isPrimary) { + verify(mDexOptHelper).performDexOptWithStatus( + argThat((option) -> option.getPackageName().equals(info.packageName) + && !option.isDexoptOnlySecondaryDex())); + } else { + inOrder.verify(mDexOptHelper).performDexOpt( + argThat((option) -> option.getPackageName().equals(info.packageName) + && option.isDexoptOnlySecondaryDex())); + } + } + + // Even InOrder cannot check the order if the same call is made multiple times. + // To check the order across multiple runs, we reset the mock so that order can be checked + // in each call. + mDexInfoSequence.clear(); + reset(mDexOptHelper); + setupDexOptHelper(); + } + + private String findDumpValueForKey(String key) { + ByteArrayOutputStream out = new ByteArrayOutputStream(); + PrintWriter pw = new PrintWriter(out, true); + IndentingPrintWriter writer = new IndentingPrintWriter(pw, ""); + try { + mService.dump(writer); + writer.flush(); + Log.i(TAG, "dump output:" + out.toString()); + for (String line : out.toString().split(System.lineSeparator())) { + String[] vals = line.split(":"); + if (vals[0].equals(key)) { + if (vals.length == 2) { + return vals[1].strip(); + } else { + break; + } + } + } + return ""; + } finally { + writer.close(); + } + } + + List findStringListFromDump(String key) { + String values = findDumpValueForKey(key); + if (values.isEmpty()) { + return Collections.emptyList(); + } + return Arrays.asList(values.split(",")); + } + + private List getFailedPackageNamesPrimary() { + return findStringListFromDump("mFailedPackageNamesPrimary"); + } + + private List getFailedPackageNamesSecondary() { + return findStringListFromDump("mFailedPackageNamesSecondary"); + } + + private int getLastExecutionStatus() { + return Integer.parseInt(findDumpValueForKey("mLastExecutionStatus")); + } + + private static class DexOptInfo { + public final String packageName; + public final boolean isPrimary; + + private DexOptInfo(String packageName, boolean isPrimary) { + this.packageName = packageName; + this.isPrimary = isPrimary; + } + } + + private void addFullRunSequence(@Nullable String expectedSkippedPackage) { + for (String packageName : DEFAULT_PACKAGE_LIST) { + if (packageName.equals(expectedSkippedPackage)) { + // only fails primary dexopt in mocking but add secodary + mDexInfoSequence.add(new DexOptInfo(packageName, /* isPrimary= */ false)); + } else { + mDexInfoSequence.add(new DexOptInfo(packageName, /* isPrimary= */ true)); + mDexInfoSequence.add(new DexOptInfo(packageName, /* isPrimary= */ false)); } } }