From 01ac45b6ff2334925c8d24b5278b44e5e30f5622 Mon Sep 17 00:00:00 2001 From: Matthew Williams Date: Tue, 22 Jul 2014 20:44:12 -0700 Subject: [PATCH] Fix JobScheduler race condition The loading of jobs from disk is now done sychronously. Bug: 16372824 Change-Id: Ica0592d6de51e89662c9e49ed1eb59209b64356c --- .../job/JobMapReadFinishedListener.java | 34 ------ .../server/job/JobSchedulerService.java | 24 +---- .../java/com/android/server/job/JobStore.java | 53 ++++++---- .../JobStoreTest.java} | 100 ++++++++---------- 4 files changed, 76 insertions(+), 135 deletions(-) delete mode 100644 services/core/java/com/android/server/job/JobMapReadFinishedListener.java rename services/tests/servicestests/src/com/android/server/{task/TaskStoreTest.java => job/JobStoreTest.java} (65%) 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 f3e77e6a36f6d..0000000000000 --- 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 jobs); -} diff --git a/services/core/java/com/android/server/job/JobSchedulerService.java b/services/core/java/com/android/server/job/JobSchedulerService.java index 3b52bafc3ff75..587f596e420d6 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 @@ import com.android.server.job.controllers.TimeController; * @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 @@ public class JobSchedulerService extends com.android.server.SystemService 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 jobs) { - synchronized (mJobs) { - for (int i=0; i(); - readJobMapFromDiskAsync(callback); + readJobMapFromDisk(mJobSet); } /** @@ -249,12 +256,9 @@ public class JobStore { } } - private void readJobMapFromDiskAsync(JobMapReadFinishedListener callback) { - mIoHandler.post(new ReadJobMapFromDiskRunnable(callback)); - } - - public void readJobMapFromDisk(JobMapReadFinishedListener callback) { - new ReadJobMapFromDiskRunnable(callback).run(); + @VisibleForTesting + public void readJobMapFromDisk(ArraySet jobSet) { + new ReadJobMapFromDiskRunnable(jobSet).run(); } /** @@ -398,13 +402,18 @@ public class JobStore { } /** - * 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 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 jobSet) { + this.jobSet = jobSet; } @Override @@ -414,11 +423,13 @@ public class JobStore { FileInputStream fis = mJobsFile.openRead(); synchronized (JobStore.this) { jobs = readJobMapImpl(fis); + if (jobs != null) { + for (int i=0; i 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 @@ public class TaskStoreTest extends AndroidTestCase { mTaskStoreUnderTest.add(ts); Thread.sleep(IO_WAIT); // Manually load tasks from xml file. - mTaskStoreUnderTest.readJobMapFromDisk(new JobMapReadFinishedListener() { - @Override - public void onJobMapReadFinished(List 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 jobStatusSet = new ArraySet(); + 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 @@ public class TaskStoreTest extends AndroidTestCase { mTaskStoreUnderTest.add(taskStatus1); mTaskStoreUnderTest.add(taskStatus2); Thread.sleep(IO_WAIT); - mTaskStoreUnderTest.readJobMapFromDisk(new JobMapReadFinishedListener() { - @Override - public void onJobMapReadFinished(List 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 jobStatusSet = new ArraySet(); + mTaskStoreUnderTest.readJobMapFromDisk(jobStatusSet); + assertEquals("Incorrect # of persisted tasks.", 2, jobStatusSet.size()); + Iterator 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 @@ public class TaskStoreTest extends AndroidTestCase { mTaskStoreUnderTest.add(taskStatus); Thread.sleep(IO_WAIT); - mTaskStoreUnderTest.readJobMapFromDisk(new JobMapReadFinishedListener() { - @Override - public void onJobMapReadFinished(List tasks) { - assertEquals("Incorrect # of persisted tasks.", 1, tasks.size()); - JobStatus loaded = tasks.get(0); - assertTasksEqual(task, loaded.getJob()); - } - }); + final ArraySet jobStatusSet = new ArraySet(); + mTaskStoreUnderTest.readJobMapFromDisk(jobStatusSet); + assertEquals("Incorrect # of persisted tasks.", 1, jobStatusSet.size()); + JobStatus loaded = jobStatusSet.iterator().next(); + assertTasksEqual(task, loaded.getJob()); } /** @@ -201,4 +187,4 @@ public class TaskStoreTest extends AndroidTestCase { private static class StubClass {} -} \ No newline at end of file +}