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 236336 b34ca540cf734796091643dad3c26978814bdef6
parent 236335 1e5dcedd40cce32ab759756f6707496de01b0173
child 236403 385840329d91d1b469111b1e0471d41e36e257be
push id12070
push usermhammond@skippinet.com.au
push dateSun, 29 Mar 2015 00:19:47 +0000
treeherderfx-team@b34ca540cf73 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1148724
milestone39.0a1
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();
 }