From b3047e566415869811fed0ff8e7094c9e7b61cc9 Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Tue, 6 Dec 2022 15:58:39 +0000 Subject: [PATCH] Update file when all persisted jobs are removed. Make sure we update the persisted file(s) when all persisted jobs are removed. Previously, we wouldn't touch the file at all when the last persisted job was removed because there was no job to copy out of the current job list to write into the file. Bug: 260147120 Test: atest FrameworksServicesTests:JobStoreTest Change-Id: I90d28e16d75bbd797997233e0d90ab5145ab72e3 --- .../java/com/android/server/job/JobStore.java | 60 +++++- .../com/android/server/job/JobStoreTest.java | 192 ++++++++++++++++-- 2 files changed, 232 insertions(+), 20 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobStore.java b/apex/jobscheduler/service/java/com/android/server/job/JobStore.java index 145ac52ccc8aa..a1153e315954b 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobStore.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobStore.java @@ -91,8 +91,11 @@ public final class JobStore { /** Threshold to adjust how often we want to write to the db. */ private static final long JOB_PERSIST_DELAY = 2000L; - private static final String JOB_FILE_SPLIT_PREFIX = "jobs_"; + @VisibleForTesting + static final String JOB_FILE_SPLIT_PREFIX = "jobs_"; private static final int ALL_UIDS = -1; + @VisibleForTesting + static final int INVALID_UID = -2; final Object mLock; final Object mWriteScheduleLock; // used solely for invariants around write scheduling @@ -529,6 +532,25 @@ public final class JobStore { return values; } + @VisibleForTesting + static int extractUidFromJobFileName(@NonNull File file) { + final String fileName = file.getName(); + if (fileName.startsWith(JOB_FILE_SPLIT_PREFIX)) { + try { + final int subEnd = fileName.length() - 4; // -4 for ".xml" + final int uid = Integer.parseInt( + fileName.substring(JOB_FILE_SPLIT_PREFIX.length(), subEnd)); + if (uid < 0) { + return INVALID_UID; + } + return uid; + } catch (Exception e) { + Slog.e(TAG, "Unexpected file name format", e); + } + } + return INVALID_UID; + } + /** * Runnable that writes {@link #mJobSet} out to xml. * NOTE: This Runnable locks on mLock @@ -543,6 +565,42 @@ public final class JobStore { private void prepare() { mCopyAllJobs = !mUseSplitFiles || mPendingJobWriteUids.get(ALL_UIDS); + if (mUseSplitFiles) { + // Put the set of changed UIDs in the copy list so that we update each file, + // especially if we've dropped all jobs for that UID. + if (mPendingJobWriteUids.get(ALL_UIDS)) { + // ALL_UIDS is only used when we switch file splitting policy or for tests, + // so going through the file list here shouldn't be + // a large performance hit on user devices. + + final File[] files; + try { + files = mJobFileDirectory.listFiles(); + } catch (SecurityException e) { + Slog.wtf(TAG, "Not allowed to read job file directory", e); + return; + } + if (files == null) { + Slog.wtfStack(TAG, "Couldn't get job file list"); + } else { + for (File file : files) { + final int uid = extractUidFromJobFileName(file); + if (uid != INVALID_UID) { + mJobStoreCopy.put(uid, new ArrayList<>()); + } + } + } + } else { + for (int i = 0; i < mPendingJobWriteUids.size(); ++i) { + mJobStoreCopy.put(mPendingJobWriteUids.keyAt(i), new ArrayList<>()); + } + } + } else { + // Single file mode. + // Put the catchall UID in the copy list so that we update the single file, + // especially if we've dropped all persisted jobs. + mJobStoreCopy.put(ALL_UIDS, new ArrayList<>()); + } } @Override diff --git a/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java b/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java index dc47b5eaea0e7..0589b3a912259 100644 --- a/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java +++ b/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java @@ -8,6 +8,7 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -22,6 +23,7 @@ import android.os.Build; import android.os.PersistableBundle; import android.os.SystemClock; import android.test.RenamingDelegatingContext; +import android.util.ArraySet; import android.util.Log; import android.util.Pair; @@ -38,9 +40,9 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import java.io.File; import java.time.Clock; import java.time.ZoneOffset; -import java.util.Iterator; /** * Test reading and writing correctly from file. @@ -93,11 +95,147 @@ public class JobStoreTest { mTaskStoreUnderTest.waitForWriteToCompleteForTesting(5_000L); } + private void setUseSplitFiles(boolean useSplitFiles) throws Exception { + mTaskStoreUnderTest.setUseSplitFiles(useSplitFiles); + waitForPendingIo(); + } + private void waitForPendingIo() throws Exception { assertTrue("Timed out waiting for persistence I/O to complete", mTaskStoreUnderTest.waitForWriteToCompleteForTesting(5_000L)); } + /** Test that we properly remove the last job of an app from the persisted file. */ + @Test + public void testRemovingLastJob_singleFile() throws Exception { + setUseSplitFiles(false); + runRemovingLastJob(); + } + + /** Test that we properly remove the last job of an app from the persisted file. */ + @Test + public void testRemovingLastJob_splitFiles() throws Exception { + setUseSplitFiles(true); + runRemovingLastJob(); + } + + private void runRemovingLastJob() throws Exception { + final JobInfo task1 = new Builder(8, mComponent) + .setRequiresDeviceIdle(true) + .setPeriodic(10000L) + .setRequiresCharging(true) + .setPersisted(true) + .build(); + final JobInfo task2 = new Builder(12, mComponent) + .setMinimumLatency(5000L) + .setBackoffCriteria(15000L, JobInfo.BACKOFF_POLICY_LINEAR) + .setOverrideDeadline(30000L) + .setRequiredNetworkType(JobInfo.NETWORK_TYPE_UNMETERED) + .setPersisted(true) + .build(); + final int uid1 = SOME_UID; + final int uid2 = uid1 + 1; + final JobStatus JobStatus1 = JobStatus.createFromJobInfo(task1, uid1, null, -1, null); + final JobStatus JobStatus2 = JobStatus.createFromJobInfo(task2, uid2, null, -1, null); + runWritingJobsToDisk(JobStatus1, JobStatus2); + + // Remove 1 job + mTaskStoreUnderTest.remove(JobStatus1, true); + waitForPendingIo(); + JobSet jobStatusSet = new JobSet(); + mTaskStoreUnderTest.readJobMapFromDisk(jobStatusSet, true); + assertEquals("Incorrect # of persisted tasks.", 1, jobStatusSet.size()); + JobStatus loaded = jobStatusSet.getAllJobs().iterator().next(); + + assertJobsEqual(JobStatus2, loaded); + assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(JobStatus2)); + + // Remove 2nd job + mTaskStoreUnderTest.remove(JobStatus2, true); + waitForPendingIo(); + jobStatusSet = new JobSet(); + mTaskStoreUnderTest.readJobMapFromDisk(jobStatusSet, true); + assertEquals("Incorrect # of persisted tasks.", 0, jobStatusSet.size()); + } + + /** Test that we properly clear the persisted file when all jobs are dropped. */ + @Test + public void testClearJobs_singleFile() throws Exception { + setUseSplitFiles(false); + runClearJobs(); + } + + /** Test that we properly clear the persisted file when all jobs are dropped. */ + @Test + public void testClearJobs_splitFiles() throws Exception { + setUseSplitFiles(true); + runClearJobs(); + } + + private void runClearJobs() throws Exception { + final JobInfo task1 = new Builder(8, mComponent) + .setRequiresDeviceIdle(true) + .setPeriodic(10000L) + .setRequiresCharging(true) + .setPersisted(true) + .build(); + final JobInfo task2 = new Builder(12, mComponent) + .setMinimumLatency(5000L) + .setBackoffCriteria(15000L, JobInfo.BACKOFF_POLICY_LINEAR) + .setOverrideDeadline(30000L) + .setRequiredNetworkType(JobInfo.NETWORK_TYPE_UNMETERED) + .setPersisted(true) + .build(); + final int uid1 = SOME_UID; + final int uid2 = uid1 + 1; + final JobStatus JobStatus1 = JobStatus.createFromJobInfo(task1, uid1, null, -1, null); + final JobStatus JobStatus2 = JobStatus.createFromJobInfo(task2, uid2, null, -1, null); + runWritingJobsToDisk(JobStatus1, JobStatus2); + + // Remove all jobs + mTaskStoreUnderTest.clear(); + waitForPendingIo(); + JobSet jobStatusSet = new JobSet(); + mTaskStoreUnderTest.readJobMapFromDisk(jobStatusSet, true); + assertEquals("Incorrect # of persisted tasks.", 0, jobStatusSet.size()); + } + + @Test + public void testExtractUidFromJobFileName() { + File file = new File(mTestContext.getFilesDir(), "randomName"); + assertEquals(JobStore.INVALID_UID, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), "jobs.xml"); + assertEquals(JobStore.INVALID_UID, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), ".xml"); + assertEquals(JobStore.INVALID_UID, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), "1000.xml"); + assertEquals(JobStore.INVALID_UID, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), "10000"); + assertEquals(JobStore.INVALID_UID, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), JobStore.JOB_FILE_SPLIT_PREFIX); + assertEquals(JobStore.INVALID_UID, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), JobStore.JOB_FILE_SPLIT_PREFIX + "text.xml"); + assertEquals(JobStore.INVALID_UID, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), JobStore.JOB_FILE_SPLIT_PREFIX + ".xml"); + assertEquals(JobStore.INVALID_UID, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), JobStore.JOB_FILE_SPLIT_PREFIX + "-10123.xml"); + assertEquals(JobStore.INVALID_UID, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), JobStore.JOB_FILE_SPLIT_PREFIX + "1.xml"); + assertEquals(1, JobStore.extractUidFromJobFileName(file)); + + file = new File(mTestContext.getFilesDir(), JobStore.JOB_FILE_SPLIT_PREFIX + "101023.xml"); + assertEquals(101023, JobStore.extractUidFromJobFileName(file)); + } + @Test public void testStringToIntArrayAndIntArrayToString() { final int[] netCapabilitiesIntArray = { 1, 3, 5, 7, 9 }; @@ -144,13 +282,13 @@ public class JobStoreTest { @Test public void testWritingTwoJobsToDisk_singleFile() throws Exception { - mTaskStoreUnderTest.setUseSplitFiles(false); + setUseSplitFiles(false); runWritingTwoJobsToDisk(); } @Test public void testWritingTwoJobsToDisk_splitFiles() throws Exception { - mTaskStoreUnderTest.setUseSplitFiles(true); + setUseSplitFiles(true); runWritingTwoJobsToDisk(); } @@ -172,28 +310,44 @@ public class JobStoreTest { final int uid2 = uid1 + 1; final JobStatus taskStatus1 = JobStatus.createFromJobInfo(task1, uid1, null, -1, null); final JobStatus taskStatus2 = JobStatus.createFromJobInfo(task2, uid2, null, -1, null); - mTaskStoreUnderTest.add(taskStatus1); - mTaskStoreUnderTest.add(taskStatus2); + + runWritingJobsToDisk(taskStatus1, taskStatus2); + } + + private void runWritingJobsToDisk(JobStatus... jobStatuses) throws Exception { + ArraySet expectedJobs = new ArraySet<>(); + for (JobStatus jobStatus : jobStatuses) { + mTaskStoreUnderTest.add(jobStatus); + expectedJobs.add(jobStatus); + } waitForPendingIo(); final JobSet jobStatusSet = new JobSet(); mTaskStoreUnderTest.readJobMapFromDisk(jobStatusSet, true); - assertEquals("Incorrect # of persisted tasks.", 2, jobStatusSet.size()); - Iterator it = jobStatusSet.getAllJobs().iterator(); - JobStatus loaded1 = it.next(); - JobStatus loaded2 = it.next(); + assertEquals("Incorrect # of persisted tasks.", expectedJobs.size(), jobStatusSet.size()); + int count = 0; + final int expectedCount = expectedJobs.size(); + for (JobStatus loaded : jobStatusSet.getAllJobs()) { + count++; + for (int i = 0; i < expectedJobs.size(); ++i) { + JobStatus expected = expectedJobs.valueAt(i); - // Reverse them so we know which comparison to make. - if (loaded1.getJobId() != 8) { - JobStatus tmp = loaded1; - loaded1 = loaded2; - loaded2 = tmp; + try { + assertJobsEqual(expected, loaded); + expectedJobs.remove(expected); + break; + } catch (AssertionError e) { + // Not equal. Move along. + } + } + } + assertEquals("Loaded more jobs than expected", expectedCount, count); + if (expectedJobs.size() > 0) { + fail("Not all expected jobs were restored"); + } + for (JobStatus jobStatus : jobStatuses) { + assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(jobStatus)); } - - assertJobsEqual(taskStatus1, loaded1); - assertJobsEqual(taskStatus2, loaded2); - assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(taskStatus1)); - assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(taskStatus2)); } @Test