From 0c1c10a0e0e49449ad60207ca6aa0528a2c3f18b Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 16 Mar 2017 18:02:17 +0000 Subject: [PATCH 01/12] WIP memleak fixes (341->295MB) --- src/client.js | 2 +- src/crypto/DeviceList.js | 2 +- src/models/event.js | 26 +++++++++++++++++++++ src/store/indexeddb.js | 2 +- src/sync.js | 49 ++++++++++++++++++++++++++-------------- 5 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/client.js b/src/client.js index eaafeb90a11..c0b88df6acb 100644 --- a/src/client.js +++ b/src/client.js @@ -2777,7 +2777,7 @@ MatrixClient.prototype.startClient = function(opts) { } // periodically poll for turn servers if we support voip - checkTurnServers(this); + //checkTurnServers(this); if (this._syncApi) { // This shouldn't happen since we thought the client was not running diff --git a/src/crypto/DeviceList.js b/src/crypto/DeviceList.js index 128afdb093e..91a710cb3e7 100644 --- a/src/crypto/DeviceList.js +++ b/src/crypto/DeviceList.js @@ -231,7 +231,7 @@ export default class DeviceList { // refresh request). // By checking it is at least a string, we can eliminate a class of // silly errors. - if (typeof userId !== 'string') { + if (typeof userId !== 'string' && typeof userId !== 'object') { throw new Error('userId must be a string; was '+userId); } this._pendingUsersWithNewDevices[userId] = true; diff --git a/src/models/event.js b/src/models/event.js index 93729a31fa6..f532bc62a9d 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -49,6 +49,8 @@ module.exports.EventStatus = { CANCELLED: "cancelled", }; +const interns = {}; + /** * Construct a Matrix Event object * @constructor @@ -75,7 +77,31 @@ module.exports.EventStatus = { module.exports.MatrixEvent = function MatrixEvent( event, ) { + ["state_key", "type", "sender", "room_id"].forEach((prop) => { + if (!event[prop]) { + return; + } + if (!interns[event[prop]]) { + interns[event[prop]] = event[prop]; + } + event[prop] = interns[event[prop]]; + }); + + ["membership", "avatar_url", "displayname"].forEach((prop) => { + if (!event.content || !event.content[prop]) { + return; + } + if (!interns[event.content[prop]]) { + interns[event.content[prop]] = event.content[prop]; + } + event.content[prop] = interns[event.content[prop]]; + }); + + + this.event = event || {}; + + this.sender = null; this.target = null; this.status = null; diff --git a/src/store/indexeddb.js b/src/store/indexeddb.js index 12b18f33e25..dbc9cf11afa 100644 --- a/src/store/indexeddb.js +++ b/src/store/indexeddb.js @@ -33,7 +33,7 @@ const VERSION = 1; // so infrequently that the /sync size gets bigger on reload. Writing more // often does not affect the length of the pause since the entire /sync // response is persisted each time. -const WRITE_DELAY_MS = 1000 * 60 * 5; // once every 5 minutes +const WRITE_DELAY_MS = 1000 * 60 * 100; // once every 5 minutes /** * Construct a new Indexed Database store backend. This requires a call to diff --git a/src/sync.js b/src/sync.js index 8eef2fbf860..3837c087d98 100644 --- a/src/sync.js +++ b/src/sync.js @@ -82,6 +82,9 @@ function SyncApi(client, opts) { this._keepAliveTimer = null; this._connectionReturnedDefer = null; this._notifEvents = []; // accumulator of sync events in the current sync response + client._fns = { + // eventName: function + }; if (client.getNotifTimelineSet()) { reEmit(client, client.getNotifTimelineSet(), @@ -425,7 +428,8 @@ SyncApi.prototype.sync = function() { // no push rules for guests, no access to POST filter for guests. self._sync({}); } else { - getPushRules(); + //getPushRules(); + self._sync({}); } }; @@ -470,6 +474,7 @@ SyncApi.prototype.retryImmediately = function() { SyncApi.prototype._sync = function(syncOptions) { const client = this.client; const self = this; + console.log("_sync"); if (!this._running) { debuglog("Sync no longer running: exiting."); @@ -532,10 +537,13 @@ SyncApi.prototype._sync = function(syncOptions) { } let isCachedResponse = false; + console.log("syncOptions => ", syncOptions, self.opts); if (self.opts.syncAccumulator && !syncOptions.hasSyncedBefore) { + let data = self.opts.syncAccumulator.getJSON(); // Don't do an HTTP hit to /sync. Instead, load up the persisted /sync data, // if there is data there. + console.log("Data: ", data); if (data.nextBatch) { debuglog("sync(): not doing HTTP hit, instead returning stored /sync data"); // We must deep copy the stored data so that the /sync processing code doesn't @@ -553,10 +561,15 @@ SyncApi.prototype._sync = function(syncOptions) { } if (!isCachedResponse) { - //debuglog('Starting sync since=' + syncToken); +/* + debuglog('Starting sync since=' + syncToken); this._currentSyncRequest = client._http.authedRequest( undefined, "GET", "/sync", qps, undefined, clientSideTimeoutMs, - ); + ); */ + + + var d = q.defer(); + this._currentSyncRequest = d.promise; } this._currentSyncRequest.done(function(data) { @@ -1214,21 +1227,23 @@ function createNewUser(client, userId) { return user; } -function reEmit(reEmitEntity, emittableEntity, eventNames) { - utils.forEach(eventNames, function(eventName) { - // setup a listener on the entity (the Room, User, etc) for this event - emittableEntity.on(eventName, function() { - // take the args from the listener and reuse them, adding the - // event name to the arg list so it works with .emit() - // Transformation Example: - // listener on "foo" => function(a,b) { ... } - // Re-emit on "thing" => thing.emit("foo", a, b) - const newArgs = [eventName]; - for (let i = 0; i < arguments.length; i++) { - newArgs.push(arguments[i]); +function reEmit(client, emittableEntity, eventNames) { + eventNames.forEach((name) => { + if (!client._fns[name]) { + client._fns[name] = function() { + // take the args from the listener and reuse them, adding the + // event name to the arg list so it works with .emit() + // Transformation Example: + // listener on "foo" => function(a,b) { ... } + // Re-emit on "thing" => thing.emit("foo", a, b) + const newArgs = [name]; + for (let i = 0; i < arguments.length; i++) { + newArgs.push(arguments[i]); + } + client.emit(...newArgs); } - reEmitEntity.emit(...newArgs); - }); + } + emittableEntity.on(name, client._fns[name]); }); } From 107ef27f690fa21a85afdebe527e0715795c67a0 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 20 Mar 2017 11:26:59 +0000 Subject: [PATCH 02/12] Remove spurious changes --- src/client.js | 2 +- src/crypto/DeviceList.js | 2 +- src/store/indexeddb.js | 2 +- src/sync.js | 49 ++++++++++++++-------------------------- 4 files changed, 20 insertions(+), 35 deletions(-) diff --git a/src/client.js b/src/client.js index c0b88df6acb..eaafeb90a11 100644 --- a/src/client.js +++ b/src/client.js @@ -2777,7 +2777,7 @@ MatrixClient.prototype.startClient = function(opts) { } // periodically poll for turn servers if we support voip - //checkTurnServers(this); + checkTurnServers(this); if (this._syncApi) { // This shouldn't happen since we thought the client was not running diff --git a/src/crypto/DeviceList.js b/src/crypto/DeviceList.js index 91a710cb3e7..128afdb093e 100644 --- a/src/crypto/DeviceList.js +++ b/src/crypto/DeviceList.js @@ -231,7 +231,7 @@ export default class DeviceList { // refresh request). // By checking it is at least a string, we can eliminate a class of // silly errors. - if (typeof userId !== 'string' && typeof userId !== 'object') { + if (typeof userId !== 'string') { throw new Error('userId must be a string; was '+userId); } this._pendingUsersWithNewDevices[userId] = true; diff --git a/src/store/indexeddb.js b/src/store/indexeddb.js index dbc9cf11afa..12b18f33e25 100644 --- a/src/store/indexeddb.js +++ b/src/store/indexeddb.js @@ -33,7 +33,7 @@ const VERSION = 1; // so infrequently that the /sync size gets bigger on reload. Writing more // often does not affect the length of the pause since the entire /sync // response is persisted each time. -const WRITE_DELAY_MS = 1000 * 60 * 100; // once every 5 minutes +const WRITE_DELAY_MS = 1000 * 60 * 5; // once every 5 minutes /** * Construct a new Indexed Database store backend. This requires a call to diff --git a/src/sync.js b/src/sync.js index 3837c087d98..0ff4f03550c 100644 --- a/src/sync.js +++ b/src/sync.js @@ -82,9 +82,6 @@ function SyncApi(client, opts) { this._keepAliveTimer = null; this._connectionReturnedDefer = null; this._notifEvents = []; // accumulator of sync events in the current sync response - client._fns = { - // eventName: function - }; if (client.getNotifTimelineSet()) { reEmit(client, client.getNotifTimelineSet(), @@ -428,8 +425,7 @@ SyncApi.prototype.sync = function() { // no push rules for guests, no access to POST filter for guests. self._sync({}); } else { - //getPushRules(); - self._sync({}); + getPushRules(); } }; @@ -474,7 +470,6 @@ SyncApi.prototype.retryImmediately = function() { SyncApi.prototype._sync = function(syncOptions) { const client = this.client; const self = this; - console.log("_sync"); if (!this._running) { debuglog("Sync no longer running: exiting."); @@ -537,13 +532,10 @@ SyncApi.prototype._sync = function(syncOptions) { } let isCachedResponse = false; - console.log("syncOptions => ", syncOptions, self.opts); if (self.opts.syncAccumulator && !syncOptions.hasSyncedBefore) { - let data = self.opts.syncAccumulator.getJSON(); // Don't do an HTTP hit to /sync. Instead, load up the persisted /sync data, // if there is data there. - console.log("Data: ", data); if (data.nextBatch) { debuglog("sync(): not doing HTTP hit, instead returning stored /sync data"); // We must deep copy the stored data so that the /sync processing code doesn't @@ -561,15 +553,10 @@ SyncApi.prototype._sync = function(syncOptions) { } if (!isCachedResponse) { -/* - debuglog('Starting sync since=' + syncToken); + // debuglog('Starting sync since=' + syncToken); this._currentSyncRequest = client._http.authedRequest( undefined, "GET", "/sync", qps, undefined, clientSideTimeoutMs, - ); */ - - - var d = q.defer(); - this._currentSyncRequest = d.promise; + ); } this._currentSyncRequest.done(function(data) { @@ -1227,23 +1214,21 @@ function createNewUser(client, userId) { return user; } -function reEmit(client, emittableEntity, eventNames) { - eventNames.forEach((name) => { - if (!client._fns[name]) { - client._fns[name] = function() { - // take the args from the listener and reuse them, adding the - // event name to the arg list so it works with .emit() - // Transformation Example: - // listener on "foo" => function(a,b) { ... } - // Re-emit on "thing" => thing.emit("foo", a, b) - const newArgs = [name]; - for (let i = 0; i < arguments.length; i++) { - newArgs.push(arguments[i]); - } - client.emit(...newArgs); +function reEmit(reEmitEntity, emittableEntity, eventNames) { + utils.forEach(eventName, function(eventName) { + // setup a listener on the entity (the Room, User, etc) for this event + emittableEntity.on(eventName, function() { + // take the args from the listener and reuse them, adding the + // event name to the arg list so it works with .emit() + // Transformation Example: + // listener on "foo" => function(a,b) { ... } + // Re-emit on "thing" => thing.emit("foo", a, b) + const newArgs = [eventName]; + for (let i = 0; i < arguments.length; i++) { + newArgs.push(arguments[i]); } - } - emittableEntity.on(name, client._fns[name]); + reEmitEntity.emit(...newArgs); + }); }); } From 37a186696af774bbf1914f7d11dd126da8b60932 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 20 Mar 2017 11:27:59 +0000 Subject: [PATCH 03/12] Remove spurious changes --- src/sync.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sync.js b/src/sync.js index 0ff4f03550c..8eef2fbf860 100644 --- a/src/sync.js +++ b/src/sync.js @@ -553,7 +553,7 @@ SyncApi.prototype._sync = function(syncOptions) { } if (!isCachedResponse) { - // debuglog('Starting sync since=' + syncToken); + //debuglog('Starting sync since=' + syncToken); this._currentSyncRequest = client._http.authedRequest( undefined, "GET", "/sync", qps, undefined, clientSideTimeoutMs, ); @@ -1215,7 +1215,7 @@ function createNewUser(client, userId) { } function reEmit(reEmitEntity, emittableEntity, eventNames) { - utils.forEach(eventName, function(eventName) { + utils.forEach(eventNames, function(eventName) { // setup a listener on the entity (the Room, User, etc) for this event emittableEntity.on(eventName, function() { // take the args from the listener and reuse them, adding the @@ -1228,7 +1228,7 @@ function reEmit(reEmitEntity, emittableEntity, eventNames) { newArgs.push(arguments[i]); } reEmitEntity.emit(...newArgs); - }); + }); }); } From 999fc076833d2b06a30337cd9e9b55c17590e405 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 20 Mar 2017 11:34:50 +0000 Subject: [PATCH 04/12] Explain the memory hack --- src/models/event.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/models/event.js b/src/models/event.js index f532bc62a9d..e91b57f95d2 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -77,6 +77,9 @@ const interns = {}; module.exports.MatrixEvent = function MatrixEvent( event, ) { + // intern the values of matrix events to force share strings and reduce the + // amount of needless string duplication. This can save moderate amounts of + // memory (~10% on a 350MB heap). ["state_key", "type", "sender", "room_id"].forEach((prop) => { if (!event[prop]) { return; @@ -97,11 +100,8 @@ module.exports.MatrixEvent = function MatrixEvent( event.content[prop] = interns[event.content[prop]]; }); - - this.event = event || {}; - this.sender = null; this.target = null; this.status = null; From b666ec1f4d9bb5d5b0bab712f54321f1bc77318e Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 20 Mar 2017 11:46:50 +0000 Subject: [PATCH 05/12] Fix memory leak on limited room responses --- src/models/room.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/models/room.js b/src/models/room.js index 238d20c84ed..93bebf00495 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -207,7 +207,12 @@ Room.prototype.getLiveTimeline = function() { */ Room.prototype.resetLiveTimeline = function(backPaginationToken) { for (let i = 0; i < this._timelineSets.length; i++) { - this._timelineSets[i].resetLiveTimeline(backPaginationToken); + // Flush out non-live timelines. We do this to reduce the amount of memory + // used, as storing multiple distinct copies of room state (each of which + // can be 10s of MBs) for each timeline is expensive. This is particularly + // noticeable when the client unsleeps and there are a lot of 'limited: true' + // responses. https://github.com/vector-im/riot-web/issues/3307#issuecomment-282895568 + this._timelineSets[i].resetLiveTimeline(backPaginationToken, true); } this._fixUpLegacyTimelineFields(); From dc8a2670ab87506b573c117d7de3cf53a179b597 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 20 Mar 2017 12:05:22 +0000 Subject: [PATCH 06/12] Unbreak tests --- spec/unit/room.spec.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/unit/room.spec.js b/spec/unit/room.spec.js index 887e60e1bd9..d0174fbcecd 100644 --- a/spec/unit/room.spec.js +++ b/spec/unit/room.spec.js @@ -446,15 +446,14 @@ describe("Room", function() { expect(callCount).toEqual(1); }); - it("should " + (timelineSupport ? "remember" : "forget") + - " old timelines", function() { + it("should forget old timelines", function() { room.addLiveEvents([events[0]]); expect(room.timeline.length).toEqual(1); const firstLiveTimeline = room.getLiveTimeline(); room.resetLiveTimeline(); const tl = room.getTimelineForEvent(events[0].getId()); - expect(tl).toBe(timelineSupport ? firstLiveTimeline : null); + expect(tl).toBe(null); }); }; From 55acf21aa610dc6268d379d0686949135abb880a Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 20 Mar 2017 12:06:37 +0000 Subject: [PATCH 07/12] Linting --- spec/unit/room.spec.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/unit/room.spec.js b/spec/unit/room.spec.js index d0174fbcecd..5d310898506 100644 --- a/spec/unit/room.spec.js +++ b/spec/unit/room.spec.js @@ -449,9 +449,7 @@ describe("Room", function() { it("should forget old timelines", function() { room.addLiveEvents([events[0]]); expect(room.timeline.length).toEqual(1); - const firstLiveTimeline = room.getLiveTimeline(); room.resetLiveTimeline(); - const tl = room.getTimelineForEvent(events[0].getId()); expect(tl).toBe(null); }); From 1e05e0d6f8c86a6a0f23d593db90ec1b23e25bab Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 22 Mar 2017 11:56:10 +0000 Subject: [PATCH 08/12] Review comments --- src/client.js | 15 +++++++++++---- src/models/event-timeline-set.js | 4 +++- src/models/room.js | 11 ++++------- src/sync.js | 15 ++++++++++++++- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/src/client.js b/src/client.js index eaafeb90a11..ae0e21a6ac0 100644 --- a/src/client.js +++ b/src/client.js @@ -2733,10 +2733,10 @@ MatrixClient.prototype.getTurnServers = function() { /** - * High level helper method to call initialSync, emit the resulting events, - * and then start polling the eventStream for new events. To listen for these + * High level helper method to begin syncing and poll for new events. To listen for these * events, add a listener for {@link module:client~MatrixClient#event:"event"} - * via {@link module:client~MatrixClient#on}. + * via {@link module:client~MatrixClient#on}. Alternatively, listen for specific + * state change events. * @param {Object=} opts Options to apply when syncing. * @param {Number=} opts.initialSyncLimit The event limit= to apply * to initial sync. Default: 8. @@ -2753,11 +2753,18 @@ MatrixClient.prototype.getTurnServers = function() { * accessbile via {@link module:models/room#getPendingEvents}. Default: * "chronological". * - * @param {Number=} opts.pollTimeout The number of milliseconds to wait on /events. + * @param {Number=} opts.pollTimeout The number of milliseconds to wait on /sync. * Default: 30000 (30 seconds). * * @param {Filter=} opts.filter The filter to apply to /sync calls. This will override * the opts.initialSyncLimit, which would normally result in a timeline limit filter. + * + * @param {Function=} opts.canResetEntireTimeline A function which is called when /sync + * returns a 'limited' response. It is called with a room ID and returns a boolean. + * It should return 'true' if the SDK can SAFELY remove events from this room. It may + * not be safe to remove events if there are other references to the timelines for this + * room, e.g because the client is actively viewing events in this room. + * Default: returns false. */ MatrixClient.prototype.startClient = function(opts) { if (this.clientRunning) { diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 927049e1352..7fba58c1f55 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -187,7 +187,7 @@ EventTimelineSet.prototype.resetLiveTimeline = function(backPaginationToken, flu newTimeline.setPaginationToken(backPaginationToken, EventTimeline.BACKWARDS); this._liveTimeline = newTimeline; - this.emit("Room.timelineReset", this.room, this); + this.emit("Room.timelineReset", this.room, this, flush); }; /** @@ -655,4 +655,6 @@ module.exports = EventTimelineSet; * @event module:client~MatrixClient#"Room.timelineReset" * @param {Room} room The room whose live timeline was reset, if any * @param {EventTimelineSet} timelineSet timelineSet room whose live timeline was reset + * @param {boolean} resetAll The return value for canResetEntireTimeline() for this room. + * If true, ALL timeline sets for this room will be reset. */ diff --git a/src/models/room.js b/src/models/room.js index 93bebf00495..2447268a02b 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -204,15 +204,12 @@ Room.prototype.getLiveTimeline = function() { *

This is used when /sync returns a 'limited' timeline. * * @param {string=} backPaginationToken token for back-paginating the new timeline + * @param {boolean=} flush True to remove all events in all timelines. If false, only + * the live timeline is reset. */ -Room.prototype.resetLiveTimeline = function(backPaginationToken) { +Room.prototype.resetLiveTimeline = function(backPaginationToken, flush) { for (let i = 0; i < this._timelineSets.length; i++) { - // Flush out non-live timelines. We do this to reduce the amount of memory - // used, as storing multiple distinct copies of room state (each of which - // can be 10s of MBs) for each timeline is expensive. This is particularly - // noticeable when the client unsleeps and there are a lot of 'limited: true' - // responses. https://github.com/vector-im/riot-web/issues/3307#issuecomment-282895568 - this._timelineSets[i].resetLiveTimeline(backPaginationToken, true); + this._timelineSets[i].resetLiveTimeline(backPaginationToken, flush); } this._fixUpLegacyTimelineFields(); diff --git a/src/sync.js b/src/sync.js index 8eef2fbf860..94e99e5e0a2 100644 --- a/src/sync.js +++ b/src/sync.js @@ -63,6 +63,11 @@ function debuglog() { * @param {SyncAccumulator=} opts.syncAccumulator An accumulator which will be * kept up-to-date. If one is supplied, the response to getJSON() will be used * initially. + * @param {Function=} opts.canResetEntireTimeline A function which is called + * with a room ID and returns a boolean. It should return 'true' if the SDK can + * SAFELY remove events from this room. It may not be safe to remove events if + * there are other references to the timelines for this room. + * Default: returns false. */ function SyncApi(client, opts) { this.client = client; @@ -73,6 +78,11 @@ function SyncApi(client, opts) { opts.resolveInvitesToProfiles = opts.resolveInvitesToProfiles || false; opts.pollTimeout = opts.pollTimeout || (30 * 1000); opts.pendingEventOrdering = opts.pendingEventOrdering || "chronological"; + if (!opts.canResetEntireTimeline) { + opts.canResetEntireTimeline = function(roomId) { + return false; + }; + } this.opts = opts; this._peekRoomId = null; this._currentSyncRequest = null; @@ -848,7 +858,10 @@ SyncApi.prototype._processSyncResponse = function(syncToken, data) { // timeline. room.currentState.paginationToken = syncToken; self._deregisterStateListeners(room); - room.resetLiveTimeline(joinObj.timeline.prev_batch); + room.resetLiveTimeline( + joinObj.timeline.prev_batch, + self.opts.canResetEntireTimeline(room.roomId) + ); // We have to assume any gap in any timeline is // reason to stop incrementally tracking notifications and From 86fd42dcb5311dfdc940205ded793f1ec985ce0c Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 22 Mar 2017 12:01:58 +0000 Subject: [PATCH 09/12] linting --- src/sync.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sync.js b/src/sync.js index 94e99e5e0a2..c9573a418c8 100644 --- a/src/sync.js +++ b/src/sync.js @@ -860,7 +860,7 @@ SyncApi.prototype._processSyncResponse = function(syncToken, data) { self._deregisterStateListeners(room); room.resetLiveTimeline( joinObj.timeline.prev_batch, - self.opts.canResetEntireTimeline(room.roomId) + self.opts.canResetEntireTimeline(room.roomId), ); // We have to assume any gap in any timeline is From 5bee0004b2ba5456bac861a839f74e2fbe99cea7 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 22 Mar 2017 13:51:00 +0000 Subject: [PATCH 10/12] Revert test as nothing has changed --- spec/unit/room.spec.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/unit/room.spec.js b/spec/unit/room.spec.js index 5d310898506..887e60e1bd9 100644 --- a/spec/unit/room.spec.js +++ b/spec/unit/room.spec.js @@ -446,12 +446,15 @@ describe("Room", function() { expect(callCount).toEqual(1); }); - it("should forget old timelines", function() { + it("should " + (timelineSupport ? "remember" : "forget") + + " old timelines", function() { room.addLiveEvents([events[0]]); expect(room.timeline.length).toEqual(1); + const firstLiveTimeline = room.getLiveTimeline(); room.resetLiveTimeline(); + const tl = room.getTimelineForEvent(events[0].getId()); - expect(tl).toBe(null); + expect(tl).toBe(timelineSupport ? firstLiveTimeline : null); }); }; From ccbc0b79b844d58fc422e3be81eb213b4625d405 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 22 Mar 2017 14:29:59 +0000 Subject: [PATCH 11/12] Add getter/setter for the callback on the MatrixClient instance rather than a startClient opt for ease of gluing code in --- src/client.js | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/client.js b/src/client.js index ae0e21a6ac0..383b7f23cf9 100644 --- a/src/client.js +++ b/src/client.js @@ -2758,13 +2758,6 @@ MatrixClient.prototype.getTurnServers = function() { * * @param {Filter=} opts.filter The filter to apply to /sync calls. This will override * the opts.initialSyncLimit, which would normally result in a timeline limit filter. - * - * @param {Function=} opts.canResetEntireTimeline A function which is called when /sync - * returns a 'limited' response. It is called with a room ID and returns a boolean. - * It should return 'true' if the SDK can SAFELY remove events from this room. It may - * not be safe to remove events if there are other references to the timelines for this - * room, e.g because the client is actively viewing events in this room. - * Default: returns false. */ MatrixClient.prototype.startClient = function(opts) { if (this.clientRunning) { @@ -2797,6 +2790,12 @@ MatrixClient.prototype.startClient = function(opts) { opts.crypto = this._crypto; opts.syncAccumulator = this._syncAccumulator; + opts.canResetEntireTimeline = (roomId) => { + if (!this._canResetTimelineCallback) { + return false; + } + return this._canResetTimelineCallback(roomId); + }; this._clientOpts = opts; this._syncApi = new SyncApi(this, opts); @@ -2817,6 +2816,27 @@ MatrixClient.prototype.stopClient = function() { global.clearTimeout(this._checkTurnServersTimeoutID); }; +/* + * Set a function which is called when /sync returns a 'limited' response. + * It is called with a room ID and returns a boolean. It should return 'true' if the SDK + * can SAFELY remove events from this room. It may not be safe to remove events if there + * are other references to the timelines for this room, e.g because the client is + * actively viewing events in this room. + * Default: returns false. + * @param {Function} cb The callback which will be invoked. + */ +MatrixClient.prototype.setCanResetTimelineCallback = function(cb) { + this._canResetTimelineCallback = cb; +}; + +/** + * Get the callback set via `setCanResetTimelineCallback`. + * @return {?Function} The callback or null + */ +MatrixClient.prototype.getCanResetTimelineCallback = function() { + return this._canResetTimelineCallback; +}; + function setupCallEventHandler(client) { const candidatesByCall = { // callId: [Candidate] From 14727d75ac90b49e25b720b01e8cadbc122ff421 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 22 Mar 2017 15:13:21 +0000 Subject: [PATCH 12/12] Review comments --- src/models/event-timeline-set.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 7fba58c1f55..4b46eccf523 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -154,10 +154,11 @@ EventTimelineSet.prototype.replaceEventId = function(oldEventId, newEventId) { * @fires module:client~MatrixClient#event:"Room.timelineReset" */ EventTimelineSet.prototype.resetLiveTimeline = function(backPaginationToken, flush) { - let newTimeline; + // if timeline support is disabled, forget about the old timelines + const resetAllTimelines = !this._timelineSupport || flush; - if (!this._timelineSupport || flush) { - // if timeline support is disabled, forget about the old timelines + let newTimeline; + if (resetAllTimelines) { newTimeline = new EventTimeline(this); this._timelines = [newTimeline]; this._eventIdToTimeline = {}; @@ -187,7 +188,7 @@ EventTimelineSet.prototype.resetLiveTimeline = function(backPaginationToken, flu newTimeline.setPaginationToken(backPaginationToken, EventTimeline.BACKWARDS); this._liveTimeline = newTimeline; - this.emit("Room.timelineReset", this.room, this, flush); + this.emit("Room.timelineReset", this.room, this, resetAllTimelines); }; /** @@ -655,6 +656,5 @@ module.exports = EventTimelineSet; * @event module:client~MatrixClient#"Room.timelineReset" * @param {Room} room The room whose live timeline was reset, if any * @param {EventTimelineSet} timelineSet timelineSet room whose live timeline was reset - * @param {boolean} resetAll The return value for canResetEntireTimeline() for this room. - * If true, ALL timeline sets for this room will be reset. + * @param {boolean} resetAllTimelines True if all timelines were reset. */