diff --git a/lib/client.js b/lib/client.js index bb0ef2555..63173f3b2 100644 --- a/lib/client.js +++ b/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 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; + return this._syncApi.retryImmediately(); }; /** diff --git a/lib/sync.js b/lib/sync.js index acd80ef6e..973476a8d 100644 --- a/lib/sync.js +++ b/lib/sync.js @@ -69,6 +69,8 @@ function SyncApi(client, opts) { this._currentSyncRequest = null; this._syncState = null; this._running = false; + this._keepAliveTimer = null; + this._connectionReturnedDefer = null; } /** @@ -304,27 +306,31 @@ SyncApi.prototype.sync = function() { this._running = true; + if (global.document) { + this._onOnlineBound = this._onOnline.bind(this); + global.document.addEventListener("online", this._onOnlineBound, 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) { - attempt = attempt || 0; - attempt += 1; - + function getPushRules() { client.getPushRules().done(function(result) { debuglog("Got push rules"); client.pushRules = result; getFilter(); // Now get the filter - }, retryHandler(attempt, getPushRules)); + }, function(err) { + self._startKeepAlives().done(function() { + getPushRules(); + }); + self._updateSyncState("ERROR", { error: err }); + }); } - function getFilter(attempt) { - attempt = attempt || 0; - attempt += 1; - + function getFilter() { var filter = new Filter(client.credentials.userId); filter.setTimelineLimit(self.opts.initialSyncLimit); @@ -333,17 +339,12 @@ SyncApi.prototype.sync = function() { ).done(function(filterId) { debuglog("Using existing filter ID %s", filterId); self._sync({ filterId: filterId }); - }, retryHandler(attempt, getFilter)); - } - - // 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); + }, function(err) { + self._startKeepAlives().done(function() { + getFilter(); }); self._updateSyncState("ERROR", { error: err }); - }; + }); } if (client.isGuest()) { @@ -360,24 +361,41 @@ SyncApi.prototype.sync = function() { */ SyncApi.prototype.stop = function() { debuglog("SyncApi.stop"); + if (global.document) { + global.document.removeEventListener("online", this._onOnlineBound, false); + this._onOnlineBound = undefined; + } this._running = false; if (this._currentSyncRequest) { this._currentSyncRequest.abort(); } }; +/** + * Retry a backed off syncing request immediately. This should only be used when + * the user explicitly attempts to retry their lost connection. + * @return {boolean} True if this resulted in a request being retried. + */ +SyncApi.prototype.retryImmediately = function() { + if (!this._connectionReturnedDefer) { 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."); + if (self._connectionReturnedDefer) { + self._connectionReturnedDefer.reject(); + self._connectionReturnedDefer = null; + } this._updateSyncState("STOPPED"); } @@ -394,10 +412,11 @@ 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. + // be a long time. Set it to 0 when doing retries so we don't have to wait + // for an event or a timeout before emiting the SYNCING event. qps.timeout = 0; } @@ -465,37 +484,23 @@ SyncApi.prototype._sync = function(syncOptions, attempt) { self._sync(syncOptions); }, function(err) { - if (self.getSyncState() == "STOPPED") { + if (!self._running) { debuglog("Sync no longer running: exiting"); + if (self._connectionReturnedDefer) { + self._connectionReturnedDefer.reject(); + self._connectionReturnedDefer = null; + } return; } - if (!self._syncConnectionLost) { - debuglog("Starting keep-alive"); - self._syncConnectionLost = true; - retryPromise(self._pokeKeepAlive.bind(self), 2000).done(function() { - debuglog("Keep-alive successful."); - // blow away the current /sync request if the connection is still - // 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("/sync error %s", err); console.error(err); - attempt += 1; - startSyncingRetryTimer(client, attempt, function(newAttempt) { - self._sync(syncOptions, newAttempt); + + debuglog("Starting keep-alive"); + self._syncConnectionLost = true; + self._startKeepAlives().done(function() { + self._sync(syncOptions); }); + self._currentSyncRequest = null; self._updateSyncState("ERROR", { error: err }); }); }; @@ -641,13 +646,52 @@ SyncApi.prototype._processSyncResponse = function(syncToken, data) { }; /** - * @return {Promise} + * @return {promise} + */ +SyncApi.prototype._startKeepAlives = function() { + if (this._keepAliveTimer !== null) { + clearTimeout(this._keepAliveTimer); + } + this._pokeKeepAlive(); + if (!this._connectionReturnedDefer) { + this._connectionReturnedDefer = q.defer(); + } + return this._connectionReturnedDefer.promise; +}; + +/** + * */ SyncApi.prototype._pokeKeepAlive = function() { - return this.client._http.requestWithPrefix( + var self = this; + function success() { + clearTimeout(self._keepAliveTimer); + if (self._connectionReturnedDefer) { + self._connectionReturnedDefer.resolve(); + self._connectionReturnedDefer = null; + } + } + + 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. + // We wait a short time though, just in case somehow the server + // is in a mode where it 400s /versions responses and sync etc. + // responses fail, this will mean we don't hammer in a loop. + self._keepAliveTimer = setTimeout(success, 2000); + } else { + self._keepAliveTimer = setTimeout( + self._pokeKeepAlive.bind(self), + 5000 + Math.floor(Math.random() * 5000) + ); + } + }); }; /** @@ -835,43 +879,16 @@ SyncApi.prototype._updateSyncState = function(newState, data) { this.client.emit("sync", this._syncState, old, data); }; - -function retryTimeMsForAttempt(attempt) { - // 2,4,8,16,32,32,32,32,... seconds - // max 2^5 secs = 32 secs - return Math.pow(2, Math.min(attempt, 5)) * 1000; -} - -function retryPromise(promiseFn, delay) { - delay = delay || 0; - 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); -} +/** + * Event handler for the 'online' event + * This event is generally unreliable and precise behaviour + * varies between browsers, so we poll for connectivity too, + * but this might help us reconnect a little faster. + */ +SyncApi.prototype._onOnline = function() { + debuglog("Browser thinks we are back online"); + this._startKeepAlives(); +}; function createNewUser(client, userId) { var user = new User(userId); diff --git a/spec/unit/matrix-client.spec.js b/spec/unit/matrix-client.spec.js index b9f33aae9..b948e046c 100644 --- a/spec/unit/matrix-client.spec.js +++ b/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({ - method: "POST", path: FILTER_PATH, 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(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( - 0, "more httpLookups remaining than expected" - ); + } else if (state === "SYNCING" && httpLookups.length === 0) { 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({ - method: "GET", path: "/pushrules/", error: { errcode: "NOPE_NOPE_NOPE" } - }); + httpLookups.push(PUSH_RULES_RESPONSE); + 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();