From 33d31c5b70c7d056e799e34bb6eccbe6939714ea Mon Sep 17 00:00:00 2001 From: Dianne Hackborn Date: Tue, 16 Feb 2016 10:30:33 -0800 Subject: [PATCH] Simplify job scheduler service locking. Unify all locks to just one lock protecting the entire service. There is really no need for more complicated locking -- there is nothing in the code that can take a long time to complete. And having a single lock will allow various parts of the code to be much simpler and easier to maintain. This is just the first step of the change, switching all of the locking to use one lock. With this done, we can now start simplifying the code. For example, JobStatus no longer needs to do any locking (or have atomic variables and such), it can just rely on its callers holding the global service lock. Change-Id: I502916ed7f2994b601750c67a59a96b1a4e95c6d --- .../server/job/JobSchedulerService.java | 44 +++--- .../android/server/job/JobServiceContext.java | 7 +- .../java/com/android/server/job/JobStore.java | 18 ++- .../job/controllers/AppIdleController.java | 18 ++- .../job/controllers/BatteryController.java | 17 +- .../controllers/ConnectivityController.java | 15 +- .../ContentObserverController.java | 21 +-- .../job/controllers/IdleController.java | 15 +- .../server/job/controllers/JobStatus.java | 63 +++++--- .../job/controllers/StateController.java | 9 +- .../job/controllers/TimeController.java | 146 +++++++++--------- .../com/android/server/job/JobStoreTest.java | 22 +-- 12 files changed, 218 insertions(+), 177 deletions(-) diff --git a/services/core/java/com/android/server/job/JobSchedulerService.java b/services/core/java/com/android/server/job/JobSchedulerService.java index 57cede8e7d092..25b54acf6b786 100644 --- a/services/core/java/com/android/server/job/JobSchedulerService.java +++ b/services/core/java/com/android/server/job/JobSchedulerService.java @@ -78,13 +78,15 @@ import com.android.server.job.controllers.TimeController; * Any function with the suffix 'Locked' also needs to lock on {@link #mJobs}. * @hide */ -public class JobSchedulerService extends com.android.server.SystemService +public final class JobSchedulerService extends com.android.server.SystemService implements StateChangedListener, JobCompletedListener { public static final boolean DEBUG = false; /** The number of concurrent jobs we run at one time. */ private static final int MAX_JOB_CONTEXTS_COUNT = ActivityManager.isLowRamDeviceStatic() ? 3 : 6; static final String TAG = "JobSchedulerService"; + /** Global local for all job scheduler state. */ + final Object mLock = new Object(); /** Master list of jobs. */ final JobStore mJobs; @@ -207,6 +209,10 @@ public class JobSchedulerService extends com.android.server.SystemService } }; + public Object getLock() { + return mLock; + } + @Override public void onStartUser(int userHandle) { mStartedUsers.add(userHandle); @@ -231,7 +237,7 @@ public class JobSchedulerService extends com.android.server.SystemService } public int scheduleAsPackage(JobInfo job, int uId, String packageName, int userId) { - JobStatus jobStatus = new JobStatus(job, uId, packageName, userId); + JobStatus jobStatus = new JobStatus(getLock(), job, uId, packageName, userId); try { if (ActivityManagerNative.getDefault().getAppStartMode(uId, job.getService().getPackageName()) == ActivityManager.APP_START_MODE_DISABLED) { @@ -243,7 +249,7 @@ public class JobSchedulerService extends com.android.server.SystemService } if (DEBUG) Slog.d(TAG, "SCHEDULE: " + jobStatus.toShortString()); JobStatus toCancel; - synchronized (mJobs) { + synchronized (mLock) { toCancel = mJobs.getJobByUidAndJobId(uId, job.getId()); } startTrackingJob(jobStatus, toCancel); @@ -256,7 +262,7 @@ public class JobSchedulerService extends com.android.server.SystemService public List getPendingJobs(int uid) { ArrayList outList = new ArrayList(); - synchronized (mJobs) { + synchronized (mLock) { ArraySet jobs = mJobs.getJobs(); for (int i=0; i jobsForUser; - synchronized (mJobs) { + synchronized (mLock) { jobsForUser = mJobs.getJobsByUser(userHandle); } for (int i=0; i jobsForUid; - synchronized (mJobs) { + synchronized (mLock) { jobsForUid = mJobs.getJobsByUid(uid); } for (int i=0; i mJobSet; + final Object mLock; final Context mContext; private int mDirtyOperations; @@ -85,7 +86,7 @@ public class JobStore { synchronized (sSingletonLock) { if (sSingleton == null) { sSingleton = new JobStore(jobManagerService.getContext(), - Environment.getDataDirectory()); + jobManagerService.getLock(), Environment.getDataDirectory()); } return sSingleton; } @@ -96,7 +97,7 @@ public class JobStore { */ @VisibleForTesting public static JobStore initAndGetForTesting(Context context, File dataDir) { - JobStore jobStoreUnderTest = new JobStore(context, dataDir); + JobStore jobStoreUnderTest = new JobStore(context, new Object(), dataDir); jobStoreUnderTest.clear(); return jobStoreUnderTest; } @@ -104,7 +105,8 @@ public class JobStore { /** * Construct the instance of the job store. This results in a blocking read from disk. */ - private JobStore(Context context, File dataDir) { + private JobStore(Context context, Object lock, File dataDir) { + mLock = lock; mContext = context; mDirtyOperations = 0; @@ -266,14 +268,14 @@ public class JobStore { /** * Runnable that writes {@link #mJobSet} out to xml. - * NOTE: This Runnable locks on JobStore.this + * NOTE: This Runnable locks on mLock */ private class WriteJobsMapToDiskRunnable implements Runnable { @Override public void run() { final long startElapsed = SystemClock.elapsedRealtime(); List mStoreCopy = new ArrayList(); - synchronized (JobStore.this) { + synchronized (mLock) { // Copy over the jobs so we can release the lock before writing. for (int i=0; i jobs; FileInputStream fis = mJobsFile.openRead(); - synchronized (JobStore.this) { + synchronized (mLock) { jobs = readJobMapImpl(fis); if (jobs != null) { for (int i=0; i JobStatus js = new JobStatus( - jobBuilder.build(), uid, sourcePackageName, sourceUserId, elapsedRuntimes.first, - elapsedRuntimes.second); + mLock, jobBuilder.build(), uid, sourcePackageName, sourceUserId, + elapsedRuntimes.first, elapsedRuntimes.second); return js; } diff --git a/services/core/java/com/android/server/job/controllers/AppIdleController.java b/services/core/java/com/android/server/job/controllers/AppIdleController.java index 5f3da7577a5d1..f7f34aec60386 100644 --- a/services/core/java/com/android/server/job/controllers/AppIdleController.java +++ b/services/core/java/com/android/server/job/controllers/AppIdleController.java @@ -48,14 +48,16 @@ public class AppIdleController extends StateController { public static AppIdleController get(JobSchedulerService service) { synchronized (sCreationLock) { if (sController == null) { - sController = new AppIdleController(service, service.getContext()); + sController = new AppIdleController(service, service.getContext(), + service.getLock()); } return sController; } } - private AppIdleController(StateChangedListener stateChangedListener, Context context) { - super(stateChangedListener, context); + private AppIdleController(StateChangedListener stateChangedListener, Context context, + Object lock) { + super(stateChangedListener, context, lock); mUsageStatsInternal = LocalServices.getService(UsageStatsManagerInternal.class); mAppIdleParoleOn = mUsageStatsInternal.isAppIdleParoleOn(); mUsageStatsInternal.addAppIdleStateChangeListener(new AppIdleStateChangeListener()); @@ -63,7 +65,7 @@ public class AppIdleController extends StateController { @Override public void maybeStartTrackingJob(JobStatus jobStatus, JobStatus lastJob) { - synchronized (mTrackedTasks) { + synchronized (mLock) { mTrackedTasks.add(jobStatus); String packageName = jobStatus.getSourcePackageName(); final boolean appIdle = !mAppIdleParoleOn && mUsageStatsInternal.isAppIdle(packageName, @@ -78,7 +80,7 @@ public class AppIdleController extends StateController { @Override public void maybeStopTrackingJob(JobStatus jobStatus, boolean forUpdate) { - synchronized (mTrackedTasks) { + synchronized (mLock) { mTrackedTasks.remove(jobStatus); } } @@ -87,7 +89,7 @@ public class AppIdleController extends StateController { public void dumpControllerState(PrintWriter pw) { pw.println("AppIdle"); pw.println("Parole On: " + mAppIdleParoleOn); - synchronized (mTrackedTasks) { + synchronized (mLock) { for (JobStatus task : mTrackedTasks) { pw.print(task.getSourcePackageName()); pw.print(":idle=" + !task.appNotIdleConstraintSatisfied.get()); @@ -100,7 +102,7 @@ public class AppIdleController extends StateController { void setAppIdleParoleOn(boolean isAppIdleParoleOn) { // Flag if any app's idle state has changed boolean changed = false; - synchronized (mTrackedTasks) { + synchronized (mLock) { if (mAppIdleParoleOn == isAppIdleParoleOn) { return; } @@ -128,7 +130,7 @@ public class AppIdleController extends StateController { @Override public void onAppIdleStateChanged(String packageName, int userId, boolean idle) { boolean changed = false; - synchronized (mTrackedTasks) { + synchronized (mLock) { if (mAppIdleParoleOn) { return; } diff --git a/services/core/java/com/android/server/job/controllers/BatteryController.java b/services/core/java/com/android/server/job/controllers/BatteryController.java index b322a3e96c149..b101c82649837 100644 --- a/services/core/java/com/android/server/job/controllers/BatteryController.java +++ b/services/core/java/com/android/server/job/controllers/BatteryController.java @@ -53,7 +53,7 @@ public class BatteryController extends StateController { synchronized (sCreationLock) { if (sController == null) { sController = new BatteryController(taskManagerService, - taskManagerService.getContext()); + taskManagerService.getContext(), taskManagerService.getLock()); } } return sController; @@ -67,11 +67,12 @@ public class BatteryController extends StateController { @VisibleForTesting public static BatteryController getForTesting(StateChangedListener stateChangedListener, Context context) { - return new BatteryController(stateChangedListener, context); + return new BatteryController(stateChangedListener, context, new Object()); } - private BatteryController(StateChangedListener stateChangedListener, Context context) { - super(stateChangedListener, context); + private BatteryController(StateChangedListener stateChangedListener, Context context, + Object lock) { + super(stateChangedListener, context, lock); mChargeTracker = new ChargingTracker(); mChargeTracker.startTracking(); } @@ -80,7 +81,7 @@ public class BatteryController extends StateController { public void maybeStartTrackingJob(JobStatus taskStatus, JobStatus lastJob) { final boolean isOnStablePower = mChargeTracker.isOnStablePower(); if (taskStatus.hasChargingConstraint()) { - synchronized (mTrackedTasks) { + synchronized (mLock) { mTrackedTasks.add(taskStatus); taskStatus.chargingConstraintSatisfied.set(isOnStablePower); } @@ -90,7 +91,7 @@ public class BatteryController extends StateController { @Override public void maybeStopTrackingJob(JobStatus taskStatus, boolean forUpdate) { if (taskStatus.hasChargingConstraint()) { - synchronized (mTrackedTasks) { + synchronized (mLock) { mTrackedTasks.remove(taskStatus); } } @@ -102,7 +103,7 @@ public class BatteryController extends StateController { Slog.d(TAG, "maybeReportNewChargingState: " + stablePower); } boolean reportChange = false; - synchronized (mTrackedTasks) { + synchronized (mLock) { for (JobStatus ts : mTrackedTasks) { boolean previous = ts.chargingConstraintSatisfied.getAndSet(stablePower); if (previous != stablePower) { @@ -200,7 +201,7 @@ public class BatteryController extends StateController { public void dumpControllerState(PrintWriter pw) { pw.println("Batt."); pw.println("Stable power: " + mChargeTracker.isOnStablePower()); - synchronized (mTrackedTasks) { + synchronized (mLock) { Iterator it = mTrackedTasks.iterator(); if (it.hasNext()) { pw.print(String.valueOf(it.next().hashCode())); diff --git a/services/core/java/com/android/server/job/controllers/ConnectivityController.java b/services/core/java/com/android/server/job/controllers/ConnectivityController.java index b84658a7db96d..29b54c2eb7166 100644 --- a/services/core/java/com/android/server/job/controllers/ConnectivityController.java +++ b/services/core/java/com/android/server/job/controllers/ConnectivityController.java @@ -58,14 +58,15 @@ public class ConnectivityController extends StateController implements public static ConnectivityController get(JobSchedulerService jms) { synchronized (sCreationLock) { if (mSingleton == null) { - mSingleton = new ConnectivityController(jms, jms.getContext()); + mSingleton = new ConnectivityController(jms, jms.getContext(), jms.getLock()); } return mSingleton; } } - private ConnectivityController(StateChangedListener stateChangedListener, Context context) { - super(stateChangedListener, context); + private ConnectivityController(StateChangedListener stateChangedListener, Context context, + Object lock) { + super(stateChangedListener, context, lock); // Register connectivity changed BR. IntentFilter intentFilter = new IntentFilter(); intentFilter.addAction(ConnectivityManager.CONNECTIVITY_ACTION); @@ -84,7 +85,7 @@ public class ConnectivityController extends StateController implements @Override public void maybeStartTrackingJob(JobStatus jobStatus, JobStatus lastJob) { if (jobStatus.hasConnectivityConstraint() || jobStatus.hasUnmeteredConstraint()) { - synchronized (mTrackedJobs) { + synchronized (mLock) { jobStatus.connectivityConstraintSatisfied.set(mNetworkConnected); jobStatus.unmeteredConstraintSatisfied.set(mNetworkUnmetered); mTrackedJobs.add(jobStatus); @@ -95,7 +96,7 @@ public class ConnectivityController extends StateController implements @Override public void maybeStopTrackingJob(JobStatus jobStatus, boolean forUpdate) { if (jobStatus.hasConnectivityConstraint() || jobStatus.hasUnmeteredConstraint()) { - synchronized (mTrackedJobs) { + synchronized (mLock) { mTrackedJobs.remove(jobStatus); } } @@ -105,7 +106,7 @@ public class ConnectivityController extends StateController implements * @param userId Id of the user for whom we are updating the connectivity state. */ private void updateTrackedJobs(int userId) { - synchronized (mTrackedJobs) { + synchronized (mLock) { boolean changed = false; for (JobStatus js : mTrackedJobs) { if (js.getUserId() != userId) { @@ -128,7 +129,7 @@ public class ConnectivityController extends StateController implements * We know the network has just come up. We want to run any jobs that are ready. */ public synchronized void onNetworkActive() { - synchronized (mTrackedJobs) { + synchronized (mLock) { for (JobStatus js : mTrackedJobs) { if (js.isReady()) { if (DEBUG) { diff --git a/services/core/java/com/android/server/job/controllers/ContentObserverController.java b/services/core/java/com/android/server/job/controllers/ContentObserverController.java index 212cc949364d1..af994f037af76 100644 --- a/services/core/java/com/android/server/job/controllers/ContentObserverController.java +++ b/services/core/java/com/android/server/job/controllers/ContentObserverController.java @@ -57,7 +57,7 @@ public class ContentObserverController extends StateController { synchronized (sCreationLock) { if (sController == null) { sController = new ContentObserverController(taskManagerService, - taskManagerService.getContext()); + taskManagerService.getContext(), taskManagerService.getLock()); } } return sController; @@ -66,17 +66,18 @@ public class ContentObserverController extends StateController { @VisibleForTesting public static ContentObserverController getForTesting(StateChangedListener stateChangedListener, Context context) { - return new ContentObserverController(stateChangedListener, context); + return new ContentObserverController(stateChangedListener, context, new Object()); } - private ContentObserverController(StateChangedListener stateChangedListener, Context context) { - super(stateChangedListener, context); + private ContentObserverController(StateChangedListener stateChangedListener, Context context, + Object lock) { + super(stateChangedListener, context, lock); } @Override public void maybeStartTrackingJob(JobStatus taskStatus, JobStatus lastJob) { if (taskStatus.hasContentTriggerConstraint()) { - synchronized (mTrackedTasks) { + synchronized (mLock) { if (taskStatus.contentObserverJobInstance == null) { taskStatus.contentObserverJobInstance = new JobInstance(taskStatus); } @@ -128,7 +129,7 @@ public class ContentObserverController extends StateController { @Override public void prepareForExecution(JobStatus taskStatus) { if (taskStatus.hasContentTriggerConstraint()) { - synchronized (mTrackedTasks) { + synchronized (mLock) { if (taskStatus.contentObserverJobInstance != null) { taskStatus.changedUris = taskStatus.contentObserverJobInstance.mChangedUris; taskStatus.changedAuthorities @@ -143,7 +144,7 @@ public class ContentObserverController extends StateController { @Override public void maybeStopTrackingJob(JobStatus taskStatus, boolean forUpdate) { if (taskStatus.hasContentTriggerConstraint()) { - synchronized (mTrackedTasks) { + synchronized (mLock) { if (!forUpdate) { // We won't do this reset if being called for an update, because // we know it will be immediately followed by maybeStartTrackingJob... @@ -162,7 +163,7 @@ public class ContentObserverController extends StateController { public void rescheduleForFailure(JobStatus newJob, JobStatus failureToReschedule) { if (failureToReschedule.hasContentTriggerConstraint() && newJob.hasContentTriggerConstraint()) { - synchronized (mTrackedTasks) { + synchronized (mLock) { // Our job has failed, and we are scheduling a new job for it. // Copy the last reported content changes in to the new job, so when // we schedule the new one we will pick them up and report them again. @@ -184,7 +185,7 @@ public class ContentObserverController extends StateController { @Override public void onChange(boolean selfChange, Uri uri) { boolean reportChange = false; - synchronized (mTrackedTasks) { + synchronized (mLock) { final int N = mJobs.size(); for (int i=0; i it = mTrackedTasks.iterator(); if (it.hasNext()) { pw.print(String.valueOf(it.next().hashCode())); diff --git a/services/core/java/com/android/server/job/controllers/IdleController.java b/services/core/java/com/android/server/job/controllers/IdleController.java index 9f4cdef0e5f76..6b85f222663cb 100644 --- a/services/core/java/com/android/server/job/controllers/IdleController.java +++ b/services/core/java/com/android/server/job/controllers/IdleController.java @@ -51,14 +51,15 @@ public class IdleController extends StateController { public static IdleController get(JobSchedulerService service) { synchronized (sCreationLock) { if (sController == null) { - sController = new IdleController(service, service.getContext()); + sController = new IdleController(service, service.getContext(), service.getLock()); } return sController; } } - private IdleController(StateChangedListener stateChangedListener, Context context) { - super(stateChangedListener, context); + private IdleController(StateChangedListener stateChangedListener, Context context, + Object lock) { + super(stateChangedListener, context, lock); initIdleStateTracking(); } @@ -68,7 +69,7 @@ public class IdleController extends StateController { @Override public void maybeStartTrackingJob(JobStatus taskStatus, JobStatus lastJob) { if (taskStatus.hasIdleConstraint()) { - synchronized (mTrackedTasks) { + synchronized (mLock) { mTrackedTasks.add(taskStatus); taskStatus.idleConstraintSatisfied.set(mIdleTracker.isIdle()); } @@ -77,7 +78,7 @@ public class IdleController extends StateController { @Override public void maybeStopTrackingJob(JobStatus taskStatus, boolean forUpdate) { - synchronized (mTrackedTasks) { + synchronized (mLock) { mTrackedTasks.remove(taskStatus); } } @@ -86,7 +87,7 @@ public class IdleController extends StateController { * Interaction with the task manager service */ void reportNewIdleState(boolean isIdle) { - synchronized (mTrackedTasks) { + synchronized (mLock) { for (JobStatus task : mTrackedTasks) { task.idleConstraintSatisfied.set(isIdle); } @@ -194,7 +195,7 @@ public class IdleController extends StateController { @Override public void dumpControllerState(PrintWriter pw) { - synchronized (mTrackedTasks) { + synchronized (mLock) { pw.print("Idle: "); pw.println(mIdleTracker.isIdle() ? "true" : "false"); pw.println(mTrackedTasks.size()); diff --git a/services/core/java/com/android/server/job/controllers/JobStatus.java b/services/core/java/com/android/server/job/controllers/JobStatus.java index c4d564c4e577e..3cf105480b5d4 100644 --- a/services/core/java/com/android/server/job/controllers/JobStatus.java +++ b/services/core/java/com/android/server/job/controllers/JobStatus.java @@ -46,6 +46,11 @@ public class JobStatus { public static final long NO_LATEST_RUNTIME = Long.MAX_VALUE; public static final long NO_EARLIEST_RUNTIME = 0L; + /** + * Service global lock. NOTE: this should be removed, having this class just rely on + * its callers doing the appropriate locking. + */ + final Object lock; final JobInfo job; /** Uid of the package requesting this job. */ final int callingUid; @@ -94,8 +99,9 @@ public class JobStatus { return callingUid; } - private JobStatus(JobInfo job, int callingUid, String sourcePackageName, int sourceUserId, - int numFailures) { + private JobStatus(Object lock, JobInfo job, int callingUid, String sourcePackageName, + int sourceUserId, int numFailures) { + this.lock = lock; this.job = job; this.callingUid = callingUid; this.name = job.getService().flattenToShortString(); @@ -124,8 +130,9 @@ public class JobStatus { /** Copy constructor. */ public JobStatus(JobStatus jobStatus) { - this(jobStatus.getJob(), jobStatus.getUid(), jobStatus.getSourcePackageName(), - jobStatus.getSourceUserId(), jobStatus.getNumFailures()); + this(jobStatus.lock, jobStatus.getJob(), jobStatus.getUid(), + jobStatus.getSourcePackageName(), jobStatus.getSourceUserId(), + jobStatus.getNumFailures()); this.earliestRunTimeElapsedMillis = jobStatus.getEarliestRunTime(); this.latestRunTimeElapsedMillis = jobStatus.getLatestRunTimeElapsed(); } @@ -138,8 +145,9 @@ public class JobStatus { * @param sourceUserId User id for whom this job is scheduled. -1 indicates this is same as the * calling userId. */ - public JobStatus(JobInfo job, int callingUid, String sourcePackageName, int sourceUserId) { - this(job, callingUid, sourcePackageName, sourceUserId, 0); + public JobStatus(Object lock, JobInfo job, int callingUid, String sourcePackageName, + int sourceUserId) { + this(lock, job, callingUid, sourcePackageName, sourceUserId, 0); final long elapsedNow = SystemClock.elapsedRealtime(); @@ -161,9 +169,9 @@ public class JobStatus { * wallclock runtime rather than resetting it on every boot. * We consider a freshly loaded job to no longer be in back-off. */ - public JobStatus(JobInfo job, int callingUid, String sourcePackageName, int sourceUserId, - long earliestRunTimeElapsedMillis, long latestRunTimeElapsedMillis) { - this(job, callingUid, sourcePackageName, sourceUserId, 0); + public JobStatus(Object lock, JobInfo job, int callingUid, String sourcePackageName, + int sourceUserId, long earliestRunTimeElapsedMillis, long latestRunTimeElapsedMillis) { + this(lock, job, callingUid, sourcePackageName, sourceUserId, 0); this.earliestRunTimeElapsedMillis = earliestRunTimeElapsedMillis; this.latestRunTimeElapsedMillis = latestRunTimeElapsedMillis; @@ -172,7 +180,8 @@ public class JobStatus { /** Create a new job to be rescheduled with the provided parameters. */ public JobStatus(JobStatus rescheduling, long newEarliestRuntimeElapsedMillis, long newLatestRuntimeElapsedMillis, int backoffAttempt) { - this(rescheduling.job, rescheduling.getUid(), rescheduling.getSourcePackageName(), + this(rescheduling.lock, rescheduling.job, rescheduling.getUid(), + rescheduling.getSourcePackageName(), rescheduling.getSourceUserId(), backoffAttempt); earliestRunTimeElapsedMillis = newEarliestRuntimeElapsedMillis; @@ -275,27 +284,31 @@ public class JobStatus { * @return Whether or not this job is ready to run, based on its requirements. This is true if * the constraints are satisfied or the deadline on the job has expired. */ - public synchronized boolean isReady() { - // Deadline constraint trumps other constraints (except for periodic jobs where deadline - // (is an implementation detail. A periodic job should only run if it's constraints are - // satisfied). - // AppNotIdle implicit constraint trumps all! - return (isConstraintsSatisfied() + public boolean isReady() { + synchronized (lock) { + // Deadline constraint trumps other constraints (except for periodic jobs where deadline + // (is an implementation detail. A periodic job should only run if it's constraints are + // satisfied). + // AppNotIdle implicit constraint trumps all! + return (isConstraintsSatisfied() || (!job.isPeriodic() - && hasDeadlineConstraint() && deadlineConstraintSatisfied.get())) - && appNotIdleConstraintSatisfied.get(); + && hasDeadlineConstraint() && deadlineConstraintSatisfied.get())) + && appNotIdleConstraintSatisfied.get(); + } } /** * @return Whether the constraints set on this job are satisfied. */ - public synchronized boolean isConstraintsSatisfied() { - return (!hasChargingConstraint() || chargingConstraintSatisfied.get()) - && (!hasTimingDelayConstraint() || timeDelayConstraintSatisfied.get()) - && (!hasConnectivityConstraint() || connectivityConstraintSatisfied.get()) - && (!hasUnmeteredConstraint() || unmeteredConstraintSatisfied.get()) - && (!hasIdleConstraint() || idleConstraintSatisfied.get()) - && (!hasContentTriggerConstraint() || contentTriggerConstraintSatisfied.get()); + public boolean isConstraintsSatisfied() { + synchronized (lock) { + return (!hasChargingConstraint() || chargingConstraintSatisfied.get()) + && (!hasTimingDelayConstraint() || timeDelayConstraintSatisfied.get()) + && (!hasConnectivityConstraint() || connectivityConstraintSatisfied.get()) + && (!hasUnmeteredConstraint() || unmeteredConstraintSatisfied.get()) + && (!hasIdleConstraint() || idleConstraintSatisfied.get()) + && (!hasContentTriggerConstraint() || contentTriggerConstraintSatisfied.get()); + } } public boolean matches(int uid, int jobId) { diff --git a/services/core/java/com/android/server/job/controllers/StateController.java b/services/core/java/com/android/server/job/controllers/StateController.java index b619ea8cdd5dd..8b8611a14d69e 100644 --- a/services/core/java/com/android/server/job/controllers/StateController.java +++ b/services/core/java/com/android/server/job/controllers/StateController.java @@ -30,13 +30,16 @@ import java.io.PrintWriter; */ public abstract class StateController { protected static final boolean DEBUG = JobSchedulerService.DEBUG; - protected Context mContext; - protected StateChangedListener mStateChangedListener; + protected final Context mContext; + protected final Object mLock; + protected final StateChangedListener mStateChangedListener; protected boolean mDeviceIdleMode; - public StateController(StateChangedListener stateChangedListener, Context context) { + public StateController(StateChangedListener stateChangedListener, Context context, + Object lock) { mStateChangedListener = stateChangedListener; mContext = context; + mLock = lock; } public void deviceIdleModeChanged(boolean enabled) { diff --git a/services/core/java/com/android/server/job/controllers/TimeController.java b/services/core/java/com/android/server/job/controllers/TimeController.java index a68c3adabb087..bf17827d49f8d 100644 --- a/services/core/java/com/android/server/job/controllers/TimeController.java +++ b/services/core/java/com/android/server/job/controllers/TimeController.java @@ -54,13 +54,14 @@ public class TimeController extends StateController { public static synchronized TimeController get(JobSchedulerService jms) { if (mSingleton == null) { - mSingleton = new TimeController(jms, jms.getContext()); + mSingleton = new TimeController(jms, jms.getContext(), jms.getLock()); } return mSingleton; } - private TimeController(StateChangedListener stateChangedListener, Context context) { - super(stateChangedListener, context); + private TimeController(StateChangedListener stateChangedListener, Context context, + Object lock) { + super(stateChangedListener, context, lock); mNextJobExpiredElapsedMillis = Long.MAX_VALUE; mNextDelayExpiredElapsedMillis = Long.MAX_VALUE; @@ -71,27 +72,28 @@ public class TimeController extends StateController { * list. */ @Override - public synchronized void maybeStartTrackingJob(JobStatus job, JobStatus lastJob) { - if (job.hasTimingDelayConstraint() || job.hasDeadlineConstraint()) { - maybeStopTrackingJob(job, false); - boolean isInsert = false; - ListIterator it = mTrackedJobs.listIterator(mTrackedJobs.size()); - while (it.hasPrevious()) { - JobStatus ts = it.previous(); - if (ts.getLatestRunTimeElapsed() < job.getLatestRunTimeElapsed()) { - // Insert - isInsert = true; - break; + public void maybeStartTrackingJob(JobStatus job, JobStatus lastJob) { + synchronized (mLock) { + if (job.hasTimingDelayConstraint() || job.hasDeadlineConstraint()) { + maybeStopTrackingJob(job, false); + boolean isInsert = false; + ListIterator it = mTrackedJobs.listIterator(mTrackedJobs.size()); + while (it.hasPrevious()) { + JobStatus ts = it.previous(); + if (ts.getLatestRunTimeElapsed() < job.getLatestRunTimeElapsed()) { + // Insert + isInsert = true; + break; + } } + if (isInsert) { + it.next(); + } + it.add(job); + maybeUpdateAlarms( + job.hasTimingDelayConstraint() ? job.getEarliestRunTime() : Long.MAX_VALUE, + job.hasDeadlineConstraint() ? job.getLatestRunTimeElapsed() : Long.MAX_VALUE); } - if(isInsert) - { - it.next(); - } - it.add(job); - maybeUpdateAlarms( - job.hasTimingDelayConstraint() ? job.getEarliestRunTime() : Long.MAX_VALUE, - job.hasDeadlineConstraint() ? job.getLatestRunTimeElapsed() : Long.MAX_VALUE); } } @@ -101,10 +103,12 @@ public class TimeController extends StateController { * Really an == comparison should be enough, but why play with fate? We'll do <=. */ @Override - public synchronized void maybeStopTrackingJob(JobStatus job, boolean forUpdate) { - if (mTrackedJobs.remove(job)) { - checkExpiredDelaysAndResetAlarm(); - checkExpiredDeadlinesAndResetAlarm(); + public void maybeStopTrackingJob(JobStatus job, boolean forUpdate) { + synchronized (mLock) { + if (mTrackedJobs.remove(job)) { + checkExpiredDelaysAndResetAlarm(); + checkExpiredDeadlinesAndResetAlarm(); + } } } @@ -131,63 +135,67 @@ public class TimeController extends StateController { * Checks list of jobs for ones that have an expired deadline, sending them to the JobScheduler * if so, removing them from this list, and updating the alarm for the next expiry time. */ - private synchronized void checkExpiredDeadlinesAndResetAlarm() { - long nextExpiryTime = Long.MAX_VALUE; - final long nowElapsedMillis = SystemClock.elapsedRealtime(); + private void checkExpiredDeadlinesAndResetAlarm() { + synchronized (mLock) { + long nextExpiryTime = Long.MAX_VALUE; + final long nowElapsedMillis = SystemClock.elapsedRealtime(); - Iterator it = mTrackedJobs.iterator(); - while (it.hasNext()) { - JobStatus job = it.next(); - if (!job.hasDeadlineConstraint()) { - continue; - } - final long jobDeadline = job.getLatestRunTimeElapsed(); + Iterator it = mTrackedJobs.iterator(); + while (it.hasNext()) { + JobStatus job = it.next(); + if (!job.hasDeadlineConstraint()) { + continue; + } + final long jobDeadline = job.getLatestRunTimeElapsed(); - if (jobDeadline <= nowElapsedMillis) { - job.deadlineConstraintSatisfied.set(true); - mStateChangedListener.onRunJobNow(job); - it.remove(); - } else { // Sorted by expiry time, so take the next one and stop. - nextExpiryTime = jobDeadline; - break; + if (jobDeadline <= nowElapsedMillis) { + job.deadlineConstraintSatisfied.set(true); + mStateChangedListener.onRunJobNow(job); + it.remove(); + } else { // Sorted by expiry time, so take the next one and stop. + nextExpiryTime = jobDeadline; + break; + } } + setDeadlineExpiredAlarm(nextExpiryTime); } - setDeadlineExpiredAlarm(nextExpiryTime); } /** * Handles alarm that notifies us that a job's delay has expired. Iterates through the list of * tracked jobs and marks them as ready as appropriate. */ - private synchronized void checkExpiredDelaysAndResetAlarm() { - final long nowElapsedMillis = SystemClock.elapsedRealtime(); - long nextDelayTime = Long.MAX_VALUE; - boolean ready = false; - Iterator it = mTrackedJobs.iterator(); - while (it.hasNext()) { - final JobStatus job = it.next(); - if (!job.hasTimingDelayConstraint()) { - continue; - } - final long jobDelayTime = job.getEarliestRunTime(); - if (jobDelayTime <= nowElapsedMillis) { - job.timeDelayConstraintSatisfied.set(true); - if (canStopTrackingJob(job)) { - it.remove(); + private void checkExpiredDelaysAndResetAlarm() { + synchronized (mLock) { + final long nowElapsedMillis = SystemClock.elapsedRealtime(); + long nextDelayTime = Long.MAX_VALUE; + boolean ready = false; + Iterator it = mTrackedJobs.iterator(); + while (it.hasNext()) { + final JobStatus job = it.next(); + if (!job.hasTimingDelayConstraint()) { + continue; } - if (job.isReady()) { - ready = true; - } - } else { // Keep going through list to get next delay time. - if (nextDelayTime > jobDelayTime) { - nextDelayTime = jobDelayTime; + final long jobDelayTime = job.getEarliestRunTime(); + if (jobDelayTime <= nowElapsedMillis) { + job.timeDelayConstraintSatisfied.set(true); + if (canStopTrackingJob(job)) { + it.remove(); + } + if (job.isReady()) { + ready = true; + } + } else { // Keep going through list to get next delay time. + if (nextDelayTime > jobDelayTime) { + nextDelayTime = jobDelayTime; + } } } + if (ready) { + mStateChangedListener.onControllerStateChanged(); + } + setDelayExpiredAlarm(nextDelayTime); } - if (ready) { - mStateChangedListener.onControllerStateChanged(); - } - setDelayExpiredAlarm(nextDelayTime); } private void maybeUpdateAlarms(long delayExpiredElapsed, long deadlineExpiredElapsed) { 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 577c3a1af1957..fbcd156c99759 100644 --- a/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java +++ b/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java @@ -58,7 +58,7 @@ public class JobStoreTest extends AndroidTestCase { .setMinimumLatency(runFromMillis) .setPersisted(true) .build(); - final JobStatus ts = new JobStatus(task, SOME_UID, null, -1); + final JobStatus ts = new JobStatus(this, task, SOME_UID, null, -1); mTaskStoreUnderTest.add(ts); Thread.sleep(IO_WAIT); // Manually load tasks from xml file. @@ -91,8 +91,8 @@ public class JobStoreTest extends AndroidTestCase { .setRequiredNetworkType(JobInfo.NETWORK_TYPE_UNMETERED) .setPersisted(true) .build(); - final JobStatus taskStatus1 = new JobStatus(task1, SOME_UID, null, -1); - final JobStatus taskStatus2 = new JobStatus(task2, SOME_UID, null, -1); + final JobStatus taskStatus1 = new JobStatus(this, task1, SOME_UID, null, -1); + final JobStatus taskStatus2 = new JobStatus(this, task2, SOME_UID, null, -1); mTaskStoreUnderTest.add(taskStatus1); mTaskStoreUnderTest.add(taskStatus2); Thread.sleep(IO_WAIT); @@ -140,7 +140,7 @@ public class JobStoreTest extends AndroidTestCase { extras.putInt("into", 3); b.setExtras(extras); final JobInfo task = b.build(); - JobStatus taskStatus = new JobStatus(task, SOME_UID, null, -1); + JobStatus taskStatus = new JobStatus(this, task, SOME_UID, null, -1); mTaskStoreUnderTest.add(taskStatus); Thread.sleep(IO_WAIT); @@ -157,7 +157,8 @@ public class JobStoreTest extends AndroidTestCase { .setPeriodic(10000L) .setRequiresCharging(true) .setPersisted(true); - JobStatus taskStatus = new JobStatus(b.build(), SOME_UID, "com.google.android.gms", 0); + JobStatus taskStatus = new JobStatus(this, b.build(), SOME_UID, + "com.google.android.gms", 0); mTaskStoreUnderTest.add(taskStatus); Thread.sleep(IO_WAIT); @@ -178,7 +179,7 @@ public class JobStoreTest extends AndroidTestCase { .setPeriodic(5*60*60*1000, 1*60*60*1000) .setRequiresCharging(true) .setPersisted(true); - JobStatus taskStatus = new JobStatus(b.build(), SOME_UID, null, -1); + JobStatus taskStatus = new JobStatus(this, b.build(), SOME_UID, null, -1); mTaskStoreUnderTest.add(taskStatus); Thread.sleep(IO_WAIT); @@ -203,7 +204,8 @@ public class JobStoreTest extends AndroidTestCase { SystemClock.elapsedRealtime() + (TWO_HOURS * ONE_HOUR) + TWO_HOURS; // > period+flex final long invalidEarlyRuntimeElapsedMillis = invalidLateRuntimeElapsedMillis - TWO_HOURS; // Early is (late - period). - final JobStatus js = new JobStatus(b.build(), SOME_UID, "somePackage", 0 /* sourceUserId */, + final JobStatus js = new JobStatus(this, b.build(), SOME_UID, "somePackage", + 0 /* sourceUserId */, invalidEarlyRuntimeElapsedMillis, invalidLateRuntimeElapsedMillis); mTaskStoreUnderTest.add(js); @@ -229,7 +231,7 @@ public class JobStoreTest extends AndroidTestCase { .setOverrideDeadline(5000) .setPriority(42) .setPersisted(true); - final JobStatus js = new JobStatus(b.build(), SOME_UID, null, -1); + final JobStatus js = new JobStatus(this, b.build(), SOME_UID, null, -1); mTaskStoreUnderTest.add(js); Thread.sleep(IO_WAIT); final ArraySet jobStatusSet = new ArraySet(); @@ -245,12 +247,12 @@ public class JobStoreTest extends AndroidTestCase { JobInfo.Builder b = new Builder(42, mComponent) .setOverrideDeadline(10000) .setPersisted(false); - JobStatus jsNonPersisted = new JobStatus(b.build(), SOME_UID, null, -1); + JobStatus jsNonPersisted = new JobStatus(this, b.build(), SOME_UID, null, -1); mTaskStoreUnderTest.add(jsNonPersisted); b = new Builder(43, mComponent) .setOverrideDeadline(10000) .setPersisted(true); - JobStatus jsPersisted = new JobStatus(b.build(), SOME_UID, null, -1); + JobStatus jsPersisted = new JobStatus(this, b.build(), SOME_UID, null, -1); mTaskStoreUnderTest.add(jsPersisted); Thread.sleep(IO_WAIT); final ArraySet jobStatusSet = new ArraySet();