Fix JobScheduler race condition
The loading of jobs from disk is now done sychronously.
Bug: 16372824
Change-Id: Ica0592d6de51e89662c9e49ed1eb59209b64356c
diff --git a/services/core/java/com/android/server/job/JobMapReadFinishedListener.java b/services/core/java/com/android/server/job/JobMapReadFinishedListener.java
deleted file mode 100644
index f3e77e6..0000000
--- a/services/core/java/com/android/server/job/JobMapReadFinishedListener.java
+++ /dev/null
@@ -1,34 +0,0 @@
-/*
- * Copyright (C) 2014 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License
- */
-
-package com.android.server.job;
-
-import java.util.List;
-
-import com.android.server.job.controllers.JobStatus;
-
-/**
- * Callback definition for I/O thread to let the JobManagerService know when
- * I/O read has completed. Done this way so we don't stall the main thread on
- * boot.
- */
-public interface JobMapReadFinishedListener {
-
- /**
- * Called by the {@link JobStore} at boot, when the disk read is finished.
- */
- public void onJobMapReadFinished(List<JobStatus> jobs);
-}
diff --git a/services/core/java/com/android/server/job/JobSchedulerService.java b/services/core/java/com/android/server/job/JobSchedulerService.java
index 3b52baf..587f596 100644
--- a/services/core/java/com/android/server/job/JobSchedulerService.java
+++ b/services/core/java/com/android/server/job/JobSchedulerService.java
@@ -69,7 +69,7 @@
* @hide
*/
public class JobSchedulerService extends com.android.server.SystemService
- implements StateChangedListener, JobCompletedListener, JobMapReadFinishedListener {
+ implements StateChangedListener, JobCompletedListener {
// TODO: Switch this off for final version.
static final boolean DEBUG = true;
/** The number of concurrent jobs we run at one time. */
@@ -487,28 +487,6 @@
mHandler.obtainMessage(MSG_JOB_EXPIRED, jobStatus).sendToTarget();
}
- /**
- * Disk I/O is finished, take the list of jobs we read from disk and add them to our
- * {@link JobStore}.
- * This is run on the {@link com.android.server.IoThread} instance, which is a separate thread,
- * and is called once at boot.
- */
- @Override
- public void onJobMapReadFinished(List<JobStatus> jobs) {
- synchronized (mJobs) {
- for (int i=0; i<jobs.size(); i++) {
- JobStatus js = jobs.get(i);
- if (mJobs.containsJobIdForUid(js.getJobId(), js.getUid())) {
- // An app with BOOT_COMPLETED *might* have decided to reschedule their job, in
- // the same amount of time it took us to read it from disk. If this is the case
- // we leave it be.
- continue;
- }
- startTrackingJob(js);
- }
- }
- }
-
private class JobHandler extends Handler {
public JobHandler(Looper looper) {
diff --git a/services/core/java/com/android/server/job/JobStore.java b/services/core/java/com/android/server/job/JobStore.java
index 48312b0..46f557f 100644
--- a/services/core/java/com/android/server/job/JobStore.java
+++ b/services/core/java/com/android/server/job/JobStore.java
@@ -88,19 +88,26 @@
synchronized (sSingletonLock) {
if (sSingleton == null) {
sSingleton = new JobStore(jobManagerService.getContext(),
- Environment.getDataDirectory(), jobManagerService);
+ Environment.getDataDirectory());
}
return sSingleton;
}
}
+ /**
+ * @return A freshly initialized job store object, with no loaded jobs.
+ */
@VisibleForTesting
- public static JobStore initAndGetForTesting(Context context, File dataDir,
- JobMapReadFinishedListener callback) {
- return new JobStore(context, dataDir, callback);
+ public static JobStore initAndGetForTesting(Context context, File dataDir) {
+ JobStore jobStoreUnderTest = new JobStore(context, dataDir);
+ jobStoreUnderTest.clear();
+ return jobStoreUnderTest;
}
- private JobStore(Context context, File dataDir, JobMapReadFinishedListener callback) {
+ /**
+ * Construct the instance of the job store. This results in a blocking read from disk.
+ */
+ private JobStore(Context context, File dataDir) {
mContext = context;
mDirtyOperations = 0;
@@ -111,7 +118,7 @@
mJobSet = new ArraySet<JobStatus>();
- readJobMapFromDiskAsync(callback);
+ readJobMapFromDisk(mJobSet);
}
/**
@@ -249,12 +256,9 @@
}
}
- private void readJobMapFromDiskAsync(JobMapReadFinishedListener callback) {
- mIoHandler.post(new ReadJobMapFromDiskRunnable(callback));
- }
-
- public void readJobMapFromDisk(JobMapReadFinishedListener callback) {
- new ReadJobMapFromDiskRunnable(callback).run();
+ @VisibleForTesting
+ public void readJobMapFromDisk(ArraySet<JobStatus> jobSet) {
+ new ReadJobMapFromDiskRunnable(jobSet).run();
}
/**
@@ -398,13 +402,18 @@
}
/**
- * Runnable that reads list of persisted job from xml.
- * NOTE: This Runnable locks on JobStore.this
+ * Runnable that reads list of persisted job from xml. This is run once at start up, so doesn't
+ * need to go through {@link JobStore#add(com.android.server.job.controllers.JobStatus)}.
*/
private class ReadJobMapFromDiskRunnable implements Runnable {
- private JobMapReadFinishedListener mCallback;
- public ReadJobMapFromDiskRunnable(JobMapReadFinishedListener callback) {
- mCallback = callback;
+ private final ArraySet<JobStatus> jobSet;
+
+ /**
+ * @param jobSet Reference to the (empty) set of JobStatus objects that back the JobStore,
+ * so that after disk read we can populate it directly.
+ */
+ ReadJobMapFromDiskRunnable(ArraySet<JobStatus> jobSet) {
+ this.jobSet = jobSet;
}
@Override
@@ -414,11 +423,13 @@
FileInputStream fis = mJobsFile.openRead();
synchronized (JobStore.this) {
jobs = readJobMapImpl(fis);
+ if (jobs != null) {
+ for (int i=0; i<jobs.size(); i++) {
+ this.jobSet.add(jobs.get(i));
+ }
+ }
}
fis.close();
- if (jobs != null) {
- mCallback.onJobMapReadFinished(jobs);
- }
} catch (FileNotFoundException e) {
if (JobSchedulerService.DEBUG) {
Slog.d(TAG, "Could not find jobs file, probably there was nothing to load.");
@@ -673,4 +684,4 @@
return Pair.create(earliestRunTimeElapsed, latestRunTimeElapsed);
}
}
-}
\ No newline at end of file
+}
diff --git a/services/tests/servicestests/src/com/android/server/task/TaskStoreTest.java b/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java
similarity index 64%
rename from services/tests/servicestests/src/com/android/server/task/TaskStoreTest.java
rename to services/tests/servicestests/src/com/android/server/job/JobStoreTest.java
index cb8da70..2b693a3 100644
--- a/services/tests/servicestests/src/com/android/server/task/TaskStoreTest.java
+++ b/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java
@@ -1,4 +1,4 @@
-package com.android.server.task;
+package com.android.server.job;
import android.content.ComponentName;
@@ -9,40 +9,32 @@
import android.test.AndroidTestCase;
import android.test.RenamingDelegatingContext;
import android.util.Log;
+import android.util.ArraySet;
-import com.android.server.job.JobMapReadFinishedListener;
-import com.android.server.job.JobStore;
import com.android.server.job.controllers.JobStatus;
-import java.util.List;
+import java.util.Iterator;
/**
* Test reading and writing correctly from file.
*/
-public class TaskStoreTest extends AndroidTestCase {
+public class JobStoreTest extends AndroidTestCase {
private static final String TAG = "TaskStoreTest";
private static final String TEST_PREFIX = "_test_";
- // private static final int USER_NON_0 = 3;
+
private static final int SOME_UID = 34234;
private ComponentName mComponent;
- private static final long IO_WAIT = 600L;
+ private static final long IO_WAIT = 1000L;
JobStore mTaskStoreUnderTest;
Context mTestContext;
- JobMapReadFinishedListener mTaskMapReadFinishedListenerStub =
- new JobMapReadFinishedListener() {
- @Override
- public void onJobMapReadFinished(List<JobStatus> tasks) {
- // do nothing.
- }
- };
@Override
public void setUp() throws Exception {
mTestContext = new RenamingDelegatingContext(getContext(), TEST_PREFIX);
Log.d(TAG, "Saving tasks to '" + mTestContext.getFilesDir() + "'");
- mTaskStoreUnderTest = JobStore.initAndGetForTesting(mTestContext,
- mTestContext.getFilesDir(), mTaskMapReadFinishedListenerStub);
+ mTaskStoreUnderTest =
+ JobStore.initAndGetForTesting(mTestContext, mTestContext.getFilesDir());
mComponent = new ComponentName(getContext().getPackageName(), StubClass.class.getName());
}
@@ -69,19 +61,17 @@
mTaskStoreUnderTest.add(ts);
Thread.sleep(IO_WAIT);
// Manually load tasks from xml file.
- mTaskStoreUnderTest.readJobMapFromDisk(new JobMapReadFinishedListener() {
- @Override
- public void onJobMapReadFinished(List<JobStatus> tasks) {
- assertEquals("Didn't get expected number of persisted tasks.", 1, tasks.size());
- JobStatus loadedTaskStatus = tasks.get(0);
- assertTasksEqual(task, loadedTaskStatus.getJob());
- assertEquals("Different uids.", SOME_UID, tasks.get(0).getUid());
- compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",
- ts.getEarliestRunTime(), loadedTaskStatus.getEarliestRunTime());
- compareTimestampsSubjectToIoLatency("Late run-times not the same after read.",
- ts.getLatestRunTimeElapsed(), loadedTaskStatus.getLatestRunTimeElapsed());
- }
- });
+ final ArraySet<JobStatus> jobStatusSet = new ArraySet<JobStatus>();
+ mTaskStoreUnderTest.readJobMapFromDisk(jobStatusSet);
+
+ assertEquals("Didn't get expected number of persisted tasks.", 1, jobStatusSet.size());
+ final JobStatus loadedTaskStatus = jobStatusSet.iterator().next();
+ assertTasksEqual(task, loadedTaskStatus.getJob());
+ assertEquals("Different uids.", SOME_UID, loadedTaskStatus.getUid());
+ compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",
+ ts.getEarliestRunTime(), loadedTaskStatus.getEarliestRunTime());
+ compareTimestampsSubjectToIoLatency("Late run-times not the same after read.",
+ ts.getLatestRunTimeElapsed(), loadedTaskStatus.getLatestRunTimeElapsed());
}
@@ -104,26 +94,25 @@
mTaskStoreUnderTest.add(taskStatus1);
mTaskStoreUnderTest.add(taskStatus2);
Thread.sleep(IO_WAIT);
- mTaskStoreUnderTest.readJobMapFromDisk(new JobMapReadFinishedListener() {
- @Override
- public void onJobMapReadFinished(List<JobStatus> tasks) {
- assertEquals("Incorrect # of persisted tasks.", 2, tasks.size());
- JobStatus loaded1 = tasks.get(0);
- JobStatus loaded2 = tasks.get(1);
- assertTasksEqual(task1, loaded1.getJob());
- assertTasksEqual(task2, loaded2.getJob());
- // Check that the loaded task has the correct runtimes.
- compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",
- taskStatus1.getEarliestRunTime(), loaded1.getEarliestRunTime());
- compareTimestampsSubjectToIoLatency("Late run-times not the same after read.",
- taskStatus1.getLatestRunTimeElapsed(), loaded1.getLatestRunTimeElapsed());
- compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",
- taskStatus2.getEarliestRunTime(), loaded2.getEarliestRunTime());
- compareTimestampsSubjectToIoLatency("Late run-times not the same after read.",
- taskStatus2.getLatestRunTimeElapsed(), loaded2.getLatestRunTimeElapsed());
- }
- });
+ final ArraySet<JobStatus> jobStatusSet = new ArraySet<JobStatus>();
+ mTaskStoreUnderTest.readJobMapFromDisk(jobStatusSet);
+ assertEquals("Incorrect # of persisted tasks.", 2, jobStatusSet.size());
+ Iterator<JobStatus> it = jobStatusSet.iterator();
+ JobStatus loaded1 = it.next();
+ JobStatus loaded2 = it.next();
+ assertTasksEqual(task1, loaded1.getJob());
+ assertTasksEqual(task2, loaded2.getJob());
+
+ // Check that the loaded task has the correct runtimes.
+ compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",
+ taskStatus1.getEarliestRunTime(), loaded1.getEarliestRunTime());
+ compareTimestampsSubjectToIoLatency("Late run-times not the same after read.",
+ taskStatus1.getLatestRunTimeElapsed(), loaded1.getLatestRunTimeElapsed());
+ compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",
+ taskStatus2.getEarliestRunTime(), loaded2.getEarliestRunTime());
+ compareTimestampsSubjectToIoLatency("Late run-times not the same after read.",
+ taskStatus2.getLatestRunTimeElapsed(), loaded2.getLatestRunTimeElapsed());
}
@@ -144,15 +133,12 @@
mTaskStoreUnderTest.add(taskStatus);
Thread.sleep(IO_WAIT);
- mTaskStoreUnderTest.readJobMapFromDisk(new JobMapReadFinishedListener() {
- @Override
- public void onJobMapReadFinished(List<JobStatus> tasks) {
- assertEquals("Incorrect # of persisted tasks.", 1, tasks.size());
- JobStatus loaded = tasks.get(0);
- assertTasksEqual(task, loaded.getJob());
- }
- });
+ final ArraySet<JobStatus> jobStatusSet = new ArraySet<JobStatus>();
+ mTaskStoreUnderTest.readJobMapFromDisk(jobStatusSet);
+ assertEquals("Incorrect # of persisted tasks.", 1, jobStatusSet.size());
+ JobStatus loaded = jobStatusSet.iterator().next();
+ assertTasksEqual(task, loaded.getJob());
}
/**
@@ -201,4 +187,4 @@
private static class StubClass {}
-}
\ No newline at end of file
+}