From 2700aba1c577c72d846ac029ed8e7346b294de1d Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Wed, 23 Jun 2021 09:13:42 -0700 Subject: [PATCH] Fix pending job sorting. We want to always run an app's expedited jobs before its own regular jobs. This means that we order an app's EJs before its regular jobs. In order to satisfy the transitivity constraint, we must also order one app's EJs ahead of a differing app's jobs iff we had to order the first app's EJ ahead of a job that was earlier than the second app's jobs. The current sorting system fails the transitivity requirement. Test case: regJob1 enqueued at t1 for UID A, regJob2 enqueued at t2 for UID B, EJ3 enqueued at t3 for UID A. Because we expedite EJs, EJ3 is ordered before regJob1. Because we sort jobs of differing apps by enqueue time, regJob1 comes before regJob2 and regJob2 comes before EJ3. This violates transitivity. The new sorting system will order a later EJ ahead of a differing app's jobs iff the first app has a regular job that is earlier than the differing app's first regular job. Bug: 191592712 Test: atest FrameworksMockingServicesTests:JobSchedulerServiceTest Test: atest CtsJobSchedulerTestCases Change-Id: I5391a4f35269d7d43eefc1954788f9df160b1d22 --- .../server/job/JobSchedulerService.java | 126 ++++++++-- .../server/job/JobSchedulerServiceTest.java | 231 +++++++++++++++++- 2 files changed, 326 insertions(+), 31 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java index 96cbed75622f2..78670c7c73b1c 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java @@ -73,7 +73,9 @@ import android.util.IndentingPrintWriter; import android.util.Log; import android.util.Slog; import android.util.SparseArray; +import android.util.SparseBooleanArray; import android.util.SparseIntArray; +import android.util.SparseLongArray; import android.util.SparseSetArray; import android.util.TimeUtils; import android.util.proto.ProtoOutputStream; @@ -686,26 +688,92 @@ public class JobSchedulerService extends com.android.server.SystemService final Constants mConstants; final ConstantsObserver mConstantsObserver; - private static final Comparator sPendingJobComparator = (o1, o2) -> { - // Jobs with an override state set (via adb) should be put first as tests/developers - // expect the jobs to run immediately. - if (o1.overrideState != o2.overrideState) { - // Higher override state (OVERRIDE_FULL) should be before lower state (OVERRIDE_SOFT) - return o2.overrideState - o1.overrideState; - } - if (o1.getSourceUid() == o2.getSourceUid()) { - final boolean o1FGJ = o1.isRequestedExpeditedJob(); - if (o1FGJ != o2.isRequestedExpeditedJob()) { - // Attempt to run requested expedited jobs ahead of regular jobs, regardless of - // expedited job quota. - return o1FGJ ? -1 : 1; + @VisibleForTesting + class PendingJobComparator implements Comparator { + private final SparseBooleanArray mUidHasEjCache = new SparseBooleanArray(); + private final SparseLongArray mEarliestRegEnqueueTimeCache = new SparseLongArray(); + + /** + * Refresh sorting determinants based on the current state of {@link #mPendingJobs}. + */ + @GuardedBy("mLock") + @VisibleForTesting + void refreshLocked() { + mUidHasEjCache.clear(); + mEarliestRegEnqueueTimeCache.clear(); + for (int i = 0; i < mPendingJobs.size(); ++i) { + final JobStatus job = mPendingJobs.get(i); + final int uid = job.getSourceUid(); + if (job.isRequestedExpeditedJob()) { + mUidHasEjCache.put(uid, true); + } else { + final long earliestEnqueueTime = + mEarliestRegEnqueueTimeCache.get(uid, Long.MAX_VALUE); + mEarliestRegEnqueueTimeCache.put(uid, + Math.min(earliestEnqueueTime, job.enqueueTime)); + } } } - if (o1.enqueueTime < o2.enqueueTime) { - return -1; + + @Override + public int compare(JobStatus o1, JobStatus o2) { + if (o1 == o2) { + return 0; + } + // Jobs with an override state set (via adb) should be put first as tests/developers + // expect the jobs to run immediately. + if (o1.overrideState != o2.overrideState) { + // Higher override state (OVERRIDE_FULL) should be before lower state + // (OVERRIDE_SOFT) + return o2.overrideState - o1.overrideState; + } + final boolean o1EJ = o1.isRequestedExpeditedJob(); + final boolean o2EJ = o2.isRequestedExpeditedJob(); + if (o1.getSourceUid() == o2.getSourceUid()) { + if (o1EJ != o2EJ) { + // Attempt to run requested expedited jobs ahead of regular jobs, regardless of + // expedited job quota. + return o1EJ ? -1 : 1; + } + } + final boolean uid1HasEj = mUidHasEjCache.get(o1.getSourceUid()); + final boolean uid2HasEj = mUidHasEjCache.get(o2.getSourceUid()); + if ((uid1HasEj || uid2HasEj) && (o1EJ || o2EJ)) { + // We MUST prioritize EJs ahead of regular jobs within a single app. Since we do + // that, in order to satisfy the transitivity constraint of the comparator, if + // any UID has an EJ, we must ensure that the EJ is ordered ahead of the regular + // job of a different app IF the app with an EJ had another job that came before + // the differing app. For example, if app A has regJob1 at t1 and eJob3 at t3 and + // app B has regJob2 at t2, eJob3 must be ordered before regJob2 because it will be + // ordered before regJob1. + // Regular jobs don't need to jump the line. + + final long uid1EarliestRegEnqueueTime = Math.min(o1.enqueueTime, + mEarliestRegEnqueueTimeCache.get(o1.getSourceUid(), Long.MAX_VALUE)); + final long uid2EarliestRegEnqueueTime = Math.min(o2.enqueueTime, + mEarliestRegEnqueueTimeCache.get(o2.getSourceUid(), Long.MAX_VALUE)); + + if (o1EJ && o2EJ) { + if (uid1EarliestRegEnqueueTime < uid2EarliestRegEnqueueTime) { + return -1; + } else if (uid1EarliestRegEnqueueTime > uid2EarliestRegEnqueueTime) { + return 1; + } + } else if (o1EJ && uid1EarliestRegEnqueueTime < o2.enqueueTime) { + return -1; + } else if (o2EJ && uid2EarliestRegEnqueueTime < o1.enqueueTime) { + return 1; + } + } + if (o1.enqueueTime < o2.enqueueTime) { + return -1; + } + return o1.enqueueTime > o2.enqueueTime ? 1 : 0; } - return o1.enqueueTime > o2.enqueueTime ? 1 : 0; - }; + } + + @VisibleForTesting + final PendingJobComparator mPendingJobComparator = new PendingJobComparator(); static void addOrderedItem(ArrayList array, T newItem, Comparator comparator) { int where = Collections.binarySearch(array, newItem, comparator); @@ -1115,7 +1183,7 @@ public class JobSchedulerService extends com.android.server.SystemService // This is a new job, we can just immediately put it on the pending // list and try to run it. mJobPackageTracker.notePending(jobStatus); - addOrderedItem(mPendingJobs, jobStatus, sPendingJobComparator); + addOrderedItem(mPendingJobs, jobStatus, mPendingJobComparator); maybeRunPendingJobsLocked(); } else { evaluateControllerStatesLocked(jobStatus); @@ -1919,7 +1987,7 @@ public class JobSchedulerService extends com.android.server.SystemService if (js != null) { if (isReadyToBeExecutedLocked(js)) { mJobPackageTracker.notePending(js); - addOrderedItem(mPendingJobs, js, sPendingJobComparator); + addOrderedItem(mPendingJobs, js, mPendingJobComparator); } } else { Slog.e(TAG, "Given null job to check individually"); @@ -2064,6 +2132,7 @@ public class JobSchedulerService extends com.android.server.SystemService * Run through list of jobs and execute all possible - at least one is expired so we do * as many as we can. */ + @GuardedBy("mLock") private void queueReadyJobsForExecutionLocked() { // This method will check and capture all ready jobs, so we don't need to keep any messages // in the queue. @@ -2079,7 +2148,7 @@ public class JobSchedulerService extends com.android.server.SystemService mPendingJobs.clear(); stopNonReadyActiveJobsLocked(); mJobs.forEachJob(mReadyQueueFunctor); - mReadyQueueFunctor.postProcess(); + mReadyQueueFunctor.postProcessLocked(); if (DEBUG) { final int queuedJobs = mPendingJobs.size(); @@ -2106,16 +2175,19 @@ public class JobSchedulerService extends com.android.server.SystemService } } - public void postProcess() { + @GuardedBy("mLock") + private void postProcessLocked() { noteJobsPending(newReadyJobs); mPendingJobs.addAll(newReadyJobs); if (mPendingJobs.size() > 1) { - mPendingJobs.sort(sPendingJobComparator); + mPendingJobComparator.refreshLocked(); + mPendingJobs.sort(mPendingJobComparator); } newReadyJobs.clear(); } } + private final ReadyJobQueueFunctor mReadyQueueFunctor = new ReadyJobQueueFunctor(); /** @@ -2180,7 +2252,9 @@ public class JobSchedulerService extends com.android.server.SystemService } } - public void postProcess() { + @GuardedBy("mLock") + @VisibleForTesting + void postProcessLocked() { if (unbatchedCount > 0 || forceBatchedCount >= mConstants.MIN_READY_NON_ACTIVE_JOBS_COUNT) { if (DEBUG) { @@ -2189,7 +2263,8 @@ public class JobSchedulerService extends com.android.server.SystemService noteJobsPending(runnableJobs); mPendingJobs.addAll(runnableJobs); if (mPendingJobs.size() > 1) { - mPendingJobs.sort(sPendingJobComparator); + mPendingJobComparator.refreshLocked(); + mPendingJobs.sort(mPendingJobComparator); } } else { if (DEBUG) { @@ -2210,6 +2285,7 @@ public class JobSchedulerService extends com.android.server.SystemService } private final MaybeReadyJobQueueFunctor mMaybeQueueFunctor = new MaybeReadyJobQueueFunctor(); + @GuardedBy("mLock") private void maybeQueueReadyJobsForExecutionLocked() { if (DEBUG) Slog.d(TAG, "Maybe queuing ready jobs..."); @@ -2217,7 +2293,7 @@ public class JobSchedulerService extends com.android.server.SystemService mPendingJobs.clear(); stopNonReadyActiveJobsLocked(); mJobs.forEachJob(mMaybeQueueFunctor); - mMaybeQueueFunctor.postProcess(); + mMaybeQueueFunctor.postProcessLocked(); } /** Returns true if both the calling and source users for the job are started. */ diff --git a/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java index f2bb47bfb8ad1..b2471fac8b45c 100644 --- a/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java @@ -30,6 +30,7 @@ import static com.android.server.job.JobSchedulerService.RARE_INDEX; import static com.android.server.job.JobSchedulerService.sElapsedRealtimeClock; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; @@ -54,6 +55,9 @@ import android.os.Looper; import android.os.RemoteException; import android.os.ServiceManager; import android.os.SystemClock; +import android.util.Log; +import android.util.SparseBooleanArray; +import android.util.SparseLongArray; import com.android.server.AppStateTracker; import com.android.server.AppStateTrackerImpl; @@ -75,8 +79,11 @@ import org.mockito.quality.Strictness; import java.time.Clock; import java.time.Duration; import java.time.ZoneOffset; +import java.util.Random; public class JobSchedulerServiceTest { + private static final String TAG = JobSchedulerServiceTest.class.getSimpleName(); + private JobSchedulerService mService; private MockitoSession mMockingSession; @@ -178,12 +185,21 @@ public class JobSchedulerServiceTest { } private static JobInfo.Builder createJobInfo() { - return new JobInfo.Builder(351, new ComponentName("foo", "bar")); + return createJobInfo(351); + } + + private static JobInfo.Builder createJobInfo(int jobId) { + return new JobInfo.Builder(jobId, new ComponentName("foo", "bar")); } private JobStatus createJobStatus(String testTag, JobInfo.Builder jobInfoBuilder) { + return createJobStatus(testTag, jobInfoBuilder, 1234); + } + + private JobStatus createJobStatus(String testTag, JobInfo.Builder jobInfoBuilder, + int callingUid) { return JobStatus.createFromJobInfo( - jobInfoBuilder.build(), 1234, "com.android.test", 0, testTag); + jobInfoBuilder.build(), callingUid, "com.android.test", 0, testTag); } /** @@ -716,7 +732,7 @@ public class JobSchedulerServiceTest { assertEquals(i + 1, maybeQueueFunctor.runnableJobs.size()); assertEquals(sElapsedRealtimeClock.millis(), job.getFirstForceBatchedTimeElapsed()); } - maybeQueueFunctor.postProcess(); + maybeQueueFunctor.postProcessLocked(); assertEquals(0, mService.mPendingJobs.size()); // Enough RARE jobs to run. @@ -728,7 +744,7 @@ public class JobSchedulerServiceTest { assertEquals(i + 1, maybeQueueFunctor.runnableJobs.size()); assertEquals(sElapsedRealtimeClock.millis(), job.getFirstForceBatchedTimeElapsed()); } - maybeQueueFunctor.postProcess(); + maybeQueueFunctor.postProcessLocked(); assertEquals(5, mService.mPendingJobs.size()); // Not enough RARE jobs to run, but a non-batched job saves the day. @@ -745,7 +761,7 @@ public class JobSchedulerServiceTest { assertEquals(sElapsedRealtimeClock.millis(), job.getFirstForceBatchedTimeElapsed()); } maybeQueueFunctor.accept(activeJob); - maybeQueueFunctor.postProcess(); + maybeQueueFunctor.postProcessLocked(); assertEquals(3, mService.mPendingJobs.size()); // Not enough RARE jobs to run, but an old RARE job saves the day. @@ -764,7 +780,7 @@ public class JobSchedulerServiceTest { } maybeQueueFunctor.accept(oldRareJob); assertEquals(oldBatchTime, oldRareJob.getFirstForceBatchedTimeElapsed()); - maybeQueueFunctor.postProcess(); + maybeQueueFunctor.postProcessLocked(); assertEquals(3, mService.mPendingJobs.size()); } @@ -853,4 +869,207 @@ public class JobSchedulerServiceTest { 0, "")); } } + + @Test + public void testPendingJobSorting() { + // First letter in job variable name indicate regular (r) or expedited (e). + // Capital letters in job variable name indicate the app/UID. + // Numbers in job variable name indicate the enqueue time. + // Expected sort order: + // eA7 > rA1 > eB6 > rB2 > eC3 > rD4 > eE5 > eF9 > rF8 > eC11 > rC10 > rG12 > rG13 > eE14 + // Intentions: + // * A jobs let us test skipping both regular and expedited jobs of other apps + // * B jobs let us test skipping only regular job of another app without going too far + // * C jobs test that regular jobs don't skip over other app's jobs and that EJs only + // skip up to level of the earliest regular job + // * E jobs test that expedited jobs don't skip the line when the app has no regular jobs + // * F jobs test correct expedited/regular ordering doesn't push jobs too high in list + // * G jobs test correct ordering for regular jobs + JobStatus rA1 = createJobStatus("testPendingJobSorting", createJobInfo(1), 1); + JobStatus rB2 = createJobStatus("testPendingJobSorting", createJobInfo(2), 2); + JobStatus eC3 = createJobStatus("testPendingJobSorting", + createJobInfo(3).setExpedited(true), 3); + JobStatus rD4 = createJobStatus("testPendingJobSorting", createJobInfo(4), 4); + JobStatus eE5 = createJobStatus("testPendingJobSorting", + createJobInfo(5).setExpedited(true), 5); + JobStatus eB6 = createJobStatus("testPendingJobSorting", + createJobInfo(6).setExpedited(true), 2); + JobStatus eA7 = createJobStatus("testPendingJobSorting", + createJobInfo(7).setExpedited(true), 1); + JobStatus rF8 = createJobStatus("testPendingJobSorting", createJobInfo(8), 6); + JobStatus eF9 = createJobStatus("testPendingJobSorting", + createJobInfo(9).setExpedited(true), 6); + JobStatus rC10 = createJobStatus("testPendingJobSorting", createJobInfo(10), 3); + JobStatus eC11 = createJobStatus("testPendingJobSorting", + createJobInfo(11).setExpedited(true), 3); + JobStatus rG12 = createJobStatus("testPendingJobSorting", createJobInfo(12), 7); + JobStatus rG13 = createJobStatus("testPendingJobSorting", createJobInfo(13), 7); + JobStatus eE14 = createJobStatus("testPendingJobSorting", + createJobInfo(14).setExpedited(true), 5); + + rA1.enqueueTime = 1; + rB2.enqueueTime = 2; + eC3.enqueueTime = 3; + rD4.enqueueTime = 4; + eE5.enqueueTime = 5; + eB6.enqueueTime = 6; + eA7.enqueueTime = 7; + rF8.enqueueTime = 8; + eF9.enqueueTime = 9; + rC10.enqueueTime = 10; + eC11.enqueueTime = 11; + rG12.enqueueTime = 12; + rG13.enqueueTime = 13; + eE14.enqueueTime = 14; + + mService.mPendingJobs.clear(); + // Add in random order so sorting is apparent. + mService.mPendingJobs.add(eC3); + mService.mPendingJobs.add(eE5); + mService.mPendingJobs.add(rA1); + mService.mPendingJobs.add(rG13); + mService.mPendingJobs.add(rD4); + mService.mPendingJobs.add(eA7); + mService.mPendingJobs.add(rG12); + mService.mPendingJobs.add(rF8); + mService.mPendingJobs.add(eB6); + mService.mPendingJobs.add(eE14); + mService.mPendingJobs.add(eF9); + mService.mPendingJobs.add(rB2); + mService.mPendingJobs.add(rC10); + mService.mPendingJobs.add(eC11); + + mService.mPendingJobComparator.refreshLocked(); + mService.mPendingJobs.sort(mService.mPendingJobComparator); + + final JobStatus[] expectedOrder = new JobStatus[]{ + eA7, rA1, eB6, rB2, eC3, rD4, eE5, eF9, rF8, eC11, rC10, rG12, rG13, eE14}; + for (int i = 0; i < expectedOrder.length; ++i) { + assertEquals("List wasn't correctly sorted @ index " + i, + expectedOrder[i].getJobId(), mService.mPendingJobs.get(i).getJobId()); + } + } + + private void checkPendingJobInvariants() { + long regJobEnqueueTime = 0; + final SparseBooleanArray regJobSeen = new SparseBooleanArray(); + final SparseLongArray ejEnqueueTimes = new SparseLongArray(); + + for (int i = 0; i < mService.mPendingJobs.size(); ++i) { + final JobStatus job = mService.mPendingJobs.get(i); + final int uid = job.getSourceUid(); + + if (!job.isRequestedExpeditedJob()) { + // Invariant #1: Regular jobs are sorted by enqueue time. + assertTrue("Regular job with earlier enqueue time sorted after a later time: " + + regJobEnqueueTime + " vs " + job.enqueueTime, + regJobEnqueueTime <= job.enqueueTime); + regJobEnqueueTime = job.enqueueTime; + regJobSeen.put(uid, true); + } else { + // Invariant #2: EJs should be before regular jobs for an individual app + if (regJobSeen.get(uid)) { + fail("UID " + uid + " had an EJ ordered after a regular job"); + } + final long ejEnqueueTime = ejEnqueueTimes.get(uid, 0); + // Invariant #3: EJs for an individual app should be sorted by enqueue time. + assertTrue("EJ with earlier enqueue time sorted after a later time: " + + ejEnqueueTime + " vs " + job.enqueueTime, + ejEnqueueTime <= job.enqueueTime); + ejEnqueueTimes.put(uid, job.enqueueTime); + } + } + } + + private static String sortedJobToString(JobStatus job) { + return "testJob " + job.getSourceUid() + "/" + job.getJobId() + "/" + + job.isRequestedExpeditedJob() + "@" + job.enqueueTime; + } + + @Test + public void testPendingJobSorting_Random() { + Random random = new Random(1); // Always use the same series of pseudo random values. + + mService.mPendingJobs.clear(); + + for (int i = 0; i < 2500; ++i) { + JobStatus job = createJobStatus("testPendingJobSorting_Random", + createJobInfo(i).setExpedited(random.nextBoolean()), random.nextInt(250)); + job.enqueueTime = Math.abs(random.nextInt(1_000_000)); + mService.mPendingJobs.add(job); + + mService.mPendingJobComparator.refreshLocked(); + try { + mService.mPendingJobs.sort(mService.mPendingJobComparator); + } catch (Exception e) { + for (JobStatus toDump : mService.mPendingJobs) { + Log.i(TAG, sortedJobToString(toDump)); + } + throw e; + } + checkPendingJobInvariants(); + } + } + + private int sign(int i) { + if (i > 0) { + return 1; + } + if (i < 0) { + return -1; + } + return 0; + } + + @Test + public void testPendingJobSortingTransitivity() { + Random random = new Random(1); // Always use the same series of pseudo random values. + + mService.mPendingJobs.clear(); + + for (int i = 0; i < 250; ++i) { + JobStatus job = createJobStatus("testPendingJobSortingTransitivity", + createJobInfo(i).setExpedited(random.nextBoolean()), random.nextInt(50)); + job.enqueueTime = Math.abs(random.nextInt(1_000_000)); + job.overrideState = random.nextInt(4); + mService.mPendingJobs.add(job); + } + + mService.mPendingJobComparator.refreshLocked(); + + for (int i = 0; i < mService.mPendingJobs.size(); ++i) { + final JobStatus job1 = mService.mPendingJobs.get(i); + + for (int j = 0; j < mService.mPendingJobs.size(); ++j) { + final JobStatus job2 = mService.mPendingJobs.get(j); + final int sign12 = sign(mService.mPendingJobComparator.compare(job1, job2)); + final int sign21 = sign(mService.mPendingJobComparator.compare(job2, job1)); + if (sign12 != -sign21) { + final String job1String = sortedJobToString(job1); + final String job2String = sortedJobToString(job2); + fail("compare(" + job1String + ", " + job2String + ") != " + + "-compare(" + job2String + ", " + job1String + ")"); + } + + for (int k = 0; k < mService.mPendingJobs.size(); ++k) { + final JobStatus job3 = mService.mPendingJobs.get(k); + final int sign23 = sign(mService.mPendingJobComparator.compare(job2, job3)); + final int sign13 = sign(mService.mPendingJobComparator.compare(job1, job3)); + + // Confirm 1 < 2 < 3 or 1 > 2 > 3 or 1 == 2 == 3 + if ((sign12 == sign23 && sign12 != sign13) + // Confirm that if 1 == 2, then (1 < 3 AND 2 < 3) OR (1 > 3 && 2 > 3) + || (sign12 == 0 && sign13 != sign23)) { + final String job1String = sortedJobToString(job1); + final String job2String = sortedJobToString(job2); + final String job3String = sortedJobToString(job3); + fail("Transitivity fail" + + ": compare(" + job1String + ", " + job2String + ")=" + sign12 + + ", compare(" + job2String + ", " + job3String + ")=" + sign23 + + ", compare(" + job1String + ", " + job3String + ")=" + sign13); + } + } + } + } + } }