Just use the keepalive logic to recover from lost internet connections.

* This should fix the problem where we could end up with two concurrent syncs
   due to both retrying /sync and hitting /versions. There is now one and only
   one mechanism for connection recovery.
 * Hit /versions a little less often as I think every 2 seconds is a little
   over-aggressive. Also introduce randomness to minimize possibility of
   thundering herds.
 * Add a listener for the 'online' event in case any browser is nice enough
   to ever fire it.
 * Treat a 400 response from /versions as successful since older synapses
   will not support it.

lib/client.js

@@ -283,15 +283,7 @@ MatrixClient.prototype.setGuest = function(isGuest) {
  * @return {boolean} True if this resulted in a request being retried.
  */
 MatrixClient.prototype.retryImmediately = function() {
-    if (!this._syncingRetry) {
+    return this._syncApi.retryImmediately();
-        return false;
-    }
-    // stop waiting
-    clearTimeout(this._syncingRetry.timeoutId);
-    // invoke immediately
-    this._syncingRetry.fn(2); // FIXME: It shouldn't need to know about attempts :/
-    this._syncingRetry = null;
-    return true;
 };
 
 /**

lib/sync.js

@@ -69,6 +69,7 @@ function SyncApi(client, opts) {
     this._currentSyncRequest = null;
     this._syncState = null;
     this._running = false;
+    this._keepAliveTimer = null;
 }
 
 /**
@@ -304,27 +305,29 @@ SyncApi.prototype.sync = function() {
 
     this._running = true;
 
+    if (global.document) {
+        global.document.addEventListener("online", this._onOnline.bind(this), false);
+    }
+
     // We need to do one-off checks before we can begin the /sync loop.
     // These are:
     //   1) We need to get push rules so we can check if events should bing as we get
     //      them from /sync.
     //   2) We need to get/create a filter which we can use for /sync.
 
-    function getPushRules(attempt) {
+    function getPushRules() {
-        attempt = attempt || 0;
-        attempt += 1;
-
         client.getPushRules().done(function(result) {
             debuglog("Got push rules");
             client.pushRules = result;
             getFilter(); // Now get the filter
-        }, retryHandler(attempt, getPushRules));
+        }, function(err) {
+            self._onConnectionReturned = getPushRules;
+            self._startKeepAlives();
+            self._updateSyncState("ERROR", { error: err });
+        });
     }
 
-    function getFilter(attempt) {
+    function getFilter() {
-        attempt = attempt || 0;
-        attempt += 1;
-
         var filter = new Filter(client.credentials.userId);
         filter.setTimelineLimit(self.opts.initialSyncLimit);
 
@@ -333,17 +336,11 @@ SyncApi.prototype.sync = function() {
         ).done(function(filterId) {
             debuglog("Using existing filter ID %s", filterId);
             self._sync({ filterId: filterId });
-        }, retryHandler(attempt, getFilter));
+        }, function(err) {
-    }
+            self._onConnectionReturned = getFilter;
-
+            self._startKeepAlives();
-    // sets the sync state to error and waits a bit before re-invoking the function.
-    function retryHandler(attempt, fnToRun) {
-        return function(err) {
-            startSyncingRetryTimer(client, attempt, function(newAttempt) {
-                fnToRun(newAttempt);
-            });
             self._updateSyncState("ERROR", { error: err });
-        };
+        });
     }
 
     if (client.isGuest()) {
@@ -360,21 +357,33 @@ SyncApi.prototype.sync = function() {
  */
 SyncApi.prototype.stop = function() {
     debuglog("SyncApi.stop");
+    if (global.document) {
+        global.document.removeEventListener("online", this._onOnline.bind(this), false);
+    }
     this._running = false;
     if (this._currentSyncRequest) { this._currentSyncRequest.abort(); }
 };
 
+/**
+ * Retry a backed off syncing request immediately. This should only be used when
+ * the user <b>explicitly</b> attempts to retry their lost connection.
+ * @return {boolean} True if this resulted in a request being retried.
+ */
+SyncApi.prototype.retryImmediately = function() {
+    if (!this._onConnectionReturned) { return false; }
+    this._startKeepAlives();
+    return true;
+};
+
 /**
  * Invoke me to do /sync calls
  * @param {Object} syncOptions
  * @param {string} syncOptions.filterId
  * @param {boolean} syncOptions.hasSyncedBefore
- * @param {Number=} attempt
  */
-SyncApi.prototype._sync = function(syncOptions, attempt) {
+SyncApi.prototype._sync = function(syncOptions) {
     var client = this.client;
     var self = this;
-    attempt = attempt || 1;
 
     if (!this._running) {
         debuglog("Sync no longer running: exiting.");
@@ -394,7 +403,7 @@ SyncApi.prototype._sync = function(syncOptions, attempt) {
         since: syncToken || undefined // do not send 'null'
     };
 
-    if (attempt > 1) {
+    if (self._syncConnectionLost) {
         // we think the connection is dead. If it comes back up, we won't know
         // about it till /sync returns. If the timeout= is high, this could
         // be a long time. Set it to 0 when doing retries.
@@ -465,37 +474,18 @@ SyncApi.prototype._sync = function(syncOptions, attempt) {
 
         self._sync(syncOptions);
     }, function(err) {
-        if (self.getSyncState() == "STOPPED") {
+        if (!self._running) {
             debuglog("Sync no longer running: exiting");
             return;
         }
-        if (!self._syncConnectionLost) {
+        console.error("/sync error %s", err);
+        console.error(err);
+
         debuglog("Starting keep-alive");
         self._syncConnectionLost = true;
-            retryPromise(self._pokeKeepAlive.bind(self), 2000).done(function() {
+        self._onConnectionReturned = self._sync.bind(self, syncOptions);
-                debuglog("Keep-alive successful.");
+        self._startKeepAlives();
-                // blow away the current /sync request if the connection is still
+        self._currentSyncRequest = null;
-                // dead. It may be black-holed.
-                if (!self._syncConnectionLost) {
-                    return;
-                }
-                if (self._currentSyncRequest.abort) {
-                    // kill the current sync request
-                    debuglog("Aborting current /sync.");
-                    self._currentSyncRequest.abort();
-                }
-                // immediately retry if we were waiting
-                debuglog(
-                    "Interrupted /sync backoff: %s", self.client.retryImmediately()
-                );
-            });
-        }
-        console.error("/sync error (%s attempts): %s", attempt, err);
-        console.error(err);
-        attempt += 1;
-        startSyncingRetryTimer(client, attempt, function(newAttempt) {
-            self._sync(syncOptions, newAttempt);
-        });
         self._updateSyncState("ERROR", { error: err });
     });
 };
@@ -641,13 +631,45 @@ SyncApi.prototype._processSyncResponse = function(syncToken, data) {
 };
 
 /**
- * @return {Promise}
+ *
+ */
+SyncApi.prototype._startKeepAlives = function() {
+    if (this._keepAliveTimer !== null) {
+        clearTimeout(this._keepAliveTimer);
+    }
+    this._pokeKeepAlive();
+};
+
+/**
+ *
  */
 SyncApi.prototype._pokeKeepAlive = function() {
-    return this.client._http.requestWithPrefix(
+    var self = this;
+    function success() {
+        clearTimeout(self._keepAliveTimer);
+        if (self._onConnectionReturned) {
+            self._onConnectionReturned();
+            self._onConnectionReturned = undefined;
+        }
+    }
+
+    this.client._http.requestWithPrefix(
         undefined, "GET", "/_matrix/client/versions", undefined,
-        undefined, "", 5 * 1000
+        undefined, "", 15 * 1000
+    ).done(function() {
+        success();
+    }, function(err) {
+        if (err.httpStatus == 400) {
+            // treat this as a success because the server probably just doesn't
+            // support /versions: point is, we're getting a response.
+            success();
+        } else {
+            setTimeout(
+                self._pokeKeepAlive.bind(self),
+                5000 + Math.floor(Math.random() * 5000)
             );
+        }
+    });
 };
 
 /**
@@ -835,43 +857,16 @@ SyncApi.prototype._updateSyncState = function(newState, data) {
     this.client.emit("sync", this._syncState, old, data);
 };
 
-
+/**
-function retryTimeMsForAttempt(attempt) {
+ * Event handler for the 'online' event
-    // 2,4,8,16,32,32,32,32,... seconds
+ * This event is generally unreliable and precise behaviour
-    // max 2^5 secs = 32 secs
+ * varies between browsers, so we poll for connectivity too,
-    return Math.pow(2, Math.min(attempt, 5)) * 1000;
+ * but this might help us reconnect a little faster.
-}
+ */
-
+SyncApi.prototype._onOnline = function() {
-function retryPromise(promiseFn, delay) {
+    debuglog("Browser thinks we are back online");
-    delay = delay || 0;
+    this._startKeepAlives();
-    return promiseFn().catch(function(reason) { // if it fails
+};
-        // retry after waiting the delay time
-        return q.delay(delay).then(retryPromise.bind(null, promiseFn, delay));
-    });
-}
-
-function startSyncingRetryTimer(client, attempt, fn) {
-    client._syncingRetry = {};
-    client._syncingRetry.fn = fn;
-    var newAttempt = attempt;
-    var timeBeforeWaitingMs = Date.now();
-    var timeToWaitMs = retryTimeMsForAttempt(attempt);
-    client._syncingRetry.timeoutId = setTimeout(function() {
-        var timeAfterWaitingMs = Date.now();
-        var timeDeltaMs = timeAfterWaitingMs - timeBeforeWaitingMs;
-        if (timeDeltaMs > (2 * timeToWaitMs)) {
-            // we've waited more than twice what we were supposed to. Reset the
-            // attempt number back to 1. This can happen when the comp goes to
-            // sleep whilst the timer is running.
-            newAttempt = 1;
-            console.warn(
-                "Sync retry timer: Tried to wait %s ms but actually waited %s ms",
-                timeToWaitMs, timeDeltaMs
-            );
-        }
-        fn(newAttempt);
-    }, timeToWaitMs);
-}
 
 function createNewUser(client, userId) {
     var user = new User(userId);

spec/unit/matrix-client.spec.js

@@ -213,17 +213,19 @@ describe("MatrixClient", function() {
             httpLookups.push({
                 method: "POST", path: FILTER_PATH, error: { errcode: "NOPE_NOPE_NOPE" }
             });
-            httpLookups.push({
+            httpLookups.push(FILTER_RESPONSE);
-                method: "POST", path: FILTER_PATH, error: { errcode: "NOPE_NOPE_NOPE" }
+            httpLookups.push(SYNC_RESPONSE);
-            });
 
             client.on("sync", function syncListener(state) {
                 if (state === "ERROR" && httpLookups.length > 0) {
-                    expect(httpLookups.length).toEqual(1);
+                    expect(httpLookups.length).toEqual(2);
                     expect(client.retryImmediately()).toBe(true);
-                    expect(httpLookups.length).toEqual(0);
+                } else if (state === "PREPARED" && httpLookups.length == 0) {
                     client.removeListener("sync", syncListener);
                     done();
+                } else {
+                    // unexpected state transition!
+                    expect(state).toEqual(null);
                 }
             });
             client.startClient();
@@ -243,9 +245,7 @@ describe("MatrixClient", function() {
                     expect(client.retryImmediately()).toBe(
                         true, "retryImmediately returned false"
                     );
-                    expect(httpLookups.length).toEqual(
+                } else if (state === "SYNCING" && httpLookups.length == 0) {
-                        0, "more httpLookups remaining than expected"
-                    );
                     client.removeListener("sync", syncListener);
                     done();
                 }
@@ -258,17 +258,20 @@ describe("MatrixClient", function() {
             httpLookups.push({
                 method: "GET", path: "/pushrules/", error: { errcode: "NOPE_NOPE_NOPE" }
             });
-            httpLookups.push({
+            httpLookups.push(PUSH_RULES_RESPONSE);
-                method: "GET", path: "/pushrules/", error: { errcode: "NOPE_NOPE_NOPE" }
+            httpLookups.push(FILTER_RESPONSE);
-            });
+            httpLookups.push(SYNC_RESPONSE);
 
             client.on("sync", function syncListener(state) {
                 if (state === "ERROR" && httpLookups.length > 0) {
-                    expect(httpLookups.length).toEqual(1);
+                    expect(httpLookups.length).toEqual(3);
                     expect(client.retryImmediately()).toBe(true);
-                    expect(httpLookups.length).toEqual(0);
+                } else if (state === "PREPARED" && httpLookups.length == 0) {
                     client.removeListener("sync", syncListener);
                     done();
+                } else {
+                    // unexpected state transition!
+                    expect(state).toEqual(null);
                 }
             });
             client.startClient();