Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix leaking room state objects on limited sync responses #395

Merged
merged 12 commits into from
Mar 22, 2017
35 changes: 31 additions & 4 deletions src/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <code>limit=</code> to apply
* to initial sync. Default: 8.
Expand All @@ -2753,7 +2753,7 @@ 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
Expand Down Expand Up @@ -2790,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);
Expand All @@ -2810,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]
Expand Down
10 changes: 6 additions & 4 deletions src/models/event-timeline-set.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};
Expand Down Expand Up @@ -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);
this.emit("Room.timelineReset", this.room, this, resetAllTimelines);
};

/**
Expand Down Expand Up @@ -655,4 +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} resetAllTimelines True if all timelines were reset.
*/
26 changes: 26 additions & 0 deletions src/models/event.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ module.exports.EventStatus = {
CANCELLED: "cancelled",
};

const interns = {};

/**
* Construct a Matrix Event object
* @constructor
Expand All @@ -75,7 +77,31 @@ module.exports.EventStatus = {
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;
}
if (!interns[event[prop]]) {
interns[event[prop]] = event[prop];
}
event[prop] = interns[event[prop]];
});

["membership", "avatar_url", "displayname"].forEach((prop) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should worry about interns filling up with old displaynames and avatar_urls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never remove events either, so the strings which are there should be net positive.

If we deleted m.room.member events then absolutely, we probably would need to start caring. We still keep all previous names/avatar URLs when the user changes their name/avatar, so frequent name changes will still be referenced somewhere in the code.

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;
Expand Down
6 changes: 4 additions & 2 deletions src/models/room.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,12 @@ Room.prototype.getLiveTimeline = function() {
* <p>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++) {
this._timelineSets[i].resetLiveTimeline(backPaginationToken);
this._timelineSets[i].resetLiveTimeline(backPaginationToken, flush);
}

this._fixUpLegacyTimelineFields();
Expand Down
15 changes: 14 additions & 1 deletion src/sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down