Bug 990834 (part 2) - Add support/tweak retry and backoff header support to hawk and tokenserverclient. r=rnewman

services/common/hawkclient.js

@@ -32,6 +32,7 @@ Cu.import("resource://gre/modules/FxAccountsCommon.js");
 Cu.import("resource://services-common/utils.js");
 Cu.import("resource://services-crypto/utils.js");
 Cu.import("resource://services-common/hawkrequest.js");
+Cu.import("resource://services-common/observers.js");
 Cu.import("resource://gre/modules/Promise.jsm");
 
 /*
@@ -75,6 +76,10 @@ this.HawkClient.prototype = {
     retryAfter = retryAfter ? parseInt(retryAfter) : retryAfter;
     if (retryAfter) {
       errorObj.retryAfter = retryAfter;
+      // and notify observers of the retry interval
+      if (this.observerPrefix) {
+        Observers.notify(this.observerPrefix + ":backoff:interval", retryAfter);
+      }
     }
     return errorObj;
   },
@@ -154,6 +159,11 @@ this.HawkClient.prototype = {
                 " - Status text: " + restResponse.statusText,
                 " - Response text: " + restResponse.body);
 
+      // All responses may have backoff headers, which are a server-side safety
+      // valve to allow slowing down clients without hurting performance.
+      self._maybeNotifyBackoff(restResponse, "x-weave-backoff");
+      self._maybeNotifyBackoff(restResponse, "x-backoff");
+
       if (error) {
         // When things really blow up, reconstruct an error object that follows
         // the general format of the server on error responses.
@@ -162,7 +172,7 @@ this.HawkClient.prototype = {
 
       self._updateClockOffset(restResponse.headers["date"]);
 
-      if (status === 401 && retryOK) {
+      if (status === 401 && retryOK && !("retry-after" in restResponse.headers)) {
         // Retry once if we were rejected due to a bad timestamp.
         // Clock offset is adjusted already in the top of this function.
         log.debug("Received 401 for " + path + ": retrying");
@@ -209,6 +219,35 @@ this.HawkClient.prototype = {
     return deferred.promise;
   },
 
+  /*
+   * The prefix used for all notifications sent by this module.  This
+   * allows the handler of notifications to be sure they are handling
+   * notifications for the service they expect.
+   *
+   * If not set, no notifications will be sent.
+   */
+  observerPrefix: null,
+
+  // Given an optional header value, notify that a backoff has been requested.
+  _maybeNotifyBackoff: function (response, headerName) {
+    if (!this.observerPrefix) {
+      return;
+    }
+    let headerVal = response.headers[headerName];
+    if (!headerVal) {
+      return;
+    }
+    let backoffInterval;
+    try {
+      backoffInterval = parseInt(headerVal, 10);
+    } catch (ex) {
+      this._log.error("hawkclient response had invalid backoff value in '" +
+                      headerName + "' header: " + headerVal);
+      return;
+    }
+    Observers.notify(this.observerPrefix + ":backoff:interval", backoffInterval);
+  },
+
   // override points for testing.
   newHAWKAuthenticatedRESTRequest: function(uri, credentials, extra) {
     return new HAWKAuthenticatedRESTRequest(uri, credentials, extra);

services/common/tokenserverclient.js

@@ -329,7 +329,8 @@ TokenServerClient.prototype = {
       return;
     }
 
-    // Any response status can have an X-Backoff header.
+    // Any response status can have X-Backoff or X-Weave-Backoff headers.
+    this._maybeNotifyBackoff(response, "x-weave-backoff");
     this._maybeNotifyBackoff(response, "x-backoff");
 
     // The service shouldn't have any 3xx, so we don't need to handle those.
@@ -413,8 +414,20 @@ TokenServerClient.prototype = {
     });
   },
 
+  /*
+   * The prefix used for all notifications sent by this module.  This
+   * allows the handler of notifications to be sure they are handling
+   * notifications for the service they expect.
+   *
+   * If not set, no notifications will be sent.
+   */
+  observerPrefix: null,
+
   // Given an optional header value, notify that a backoff has been requested.
   _maybeNotifyBackoff: function (response, headerName) {
+    if (!this.observerPrefix) {
+      return;
+    }
     let headerVal = response.headers[headerName];
     if (!headerVal) {
       return;
@@ -427,7 +440,7 @@ TokenServerClient.prototype = {
                       headerName + "' header: " + headerVal);
       return;
     }
-    Observers.notify("tokenserver:backoff:interval", backoffInterval);
+    Observers.notify(this.observerPrefix + ":backoff:interval", backoffInterval);
   },
 
   // override points for testing.

services/fxaccounts/FxAccountsClient.jsm

@@ -23,6 +23,7 @@ this.FxAccountsClient = function(host = HOST) {
   // The FxA auth server expects requests to certain endpoints to be authorized
   // using Hawk.
   this.hawk = new HawkClient(host);
+  this.hawk.observerPrefix = "FxA:hawk";
 
   // Manage server backoff state. C.f.
   // https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#backoff-protocol

services/sync/modules-testing/fxa_utils.js

@@ -53,6 +53,9 @@ this.initializeIdentityWithTokenServerResponse = function(response) {
   MockTSC.prototype.newRESTRequest = function(url) {
     return new MockRESTRequest(url);
   }
+  // Arrange for the same observerPrefix as browserid_identity uses.
+  MockTSC.prototype.observerPrefix = "weave:service";
+
   // tie it all together.
   Weave.Status.__authManager = Weave.Service.identity = new BrowserIDManager();
   Weave.Service._clusterManager = Weave.Service.identity.createClusterManager(Weave.Service);

services/sync/modules/browserid_identity.js

@@ -80,6 +80,7 @@ this.BrowserIDManager = function BrowserIDManager() {
   // the test suite.
   this._fxaService = fxAccounts;
   this._tokenServerClient = new TokenServerClient();
+  this._tokenServerClient.observerPrefix = "weave:service";
   // will be a promise that resolves when we are ready to authenticate
   this.whenReadyToAuthenticate = null;
   this._log = log;

services/sync/modules/policies.js

@@ -94,7 +94,7 @@ SyncScheduler.prototype = {
     Svc.Obs.add("weave:engine:sync:applied", this);
     Svc.Obs.add("weave:service:setup-complete", this);
     Svc.Obs.add("weave:service:start-over", this);
-    Svc.Obs.add("tokenserver:backoff:interval", this);
+    Svc.Obs.add("FxA:hawk:backoff:interval", this);
 
     if (Status.checkSetup() == STATUS_OK) {
       Svc.Idle.addIdleObserver(this, Svc.Prefs.get("scheduler.idleTime"));
@@ -184,7 +184,7 @@ SyncScheduler.prototype = {
         this.nextSync = 0;
         this.handleSyncError();
         break;
-      case "tokenserver:backoff:interval":
+      case "FxA:hawk:backoff:interval":
       case "weave:service:backoff:interval":
         let requested_interval = subject * 1000;
         this._log.debug("Got backoff notification: " + requested_interval + "ms");

services/sync/tests/unit/test_browserid_identity.js

@@ -408,6 +408,74 @@ add_task(function test_getTokenErrorWithRetry() {
   Assert.ok(Status.backoffInterval >= 200000);
 });
 
+add_task(function test_getKeysErrorWithBackoff() {
+  _("Auth server (via hawk) sends an observer notification on backoff headers.");
+
+  // Set Sync's backoffInterval to zero - after we simulated the backoff header
+  // it should reflect the value we sent.
+  Status.backoffInterval = 0;
+  _("Arrange for a 503 with a X-Backoff header.");
+
+  let config = makeIdentityConfig();
+  // We want no kA or kB so we attempt to fetch them.
+  delete config.fxaccount.user.kA;
+  delete config.fxaccount.user.kB;
+  config.fxaccount.user.keyFetchToken = "keyfetchtoken";
+  yield initializeIdentityWithHAWKResponseFactory(config, function(method, data, uri) {
+    Assert.equal(method, "get");
+    Assert.equal(uri, "http://mockedserver:9999/account/keys")
+    return {
+      status: 503,
+      headers: {"content-type": "application/json",
+                "x-backoff": "100"},
+      body: "{}",
+    }
+  });
+
+  let browseridManager = Service.identity;
+  yield Assert_rejects(browseridManager.whenReadyToAuthenticate.promise,
+                       "should reject due to 503");
+
+  // The observer should have fired - check it got the value in the response.
+  Assert.equal(Status.login, LOGIN_FAILED_NETWORK_ERROR, "login was rejected");
+  // Sync will have the value in ms with some slop - so check it is at least that.
+  Assert.ok(Status.backoffInterval >= 100000);
+});
+
+add_task(function test_getKeysErrorWithRetry() {
+  _("Auth server (via hawk) sends an observer notification on retry headers.");
+
+  // Set Sync's backoffInterval to zero - after we simulated the backoff header
+  // it should reflect the value we sent.
+  Status.backoffInterval = 0;
+  _("Arrange for a 503 with a Retry-After header.");
+
+  let config = makeIdentityConfig();
+  // We want no kA or kB so we attempt to fetch them.
+  delete config.fxaccount.user.kA;
+  delete config.fxaccount.user.kB;
+  config.fxaccount.user.keyFetchToken = "keyfetchtoken";
+  yield initializeIdentityWithHAWKResponseFactory(config, function(method, data, uri) {
+    Assert.equal(method, "get");
+    Assert.equal(uri, "http://mockedserver:9999/account/keys")
+    return {
+      status: 503,
+      headers: {"content-type": "application/json",
+                "retry-after": "100"},
+      body: "{}",
+    }
+  });
+
+  let browseridManager = Service.identity;
+  yield Assert_rejects(browseridManager.whenReadyToAuthenticate.promise,
+                       "should reject due to 503");
+
+  // The observer should have fired - check it got the value in the response.
+  Assert.equal(Status.login, LOGIN_FAILED_NETWORK_ERROR, "login was rejected");
+  // Sync will have the value in ms with some slop - so check it is at least that.
+  Assert.ok(Status.backoffInterval >= 100000);
+});
+
 add_task(function test_getHAWKErrors() {
   _("BrowserIDManager correctly handles various HAWK failures.");
 
@@ -523,6 +591,8 @@ function* initializeIdentityWithHAWKResponseFactory(config, cbGetResponse) {
   MockedHawkClient.prototype.newHAWKAuthenticatedRESTRequest = function(uri, credentials, extra) {
     return new MockRESTRequest(uri, credentials, extra);
   }
+  // Arrange for the same observerPrefix as FxAccountsClient uses
+  MockedHawkClient.prototype.observerPrefix = "FxA:hawk";
 
   // tie it all together - configureFxAccountIdentity isn't useful here :(
   let fxaClient = new MockFxAccountsClient();