Bug 990834 (part 2) - Add support/tweak retry and backoff header support to hawk and tokenserverclient. r=rnewman, a=sylvestre
authorMark Hammond <mhammond@skippinet.com.au>
Mon, 14 Apr 2014 11:53:29 +1000
changeset 183733 329a2a180a8b7d627dd2d42cd45b500aa707b8e1
parent 183732 deb83f2f75faf0dc402e934cb5f52d3539e87e08
child 183734 b074e386a410372f41afd3f7bbe9ab134db5442b
push id3463
push usermhammond@skippinet.com.au
push dateMon, 14 Apr 2014 03:15:06 +0000
treeherdermozilla-beta@b074e386a410 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, sylvestre
bugs990834
milestone29.0
Bug 990834 (part 2) - Add support/tweak retry and backoff header support to hawk and tokenserverclient. r=rnewman, a=sylvestre Conflicts: services/common/hawk.js
services/common/hawk.js
services/common/tokenserverclient.js
services/fxaccounts/FxAccountsClient.jsm
services/sync/modules-testing/fxa_utils.js
services/sync/modules/browserid_identity.js
services/sync/modules/policies.js
services/sync/tests/unit/test_browserid_identity.js
--- a/services/common/hawk.js
+++ b/services/common/hawk.js
@@ -27,16 +27,17 @@
 this.EXPORTED_SYMBOLS = ["HawkClient"];
 
 const {interfaces: Ci, utils: Cu} = Components;
 
 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/rest.js");
+Cu.import("resource://services-common/observers.js");
 Cu.import("resource://gre/modules/Promise.jsm");
 
 /*
  * A general purpose client for making HAWK authenticated requests to a single
  * host.  Keeps track of the clock offset between the client and the host for
  * computation of the timestamp in the HAWK Authorization header.
  *
  * Clients should create one HawkClient object per each server they wish to
@@ -70,16 +71,20 @@ HawkClient.prototype = {
       message: restResponse.statusText,
       code: restResponse.status,
       errno: restResponse.status
     };
     let retryAfter = restResponse.headers && restResponse.headers["retry-after"];
     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;
   },
 
   /*
    *
    * Update clock offset by determining difference from date gives in the (RFC
    * 1123) Date header of a server response.  Because HAWK tolerates a window
@@ -149,25 +154,30 @@ HawkClient.prototype = {
     function onComplete(error) {
       let restResponse = this.response;
       let status = restResponse.status;
 
       log.debug("(Response) code: " + status +
                 " - 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.
         return deferred.reject(self._constructError(restResponse, error));
       }
 
       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");
         return deferred.resolve(
             self.request(path, method, credentials, payloadObj, false));
       }
 
       // If the server returned a json error message, use it in the rejection
@@ -204,14 +214,43 @@ HawkClient.prototype = {
       request[method](payloadObj, onComplete);
     } else {
       request[method](onComplete);
     }
 
     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);
   },
 
 }
--- a/services/common/tokenserverclient.js
+++ b/services/common/tokenserverclient.js
@@ -324,17 +324,18 @@ TokenServerClient.prototype = {
       this._log.warn("Invalid JSON returned by server: " + response.body);
       let error = new TokenServerClientServerError("Malformed JSON.",
                                                    "malformed-response");
       error.response = response;
       cb(error, null);
       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.
     if (response.status != 200) {
       // We /should/ have a Cornice error report in the JSON. We log that to
       // help with debugging.
       if ("errors" in result) {
         // This could throw, but this entire function is wrapped in a try. If
@@ -408,30 +409,42 @@ TokenServerClient.prototype = {
       id:       result.id,
       key:      result.key,
       endpoint: result.api_endpoint,
       uid:      result.uid,
       duration: result.duration,
     });
   },
 
+  /*
+   * 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("TokenServer response had invalid backoff value in '" +
                       headerName + "' header: " + headerVal);
       return;
     }
-    Observers.notify("tokenserver:backoff:interval", backoffInterval);
+    Observers.notify(this.observerPrefix + ":backoff:interval", backoffInterval);
   },
 
   // override points for testing.
   newRESTRequest: function(url) {
     return new RESTRequest(url);
   }
 };
--- a/services/fxaccounts/FxAccountsClient.jsm
+++ b/services/fxaccounts/FxAccountsClient.jsm
@@ -23,16 +23,17 @@ try {
 
 const HOST = _host;
 this.FxAccountsClient = function(host = HOST) {
   this.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
   this.backoffError = null;
 };
 
 this.FxAccountsClient.prototype = {
 
--- a/services/sync/modules-testing/fxa_utils.js
+++ b/services/sync/modules-testing/fxa_utils.js
@@ -48,16 +48,19 @@ this.initializeIdentityWithTokenServerRe
   }
   // The mocked TokenServer client which will get the response.
   function MockTSC() { }
   MockTSC.prototype = new TokenServerClient();
   MockTSC.prototype.constructor = MockTSC;
   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);
   let browseridManager = Weave.Service.identity;
   // a sanity check
   if (!(browseridManager instanceof BrowserIDManager)) {
     throw new Error("sync isn't configured for browserid_identity");
   }
--- a/services/sync/modules/browserid_identity.js
+++ b/services/sync/modules/browserid_identity.js
@@ -75,16 +75,17 @@ AuthenticationError.prototype = {
   }
 }
 
 this.BrowserIDManager = function BrowserIDManager() {
   // NOTE: _fxaService and _tokenServerClient are replaced with mocks by
   // 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;
 };
 
 this.BrowserIDManager.prototype = {
   __proto__: IdentityManager.prototype,
 
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -87,17 +87,17 @@ SyncScheduler.prototype = {
     Svc.Obs.add("weave:service:login:error", this);
     Svc.Obs.add("weave:service:logout:finish", this);
     Svc.Obs.add("weave:service:sync:error", this);
     Svc.Obs.add("weave:service:backoff:interval", this);
     Svc.Obs.add("weave:service:ready", this);
     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"));
     }
   },
 
   observe: function observe(subject, topic, data) {
     this._log.trace("Handling " + topic);
@@ -177,17 +177,17 @@ SyncScheduler.prototype = {
       case "weave:service:sync:error":
         // There may be multiple clients but if the sync fails, client mode
         // should still be updated so that the next sync has a correct interval.
         this.updateClientMode();
         this.adjustSyncInterval();
         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");
         // Leave up to 25% more time for the back off.
         let interval = requested_interval * (1 + Math.random() * 0.25);
         Status.backoffInterval = interval;
         Status.minimumNextSync = Date.now() + requested_interval;
         this._log.debug("Fuzzed minimum next sync: " + Status.minimumNextSync);
--- a/services/sync/tests/unit/test_browserid_identity.js
+++ b/services/sync/tests/unit/test_browserid_identity.js
@@ -403,16 +403,84 @@ add_task(function test_getTokenErrorWith
   yield browseridManager.initializeWithCurrentIdentity();
   yield Assert_rejects(browseridManager.whenReadyToAuthenticate.promise,
                        "should reject due to no token in response");
 
   // The observer should have fired - check it got the value in the response.
   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.");
 
   _("Arrange for a 401 - Sync should reflect an auth error.");
   let config = makeIdentityConfig();
   yield initializeIdentityWithHAWKResponseFactory(config, function(method, data, uri) {
     Assert.equal(method, "post");
     Assert.equal(uri, "http://mockedserver:9999/certificate/sign")
@@ -472,16 +540,18 @@ function* initializeIdentityWithHAWKResp
 
   // The hawk client.
   function MockedHawkClient() {}
   MockedHawkClient.prototype = new HawkClient("http://mockedserver:9999");
   MockedHawkClient.prototype.constructor = MockedHawkClient;
   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();
   fxaClient.hawk = new MockedHawkClient();
   let internal = {
     fxAccountsClient: fxaClient,
   }
   let fxa = new FxAccounts(internal);