Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 48a30db7 authored by Matthew Williams's avatar Matthew Williams
Browse files

Fix lock ordering in JobScheduler 

BUG: 17625667
Two part clean-up.
1) Don't try to lock in onControllerStateChanged. Do it in the handleMessage
instead where the rest of the locking is. This is sufficient to fix this bug.
2) The other side of the deadlock came b/c we lock when cancelling and calling
stopTrackingJob. Controllers handle their own locking so this isn't
necessary. B/c of a potential race from the controller side, added an explicit
check for the JSS to only run an expired job if it still exists.

Change-Id: Iaeebbc19437eb5b73e3ced3168f1fc13e564a4be
parent f7efd5c6

services/core/java/com/android/server/job/JobSchedulerService.java

+99 −101
Original line number Diff line number Diff line
@@ -195,12 +195,13 @@ public class JobSchedulerService extends com.android.server.SystemService 
    }



    private void cancelJobsForUser(int userHandle) {

        List<JobStatus> jobsForUser;

        synchronized (mJobs) {

            List<JobStatus> jobsForUser = mJobs.getJobsByUser(userHandle);

            jobsForUser = mJobs.getJobsByUser(userHandle);

        }

        for (int i=0; i<jobsForUser.size(); i++) {

            JobStatus toRemove = jobsForUser.get(i);

                cancelJobLocked(toRemove);

            }

            cancelJobImpl(toRemove);

        }

    }
@@ -208,16 +209,16 @@ public class JobSchedulerService extends com.android.server.SystemService 
     * Entry point from client to cancel all jobs originating from their uid.

     * This will remove the job from the master list, and cancel the job if it was staged for

     * execution or being executed.

     * @param uid To check against for removal of a job.

     * @param uid Uid to check against for removal of a job.

     */

    public void cancelJobsForUid(int uid) {

        // Remove from master list.

        List<JobStatus> jobsForUid;

        synchronized (mJobs) {

            List<JobStatus> jobsForUid = mJobs.getJobsByUid(uid);

            jobsForUid = mJobs.getJobsByUid(uid);

        }

        for (int i=0; i<jobsForUid.size(); i++) {

            JobStatus toRemove = jobsForUid.get(i);

                cancelJobLocked(toRemove);

            }

            cancelJobImpl(toRemove);

        }

    }
@@ -232,23 +233,24 @@ public class JobSchedulerService extends com.android.server.SystemService 
        JobStatus toCancel;

        synchronized (mJobs) {

            toCancel = mJobs.getJobByUidAndJobId(uid, jobId);

            if (toCancel != null) {

                cancelJobLocked(toCancel);

        }

        if (toCancel != null) {

            cancelJobImpl(toCancel);

        }

    }



    private void cancelJobLocked(JobStatus cancelled) {

    private void cancelJobImpl(JobStatus cancelled) {

        if (DEBUG) {

            Slog.d(TAG, "Cancelling: " + cancelled);

        }

        // Remove from store.

        stopTrackingJob(cancelled);

        synchronized (mJobs) {

            // Remove from pending queue.

            mPendingJobs.remove(cancelled);

            // Cancel if running.

            stopJobOnServiceContextLocked(cancelled);

        }

    }



    /**

     * Initializes the system service.
@@ -482,22 +484,14 @@ public class JobSchedulerService extends com.android.server.SystemService 
    // StateChangedListener implementations.



    /**

     * Off-board work to our handler thread as quickly as possible, b/c this call is probably being

     * made on the main thread.

     * For now this takes the job and if it's ready to run it will run it. In future we might not

     * provide the job, so that the StateChangedListener has to run through its list of jobs to

     * see which are ready. This will further decouple the controllers from the execution logic.

     * Posts a message to the {@link com.android.server.job.JobSchedulerService.JobHandler} that

     * some controller's state has changed, so as to run through the list of jobs and start/stop

     * any that are eligible.

     */

    @Override

    public void onControllerStateChanged() {

        synchronized (mJobs) {

            if (mReadyToRock) {

                // Post a message to to run through the list of jobs and start/stop any that

                // are eligible.

        mHandler.obtainMessage(MSG_CHECK_JOB).sendToTarget();

    }

        }

    }



    @Override

    public void onRunJobNow(JobStatus jobStatus) {
@@ -512,21 +506,29 @@ public class JobSchedulerService extends com.android.server.SystemService 


        @Override

        public void handleMessage(Message message) {

            synchronized (mJobs) {

                if (!mReadyToRock) {

                    return;

                }

            }

            switch (message.what) {

                case MSG_JOB_EXPIRED:

                    synchronized (mJobs) {

                        JobStatus runNow = (JobStatus) message.obj;

                        // runNow can be null, which is a controller's way of indicating that its

                        // state is such that all ready jobs should be run immediately.

                        if (runNow != null && !mPendingJobs.contains(runNow)) {

                        if (runNow != null && !mPendingJobs.contains(runNow)

                                && mJobs.containsJob(runNow)) {

                            mPendingJobs.add(runNow);

                        }

                        queueReadyJobsForExecutionLockedH();

                    }

                    queueReadyJobsForExecutionH();

                    break;

                case MSG_CHECK_JOB:

                    synchronized (mJobs) {

                        // Check the list of jobs and run some of them if we feel inclined.

                    maybeQueueReadyJobsForExecutionH();

                        maybeQueueReadyJobsForExecutionLockedH();

                    }

                    break;

            }

            maybeRunPendingJobsH();
@@ -538,8 +540,7 @@ public class JobSchedulerService extends com.android.server.SystemService 
         * Run through list of jobs and execute all possible - at least one is expired so we do

         * as many as we can.

         */

        private void queueReadyJobsForExecutionH() {

            synchronized (mJobs) {

        private void queueReadyJobsForExecutionLockedH() {

            ArraySet<JobStatus> jobs = mJobs.getJobs();

            if (DEBUG) {

                Slog.d(TAG, "queuing all ready jobs for execution:");
@@ -564,7 +565,6 @@ public class JobSchedulerService extends com.android.server.SystemService 
                }

            }

        }

        }



        /**

         * The state of at least one job has changed. Here is where we could enforce various
@@ -575,8 +575,7 @@ public class JobSchedulerService extends com.android.server.SystemService 
         * If more than 4 jobs total are ready we send them all off.

         * TODO: It would be nice to consolidate these sort of high-level policies somewhere.

         */

        private void maybeQueueReadyJobsForExecutionH() {

            synchronized (mJobs) {

        private void maybeQueueReadyJobsForExecutionLockedH() {

            int chargingCount = 0;

            int idleCount =  0;

            int backoffCount = 0;
@@ -609,14 +608,14 @@ public class JobSchedulerService extends com.android.server.SystemService 
                    chargingCount >= MIN_CHARGING_COUNT ||

                    runnableJobs.size() >= MIN_READY_JOBS_COUNT) {

                if (DEBUG) {

                        Slog.d(TAG, "maybeQueueReadyJobsForExecutionH: Running jobs.");

                    Slog.d(TAG, "maybeQueueReadyJobsForExecutionLockedH: Running jobs.");

                }

                for (int i=0; i<runnableJobs.size(); i++) {

                    mPendingJobs.add(runnableJobs.get(i));

                }

            } else {

                if (DEBUG) {

                        Slog.d(TAG, "maybeQueueReadyJobsForExecutionH: Not running anything.");

                    Slog.d(TAG, "maybeQueueReadyJobsForExecutionLockedH: Not running anything.");

                }

            }

            if (DEBUG) {
@@ -625,7 +624,6 @@ public class JobSchedulerService extends com.android.server.SystemService 
                        runnableJobs.size());

            }

        }

        }



        /**

         * Criteria for moving a job into the pending queue:

services/core/java/com/android/server/job/JobStore.java

+12 −10
Original line number Diff line number Diff line
@@ -50,15 +50,9 @@ import org.xmlpull.v1.XmlPullParserException; 
import org.xmlpull.v1.XmlSerializer;



/**

 * Maintain a list of classes, and accessor methods/logic for these jobs.

 * This class offers the following functionality:

 *     - When a job is added, it will determine if the job requirements have changed (update) and

 *       whether the controllers need to be updated.

 *     - Persists JobInfos, figures out when to to rewrite the JobInfo to disk.

 *     - Handles rescheduling of jobs.

 *       - When a periodic job is executed and must be re-added.

 *       - When a job fails and the client requests that it be retried with backoff.

 *       - This class <strong>is not</strong> thread-safe.

 * Maintains the master list of jobs that the job scheduler is tracking. These jobs are compared by

 * reference, so none of the functions in this class should make a copy.

 * Also handles read/write of persisted jobs.

 *

 * Note on locking:

 *      All callers to this class must <strong>lock on the class object they are calling</strong>.
@@ -152,6 +146,10 @@ public class JobStore { 
        return false;

    }



    boolean containsJob(JobStatus jobStatus) {

        return mJobSet.contains(jobStatus);

    }



    public int size() {

        return mJobSet.size();

    }
@@ -180,6 +178,10 @@ public class JobStore { 
        maybeWriteStatusToDiskAsync();

    }



    /**

     * @param userHandle User for whom we are querying the list of jobs.

     * @return A list of all the jobs scheduled by the provided user. Never null.

     */

    public List<JobStatus> getJobsByUser(int userHandle) {

        List<JobStatus> matchingJobs = new ArrayList<JobStatus>();

        Iterator<JobStatus> it = mJobSet.iterator();
@@ -194,7 +196,7 @@ public class JobStore { 


    /**

     * @param uid Uid of the requesting app.

     * @return All JobStatus objects for a given uid from the master list.

     * @return All JobStatus objects for a given uid from the master list. Never null.

     */

    public List<JobStatus> getJobsByUid(int uid) {

        List<JobStatus> matchingJobs = new ArrayList<JobStatus>();

services/tests/servicestests/src/com/android/server/job/JobStoreTest.java

+3 −1
Original line number Diff line number Diff line
@@ -67,6 +67,7 @@ public class JobStoreTest extends AndroidTestCase { 
        assertEquals("Didn't get expected number of persisted tasks.", 1, jobStatusSet.size());

        final JobStatus loadedTaskStatus = jobStatusSet.iterator().next();

        assertTasksEqual(task, loadedTaskStatus.getJob());

        assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(ts));

        assertEquals("Different uids.", SOME_UID, loadedTaskStatus.getUid());

        compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",

                ts.getEarliestRunTime(), loadedTaskStatus.getEarliestRunTime());
@@ -103,7 +104,8 @@ public class JobStoreTest extends AndroidTestCase { 
        JobStatus loaded2 = it.next();

        assertTasksEqual(task1, loaded1.getJob());

        assertTasksEqual(task2, loaded2.getJob());



        assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(taskStatus1));

        assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(taskStatus2));

        // Check that the loaded task has the correct runtimes.

        compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",

                taskStatus1.getEarliestRunTime(), loaded1.getEarliestRunTime());