Bug 1140812 - React to Backoff and Retry-After headers from Reading List storage servers. r=rnewman, a=readinglist
authorNick Alexander <nalexander@mozilla.com>
Fri, 27 Mar 2015 15:30:33 -0700
changeset 258213 dff4ad268667
parent 258212 ec6516ecdd71
child 258214 27f61020a9e4
push id4620
push userrnewman@mozilla.com
push date2015-04-02 16:21 +0000
treeherdermozilla-beta@27f61020a9e4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, readinglist
bugs1140812
milestone38.0
Bug 1140812 - React to Backoff and Retry-After headers from Reading List storage servers. r=rnewman, a=readinglist ======== https://github.com/mozilla-services/android-sync/commit/cd7edfa0b5d3ee8c4344af1183c6e31a584757ef Author: Nick Alexander <nalexander@mozilla.com> Bug 1140812 - Part 3: React to Backoff and Retry-After headers. ======== https://github.com/mozilla-services/android-sync/commit/8581f5a572d387f7540088210a5b745574dcf6af Author: Nick Alexander <nalexander@mozilla.com> Bug 1140812 - Part 2: Include request in HTTP response observation callbacks. This allows to only handle responses from certain hosts. ======== https://github.com/mozilla-services/android-sync/commit/05b50325dbd262bd69df27f1ed023ecca250cc8a Author: Nick Alexander <nalexander@mozilla.com> Date: Fri Mar 27 14:47:38 2015 -0700 Bug 1140812 - Part 1: Generalize from one to many HTTP response observers. CopyOnWriteArrayList is a reasonable choice here: we have few writes but many iterations. See http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/CopyOnWriteArrayList.html ======== https://github.com/mozilla-services/android-sync/commit/5950219343910cbf9921f0e232562ab2d12b2387 Author: Nick Alexander <nalexander@mozilla.com> Date: Fri Mar 27 16:04:07 2015 -0700 Bug 1140812 - Pre: Modernize backoffInSeconds. Sync uses X-Backoff; newer services, including Reading List, use Backoff.
mobile/android/base/android-services.mozbuild
mobile/android/base/reading/ReadingListBackoffObserver.java
mobile/android/base/reading/ReadingListSyncAdapter.java
mobile/android/base/sync/GlobalSession.java
mobile/android/base/sync/net/BaseResource.java
mobile/android/base/sync/net/HttpResponseObserver.java
mobile/android/base/sync/net/MozResponse.java
mobile/android/base/sync/net/SyncResponse.java
--- a/mobile/android/base/android-services.mozbuild
+++ b/mobile/android/base/android-services.mozbuild
@@ -1162,16 +1162,17 @@ sync_java_files = [
     'tokenserver/TokenServerException.java',
     'tokenserver/TokenServerToken.java',
 ]
 reading_list_service_java_files = [
     'reading/ClientMetadata.java',
     'reading/ClientReadingListRecord.java',
     'reading/FetchSpec.java',
     'reading/LocalReadingListStorage.java',
+    'reading/ReadingListBackoffObserver.java',
     'reading/ReadingListChangeAccumulator.java',
     'reading/ReadingListClient.java',
     'reading/ReadingListClientContentValuesFactory.java',
     'reading/ReadingListClientRecordFactory.java',
     'reading/ReadingListDeleteDelegate.java',
     'reading/ReadingListInvalidAuthenticationException.java',
     'reading/ReadingListRecord.java',
     'reading/ReadingListRecordDelegate.java',
new file mode 100644
--- /dev/null
+++ b/mobile/android/base/reading/ReadingListBackoffObserver.java
@@ -0,0 +1,54 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.gecko.reading;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.mozilla.gecko.sync.Utils;
+import org.mozilla.gecko.sync.net.HttpResponseObserver;
+import org.mozilla.gecko.sync.net.MozResponse;
+
+import ch.boye.httpclientandroidlib.HttpResponse;
+import ch.boye.httpclientandroidlib.client.methods.HttpUriRequest;
+
+public class ReadingListBackoffObserver implements HttpResponseObserver {
+  protected final String host;
+  protected final AtomicLong largestBackoffObservedInSeconds = new AtomicLong(-1);
+
+  public ReadingListBackoffObserver(String host) {
+    this.host = host;
+    Utils.throwIfNull(host);
+  }
+
+  @Override
+  public void observeHttpResponse(HttpUriRequest request, HttpResponse response) {
+    // Ignore non-Reading List storage requests.
+    if (!host.equals(request.getURI().getHost())) {
+      return;
+    }
+
+    final MozResponse res = new MozResponse(response);
+    long backoffInSeconds = -1;
+    try {
+      backoffInSeconds = Math.max(res.backoffInSeconds(), res.retryAfterInSeconds());
+    } catch (NumberFormatException e) {
+      // Ignore.
+    }
+
+    if (backoffInSeconds <= 0) {
+      return;
+    }
+
+    while (true) {
+      long existingBackoff = largestBackoffObservedInSeconds.get();
+      if (existingBackoff >= backoffInSeconds) {
+        return;
+      }
+      if (largestBackoffObservedInSeconds.compareAndSet(existingBackoff, backoffInSeconds)) {
+        return;
+      }
+    }
+  }
+}
--- a/mobile/android/base/reading/ReadingListSyncAdapter.java
+++ b/mobile/android/base/reading/ReadingListSyncAdapter.java
@@ -16,17 +16,20 @@ import org.mozilla.gecko.background.Read
 import org.mozilla.gecko.background.common.PrefsBranch;
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.background.fxa.FxAccountUtils;
 import org.mozilla.gecko.db.BrowserContract;
 import org.mozilla.gecko.db.BrowserContract.ReadingListItems;
 import org.mozilla.gecko.fxa.FxAccountConstants;
 import org.mozilla.gecko.fxa.authenticator.AndroidFxAccount;
 import org.mozilla.gecko.fxa.sync.FxAccountSyncDelegate;
+import org.mozilla.gecko.sync.BackoffHandler;
+import org.mozilla.gecko.sync.PrefsBackoffHandler;
 import org.mozilla.gecko.sync.net.AuthHeaderProvider;
+import org.mozilla.gecko.sync.net.BaseResource;
 import org.mozilla.gecko.sync.net.BearerAuthHeaderProvider;
 
 import android.accounts.Account;
 import android.accounts.AccountManager;
 import android.content.AbstractThreadedSyncAdapter;
 import android.content.ContentProviderClient;
 import android.content.ContentResolver;
 import android.content.Context;
@@ -107,35 +110,24 @@ public class ReadingListSyncAdapter exte
     public void onComplete() {
       Logger.info(LOG_TAG, "Reading list synchronization complete.");
       cpc.release();
       syncDelegate.handleSuccess();
     }
   }
 
   private void syncWithAuthorization(final Context context,
-                                     final String endpointString,
+                                     final URI endpoint,
                                      final SyncResult syncResult,
                                      final FxAccountSyncDelegate syncDelegate,
                                      final String authToken,
                                      final SharedPreferences sharedPrefs,
                                      final Bundle extras) {
     final AuthHeaderProvider auth = new BearerAuthHeaderProvider(authToken);
 
-    final URI endpoint;
-    Logger.info(LOG_TAG, "Syncing reading list against " + endpointString);
-    try {
-      endpoint = new URI(endpointString);
-    } catch (URISyntaxException e) {
-      // Should never happen.
-      Logger.error(LOG_TAG, "Unexpected malformed URI for reading list service: " + endpointString);
-      syncDelegate.handleError(e);
-      return;
-    }
-
     final PrefsBranch branch = new PrefsBranch(sharedPrefs, "readinglist.");
     final ReadingListClient remote = new ReadingListClient(endpoint, auth);
     final ContentProviderClient cpc = getContentProviderClient(context); // Released by the inner SyncAdapterSynchronizerDelegate.
 
     final LocalReadingListStorage local = new LocalReadingListStorage(cpc);
     String localName = branch.getString(PREF_LOCAL_NAME, null);
     if (localName == null) {
       localName = FxAccountUtils.defaultClientName(context);
@@ -191,34 +183,65 @@ public class ReadingListSyncAdapter exte
     // Allow testing against stage.
     final String endpointString;
     if (usingStageAuthServer) {
       endpointString = ReadingListConstants.DEFAULT_DEV_ENDPOINT;
     } else {
       endpointString = ReadingListConstants.DEFAULT_PROD_ENDPOINT;
     }
 
+    Logger.info(LOG_TAG, "Syncing reading list against " + endpointString);
+    final URI endpointURI;
+    try {
+      endpointURI = new URI(endpointString);
+    } catch (URISyntaxException e) {
+      // Should never happen.
+      Logger.error(LOG_TAG, "Unexpected malformed URI for reading list service: " + endpointString);
+      return;
+    }
+
     final CountDownLatch latch = new CountDownLatch(1);
     final FxAccountSyncDelegate syncDelegate = new FxAccountSyncDelegate(latch, syncResult);
 
     final AccountManager accountManager = AccountManager.get(context);
     // If we have an auth failure that requires user intervention, FxA will show system
     // notifications prompting the user to re-connect as it advances the internal account state.
     // true causes the auth token fetch to return null on failure immediately, rather than doing
     // Mysterious Internal Work to try to get the token.
     final boolean notifyAuthFailure = true;
     try {
+      final SharedPreferences sharedPrefs = fxAccount.getReadingListPrefs();
+      final BackoffHandler storageBackoffHandler = new PrefsBackoffHandler(sharedPrefs, "storage");
+
+      // TODO: allow overriding based on flags.
+      final long delayMilliseconds = storageBackoffHandler.delayMilliseconds();
+      if (delayMilliseconds > 0) {
+        Logger.warn(LOG_TAG, "Not syncing: storage requested additional backoff: " + delayMilliseconds + " milliseconds.");
+        return;
+      }
+
       final String authToken = accountManager.blockingGetAuthToken(account, ReadingListConstants.AUTH_TOKEN_TYPE, notifyAuthFailure);
       if (authToken == null) {
         throw new RuntimeException("Couldn't get oauth token!  Aborting sync.");
       }
-      final SharedPreferences sharedPrefs = fxAccount.getReadingListPrefs();
-      syncWithAuthorization(context, endpointString, syncResult, syncDelegate, authToken, sharedPrefs, extras);
 
-      latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS);
+      final ReadingListBackoffObserver observer = new ReadingListBackoffObserver(endpointURI.getHost());
+      BaseResource.addHttpResponseObserver(observer);
+      try {
+        syncWithAuthorization(context, endpointURI, syncResult, syncDelegate, authToken, sharedPrefs, extras);
+        latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS);
+      } finally {
+        long backoffInSeconds = observer.largestBackoffObservedInSeconds.get();
+        BaseResource.removeHttpResponseObserver(observer);
+        if (backoffInSeconds > 0) {
+          Logger.warn(LOG_TAG, "Observed " + backoffInSeconds + " second backoff request.");
+          storageBackoffHandler.extendEarliestNextRequest(System.currentTimeMillis() + 1000 * backoffInSeconds);
+        }
+      }
+
       Logger.info(LOG_TAG, "Reading list sync done.");
     } catch (Exception e) {
       // We can get lots of exceptions here; handle them uniformly.
       Logger.error(LOG_TAG, "Got error syncing.", e);
       syncDelegate.handleError(e);
     }
 
     /*
@@ -228,17 +251,16 @@ public class ReadingListSyncAdapter exte
      */
 
     /*
      * TODO:
      * * Auth.
      * * Server URI lookup.
      * * Syncing.
      * * Error handling.
-     * * Backoff and retry-after.
      * * Sync scheduling.
      * * Forcing syncs/interactive use.
      */
   }
 
   private ContentProviderClient getContentProviderClient(Context context) {
     final ContentResolver contentResolver = context.getContentResolver();
     final ContentProviderClient client = contentResolver.acquireContentProviderClient(ReadingListItems.CONTENT_URI);
--- a/mobile/android/base/sync/GlobalSession.java
+++ b/mobile/android/base/sync/GlobalSession.java
@@ -54,16 +54,17 @@ import org.mozilla.gecko.sync.stage.Glob
 import org.mozilla.gecko.sync.stage.GlobalSyncStage.Stage;
 import org.mozilla.gecko.sync.stage.NoSuchStageException;
 import org.mozilla.gecko.sync.stage.PasswordsServerSyncStage;
 import org.mozilla.gecko.sync.stage.SyncClientsEngineStage;
 import org.mozilla.gecko.sync.stage.UploadMetaGlobalStage;
 
 import android.content.Context;
 import ch.boye.httpclientandroidlib.HttpResponse;
+import ch.boye.httpclientandroidlib.client.methods.HttpUriRequest;
 
 public class GlobalSession implements HttpResponseObserver {
   private static final String LOG_TAG = "GlobalSession";
 
   public static final long STORAGE_VERSION = 5;
 
   public SyncConfiguration config = null;
 
@@ -1119,34 +1120,42 @@ public class GlobalSession implements Ht
    */
   protected final AtomicLong largestBackoffObserved = new AtomicLong(-1);
 
   /**
    * Reset any observed backoff and start observing HTTP responses for backoff
    * requests.
    */
   protected void installAsHttpResponseObserver() {
-    Logger.debug(LOG_TAG, "Installing " + this + " as BaseResource HttpResponseObserver.");
-    BaseResource.setHttpResponseObserver(this);
+    Logger.debug(LOG_TAG, "Adding " + this + " as a BaseResource HttpResponseObserver.");
+    BaseResource.addHttpResponseObserver(this);
     largestBackoffObserved.set(-1);
   }
 
   /**
    * Stop observing HttpResponses for backoff requests.
    */
   protected void uninstallAsHttpResponseObserver() {
-    Logger.debug(LOG_TAG, "Uninstalling " + this + " as BaseResource HttpResponseObserver.");
-    BaseResource.setHttpResponseObserver(null);
+    Logger.debug(LOG_TAG, "Removing " + this + " as a BaseResource HttpResponseObserver.");
+    BaseResource.removeHttpResponseObserver(this);
   }
 
   /**
    * Observe all HTTP response for backoff requests on all status codes, not just errors.
    */
   @Override
-  public void observeHttpResponse(HttpResponse response) {
+  public void observeHttpResponse(HttpUriRequest request, HttpResponse response) {
+    // Ignore non-Sync storage requests.
+    final URI clusterURL = config.getClusterURL();
+    if (clusterURL != null && !clusterURL.getHost().equals(request.getURI().getHost())) {
+      // It's possible to see requests without a clusterURL (in particular,
+      // during testing); allow some extra backoffs in this case.
+      return;
+    }
+
     long responseBackoff = (new SyncResponse(response)).totalBackoffInMilliseconds(); // TODO: don't allocate object?
     if (responseBackoff <= 0) {
       return;
     }
 
     Logger.debug(LOG_TAG, "Observed " + responseBackoff + " millisecond backoff request.");
     while (true) {
       long existingBackoff = largestBackoffObserved.get();
--- a/mobile/android/base/sync/net/BaseResource.java
+++ b/mobile/android/base/sync/net/BaseResource.java
@@ -9,16 +9,17 @@ import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.lang.ref.WeakReference;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.security.GeneralSecurityException;
 import java.security.KeyManagementException;
 import java.security.NoSuchAlgorithmException;
 import java.security.SecureRandom;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import javax.net.ssl.SSLContext;
 
 import org.json.simple.JSONArray;
 import org.json.simple.JSONObject;
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
 
@@ -53,16 +54,17 @@ import ch.boye.httpclientandroidlib.prot
 import ch.boye.httpclientandroidlib.util.EntityUtils;
 
 /**
  * Provide simple HTTP access to a Sync server or similar.
  * Implements Basic Auth by asking its delegate for credentials.
  * Communicates with a ResourceDelegate to asynchronously return responses and errors.
  * Exposes simple get/post/put/delete methods.
  */
+@SuppressWarnings("deprecation")
 public class BaseResource implements Resource {
   private static final String ANDROID_LOOPBACK_IP = "10.0.2.2";
 
   private static final int MAX_TOTAL_CONNECTIONS     = 20;
   private static final int MAX_CONNECTIONS_PER_ROUTE = 10;
 
   private boolean retryOnFailedRequest = true;
 
@@ -72,17 +74,23 @@ public class BaseResource implements Res
 
   protected final URI uri;
   protected BasicHttpContext context;
   protected DefaultHttpClient client;
   public    ResourceDelegate delegate;
   protected HttpRequestBase request;
   public final String charset = "utf-8";
 
-  protected static WeakReference<HttpResponseObserver> httpResponseObserver = null;
+  /**
+   * We have very few writes (observers tend to be installed around sync
+   * sessions) and many iterations (every HTTP request iterates observers), so
+   * CopyOnWriteArrayList is a reasonable choice.
+   */
+  protected static final CopyOnWriteArrayList<WeakReference<HttpResponseObserver>>
+    httpResponseObservers = new CopyOnWriteArrayList<>();
 
   public BaseResource(String uri) throws URISyntaxException {
     this(uri, rewriteLocalhost);
   }
 
   public BaseResource(URI uri) {
     this(uri, rewriteLocalhost);
   }
@@ -104,28 +112,43 @@ public class BaseResource implements Res
         Logger.error(LOG_TAG, "Got error rewriting URI for Android emulator.", e);
         throw new IllegalArgumentException("Invalid URI", e);
       }
     } else {
       this.uri = uri;
     }
   }
 
-  public static synchronized HttpResponseObserver getHttpResponseObserver() {
-    if (httpResponseObserver == null) {
-      return null;
+  public static void addHttpResponseObserver(HttpResponseObserver newHttpResponseObserver) {
+    if (newHttpResponseObserver == null) {
+      return;
     }
-    return httpResponseObserver.get();
+    httpResponseObservers.add(new WeakReference<HttpResponseObserver>(newHttpResponseObserver));
   }
 
-  public static synchronized void setHttpResponseObserver(HttpResponseObserver newHttpResponseObserver) {
-    if (httpResponseObserver != null) {
-      httpResponseObserver.clear();
+  public static boolean isHttpResponseObserver(HttpResponseObserver httpResponseObserver) {
+    for (WeakReference<HttpResponseObserver> weakReference : httpResponseObservers) {
+      HttpResponseObserver innerHttpResponseObserver = weakReference.get();
+      if (innerHttpResponseObserver == httpResponseObserver) {
+        return true;
+      }
     }
-    httpResponseObserver = new WeakReference<HttpResponseObserver>(newHttpResponseObserver);
+    return false;
+  }
+
+  public static boolean removeHttpResponseObserver(HttpResponseObserver httpResponseObserver) {
+    for (WeakReference<HttpResponseObserver> weakReference : httpResponseObservers) {
+      HttpResponseObserver innerHttpResponseObserver = weakReference.get();
+      if (innerHttpResponseObserver == httpResponseObserver) {
+        // It's safe to mutate the observers while iterating.
+        httpResponseObservers.remove(weakReference);
+        return true;
+      }
+    }
+    return false;
   }
 
   @Override
   public URI getURI() {
     return this.uri;
   }
 
   @Override
@@ -269,19 +292,21 @@ public class BaseResource implements Res
         delegate.handleHttpIOException(ex);
       } else {
         retryRequest();
       }
       return;
     }
 
     // Don't retry if the observer or delegate throws!
-    HttpResponseObserver observer = getHttpResponseObserver();
-    if (observer != null) {
-      observer.observeHttpResponse(response);
+    for (WeakReference<HttpResponseObserver> weakReference : httpResponseObservers) {
+      HttpResponseObserver observer = weakReference.get();
+      if (observer != null) {
+        observer.observeHttpResponse(request, response);
+      }
     }
     delegate.handleHttpResponse(response);
   }
 
   private void retryRequest() {
     // Only retry once.
     retryOnFailedRequest = false;
     Logger.debug(LOG_TAG, "Retrying request...");
--- a/mobile/android/base/sync/net/HttpResponseObserver.java
+++ b/mobile/android/base/sync/net/HttpResponseObserver.java
@@ -1,17 +1,20 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.sync.net;
 
 import ch.boye.httpclientandroidlib.HttpResponse;
+import ch.boye.httpclientandroidlib.client.methods.HttpUriRequest;
 
 public interface HttpResponseObserver {
   /**
    * Observe an HTTP response.
+   * @param request
+   *          The <code>HttpUriRequest<code> that elicited the response.
    *
    * @param response
    *          The <code>HttpResponse</code> to observe.
    */
-  public void observeHttpResponse(HttpResponse response);
+  public void observeHttpResponse(HttpUriRequest request, HttpResponse response);
 }
--- a/mobile/android/base/sync/net/MozResponse.java
+++ b/mobile/android/base/sync/net/MozResponse.java
@@ -172,21 +172,21 @@ public class MozResponse {
       return (int)((then - now) / 1000);     // Convert milliseconds to seconds.
     } catch (DateParseException e) {
       Logger.warn(LOG_TAG, "Retry-After header neither integer nor date: " + retryAfter);
       return -1;
     }
   }
 
   /**
-   * @return A number of seconds, or -1 if the 'X-Backoff' header was not
+   * @return A number of seconds, or -1 if the 'Backoff' header was not
    *         present.
    */
   public int backoffInSeconds() throws NumberFormatException {
-    return this.getIntegerHeader("x-backoff");
+    return this.getIntegerHeader("backoff");
   }
 
   public void logResponseBody(final String logTag) {
     if (!Logger.LOG_PERSONAL_INFORMATION) {
       return;
     }
     try {
       Logger.pii(logTag, "Response body: " + body());
--- a/mobile/android/base/sync/net/SyncResponse.java
+++ b/mobile/android/base/sync/net/SyncResponse.java
@@ -17,16 +17,24 @@ public class SyncResponse extends MozRes
    * @return A number of seconds, or -1 if the 'X-Weave-Backoff' header was not
    *         present.
    */
   public int weaveBackoffInSeconds() throws NumberFormatException {
     return this.getIntegerHeader("x-weave-backoff");
   }
 
   /**
+   * @return A number of seconds, or -1 if the 'X-Backoff' header was not
+   *         present.
+   */
+  public int xBackoffInSeconds() throws NumberFormatException {
+    return this.getIntegerHeader("x-backoff");
+  }
+
+  /**
    * Extract a number of seconds, or -1 if none of the specified headers were present.
    *
    * @param includeRetryAfter
    *          if <code>true</code>, the Retry-After header is excluded. This is
    *          useful for processing non-error responses where a Retry-After
    *          header would be unexpected.
    * @return the maximum of the three possible backoff headers, in seconds.
    */
@@ -42,17 +50,17 @@ public class SyncResponse extends MozRes
     int weaveBackoffInSeconds = -1;
     try {
       weaveBackoffInSeconds = weaveBackoffInSeconds();
     } catch (NumberFormatException e) {
     }
 
     int backoffInSeconds = -1;
     try {
-      backoffInSeconds = backoffInSeconds();
+      backoffInSeconds = xBackoffInSeconds();
     } catch (NumberFormatException e) {
     }
 
     int totalBackoff = Math.max(retryAfterInSeconds, Math.max(backoffInSeconds, weaveBackoffInSeconds));
     if (totalBackoff < 0) {
       return -1;
     } else {
       return totalBackoff;