From 36ccc9004e5614a20350b723271d9e7df111ce31 Mon Sep 17 00:00:00 2001 From: alexbianchi Date: Mon, 22 Aug 2022 17:58:36 +0000 Subject: [PATCH] Keep Tracking Jobs Whose Constraints Have Been Dropped If a job is close to its deadline than FC will stop tracking it. But if the deadline changes then the job could wind up in some FC data structures but not others, and data will not be cleaned up properly when the job is complete. By continuing to track jobs with 0 constraints and only stopping when the job is completed, we can avoid this edge case. Bug:243167593 Test: atest frameworks/base/services/tests/mockingservicestests/src/com/android/server/job Test: atest frameworks/base/services/tests/servicestests/src/com/android/server/job Test: atest CtsJobSchedulerTestCases Change-Id: I665b9f07675e8c18f6dcdaf8813794213f95139b --- .../controllers/FlexibilityController.java | 32 ++++----- .../FlexibilityControllerTest.java | 72 ++++++++++--------- 2 files changed, 53 insertions(+), 51 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/job/controllers/FlexibilityController.java b/apex/jobscheduler/service/java/com/android/server/job/controllers/FlexibilityController.java index 3ca1ad58955dc..88690351ee617 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/controllers/FlexibilityController.java +++ b/apex/jobscheduler/service/java/com/android/server/job/controllers/FlexibilityController.java @@ -406,7 +406,7 @@ public final class FlexibilityController extends StateController { final ArraySet changedJobs = new ArraySet<>(); synchronized (mLock) { final long nowElapsed = sElapsedRealtimeClock.millis(); - for (int j = 1; j <= mFlexibilityTracker.size(); j++) { + for (int j = 0; j < mFlexibilityTracker.size(); j++) { final ArraySet jobs = mFlexibilityTracker .getJobsByNumRequiredConstraints(j); for (int i = 0; i < jobs.size(); i++) { @@ -445,7 +445,7 @@ public final class FlexibilityController extends StateController { FlexibilityTracker(int numFlexibleConstraints) { mTrackedJobs = new ArrayList<>(); - for (int i = 0; i < numFlexibleConstraints; i++) { + for (int i = 0; i <= numFlexibleConstraints; i++) { mTrackedJobs.add(new ArraySet()); } } @@ -457,15 +457,15 @@ public final class FlexibilityController extends StateController { Slog.wtfStack(TAG, "Asked for a larger number of constraints than exists."); return null; } - return mTrackedJobs.get(numRequired - 1); + return mTrackedJobs.get(numRequired); } /** adds a JobStatus object based on number of required flexible constraints. */ public void add(JobStatus js) { - if (js.getNumRequiredFlexibleConstraints() <= 0) { + if (js.getNumRequiredFlexibleConstraints() < 0) { return; } - mTrackedJobs.get(js.getNumRequiredFlexibleConstraints() - 1).add(js); + mTrackedJobs.get(js.getNumRequiredFlexibleConstraints()).add(js); } /** Removes a JobStatus object. */ @@ -473,7 +473,7 @@ public final class FlexibilityController extends StateController { if (js.getNumRequiredFlexibleConstraints() == 0) { return; } - mTrackedJobs.get(js.getNumRequiredFlexibleConstraints() - 1).remove(js); + mTrackedJobs.get(js.getNumRequiredFlexibleConstraints()).remove(js); } public void resetJobNumDroppedConstraints(JobStatus js, long nowElapsed) { @@ -498,21 +498,15 @@ public final class FlexibilityController extends StateController { /** * Adjusts number of required flexible constraints and sorts it into the tracker. * Returns false if the job status's number of flexible constraints is now 0. - * Jobs with 0 required flexible constraints are removed from the tracker. */ - public boolean adjustJobsRequiredConstraints(JobStatus js, int n, long nowElapsed) { - if (n == 0) { - return false; + public boolean adjustJobsRequiredConstraints(JobStatus js, int adjustBy, long nowElapsed) { + if (adjustBy != 0) { + remove(js); + js.adjustNumRequiredFlexibleConstraints(adjustBy); + js.setFlexibilityConstraintSatisfied(nowElapsed, isFlexibilitySatisfiedLocked(js)); + add(js); } - remove(js); - js.adjustNumRequiredFlexibleConstraints(n); - js.setFlexibilityConstraintSatisfied(nowElapsed, isFlexibilitySatisfiedLocked(js)); - if (js.getNumRequiredFlexibleConstraints() <= 0) { - maybeStopTrackingJobLocked(js, null, false); - return false; - } - add(js); - return true; + return js.getNumRequiredFlexibleConstraints() > 0; } public int size() { diff --git a/services/tests/mockingservicestests/src/com/android/server/job/controllers/FlexibilityControllerTest.java b/services/tests/mockingservicestests/src/com/android/server/job/controllers/FlexibilityControllerTest.java index 9d039bd09a2fc..0fb8af0916230 100644 --- a/services/tests/mockingservicestests/src/com/android/server/job/controllers/FlexibilityControllerTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/job/controllers/FlexibilityControllerTest.java @@ -29,6 +29,7 @@ import static com.android.server.job.controllers.FlexibilityController.FcConfig. import static com.android.server.job.controllers.FlexibilityController.FcConfig.KEY_FALLBACK_FLEXIBILITY_DEADLINE; import static com.android.server.job.controllers.FlexibilityController.FcConfig.KEY_FLEXIBILITY_ENABLED; import static com.android.server.job.controllers.FlexibilityController.FcConfig.KEY_PERCENTS_TO_DROP_NUM_FLEXIBLE_CONSTRAINTS; +import static com.android.server.job.controllers.FlexibilityController.NUM_FLEXIBLE_CONSTRAINTS; import static com.android.server.job.controllers.JobStatus.CONSTRAINT_BATTERY_NOT_LOW; import static com.android.server.job.controllers.JobStatus.CONSTRAINT_CHARGING; import static com.android.server.job.controllers.JobStatus.CONSTRAINT_FLEXIBLE; @@ -190,7 +191,7 @@ public class FlexibilityControllerTest { */ @Test public void testDefaultVariableValues() { - assertEquals(FlexibilityController.NUM_FLEXIBLE_CONSTRAINTS, + assertEquals(NUM_FLEXIBLE_CONSTRAINTS, mFlexibilityController.mFcConfig.DEFAULT_PERCENT_TO_DROP_FLEXIBLE_CONSTRAINTS.length ); } @@ -473,9 +474,9 @@ public class FlexibilityControllerTest { public void testFlexibilityTracker() { FlexibilityController.FlexibilityTracker flexTracker = mFlexibilityController.new - FlexibilityTracker(FlexibilityController.NUM_FLEXIBLE_CONSTRAINTS); - - assertEquals(4, flexTracker.size()); + FlexibilityTracker(NUM_FLEXIBLE_CONSTRAINTS); + // Plus one for jobs with 0 required constraint. + assertEquals(NUM_FLEXIBLE_CONSTRAINTS + 1, flexTracker.size()); JobStatus[] jobs = new JobStatus[4]; JobInfo.Builder jb; for (int i = 0; i < jobs.length; i++) { @@ -495,47 +496,53 @@ public class FlexibilityControllerTest { synchronized (mFlexibilityController.mLock) { ArrayList> trackedJobs = flexTracker.getArrayList(); - assertEquals(0, trackedJobs.get(0).size()); + assertEquals(1, trackedJobs.get(0).size()); assertEquals(0, trackedJobs.get(1).size()); - assertEquals(3, trackedJobs.get(2).size()); - assertEquals(0, trackedJobs.get(3).size()); - - flexTracker.adjustJobsRequiredConstraints(jobs[0], -1, FROZEN_TIME); - assertEquals(0, trackedJobs.get(0).size()); - assertEquals(1, trackedJobs.get(1).size()); - assertEquals(2, trackedJobs.get(2).size()); - assertEquals(0, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(2).size()); + assertEquals(3, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(4).size()); flexTracker.adjustJobsRequiredConstraints(jobs[0], -1, FROZEN_TIME); assertEquals(1, trackedJobs.get(0).size()); assertEquals(0, trackedJobs.get(1).size()); - assertEquals(2, trackedJobs.get(2).size()); - assertEquals(0, trackedJobs.get(3).size()); + assertEquals(1, trackedJobs.get(2).size()); + assertEquals(2, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(4).size()); flexTracker.adjustJobsRequiredConstraints(jobs[0], -1, FROZEN_TIME); - assertEquals(0, trackedJobs.get(0).size()); + assertEquals(1, trackedJobs.get(0).size()); + assertEquals(1, trackedJobs.get(1).size()); + assertEquals(0, trackedJobs.get(2).size()); + assertEquals(2, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(4).size()); + + flexTracker.adjustJobsRequiredConstraints(jobs[0], -1, FROZEN_TIME); + assertEquals(2, trackedJobs.get(0).size()); assertEquals(0, trackedJobs.get(1).size()); - assertEquals(2, trackedJobs.get(2).size()); - assertEquals(0, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(2).size()); + assertEquals(2, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(4).size()); flexTracker.remove(jobs[1]); - assertEquals(0, trackedJobs.get(0).size()); + assertEquals(2, trackedJobs.get(0).size()); assertEquals(0, trackedJobs.get(1).size()); - assertEquals(1, trackedJobs.get(2).size()); - assertEquals(0, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(2).size()); + assertEquals(1, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(4).size()); flexTracker.resetJobNumDroppedConstraints(jobs[0], FROZEN_TIME); - assertEquals(0, trackedJobs.get(0).size()); + assertEquals(2, trackedJobs.get(0).size()); assertEquals(0, trackedJobs.get(1).size()); - assertEquals(2, trackedJobs.get(2).size()); - assertEquals(0, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(2).size()); + assertEquals(2, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(4).size()); flexTracker.adjustJobsRequiredConstraints(jobs[0], -2, FROZEN_TIME); - - assertEquals(1, trackedJobs.get(0).size()); - assertEquals(0, trackedJobs.get(1).size()); - assertEquals(1, trackedJobs.get(2).size()); - assertEquals(0, trackedJobs.get(3).size()); + assertEquals(2, trackedJobs.get(0).size()); + assertEquals(1, trackedJobs.get(1).size()); + assertEquals(0, trackedJobs.get(2).size()); + assertEquals(1, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(4).size()); final long nowElapsed = ((DEFAULT_FALLBACK_FLEXIBILITY_DEADLINE_MS / 2) + HOUR_IN_MILLIS); @@ -543,10 +550,11 @@ public class FlexibilityControllerTest { Clock.fixed(Instant.ofEpochMilli(nowElapsed), ZoneOffset.UTC); flexTracker.resetJobNumDroppedConstraints(jobs[0], nowElapsed); - assertEquals(0, trackedJobs.get(0).size()); - assertEquals(1, trackedJobs.get(1).size()); + assertEquals(2, trackedJobs.get(0).size()); + assertEquals(0, trackedJobs.get(1).size()); assertEquals(1, trackedJobs.get(2).size()); - assertEquals(0, trackedJobs.get(3).size()); + assertEquals(1, trackedJobs.get(3).size()); + assertEquals(0, trackedJobs.get(4).size()); } }