Bug 1018022 - Improve polling for FxA verification email. r=markh, a=sledru
authorDrew Willcoxon <adw@mozilla.com>
Thu, 22 Jan 2015 16:22:47 -0800
changeset 243092 9f89c4328e49
parent 243091 344958aebbe2
child 243093 aa4a0caf1dae
push id4402
push userryanvm@gmail.com
push date2015-01-29 16:16 +0000
treeherdermozilla-beta@e25b169e456b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, sledru
bugs1018022
milestone36.0
Bug 1018022 - Improve polling for FxA verification email. r=markh, a=sledru
services/fxaccounts/FxAccounts.jsm
services/fxaccounts/FxAccountsCommon.js
services/fxaccounts/tests/xpcshell/test_accounts.js
--- a/services/fxaccounts/FxAccounts.jsm
+++ b/services/fxaccounts/FxAccounts.jsm
@@ -292,21 +292,18 @@ this.FxAccounts = function (mockInternal
 }
 
 /**
  * The internal API's constructor.
  */
 function FxAccountsInternal() {
   this.version = DATA_FORMAT_VERSION;
 
-  // Make a local copy of these constants so we can mock it in testing
-  this.POLL_STEP = POLL_STEP;
+  // Make a local copy of this constant so we can mock it in testing
   this.POLL_SESSION = POLL_SESSION;
-  // We will create this.pollTimeRemaining below; it will initially be
-  // set to the value of POLL_SESSION.
 
   // We interact with the Firefox Accounts auth server in order to confirm that
   // a user's email has been verified and also to fetch the user's keys from
   // the server.  We manage these processes in possibly long-lived promises
   // that are internal to this object (never exposed to callers).  Because
   // Firefox Accounts allows for only one logged-in user, and because it's
   // conceivable that while we are waiting to verify one identity, a caller
   // could start verification on a second, different identity, we need to be
@@ -786,35 +783,33 @@ FxAccountsInternal.prototype = {
 
   // XXX - pollEmailStatus should maybe be on the AccountState object?
   pollEmailStatus: function pollEmailStatus(currentState, sessionToken, why) {
     log.debug("entering pollEmailStatus: " + why);
     if (why == "start") {
       // If we were already polling, stop and start again.  This could happen
       // if the user requested the verification email to be resent while we
       // were already polling for receipt of an earlier email.
-      this.pollTimeRemaining = this.POLL_SESSION;
+      this.pollStartDate = Date.now();
       if (!currentState.whenVerifiedDeferred) {
         currentState.whenVerifiedDeferred = Promise.defer();
         // This deferred might not end up with any handlers (eg, if sync
         // is yet to start up.)  This might cause "A promise chain failed to
         // handle a rejection" messages, so add an error handler directly
         // on the promise to log the error.
         currentState.whenVerifiedDeferred.promise.then(null, err => {
           log.info("the wait for user verification was stopped: " + err);
         });
       }
     }
 
     this.checkEmailStatus(sessionToken)
       .then((response) => {
         log.debug("checkEmailStatus -> " + JSON.stringify(response));
         if (response && response.verified) {
-          // Bug 947056 - Server should be able to tell FxAccounts.jsm to back
-          // off or stop polling altogether
           currentState.getUserAccountData()
             .then((data) => {
               data.verified = true;
               return currentState.setUserAccountData(data);
             })
             .then((data) => {
               // Now that the user is verified, we can proceed to fetch keys
               if (currentState.whenVerifiedDeferred) {
@@ -824,41 +819,49 @@ FxAccountsInternal.prototype = {
               // Tell FxAccountsManager to clear its cache
               this.notifyObservers(ON_FXA_UPDATE_NOTIFICATION, ONVERIFIED_NOTIFICATION);
             });
         } else {
           // Poll email status again after a short delay.
           this.pollEmailStatusAgain(currentState, sessionToken);
         }
       }, error => {
+        let timeoutMs = undefined;
+        if (error && error.retryAfter) {
+          // If the server told us to back off, back off the requested amount.
+          timeoutMs = (error.retryAfter + 3) * 1000;
+        }
         // The server will return 401 if a request parameter is erroneous or
         // if the session token expired. Let's continue polling otherwise.
         if (!error || !error.code || error.code != 401) {
-          this.pollEmailStatusAgain(currentState, sessionToken);
+          this.pollEmailStatusAgain(currentState, sessionToken, timeoutMs);
         }
       });
   },
 
-  // Poll email status after a short timeout.
-  pollEmailStatusAgain: function (currentState, sessionToken) {
-    log.debug("polling with step = " + this.POLL_STEP);
-    this.pollTimeRemaining -= this.POLL_STEP;
-    log.debug("time remaining: " + this.pollTimeRemaining);
-    if (this.pollTimeRemaining > 0) {
-      this.currentTimer = setTimeout(() => {
-        this.pollEmailStatus(currentState, sessionToken, "timer");
-      }, this.POLL_STEP);
-      log.debug("started timer " + this.currentTimer);
-    } else {
+  // Poll email status using truncated exponential back-off.
+  pollEmailStatusAgain: function (currentState, sessionToken, timeoutMs) {
+    let ageMs = Date.now() - this.pollStartDate;
+    if (ageMs >= this.POLL_SESSION) {
       if (currentState.whenVerifiedDeferred) {
         let error = new Error("User email verification timed out.")
         currentState.whenVerifiedDeferred.reject(error);
         delete currentState.whenVerifiedDeferred;
       }
+      log.debug("polling session exceeded, giving up");
+      return;
     }
+    if (timeoutMs === undefined) {
+      let currentMinute = Math.ceil(ageMs / 60000);
+      timeoutMs = 1000 * (currentMinute <= 2 ? 5 : 15);
+    }
+    log.debug("polling with timeout = " + timeoutMs);
+    this.currentTimer = setTimeout(() => {
+      this.pollEmailStatus(currentState, sessionToken, "timer");
+    }, timeoutMs);
   },
 
   // Return the URI of the remote UI flows.
   getAccountsSignUpURI: function() {
     let url = Services.urlFormatter.formatURLPref("identity.fxaccounts.remote.signup.uri");
     if (!/^https:/.test(url)) { // Comment to un-break emacs js-mode highlighting
       throw new Error("Firefox Accounts server must use HTTPS");
     }
--- a/services/fxaccounts/FxAccountsCommon.js
+++ b/services/fxaccounts/FxAccountsCommon.js
@@ -74,19 +74,19 @@ exports.DEFAULT_STORAGE_FILENAME = "sign
 exports.ASSERTION_LIFETIME = 1000 * 3600 * 24 * 365 * 25; // 25 years
 // This is a time period we want to guarantee that the assertion will be
 // valid after we generate it (e.g., the signed cert won't expire in this
 // period).
 exports.ASSERTION_USE_PERIOD = 1000 * 60 * 5; // 5 minutes
 exports.CERT_LIFETIME      = 1000 * 3600 * 6;  // 6 hours
 exports.KEY_LIFETIME       = 1000 * 3600 * 12; // 12 hours
 
-// Polling timings.
-exports.POLL_SESSION       = 1000 * 60 * 5;    // 5 minutes
-exports.POLL_STEP          = 1000 * 3;         // 3 seconds
+// After we start polling for account verification, we stop polling when this
+// many milliseconds have elapsed.
+exports.POLL_SESSION       = 1000 * 60 * 20;   // 20 minutes
 
 // Observer notifications.
 exports.ONLOGIN_NOTIFICATION = "fxaccounts:onlogin";
 exports.ONVERIFIED_NOTIFICATION = "fxaccounts:onverified";
 exports.ONLOGOUT_NOTIFICATION = "fxaccounts:onlogout";
 // Internal to services/fxaccounts only
 exports.ON_FXA_UPDATE_NOTIFICATION = "fxaccounts:update";
 
--- a/services/fxaccounts/tests/xpcshell/test_accounts.js
+++ b/services/fxaccounts/tests/xpcshell/test_accounts.js
@@ -297,17 +297,16 @@ add_test(function test_polling_timeout()
   let fxa = new MockFxAccounts();
   let test_user = getTestUser("carol");
 
   let removeObserver = makeObserver(ONVERIFIED_NOTIFICATION, function() {
     do_throw("We should not be getting a login event!");
   });
 
   fxa.internal.POLL_SESSION = 1;
-  fxa.internal.POLL_STEP = 2;
 
   let p = fxa.internal.whenVerified({});
 
   fxa.setSignedInUser(test_user).then(() => {
     p.then(
       (success) => {
         do_throw("this should not succeed");
       },