Skip to content

Commit d41fe75

Browse files
alonalbertalonalbert
committed
Improved ignore-backoff handling
Allow a non-epidited ignore-backoff op to pass through an expidited backed off op. To do this, I first refactored the complicated if statement: if (best == null || ((bestSyncableIsUnknownAndNotARetry == syncableIsUnknownAndNotARetry) ? (best.expedited == op.expedited ? opRunTime < bestRunTime : op.expedited) : syncableIsUnknownAndNotARetry)) { best = op; bestSyncableIsUnknownAndNotARetry = syncableIsUnknownAndNotARetry; bestRunTime = opRunTime; } Into a more readable: boolean setBest = false; if (best == null) { setBest = true; } else if (bestSyncableIsUnknownAndNotARetry == syncableIsUnknownAndNotARetry) { if (best.expedited == op.expedited) { if (opRunTime < bestRunTime) { // if both have same level, earlier time wins setBest = true; } } else { if (op.expedited) { setBest = true; } } } else { if (syncableIsUnknownAndNotARetry) { setBest = true; } } if (setBest) { best = op; bestSyncableIsUnknownAndNotARetry = syncableIsUnknownAndNotARetry; bestRunTime = opRunTime; } The refactoring was all done automatically with IntelliJ to avoid human error in the conversion. After verifying this code still behaved as expected including the error condition in the bug, I added handling for the cases when a non-expidited op may override an expedited op if certain conditions occur, specificaly, if the expidited op is backed off and the non-expidited op is not. Finally, refactored to make it testable and added tests and logging. Bug: 3128963 Change-Id: I131cbcec6073ea5fe425f6b5aa88ca56c02b6598
1 parent 0c8ad64 commit d41fe75

File tree

2 files changed

+131
-27
lines changed

2 files changed

+131
-27
lines changed

‎core/java/android/content/SyncQueue.java‎

Lines changed: 91 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616

1717
package android.content;
1818

19-
import com.google.android.collect.Lists;
19+
import android.os.SystemClock;
2020
import com.google.android.collect.Maps;
2121

2222
import android.util.Pair;
@@ -130,33 +130,13 @@ public void remove(SyncOperation operation) {
130130
public Pair<SyncOperation, Long> nextOperation() {
131131
SyncOperation best = null;
132132
long bestRunTime = 0;
133-
boolean bestSyncableIsUnknownAndNotARetry = false;
133+
boolean bestIsInitial = false;
134134
for (SyncOperation op : mOperationsMap.values()) {
135-
long opRunTime = op.earliestRunTime;
136-
if (!op.extras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, false)) {
137-
Pair<Long, Long> backoff = mSyncStorageEngine.getBackoff(op.account, op.authority);
138-
long delayUntil = mSyncStorageEngine.getDelayUntilTime(op.account, op.authority);
139-
opRunTime = Math.max(
140-
Math.max(opRunTime, delayUntil),
141-
backoff != null ? backoff.first : 0);
142-
}
143-
// we know a sync is a retry if the intialization flag is set, since that will only
144-
// be set by the sync dispatching code, thus if it is set it must have already been
145-
// dispatched
146-
final boolean syncableIsUnknownAndNotARetry =
147-
!op.extras.getBoolean(ContentResolver.SYNC_EXTRAS_INITIALIZE, false)
148-
&& mSyncStorageEngine.getIsSyncable(op.account, op.authority) < 0;
149-
// if the unsyncable state differs, make the current the best if it is unsyncable
150-
// else, if the expedited state differs, make the current the best if it is expedited
151-
// else, make the current the best if it is earlier than the best
152-
if (best == null
153-
|| ((bestSyncableIsUnknownAndNotARetry == syncableIsUnknownAndNotARetry)
154-
? (best.expedited == op.expedited
155-
? opRunTime < bestRunTime
156-
: op.expedited)
157-
: syncableIsUnknownAndNotARetry)) {
135+
final long opRunTime = getOpTime(op);
136+
final boolean opIsInitial = getIsInitial(op);
137+
if (isOpBetter(best, bestRunTime, bestIsInitial, op, opRunTime, opIsInitial)) {
158138
best = op;
159-
bestSyncableIsUnknownAndNotARetry = syncableIsUnknownAndNotARetry;
139+
bestIsInitial = opIsInitial;
160140
bestRunTime = opRunTime;
161141
}
162142
}
@@ -166,6 +146,91 @@ public Pair<SyncOperation, Long> nextOperation() {
166146
return Pair.create(best, bestRunTime);
167147
}
168148

149+
// VisibleForTesting
150+
long getOpTime(SyncOperation op) {
151+
long opRunTime = op.earliestRunTime;
152+
if (!op.extras.getBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, false)) {
153+
Pair<Long, Long> backoff = mSyncStorageEngine.getBackoff(op.account, op.authority);
154+
long delayUntil = mSyncStorageEngine.getDelayUntilTime(op.account, op.authority);
155+
opRunTime = Math.max(
156+
Math.max(opRunTime, delayUntil),
157+
backoff != null ? backoff.first : 0);
158+
}
159+
return opRunTime;
160+
}
161+
162+
// VisibleForTesting
163+
boolean getIsInitial(SyncOperation op) {
164+
// Initial op is defined as an op with an unknown syncable that is not a retry.
165+
// We know a sync is a retry if the intialization flag is set, since that will only
166+
// be set by the sync dispatching code, thus if it is set it must have already been
167+
// dispatched
168+
return !op.extras.getBoolean(ContentResolver.SYNC_EXTRAS_INITIALIZE, false)
169+
&& mSyncStorageEngine.getIsSyncable(op.account, op.authority) < 0;
170+
}
171+
172+
// return true if op is a better candidate than best. Rules:
173+
// if the "Initial" state differs, make the current the best if it is "Initial".
174+
// else, if the expedited state differs, pick the expedited unless it is backed off and the
175+
// non-expedited isn't
176+
// VisibleForTesting
177+
boolean isOpBetter(
178+
SyncOperation best, long bestRunTime, boolean bestIsInitial,
179+
SyncOperation op, long opRunTime, boolean opIsInitial) {
180+
boolean setBest = false;
181+
if (Log.isLoggable(TAG, Log.VERBOSE)) {
182+
Log.v(TAG, "nextOperation: Processing op: " + op);
183+
}
184+
if (best == null) {
185+
if (Log.isLoggable(TAG, Log.VERBOSE)) {
186+
Log.v(TAG, " First op selected");
187+
}
188+
setBest = true;
189+
} else if (bestIsInitial == opIsInitial) {
190+
if (best.expedited == op.expedited) {
191+
if (opRunTime < bestRunTime) {
192+
// if both have same level, earlier time wins
193+
if (Log.isLoggable(TAG, Log.VERBOSE)) {
194+
Log.v(TAG, " Same expedite level - new op selected");
195+
}
196+
setBest = true;
197+
}
198+
} else {
199+
final long now = SystemClock.elapsedRealtime();
200+
if (op.expedited) {
201+
if (opRunTime <= now || bestRunTime > now) {
202+
// if op is expedited, it wins unless op can't run yet and best can
203+
if (Log.isLoggable(TAG, Log.VERBOSE)) {
204+
Log.v(TAG, " New op is expedited and can run - new op selected");
205+
}
206+
setBest = true;
207+
} else {
208+
if (Log.isLoggable(TAG, Log.VERBOSE)) {
209+
Log.v(TAG, " New op is expedited but can't run and best can");
210+
}
211+
}
212+
} else {
213+
if (bestRunTime > now && opRunTime <= now) {
214+
// if best is expedited but can't run yet and op can run, op wins
215+
if (Log.isLoggable(TAG, Log.VERBOSE)) {
216+
Log.v(TAG,
217+
" New op is not expedited but can run - new op selected");
218+
}
219+
setBest = true;
220+
}
221+
}
222+
}
223+
} else {
224+
if (opIsInitial) {
225+
if (Log.isLoggable(TAG, Log.VERBOSE)) {
226+
Log.v(TAG, " New op is init - new op selected");
227+
}
228+
setBest = true;
229+
}
230+
}
231+
return setBest;
232+
}
233+
169234
/**
170235
* Find and return the SyncOperation that should be run next and is ready to run.
171236
* @param now the current {@link android.os.SystemClock#elapsedRealtime()}, used to

‎core/tests/coretests/src/android/content/SyncQueueTest.java‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
import android.os.Bundle;
2525
import android.os.SystemClock;
2626

27+
import java.io.File;
28+
2729
public class SyncQueueTest extends AndroidTestCase {
2830
private static final Account ACCOUNT1 = new Account("test.account1", "test.type1");
2931
private static final Account ACCOUNT2 = new Account("test.account2", "test.type2");
@@ -138,6 +140,38 @@ public void testOrderWithBackoff() throws Exception {
138140
mSyncQueue.remove(op3);
139141
}
140142

143+
public void testExpeditedVsBackoff() throws Exception {
144+
145+
final SyncOperation op1 = new SyncOperation(
146+
ACCOUNT1, SyncStorageEngine.SOURCE_USER, AUTHORITY1, newTestBundle("1"), 0);
147+
final SyncOperation op2 = new SyncOperation(
148+
ACCOUNT1, SyncStorageEngine.SOURCE_USER, AUTHORITY1, newTestBundle("2"), 0);
149+
150+
// op1 is expedited but backed off
151+
mSettings.setBackoff(ACCOUNT1, AUTHORITY1, SystemClock.elapsedRealtime() + 1000000, 5);
152+
op1.expedited = true;
153+
154+
// op2 is not expidaited but ignores back off so it should run
155+
op2.extras.putBoolean(ContentResolver.SYNC_EXTRAS_IGNORE_BACKOFF, true);
156+
157+
// First test top level method
158+
mSyncQueue.remove(ACCOUNT1, AUTHORITY1);
159+
mSyncQueue.add(op1);
160+
mSyncQueue.add(op2);
161+
assertEquals(op2, mSyncQueue.nextReadyToRun(SystemClock.elapsedRealtime()).first);
162+
163+
// Since the queue is implemented as a hash, we cannot control the order the op's get
164+
// fed into the algorithm so test the inner method in both cases
165+
final long opTime1 = mSyncQueue.getOpTime(op1);
166+
final boolean isInitial1 = mSyncQueue.getIsInitial(op1);
167+
final long opTime2 = mSyncQueue.getOpTime(op2);
168+
final boolean opIsInitial2 = mSyncQueue.getIsInitial(op2);
169+
170+
assertTrue(mSyncQueue.isOpBetter(op1, opTime1, isInitial1, op2, opTime2, opIsInitial2));
171+
assertFalse(mSyncQueue.isOpBetter(op2, opTime2, opIsInitial2, op1, opTime1, isInitial1));
172+
}
173+
174+
141175
Bundle newTestBundle(String val) {
142176
Bundle bundle = new Bundle();
143177
bundle.putString("test", val);
@@ -148,7 +182,12 @@ static class TestContext extends ContextWrapper {
148182
ContentResolver mResolver;
149183

150184
public TestContext(ContentResolver resolver, Context realContext) {
151-
super(new RenamingDelegatingContext(new MockContext(), realContext, "test."));
185+
super(new RenamingDelegatingContext(new MockContext() {
186+
@Override
187+
public File getFilesDir() {
188+
return new File("/data");
189+
}
190+
}, realContext, "test."));
152191
mResolver = resolver;
153192
}
154193

0 commit comments

Comments
 (0)