Bug 1148724 - reading list scheduler gets a back-off strategy on errors. r=adw
authorMark Hammond <mhammond@skippinet.com.au>
Sun, 29 Mar 2015 11:18:25 +1100
changeset 266702 b34ca540cf734796091643dad3c26978814bdef6
parent 266701 1e5dcedd40cce32ab759756f6707496de01b0173
child 266703 385840329d91d1b469111b1e0471d41e36e257be
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1148724
milestone39.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1148724 - reading list scheduler gets a back-off strategy on errors. r=adw
browser/components/readinglist/Scheduler.jsm
browser/components/readinglist/test/xpcshell/test_scheduler.js
--- a/browser/components/readinglist/Scheduler.jsm
+++ b/browser/components/readinglist/Scheduler.jsm
@@ -65,17 +65,17 @@ let intervals = {
     // All pref values are seconds, but we return ms.
     return prefs.get(prefName, def) * 1000;
   },
 
   // How long after startup do we do an initial sync?
   get initial() this._fixupIntervalPref("initial", 10), // 10 seconds.
   // Every interval after the first.
   get schedule() this._fixupIntervalPref("schedule", 2 * 60 * 60), // 2 hours
-  // After an error
+  // Initial retry after an error (exponentially backed-off to .schedule)
   get retry() this._fixupIntervalPref("retry", 2 * 60), // 2 mins
 };
 
 // This is the implementation, but it's not exposed directly.
 function InternalScheduler(readingList = null) {
   // oh, I don't know what logs yet - let's guess!
   let logs = [
     "browserwindow.syncui",
@@ -106,16 +106,19 @@ InternalScheduler.prototype = {
   _backoffUntil: 0,
   // Our current timer.
   _timer: null,
   // Our timer fires a promise - _timerRunning is true until it resolves or
   // rejects.
   _timerRunning: false,
   // Our sync engine - XXX - maybe just a callback?
   _engine: Sync,
+  // Our current "error backoff" timeout. zero if no error backoff is in
+  // progress and incremented after successive errors until a max is reached.
+  _currentErrorBackoff: 0,
 
   // Our state variable and constants.
   state: null,
   STATE_OK: "ok",
   STATE_ERROR_AUTHENTICATION: "authentication error",
   STATE_ERROR_OTHER: "other error",
 
   init() {
@@ -287,39 +290,44 @@ InternalScheduler.prototype = {
     this._engine.start().then(() => {
       this.log.info("Sync completed successfully");
       // Write a pref in the same format used to services/sync to indicate
       // the last success.
       prefs.set("lastSync", new Date().toString());
       this.state = this.STATE_OK;
       this._logManager.resetFileLog(this._logManager.REASON_SUCCESS);
       Services.obs.notifyObservers(null, "readinglist:sync:finish", null);
+      this._currentErrorBackoff = 0; // error retry interval is reset on success.
       return intervals.schedule;
     }).catch(err => {
       // This isn't ideal - we really should have _canSync() check this - but
       // that requires a refactor to turn _canSync() into a promise-based
       // function.
       if (err.message == fxAccountsCommon.ERROR_NO_ACCOUNT ||
           err.message == fxAccountsCommon.ERROR_UNVERIFIED_ACCOUNT) {
         // make everything look like success.
+        this._currentErrorBackoff = 0; // error retry interval is reset on success.
         this.log.info("Can't sync due to FxA account state " + err.message);
         this.state = this.STATE_OK;
         this._logManager.resetFileLog(this._logManager.REASON_SUCCESS);
         Services.obs.notifyObservers(null, "readinglist:sync:finish", null);
         // it's unfortunate that we are probably going to hit this every
         // 2 hours, but it should be invisible to the user.
         return intervals.schedule;
       }
       this.state = err.message == fxAccountsCommon.ERROR_AUTH_ERROR ?
                    this.STATE_ERROR_AUTHENTICATION : this.STATE_ERROR_OTHER;
       this.log.error("Sync failed, now in state '${state}': ${err}",
                      {state: this.state, err});
       this._logManager.resetFileLog(this._logManager.REASON_ERROR);
       Services.obs.notifyObservers(null, "readinglist:sync:error", null);
-      return intervals.retry;
+      // We back-off on error retries until it hits our normally scheduled interval.
+      this._currentErrorBackoff = this._currentErrorBackoff == 0 ? intervals.retry :
+                                  Math.min(intervals.schedule, this._currentErrorBackoff * 2);
+      return this._currentErrorBackoff;
     }).then(nextDelay => {
       this._timerRunning = false;
       // ensure a new timer is setup for the appropriate next time.
       this._maybeReschedule(nextDelay);
       this._setupTimer();
       this._onAutoReschedule(); // just for tests...
     }).catch(err => {
       // We should never get here, but better safe than sorry...
--- a/browser/components/readinglist/test/xpcshell/test_scheduler.js
+++ b/browser/components/readinglist/test/xpcshell/test_scheduler.js
@@ -169,11 +169,86 @@ add_task(function* testAuthError() {
 add_task(function* testBackoff() {
   let scheduler = createScheduler({expectedDelay: 1000});
   Services.obs.notifyObservers(null, "readinglist:backoff-requested", 1000);
   // XXX - this needs a little love as nothing checks createScheduler actually
   // made the checks we think it does.
   scheduler.finalize();
 });
 
+add_task(function testErrorBackoff() {
+  // This test can't sanely use the "test scheduler" above, so make one more
+  // suited.
+  let rlMock = new ReadingListMock();
+  let scheduler = createTestableScheduler(rlMock);
+  scheduler._setTimeout = function(delay) {
+    // create a timer that fires immediately
+    return setTimeout(() => scheduler._doSync(), 0);
+  }
+
+  // This does all the work...
+  function checkBackoffs(expectedSequences) {
+    let orig_maybeReschedule = scheduler._maybeReschedule;
+    return new Promise(resolve => {
+      let isSuccess = true; // ie, first run will put us in "fail" mode.
+      let expected;
+      function nextSequence() {
+        if (expectedSequences.length == 0) {
+          resolve();
+          return true; // we are done.
+        }
+        // setup the current set of expected results.
+        expected = expectedSequences.shift()
+        // and toggle the success status of the engine.
+        isSuccess = !isSuccess;
+        if (isSuccess) {
+          scheduler._engine.start = Promise.resolve;
+        } else {
+          scheduler._engine.start = () => {
+            return Promise.reject(new Error("oh no"))
+          }
+        }
+        return false; // not done.
+      };
+      // get the first sequence;
+      nextSequence();
+      // and setup the scheduler to check the sequences.
+      scheduler._maybeReschedule = function(nextDelay) {
+        let thisExpected = expected.shift();
+        equal(thisExpected * 1000, nextDelay);
+        if (expected.length == 0) {
+          if (nextSequence()) {
+            // we are done, so do nothing.
+            return;
+          }
+        }
+        // call the original impl to get the next schedule.
+        return orig_maybeReschedule.call(scheduler, nextDelay);
+      }
+    });
+  }
+
+  prefs.set("schedule", 100);
+  prefs.set("retry", 5);
+  // The sequences of timeouts we expect as the Sync error state changes.
+  let backoffsChecked = checkBackoffs([
+    // first sequence is in failure mode - expect the timeout to double until 'schedule'
+    [5, 10, 20, 40, 80, 100, 100],
+    // Sync just started working - more 'schedule'
+    [100, 100],
+    // Just stopped again - error backoff process restarts.
+    [5, 10],
+    // Another success and we are back to 'schedule'
+    [100, 100],
+  ]);
+
+  // fire things off.
+  scheduler.init();
+
+  // and wait for completion.
+  yield backoffsChecked;
+
+  scheduler.finalize();
+});
+
 function run_test() {
   run_next_test();
 }