Fix JobScheduler race condition

The loading of jobs from disk is now done sychronously.

Bug: 16372824
Change-Id: Ica0592d6de51e89662c9e49ed1eb59209b64356c

4 files changed, 77 insertions, 136 deletions

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 f3e77e6a36f6..000000000000
--- 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 3b52bafc3ff7..587f596e420d 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<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 48312b04c9bd..46f557f5e1bb 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 @@ public class JobStore {
         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 @@ public class JobStore {
 
         mJobSet = new ArraySet<JobStatus>();
 
-        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<JobStatus> 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<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 @@ public class JobStore {
                 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 @@ public class JobStore {
             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
index cb8da707b809..2b693a31408e 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.os.PersistableBundle;
 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 @@ 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<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 @@ public class TaskStoreTest extends AndroidTestCase {
         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 @@ public class TaskStoreTest extends AndroidTestCase {
 
         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 @@ public class TaskStoreTest extends AndroidTestCase {
 
     private static class StubClass {}
 
-}
\ No newline at end of file
+}