OSDN Git Service

Downgrade expedited to normal on reschedule.
authorMatthew Williams <mjwilliams@google.com>
Thu, 9 Jan 2014 18:49:23 +0000 (10:49 -0800)
committerLiam Byrne <morepork@google.com>
Thu, 6 Feb 2014 17:22:08 +0000 (17:22 +0000)
bug: 12033540
Expedited was previously tracked by a redundant internal variable, ostensibly
as an optimisation. This variable could differ from the value in the bundle
depending on how the operation is initialised, which led to confusion. Now an
expedited sync will only be treated as such on its first execution.

Change-Id: I9979102317aecbe8bc53a36381d4b2782ac131be

Conflicts:
services/core/java/com/android/server/content/SyncOperation.java
services/core/java/com/android/server/content/SyncQueue.java

services/core/java/com/android/server/content/SyncManager.java
services/core/java/com/android/server/content/SyncOperation.java
services/core/java/com/android/server/content/SyncQueue.java
services/core/java/com/android/server/content/SyncStorageEngine.java
services/tests/servicestests/src/com/android/server/content/SyncOperationTest.java
services/tests/servicestests/src/com/android/server/content/SyncStorageEngineTest.java

index 0185a21..9e9cdf4 100644 (file)
@@ -2431,7 +2431,7 @@ public class SyncManager {
                             Log.v(TAG, "canceling and rescheduling sync since an initialization "
                                     + "takes higher priority, " + conflict);
                         }
-                    } else if (candidate.expedited && !conflict.mSyncOperation.expedited
+                    } else if (candidate.isExpedited() && !conflict.mSyncOperation.isExpedited()
                             && (candidateIsInitialization
                                 == conflict.mSyncOperation.isInitialization())) {
                         toReschedule = conflict;
index 036b21f..cbfa217 100644 (file)
@@ -65,17 +65,18 @@ public class SyncOperation implements Comparable {
     /** Why this sync was kicked off. {@link #REASON_NAMES} */
     public final int reason;
     /** Where this sync was initiated. */
-    public int syncSource;
+    public final int syncSource;
     public final boolean allowParallelSyncs;
-    public Bundle extras;
     public final String key;
-    public boolean expedited;
+    /** Internal boolean to avoid reading a bundle everytime we want to compare operations. */
+    private final boolean expedited;
+    public Bundle extras;
     /** Bare-bones version of this operation that is persisted across reboots. */
     public SyncStorageEngine.PendingOperation pendingOperation;
     /** Elapsed real time in millis at which to run this sync. */
     public long latestRunTime;
     /** Set by the SyncManager in order to delay retries. */
-    public Long backoff;
+    public long backoff;
     /** Specified by the adapter to delay subsequent sync operations. */
     public long delayUntil;
     /**
@@ -89,61 +90,58 @@ public class SyncOperation implements Comparable {
     public SyncOperation(Account account, int userId, int reason, int source, String provider,
             Bundle extras, long runTimeFromNow, long flexTime, long backoff,
             long delayUntil, boolean allowParallelSyncs) {
-        this.target = new SyncStorageEngine.EndPoint(account, provider, userId);
-        this.reason = reason;
-        this.allowParallelSyncs = allowParallelSyncs;
-        this.key = initialiseOperation(this.target, source, extras, runTimeFromNow, flexTime,
-                backoff, delayUntil);
+        this(new SyncStorageEngine.EndPoint(account, provider, userId),
+                reason, source, extras, runTimeFromNow, flexTime, backoff, delayUntil,
+                allowParallelSyncs);
     }
 
     public SyncOperation(ComponentName service, int userId, int reason, int source,
             Bundle extras, long runTimeFromNow, long flexTime, long backoff,
             long delayUntil) {
-        this.target = new SyncStorageEngine.EndPoint(service, userId);
-        // Default to true for sync service. The service itself decides how to handle this.
-        this.allowParallelSyncs = true;
-        this.reason = reason;
-        this.key =
-                initialiseOperation(this.target,
-                        source, extras, runTimeFromNow, flexTime, backoff, delayUntil);
-    }
-
-    /** Used to reschedule a sync at a new point in time. */
-    SyncOperation(SyncOperation other, long newRunTimeFromNow) {
-        this.target = other.target;
-        this.reason = other.reason;
-        this.expedited = other.expedited;
-        this.allowParallelSyncs = other.allowParallelSyncs;
-        // re-use old flex, but only 
-        long newFlexTime = Math.min(other.flexTime, newRunTimeFromNow);
-        this.key =
-                initialiseOperation(this.target,
-                    other.syncSource, other.extras,
-                    newRunTimeFromNow /* runTimeFromNow*/,
-                    newFlexTime /* flexTime */,
-                    other.backoff,
-                    0L /* delayUntil */);
+        this(new SyncStorageEngine.EndPoint(service, userId), reason, source, extras,
+                runTimeFromNow, flexTime, backoff, delayUntil, true /* allowParallelSyncs */);
     }
 
-    private String initialiseOperation(SyncStorageEngine.EndPoint info, int source, Bundle extras,
-            long runTimeFromNow, long flexTime, long backoff, long delayUntil) {
+    private SyncOperation(SyncStorageEngine.EndPoint info, int reason, int source, Bundle extras,
+            long runTimeFromNow, long flexTime, long backoff, long delayUntil,
+            boolean allowParallelSyncs) {
+        this.target = info;
+        this.reason = reason;
         this.syncSource = source;
         this.extras = new Bundle(extras);
         cleanBundle(this.extras);
         this.delayUntil = delayUntil;
         this.backoff = backoff;
+        this.allowParallelSyncs = allowParallelSyncs;
         final long now = SystemClock.elapsedRealtime();
-        if (runTimeFromNow < 0 || isExpedited()) {
+        // Set expedited based on runTimeFromNow. The SyncManager specifies whether the op is
+        // expedited (Not done solely based on bundle).
+        if (runTimeFromNow < 0) {
             this.expedited = true;
+            // Sanity check: Will always be true.
+            if (!this.extras.getBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, false)) {
+                this.extras.putBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, true);
+            }
             this.latestRunTime = now;
             this.flexTime = 0;
         } else {
             this.expedited = false;
+            this.extras.remove(ContentResolver.SYNC_EXTRAS_EXPEDITED);
             this.latestRunTime = now + runTimeFromNow;
             this.flexTime = flexTime;
         }
         updateEffectiveRunTime();
-        return toKey(info, this.extras);
+        this.key = toKey(info, this.extras);
+    }
+
+    /** Used to reschedule a sync at a new point in time. */
+    public SyncOperation(SyncOperation other, long newRunTimeFromNow) {
+        this(other.target, other.reason, other.syncSource, new Bundle(other.extras),
+                newRunTimeFromNow,
+                0L /* In back-off so no flex */,
+                other.backoff,
+                other.delayUntil,
+                other.allowParallelSyncs);
     }
 
     public boolean matchesAuthority(SyncOperation other) {
@@ -261,7 +259,7 @@ public class SyncOperation implements Comparable {
     }
 
     public boolean isExpedited() {
-        return extras.getBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, false) || expedited;
+        return expedited;
     }
 
     public boolean ignoreBackoff() {
index 5d93882..587de1c 100644 (file)
@@ -76,12 +76,11 @@ public class SyncQueue {
                 operationToAdd = new SyncOperation(
                         info.account, info.userId, op.reason, op.syncSource, info.provider,
                         op.extras,
-                        0 /* delay */,
+                        op.expedited ? -1 : 0 /* delay */,
                         0 /* flex */,
-                        backoff != null ? backoff.first : 0,
+                        backoff != null ? backoff.first : 0L,
                         mSyncStorageEngine.getDelayUntilTime(info),
                         syncAdapterInfo.type.allowParallelSyncs());
-                operationToAdd.expedited = op.expedited;
                 operationToAdd.pendingOperation = op;
                 add(operationToAdd, op);
             } else if (info.target_service) {
@@ -96,11 +95,10 @@ public class SyncQueue {
                 operationToAdd = new SyncOperation(
                         info.service, info.userId, op.reason, op.syncSource,
                         op.extras,
-                        0 /* delay */,
+                        op.expedited ? -1 : 0 /* delay */,
                         0 /* flex */,
                         backoff != null ? backoff.first : 0,
                         mSyncStorageEngine.getDelayUntilTime(info));
-                operationToAdd.expedited = op.expedited;
                 operationToAdd.pendingOperation = op;
                 add(operationToAdd, op);
             }
@@ -129,7 +127,6 @@ public class SyncQueue {
         if (existingOperation != null) {
             boolean changed = false;
             if (operation.compareTo(existingOperation) <= 0 ) {
-                existingOperation.expedited = operation.expedited;
                 long newRunTime =
                         Math.min(existingOperation.latestRunTime, operation.latestRunTime);
                 // Take smaller runtime.
index 5df7fc6..3a2e4db 100644 (file)
@@ -1088,7 +1088,7 @@ public class SyncStorageEngine extends Handler {
             }
 
             pop = new PendingOperation(authority, op.reason, op.syncSource, op.extras,
-                    op.expedited);
+                    op.isExpedited());
             mPendingOperations.add(pop);
             appendPendingOperationLocked(pop);
 
index 37176d6..b0296a0 100644 (file)
@@ -18,12 +18,13 @@ package com.android.server.content;
 
 import android.accounts.Account;
 import android.content.ContentResolver;
+import android.content.Context;
 import android.os.Bundle;
+import android.os.SystemClock;
+import android.provider.Settings;
 import android.test.AndroidTestCase;
 import android.test.suitebuilder.annotation.SmallTest;
 
-import com.android.server.content.SyncOperation;
-
 /**
  * You can run those tests with:
  *
@@ -35,6 +36,21 @@ import com.android.server.content.SyncOperation;
 
 public class SyncOperationTest extends AndroidTestCase {
 
+    Account mDummy;
+    /** Indicate an unimportant long that we're not testing. */
+    long mUnimportantLong = 0L;
+    /** Empty bundle. */
+    Bundle mEmpty;
+    /** Silly authority. */
+    String mAuthority;
+
+    @Override
+    public void setUp() {
+        mDummy = new Account("account1", "type1");
+        mEmpty = new Bundle();
+        mAuthority = "authority1";
+    }
+
     @SmallTest
     public void testToKey() {
         Account account1 = new Account("account1", "type1");
@@ -111,35 +127,64 @@ public class SyncOperationTest extends AndroidTestCase {
 
     @SmallTest
     public void testCompareTo() {
-        Account dummy = new Account("account1", "type1");
-        Bundle b1 = new Bundle();
-        final long unimportant = 0L;
         long soon = 1000;
         long soonFlex = 50;
         long after = 1500;
         long afterFlex = 100;
-        SyncOperation op1 = new SyncOperation(dummy, 0, 0, SyncOperation.REASON_PERIODIC,
-                "authority1", b1, soon, soonFlex, unimportant, unimportant, true);
+        SyncOperation op1 = new SyncOperation(mDummy, 0, 0, SyncOperation.REASON_PERIODIC,
+                "authority1", mEmpty, soon, soonFlex, mUnimportantLong, mUnimportantLong, true);
 
         // Interval disjoint from and after op1.
-        SyncOperation op2 = new SyncOperation(dummy, 0, 0, SyncOperation.REASON_PERIODIC,
-                "authority1", b1, after, afterFlex, unimportant, unimportant, true);
+        SyncOperation op2 = new SyncOperation(mDummy, 0, 0, SyncOperation.REASON_PERIODIC,
+                "authority1", mEmpty, after, afterFlex, mUnimportantLong, mUnimportantLong, true);
 
         // Interval equivalent to op1, but expedited.
         Bundle b2 = new Bundle();
         b2.putBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, true);
-        SyncOperation op3 = new SyncOperation(dummy, 0, 0, 0,
-                "authority1", b2, soon, soonFlex, unimportant, unimportant, true);
+        SyncOperation op3 = new SyncOperation(mDummy, 0, 0, 0,
+                "authority1", b2, -1, soonFlex, mUnimportantLong, mUnimportantLong, true);
 
         // Interval overlaps but not equivalent to op1.
-        SyncOperation op4 = new SyncOperation(dummy, 0, 0, SyncOperation.REASON_PERIODIC,
-                "authority1", b1, soon + 100, soonFlex + 100, unimportant, unimportant, true);
+        SyncOperation op4 = new SyncOperation(mDummy, 0, 0, SyncOperation.REASON_PERIODIC,
+                "authority1", mEmpty, soon + 100, soonFlex + 100, mUnimportantLong, mUnimportantLong, true);
 
         assertTrue(op1.compareTo(op2) == -1);
         assertTrue("less than not transitive.", op2.compareTo(op1) == 1);
-        assertTrue(op1.compareTo(op3) == 1);
+        assertTrue("Expedited sync not smaller than non-expedited.", op1.compareTo(op3) == 1);
         assertTrue("greater than not transitive. ", op3.compareTo(op1) == -1);
-        assertTrue("overlapping intervals not the same.", op1.compareTo(op4) == 0);
-        assertTrue("equality not transitive.", op4.compareTo(op1) == 0);
+        assertTrue("overlapping intervals not correctly compared.", op1.compareTo(op4) == -1);
+        assertTrue("equality not transitive.", op4.compareTo(op1) == 1);
+    }
+
+    @SmallTest
+    public void testCopyConstructor() {
+        long fiveSecondsFromNow = 5 * 1000L;
+        long twoSecondsFlex = 2 * 1000L;
+        long eightSeconds = 8 * 1000L;
+        long fourSeconds = 4 * 1000L;
+
+        Bundle withExpedited = new Bundle();
+        withExpedited.putBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, true);
+        SyncOperation op = new SyncOperation(mDummy, 0, 0, SyncOperation.REASON_USER_START,
+                mAuthority, withExpedited, fiveSecondsFromNow, twoSecondsFlex,
+                eightSeconds /* backoff */, fourSeconds /* delayUntil */, true);
+        // Create another sync op to be rerun in 5 minutes.
+        long now = SystemClock.elapsedRealtime();
+        SyncOperation copy = new SyncOperation(op, fiveSecondsFromNow * 60);
+        // Copying an expedited sync to be re-run should not keep expedited property.
+        assertFalse("A rescheduled sync based off an expedited should not be expedited!",
+                copy.isExpedited());
+        assertFalse("A rescheduled sync based off an expedited should not have expedited=true in"
+                + "its bundle.",
+                copy.extras.getBoolean(ContentResolver.SYNC_EXTRAS_EXPEDITED, false));
+        assertTrue("Copied sync is not respecting new provided run-time.",
+                copy.latestRunTime == (now + fiveSecondsFromNow * 60));
+        assertTrue("A rescheduled sync should not have any flex.",
+                copy.flexTime == 0L);
+        assertTrue("A rescheduled op should honour the old op's backoff.",
+                copy.backoff == eightSeconds);
+        assertTrue("A rescheduled op should honour the old op's delayUntil param.",
+                copy.delayUntil == fourSeconds);
+
     }
 }
index 70fd810..ae1967e 100644 (file)
@@ -131,7 +131,7 @@ public class SyncStorageEngineTest extends AndroidTestCase {
         assertEquals(sop.target.userId, popRetrieved.target.userId);
         assertEquals(sop.reason, popRetrieved.reason);
         assertEquals(sop.syncSource, popRetrieved.syncSource);
-        assertEquals(sop.expedited, popRetrieved.expedited);
+        assertEquals(sop.isExpedited(), popRetrieved.expedited);
         assert(android.content.PeriodicSync.syncExtrasEquals(sop.extras, popRetrieved.extras));
     }
 
@@ -169,7 +169,7 @@ public class SyncStorageEngineTest extends AndroidTestCase {
         assertEquals(deleted.target.userId, popDeleted.target.userId);
         assertEquals(deleted.reason, popDeleted.reason);
         assertEquals(deleted.syncSource, popDeleted.syncSource);
-        assertEquals(deleted.expedited, popDeleted.expedited);
+        assertEquals(deleted.isExpedited(), popDeleted.expedited);
         assert(android.content.PeriodicSync.syncExtrasEquals(deleted.extras, popDeleted.extras));
         // Delete one to force write-all
         engine.deleteFromPending(popDeleted);
@@ -192,7 +192,7 @@ public class SyncStorageEngineTest extends AndroidTestCase {
         assertEquals(sop.target.userId, popRetrieved.target.userId);
         assertEquals(sop.reason, popRetrieved.reason);
         assertEquals(sop.syncSource, popRetrieved.syncSource);
-        assertEquals(sop.expedited, popRetrieved.expedited);
+        assertEquals(sop.isExpedited(), popRetrieved.expedited);
         assert(android.content.PeriodicSync.syncExtrasEquals(sop.extras, popRetrieved.extras));
 
         popRetrieved = pops.get(1);
@@ -202,7 +202,7 @@ public class SyncStorageEngineTest extends AndroidTestCase {
         assertEquals(sop1.target.userId, popRetrieved.target.userId);
         assertEquals(sop1.reason, popRetrieved.reason);
         assertEquals(sop1.syncSource, popRetrieved.syncSource);
-        assertEquals(sop1.expedited, popRetrieved.expedited);
+        assertEquals(sop1.isExpedited(), popRetrieved.expedited);
         assert(android.content.PeriodicSync.syncExtrasEquals(sop1.extras, popRetrieved.extras));
     }