Bug 984723 - Rework intervals and scheduling for Android Sync. r=nalexander, a=sylvestre
authorRichard Newman <rnewman@mozilla.com>
Thu, 20 Mar 2014 12:11:07 -0700
changeset 191355 c60a64a29bf485de0fd2030cd30eca4a7e9e8946
parent 191354 dd0849d5322844945ec36ac877eceb7081eb8656
child 191356 a51df1a52ed70164952630bd79321ec0a9f3a5b5
push id3503
push userraliiev@mozilla.com
push dateMon, 28 Apr 2014 18:51:11 +0000
treeherdermozilla-beta@c95ac01e332e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander, sylvestre
bugs984723
milestone30.0a2
Bug 984723 - Rework intervals and scheduling for Android Sync. r=nalexander, a=sylvestre
mobile/android/base/fxa/sync/FxAccountSchedulePolicy.java
mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
mobile/android/base/fxa/sync/SchedulePolicy.java
--- a/mobile/android/base/fxa/sync/FxAccountSchedulePolicy.java
+++ b/mobile/android/base/fxa/sync/FxAccountSchedulePolicy.java
@@ -15,51 +15,67 @@ import android.content.ContentResolver;
 import android.content.Context;
 import android.os.Bundle;
 
 public class FxAccountSchedulePolicy implements SchedulePolicy {
   private static final String LOG_TAG = "FxAccountSchedulePolicy";
 
   // Our poll intervals are used to trigger automatic background syncs
   // in the absence of user activity.
-
-  // If we're waiting for the user to click on a verification link, we
-  // sync very often in order to detect a change in state.
+  //
+  // We also receive sync requests as a result of network tickles, so
+  // these intervals are long, with the exception of the rapid polling
+  // while we wait for verification: if we're waiting for the user to
+  // click on a verification link, we sync very often in order to detect
+  // a change in state.
   //
   // In the case of unverified -> unverified (no transition), this should be
   // very close to a single HTTP request (with the SyncAdapter overhead, of
   // course, but that's not wildly different from alarm manager overhead).
   //
   // The /account/status endpoint is HAWK authed by sessionToken, so we still
   // have to do some crypto no matter what.
 
   // TODO: only do this for a while...
   public static final long POLL_INTERVAL_PENDING_VERIFICATION = 60;         // 1 minute.
 
   // If we're in some kind of error state, there's no point trying often.
   // This is not the same as a server-imposed backoff, which will be
   // reflected dynamically.
-  public static final long POLL_INTERVAL_ERROR_STATE = 24 * 60 * 60;        // 24 hours.
+  public static final long POLL_INTERVAL_ERROR_STATE_SEC = 24 * 60 * 60;    // 24 hours.
 
-  // If we're the only device, just sync a few times a day in case that
+  // If we're the only device, just sync once or twice a day in case that
   // changes.
-  public static final long POLL_INTERVAL_SINGLE_DEVICE_SEC = 8 * 60 * 60;   // 8 hours.
+  public static final long POLL_INTERVAL_SINGLE_DEVICE_SEC = 18 * 60 * 60;  // 18 hours.
 
   // And if we know there are other devices, let's sync often enough that
-  // we'll likely be caught up (even if not completely) by the time you
-  // next use this device.
-  public static final long POLL_INTERVAL_MULTI_DEVICE_SEC = 30 * 60;        // 30 minutes.
-
-  // Never sync more frequently than this, unless forced.
-  public static final long POLL_INTERVAL_MINIMUM_SEC = 45;                  // 45 seconds.
+  // we'll be more likely to be caught up (even if not completely) by the
+  // time you next use this device. This is also achieved via Android's
+  // network tickles.
+  public static final long POLL_INTERVAL_MULTI_DEVICE_SEC = 12 * 60 * 60;   // 12 hours.
 
   // This is used solely as an optimization for backoff handling, so it's not
   // persisted.
   private static volatile long POLL_INTERVAL_CURRENT_SEC = POLL_INTERVAL_SINGLE_DEVICE_SEC;
 
+  // Never sync more frequently than this, unless forced.
+  // This is to avoid overly-frequent syncs during active browsing.
+  public static final long RATE_LIMIT_FUNDAMENTAL_SEC = 90;                 // 90 seconds.
+
+  /**
+   * We are prompted to sync by several inputs:
+   * * Periodic syncs that we schedule at long intervals. See the POLL constants.
+   * * Network-tickle-based syncs that Android starts.
+   * * Upload-only syncs that are caused by local database writes.
+   *
+   * We rate-limit periodic and network-sourced events with this constant.
+   * We rate limit <b>both</b> with {@link FxAccountSchedulePolicy#RATE_LIMIT_FUNDAMENTAL_SEC}.
+   */
+  public static final long RATE_LIMIT_BACKGROUND_SEC = 60 * 60;             // 1 hour.
+
   private final AndroidFxAccount account;
   private final Context context;
 
   public FxAccountSchedulePolicy(Context context, AndroidFxAccount account) {
     this.account = account;
     this.context = context;
   }
 
@@ -96,40 +112,40 @@ public class FxAccountSchedulePolicy imp
     requestPeriodicSync(interval);
   }
 
   @Override
   public void onHandleFinal(Action needed) {
     switch (needed) {
     case NeedsPassword:
     case NeedsUpgrade:
-      requestPeriodicSync(POLL_INTERVAL_ERROR_STATE);
+      requestPeriodicSync(POLL_INTERVAL_ERROR_STATE_SEC);
       break;
     case NeedsVerification:
       requestPeriodicSync(POLL_INTERVAL_PENDING_VERIFICATION);
       break;
     case None:
       // No action needed: we'll set the periodic sync interval
       // when the sync finishes, via the SessionCallback.
       break;
     }
   }
 
   @Override
   public void onUpgradeRequired() {
     // TODO: this shouldn't occur in FxA, but when we upgrade we
     // need to reduce the interval again.
-    requestPeriodicSync(POLL_INTERVAL_ERROR_STATE);
+    requestPeriodicSync(POLL_INTERVAL_ERROR_STATE_SEC);
   }
 
   @Override
   public void onUnauthorized() {
     // TODO: this shouldn't occur in FxA, but when we fix our credentials
     // we need to reduce the interval again.
-    requestPeriodicSync(POLL_INTERVAL_ERROR_STATE);
+    requestPeriodicSync(POLL_INTERVAL_ERROR_STATE_SEC);
   }
 
   @Override
   public void configureBackoffMillisOnBackoff(BackoffHandler backoffHandler, long backoffMillis, boolean onlyExtend) {
     if (onlyExtend) {
       backoffHandler.extendEarliestNextRequest(delay(backoffMillis));
     } else {
       backoffHandler.setEarliestNextRequest(delay(backoffMillis));
@@ -142,13 +158,19 @@ public class FxAccountSchedulePolicy imp
     if (backoffMillis > (POLL_INTERVAL_CURRENT_SEC * 1000)) {
       // Slightly inflate the backoff duration to ensure that a fuzzed
       // periodic sync doesn't occur before our backoff has passed. Android
       // 19+ default to a 4% fuzz factor.
       requestPeriodicSync((long) Math.ceil((1.05 * backoffMillis) / 1000));
     }
   }
 
+  /**
+   * Accepts two {@link BackoffHandler} instances as input. These are used
+   * respectively to track fundamental rate limiting, and to separately
+   * rate-limit periodic and network-tickled syncs.
+   */
   @Override
-  public void configureBackoffMillisBeforeSyncing(BackoffHandler backoffHandler) {
-    backoffHandler.setEarliestNextRequest(delay(POLL_INTERVAL_MINIMUM_SEC * 1000));
+  public void configureBackoffMillisBeforeSyncing(BackoffHandler fundamentalRateHandler, BackoffHandler backgroundRateHandler) {
+    fundamentalRateHandler.setEarliestNextRequest(delay(RATE_LIMIT_FUNDAMENTAL_SEC * 1000));
+    backgroundRateHandler.setEarliestNextRequest(delay(RATE_LIMIT_BACKGROUND_SEC * 1000));
   }
 }
\ No newline at end of file
--- a/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
+++ b/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
@@ -60,18 +60,19 @@ import android.os.SystemClock;
 public class FxAccountSyncAdapter extends AbstractThreadedSyncAdapter {
   private static final String LOG_TAG = FxAccountSyncAdapter.class.getSimpleName();
 
   public static final int NOTIFICATION_ID = LOG_TAG.hashCode();
 
   // Tracks the last seen storage hostname for backoff purposes.
   private static final String PREF_BACKOFF_STORAGE_HOST = "backoffStorageHost";
 
-  // Used to do cheap in-memory rate limiting.
-  private static final int MINIMUM_SYNC_DELAY_MILLIS = 5000;
+  // Used to do cheap in-memory rate limiting. Don't sync again if we
+  // successfully synced within this duration.
+  private static final int MINIMUM_SYNC_DELAY_MILLIS = 15 * 1000;        // 15 seconds.
   private volatile long lastSyncRealtimeMillis = 0L;
 
   protected final ExecutorService executor;
   protected final FxAccountNotificationManager notificationManager;
 
   public FxAccountSyncAdapter(Context context, boolean autoInitialize) {
     super(context, autoInitialize);
     this.executor = Executors.newSingleThreadExecutor();
@@ -185,16 +186,25 @@ public class FxAccountSyncAdapter extend
         /*
         Logger.warn(LOG_TAG, "Postponing sync by " + millis + "ms.");
         syncResult.delayUntil = millis / 1000;
          */
       }
       setSyncResultSoftError();
       latch.countDown();
     }
+
+    /**
+     * Simply don't sync, without setting any error flags.
+     * This is the appropriate behavior when a routine backoff has not yet
+     * been met.
+     */
+    public void rejectSync() {
+      latch.countDown();
+    }
   }
 
   protected static class SessionCallback implements BaseGlobalSessionCallback {
     protected final SyncDelegate syncDelegate;
     protected final SchedulePolicy schedulePolicy;
     protected volatile BackoffHandler storageBackoffHandler;
 
     public SessionCallback(SyncDelegate syncDelegate, SchedulePolicy schedulePolicy) {
@@ -406,16 +416,17 @@ public class FxAccountSyncAdapter extend
    * This should be replaced with a full {@link FxAccountAuthenticator}-based
    * token implementation.
    */
   @Override
   public void onPerformSync(final Account account, final Bundle extras, final String authority, ContentProviderClient provider, final SyncResult syncResult) {
     Logger.setThreadLogTag(FxAccountConstants.GLOBAL_LOG_TAG);
     Logger.resetLogging();
 
+    // This applies even to forced syncs, but only on success.
     if (this.lastSyncRealtimeMillis > 0L &&
         (this.lastSyncRealtimeMillis + MINIMUM_SYNC_DELAY_MILLIS) > SystemClock.elapsedRealtime()) {
       Logger.info(LOG_TAG, "Not syncing FxAccount " + Utils.obfuscateEmail(account.name) +
                            ": minimum interval not met.");
       return;
     }
 
     Logger.info(LOG_TAG, "Syncing FxAccount" +
@@ -439,28 +450,44 @@ public class FxAccountSyncAdapter extend
       } catch (Exception e) {
         syncDelegate.handleError(e);
         return;
       }
 
       // This will be the same chunk of SharedPreferences that we pass through to GlobalSession/SyncConfiguration.
       final SharedPreferences sharedPrefs = fxAccount.getSyncPrefs();
 
-      // Check for a backoff right here.
-      final BackoffHandler schedulerBackoffHandler = new PrefsBackoffHandler(sharedPrefs, "scheduler");
-      if (!shouldPerformSync(schedulerBackoffHandler, "scheduler", extras)) {
-        Logger.info(LOG_TAG, "Not syncing (scheduler).");
-        syncDelegate.postponeSync(schedulerBackoffHandler.delayMilliseconds());
+      final BackoffHandler backgroundBackoffHandler = new PrefsBackoffHandler(sharedPrefs, "background");
+      final BackoffHandler rateLimitBackoffHandler = new PrefsBackoffHandler(sharedPrefs, "rate");
+
+      // If this sync was triggered by user action, this will be true.
+      final boolean isImmediate = (extras != null) &&
+                                  (extras.getBoolean(ContentResolver.SYNC_EXTRAS_UPLOAD, false) ||
+                                   extras.getBoolean(ContentResolver.SYNC_EXTRAS_FORCE, false));
+
+      // If it's not an immediate sync, it must be either periodic or tickled.
+      // Check our background rate limiter.
+      if (!isImmediate) {
+        if (!shouldPerformSync(backgroundBackoffHandler, "background", extras)) {
+          syncDelegate.rejectSync();
+          return;
+        }
+      }
+
+      // Regardless, let's make sure we're not syncing too often.
+      if (!shouldPerformSync(rateLimitBackoffHandler, "rate", extras)) {
+        syncDelegate.postponeSync(rateLimitBackoffHandler.delayMilliseconds());
         return;
       }
 
       final SchedulePolicy schedulePolicy = new FxAccountSchedulePolicy(context, fxAccount);
 
-      // Set a small scheduled 'backoff' to rate-limit the next sync.
-      schedulePolicy.configureBackoffMillisBeforeSyncing(schedulerBackoffHandler);
+      // Set a small scheduled 'backoff' to rate-limit the next sync,
+      // and extend the background delay even further into the future.
+      schedulePolicy.configureBackoffMillisBeforeSyncing(rateLimitBackoffHandler, backgroundBackoffHandler);
 
       final String audience = fxAccount.getAudience();
       final String authServerEndpoint = fxAccount.getAccountServerURI();
       final String tokenServerEndpoint = fxAccount.getTokenServerURI();
       final URI tokenServerEndpointURI = new URI(tokenServerEndpoint);
 
       // TODO: why doesn't the loginPolicy extract the audience from the account?
       final FxAccountClient client = new FxAccountClient20(authServerEndpoint, executor);
--- a/mobile/android/base/fxa/sync/SchedulePolicy.java
+++ b/mobile/android/base/fxa/sync/SchedulePolicy.java
@@ -21,19 +21,20 @@ public interface SchedulePolicy {
    * the slate prior to encountering a new backoff, and also functions as a rate
    * limiter.
    *
    * The {@link SchedulePolicy} acts as a controller for the {@link BackoffHandler}.
    * As a result of calling these two methods, the {@link BackoffHandler} will be
    * mutated, and additional side-effects (such as scheduling periodic syncs) can
    * occur.
    *
-   * @param backoffHandler the backoff handler to configure.
+   * @param rateHandler the backoff handler to configure for basic rate limiting.
+   * @param backgroundHandler the backoff handler to configure for background operations.
    */
-  public abstract void configureBackoffMillisBeforeSyncing(BackoffHandler backoffHandler);
+  public abstract void configureBackoffMillisBeforeSyncing(BackoffHandler rateHandler, BackoffHandler backgroundHandler);
 
   /**
    * We received an explicit backoff instruction, typically from a server.
    *
    * @param onlyExtend
    *          if <code>true</code>, the backoff handler will be asked to update
    *          its backoff only if the provided value is greater than the current
    *          backoff.