Bug 958447 - respect Retry-After header from token server. r=rnewman
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 12 Mar 2014 19:27:22 -0700
changeset 190532 dd003bff09ee862c872ae4d2a5ee7474ccff1dad
parent 190531 74bb8fa197a178c1a5ff6663624608f45d8166d9
child 190533 1a14c3cdfa7ec18f47a8d48b22dbff75853d96ba
push idunknown
push userunknown
push dateunknown
reviewersrnewman
bugs958447
milestone30.0a1
Bug 958447 - respect Retry-After header from token server. r=rnewman
services/common/tokenserverclient.js
services/sync/modules/policies.js
services/sync/tests/unit/test_browserid_identity.js
--- a/services/common/tokenserverclient.js
+++ b/services/common/tokenserverclient.js
@@ -12,16 +12,17 @@ this.EXPORTED_SYMBOLS = [
 ];
 
 const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://services-common/rest.js");
 Cu.import("resource://services-common/utils.js");
+Cu.import("resource://services-common/observers.js");
 
 const Prefs = new Preferences("services.common.tokenserverclient.");
 
 /**
  * Represents a TokenServerClient error that occurred on the client.
  *
  * This is the base type for all errors raised by client operations.
  *
@@ -323,16 +324,19 @@ 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.
+    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
         // the server is sending something not an array of objects, it has
         // failed to keep its contract with us and there is little we can do.
@@ -374,16 +378,20 @@ TokenServerClient.prototype = {
           error.cause = "conditions-required";
           error.urls = result.urls;
         }
       } else if (response.status == 404) {
         error.message = "Unknown service.";
         error.cause = "unknown-service";
       }
 
+      // A Retry-After header should theoretically only appear on a 503, but
+      // we'll look for it on any error response.
+      this._maybeNotifyBackoff(response, "retry-after");
+
       cb(error, null);
       return;
     }
 
     for (let k of ["id", "key", "api_endpoint", "uid", "duration"]) {
       if (!(k in result)) {
         let error = new TokenServerClientServerError("Expected key not " +
                                                      " present in result: " +
@@ -400,13 +408,30 @@ TokenServerClient.prototype = {
       id:       result.id,
       key:      result.key,
       endpoint: result.api_endpoint,
       uid:      result.uid,
       duration: result.duration,
     });
   },
 
+  // Given an optional header value, notify that a backoff has been requested.
+  _maybeNotifyBackoff: function (response, headerName) {
+    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);
+  },
+
   // override points for testing.
   newRESTRequest: function(url) {
     return new RESTRequest(url);
   }
 };
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -87,16 +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);
 
     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);
@@ -176,16 +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 "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
@@ -8,16 +8,17 @@ Cu.import("resource://services-sync/util
 Cu.import("resource://services-common/utils.js");
 Cu.import("resource://services-crypto/utils.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 Cu.import("resource://services-common/hawkclient.js");
 Cu.import("resource://gre/modules/FxAccounts.jsm");
 Cu.import("resource://gre/modules/FxAccountsClient.jsm");
 Cu.import("resource://gre/modules/FxAccountsCommon.js");
 Cu.import("resource://services-common/tokenserverclient.js");
+Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/status.js");
 Cu.import("resource://services-sync/constants.js");
 
 const SECOND_MS = 1000;
 const MINUTE_MS = SECOND_MS * 60;
 const HOUR_MS = MINUTE_MS * 60;
 
 let identityConfig = makeIdentityConfig();
@@ -358,16 +359,46 @@ add_task(function test_getTokenErrors() 
   yield initializeIdentityWithTokenServerFailure({
     status: 200,
     headers: [],
     body: "",
   });
   Assert.equal(Status.login, LOGIN_FAILED_NETWORK_ERROR, "login state is LOGIN_FAILED_NETWORK_ERROR");
 });
 
+add_task(function test_getTokenErrorWithRetry() {
+  _("tokenserver sends an observer notification on various 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 Retry-After header.");
+  yield initializeIdentityWithTokenServerFailure({
+    status: 503,
+    headers: {"content-type": "application/json",
+              "retry-after": "100"},
+    body: JSON.stringify({}),
+  });
+  // 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);
+
+  _("Arrange for a 200 with an X-Backoff header.");
+  Status.backoffInterval = 0;
+  yield initializeIdentityWithTokenServerFailure({
+    status: 503,
+    headers: {"content-type": "application/json",
+              "x-backoff": "200"},
+    body: JSON.stringify({}),
+  });
+  // The observer should have fired - check it got the value in the response.
+  Assert.ok(Status.backoffInterval >= 200000);
+});
+
 add_task(function test_getHAWKErrors() {
   _("BrowserIDManager correctly handles various HAWK failures.");
 
   _("Arrange for a 401 - Sync should reflect an auth error.");
   yield initializeIdentityWithHAWKFailure({
     status: 401,
     headers: {"content-type": "application/json"},
     body: JSON.stringify({}),