Use /members api for lazy loading

This commit is a substantial change, as /members returns state events,
not profile information as /joined_members, and this allows to simplify
the implementation quite a bit. We can assume again all members have
a state event associated with it.

I also changed most of the naming of lazy loaded members to
out-of-band members to reflect that this is the relevant bit for most
of the code, that the members didn't come through /sync but through
another channel.

This commit also addresses the race condition between /(joined_)members
and /sync. /members returns the members at the point in the timeline
at a given event id. Members are loaded at the last event
in the live timeline, and all members that come in from sync
in the mean time are marked as superseding the out of band members,
so they won't be overwritten, even if the timeline is reset in the
mean time.

Members are also marked if they originate from an out-of-band channel
(/members) so they can be stored accordingly (future PR).

The loading status is kept in room state now, as this made resolving
the race condition easier. One consequence is that the status needs
to be shared across cloned instances of RoomState. When resetting
the timeline (and cloning the room state) while lazy loading is in
progress, one of the RoomStates could be left in progress indefinitely.
Though that is more for clarity than avoiding any actual bugs.

src/base-apis.js

@@ -419,12 +419,30 @@ MatrixBaseApis.prototype.roomState = function(roomId, callback) {
 
 /**
  * @param {string} roomId
+ * @param {string} includeMembership the membership type to include in the response
+ * @param {string} excludeMembership the membership type to exclude from the response
+ * @param {string} atEventId the id of the event for which moment in the timeline the members should be returned for
  * @param {module:client.callback} callback Optional.
  * @return {module:client.Promise} Resolves: dictionary of userid to profile information
  * @return {module:http-api.MatrixError} Rejects: with an error response.
  */
-MatrixBaseApis.prototype.joinedMembers = function(roomId, callback) {
-    const path = utils.encodeUri("/rooms/$roomId/joined_members", {$roomId: roomId});
+MatrixBaseApis.prototype.members =
+function(roomId, includeMembership, excludeMembership, atEventId, callback) {
+    const queryParams = {};
+    if (includeMembership) {
+        queryParams.membership = includeMembership;
+    }
+    if (excludeMembership) {
+        queryParams.not_membership = excludeMembership;
+    }
+    if (atEventId) {
+        queryParams.at = atEventId;
+    }
+
+    const queryString = utils.encodeParams(queryParams);
+
+    const path = utils.encodeUri("/rooms/$roomId/members?" + queryString,
+        {$roomId: roomId});
     return this._http.authedRequest(callback, "GET", path);
 };
 

src/client.js

@@ -764,36 +764,14 @@ MatrixClient.prototype.getRoom = function(roomId) {
  */
 MatrixClient.prototype.loadRoomMembersIfNeeded = async function(roomId) {
     const room = this.getRoom(roomId);
-    if (!room || !room.membersNeedLoading()) {
+    if (!room || !room.needsOutOfBandMembers()) {
         return;
     }
-    // XXX: we should make sure that the members we get back represent the
-    // room state at a given point in time. The plan is to do this by
-    // passing the current next_batch sync token to the endpoint we use
-    // to fetch the members. For now, this is a prototype that uses
-    // the /joined_members api, which only tells us about the joined members
-    // (not invites for example) and does not support this synchronization.
-    // So there is a race condition here between the current /sync call
-    // and the /joined_members call: if the have conflicting information, which one
-    // represents the most recent state?
-    //
-    // Addressing this race condition and the fact that this only tells us about
-    // joined members is a prerequisite for taking this out of the prototype stage and
-    // enabling the feature flag (feature_lazyloading) that
-    // the call to this method is behind.
-    const joinedMembersPromise = this.joinedMembers(roomId);
-    const membersPromise = joinedMembersPromise.then((profiles) => {
-        return Object.entries(profiles.joined).map(([userId, profile]) => {
-            return {
-                userId: userId,
-                avatarUrl: profile.avatar_url,
-                displayName: profile.display_name,
-                membership: "join",  // as we need to support invitees as well
-                                    // in the future, already include but hardcode it
-            };
-        });
-    });
-    await room.setLazyLoadedMembers(membersPromise);
+
+    const lastEventId = room.getLastEventId();
+    const responsePromise = this.members(roomId, "join", "leave", lastEventId);
+    const eventsPromise = responsePromise.then((response) => response.chunk);
+    await room.loadOutOfBandMembers(eventsPromise);
 };
 
 /**

src/models/room-member.js

@@ -58,14 +58,50 @@ function RoomMember(roomId, userId) {
     this.events = {
         member: null,
     };
-    this._lazyLoadAvatarUrl = null;
-    this._isLazyLoaded = false;
+    this._isOutOfBand = false;
+    this._supersedesOutOfBand = false;
     this._updateModifiedTime();
 }
 utils.inherits(RoomMember, EventEmitter);
 
-RoomMember.prototype.isLazyLoaded = function() {
-    return this._isLazyLoaded;
+/**
+ * Mark the member as coming from a channel that is not sync
+ */
+RoomMember.prototype.markOutOfBand = function() {
+    this._isOutOfBand = true;
+};
+
+/**
+ * @returns {bool} does the member come from a channel that is not sync?
+ * This is used to store the member seperately
+ * from the sync state so it available across browser sessions.
+ */
+RoomMember.prototype.isOutOfBand = function() {
+    return this._isOutOfBand;
+};
+
+/**
+ * Does the member supersede an incoming out-of-band
+ * member? If so the out-of-band member should be ignored.
+ */
+RoomMember.prototype.supersedesOutOfBand = function() {
+    this._supersedesOutOfBand;
+};
+
+/**
+ * Mark the member as superseding the future incoming
+ * out-of-band members.
+ */
+RoomMember.prototype.markSupersedesOutOfBand = function() {
+    this._supersedesOutOfBand = true;
+};
+
+/**
+ * Clear the member superseding the future incoming
+ * out-of-band members, as loading finished or failed.
+ */
+RoomMember.prototype.clearSupersedesOutOfBand = function() {
+    this._supersedesOutOfBand = false;
 };
 
 /**
@@ -82,8 +118,7 @@ RoomMember.prototype.setMembershipEvent = function(event, roomState) {
         return;
     }
 
-    this._lazyLoadAvatarUrl = null;
-    this._isLazyLoaded = false;
+    this._isOutOfBand = false;
 
     this.events.member = event;
 
@@ -107,25 +142,6 @@ RoomMember.prototype.setMembershipEvent = function(event, roomState) {
     }
 };
 
-/**
- * Update this room member from a lazily loaded member
- * @param {string} displayName display name for lazy loaded member
- * @param {string} avatarUrl avatar url for lazy loaded member
- * @param {string} membership membership (join|invite|...) state for lazy loaded member
- * @param {RoomState} roomState the room state this member is part of, needed to disambiguate the display name
- */
-RoomMember.prototype.setAsLazyLoadedMember =
-function(displayName, avatarUrl, membership, roomState) {
-    if (this.events.member) {
-        return;
-    }
-    this.membership = membership;
-    this.name = calculateDisplayName(this.userId, displayName, roomState);
-    this.rawDisplayName = displayName || this.userId;
-    this._lazyLoadAvatarUrl = avatarUrl;
-    this._isLazyLoaded = true;
-};
-
 /**
  * Update this room member's power level event. May fire
  * "RoomMember.powerLevel" if this event updates this member's power levels.
@@ -294,9 +310,7 @@ RoomMember.prototype.getAvatarUrl =
  * @return {string} the mxc avatar url
  */
 RoomMember.prototype.getMxcAvatarUrl = function() {
-    if (this._lazyLoadAvatarUrl) {
-        return this._lazyLoadAvatarUrl;
-    } else if(this.events.member) {
+    if(this.events.member) {
         return this.events.member.getContent().avatar_url;
     } else if(this.user) {
         return this.user.avatarUrl;

src/models/room-state.js

@@ -22,6 +22,11 @@ const EventEmitter = require("events").EventEmitter;
 const utils = require("../utils");
 const RoomMember = require("./room-member");
 
+// possible statuses for out-of-band member loading
+const OOB_STATUS_NOTSTARTED = 1;
+const OOB_STATUS_INPROGRESS = 2;
+const OOB_STATUS_FINISHED = 3;
+
 /**
  * Construct room state.
  *
@@ -46,13 +51,17 @@ const RoomMember = require("./room-member");
  * @constructor
  * @param {?string} roomId Optional. The ID of the room which has this state.
  * If none is specified it just tracks paginationTokens, useful for notifTimelineSet
+ * @param {?object} oobMemberFlags Optional. The state of loading out of bound members.
+ * As the timeline might get reset while they are loading, this state needs to be inherited
+ * and shared when the room state is cloned for the new timeline.
+ * This should only be passed from clone.
  * @prop {Object.<string, RoomMember>} members The room member dictionary, keyed
  * on the user's ID.
  * @prop {Object.<string, Object.<string, MatrixEvent>>} events The state
  * events dictionary, keyed on the event type and then the state_key value.
  * @prop {string} paginationToken The pagination token for this state.
  */
-function RoomState(roomId) {
+function RoomState(roomId, oobMemberFlags = undefined) {
     this.roomId = roomId;
     this.members = {
         // userId: RoomMember
@@ -70,6 +79,12 @@ function RoomState(roomId) {
     this._userIdsToDisplayNames = {};
     this._tokenToInvite = {}; // 3pid invite state_key to m.room.member invite
     this._joinedMemberCount = null; // cache of the number of joined members
+    if (!oobMemberFlags) {
+        oobMemberFlags = {
+            status: OOB_STATUS_NOTSTARTED,
+        };
+    }
+    this._oobMemberFlags = oobMemberFlags;
 }
 utils.inherits(RoomState, EventEmitter);
 
@@ -154,21 +169,45 @@ RoomState.prototype.getStateEvents = function(eventType, stateKey) {
  * @return {RoomState} the copy of the room state
  */
 RoomState.prototype.clone = function() {
-    const copy = new RoomState(this.roomId);
+    const copy = new RoomState(this.roomId, this._oobMemberFlags);
+
+    // Ugly hack: because setStateEvents will mark
+    // members as susperseding future out of bound members
+    // if loading is in progress (through _oobMemberFlags)
+    // since these are not new members, we're merely copying them
+    // set the status to not started
+    // after copying, we set back the status and
+    // copy the superseding flag from the current state
+    const status = this._oobMemberFlags.status;
+    this._oobMemberFlags.status = OOB_STATUS_NOTSTARTED;
+
     Object.values(this.events).forEach((eventsByStateKey) => {
         const eventsForType = Object.values(eventsByStateKey);
         copy.setStateEvents(eventsForType);
     });
-    // clone lazily loaded members
-    const lazyLoadedMembers = Object.values(this.members)
-        .filter((member) => member.isLazyLoaded());
-    lazyLoadedMembers.forEach((m) => {
-        copy._setLazyLoadedMember(
-            m.userId,
-            m.rawDisplayName,
-            m.getMxcAvatarUrl(),
-            m.membership);
-    });
+
+    // Ugly hack: see above
+    this._oobMemberFlags.status = status;
+
+    // copy out of band flags if needed
+    if (this._oobMemberFlags.status == OOB_STATUS_FINISHED) {
+        // copy markOutOfBand flags
+        this.getMembers().forEach((member) => {
+            if (member.isOutOfBand()) {
+                const copyMember = copy.getMember(member.userId);
+                copyMember.markOutOfBand();
+            }
+        });
+    } else if (this._oobMemberFlags.status == OOB_STATUS_INPROGRESS) {
+        // copy markSupersedesOutOfBand flags
+        this.getMembers().forEach((member) => {
+            if (member.supersedesOutOfBand()) {
+                const copyMember = copy.getMember(member.userId);
+                copyMember.markSupersedesOutOfBand();
+            }
+        });
+    }
+
     return copy;
 };
 
@@ -195,10 +234,7 @@ RoomState.prototype.setStateEvents = function(stateEvents) {
             return;
         }
 
-        if (self.events[event.getType()] === undefined) {
-            self.events[event.getType()] = {};
-        }
-        self.events[event.getType()][event.getStateKey()] = event;
+        self._setStateEvent(event);
         if (event.getType() === "m.room.member") {
             _updateDisplayNameCache(
                 self, event.getStateKey(), event.getContent().displayname,
@@ -248,6 +284,13 @@ RoomState.prototype.setStateEvents = function(stateEvents) {
             }
 
             member.setMembershipEvent(event, self);
+
+            // if out of band members are loading,
+            // mark the member as more recent
+            if (self._oobMemberFlags.status == OOB_STATUS_INPROGRESS) {
+                member.markSupersedesOutOfBand();
+            }
+
             self._updateMember(member);
             self.emit("RoomState.members", event, self, member);
         } else if (event.getType() === "m.room.power_levels") {
@@ -263,6 +306,13 @@ RoomState.prototype.setStateEvents = function(stateEvents) {
     });
 };
 
+RoomState.prototype._setStateEvent = function(event) {
+    if (this.events[event.getType()] === undefined) {
+        this.events[event.getType()] = {};
+    }
+    this.events[event.getType()][event.getStateKey()] = event;
+};
+
 RoomState.prototype._updateMember = function(member) {
     // this member may have a power level already, so set it.
     const pwrLvlEvent = this.getStateEvents("m.room.power_levels", "");
@@ -278,39 +328,95 @@ RoomState.prototype._updateMember = function(member) {
 };
 
 /**
- * Sets the lazily loaded members.
- * @param {Member[]} members array of {userId, avatarUrl, displayName, membership} tuples
+ * Get the out-of-band members loading state, whether loading is needed or not.
+ * Note that loading might be in progress and hence isn't needed.
+ * @return {bool} whether or not the members of this room need to be loaded
  */
-RoomState.prototype.setLazyLoadedMembers = function(members) {
-    members.forEach((m) => {
-        this._setLazyLoadedMember(
-            m.userId,
-            m.displayName,
-            m.avatarUrl,
-            m.membership);
-    });
+RoomState.prototype.needsOutOfBandMembers = function() {
+    return this._oobMemberFlags.status === OOB_STATUS_NOTSTARTED;
 };
 
 /**
- * Sets a single lazily loaded member, used by both setLazyLoadedMembers and clone
- * @param {string} userId user id for lazy loaded member
- * @param {string} displayName display name for lazy loaded member
- * @param {string} avatarUrl avatar url for lazy loaded member
- * @param {string} membership membership (join|invite|...) state for lazy loaded member
+ * Mark this room state as waiting for out-of-band members,
+ * ensuring it doesn't ask for them to be requested again
+ * through needsOutOfBandMembers
  */
-RoomState.prototype._setLazyLoadedMember =
-function(userId, displayName, avatarUrl, membership) {
-    const preExistingMember = this.getMember(userId);
-    // don't overwrite existing state event members
-    if (preExistingMember && !preExistingMember.isLazyLoaded()) {
+RoomState.prototype.markOutOfBandMembersStarted = function() {
+    if (this._oobMemberFlags.status !== OOB_STATUS_NOTSTARTED) {
         return;
     }
-    const member = new RoomMember(this.roomId, userId);
-    member.setAsLazyLoadedMember(displayName, avatarUrl, membership, this);
+    this._oobMemberFlags.status = OOB_STATUS_INPROGRESS;
+};
+
+/**
+ * Mark this room state as having failed to fetch out-of-band members
+ */
+RoomState.prototype.markOutOfBandMembersFailed = function() {
+    if (this._oobMemberFlags.status !== OOB_STATUS_INPROGRESS) {
+        return;
+    }
+    // the request failed, there is nothing to supersede
+    // in case of a retry, these event would not supersede the
+    // retry anymore.
+    this.getMembers().forEach((m) => {
+        m.clearSupersedesOutOfBand();
+    });
+    this._oobMemberFlags.status = OOB_STATUS_NOTSTARTED;
+};
+
+/**
+ * Sets the loaded out-of-band members.
+ * @param {MatrixEvent[]} stateEvents array of membership state events
+ */
+RoomState.prototype.setOutOfBandMembers = function(stateEvents) {
+    if (this._oobMemberFlags.status !== OOB_STATUS_INPROGRESS) {
+        return;
+    }
+    this._oobMemberFlags.status = OOB_STATUS_FINISHED;
+    stateEvents.forEach((e) => this._setOutOfBandMember(e));
+};
+
+/**
+ * Sets a single out of band member, used by both setOutOfBandMembers and clone
+ * @param {MatrixEvent} stateEvent membership state event
+ */
+RoomState.prototype._setOutOfBandMember = function(stateEvent) {
+    if (stateEvent.getType() !== 'm.room.member') {
+        return;
+    }
+    const userId = stateEvent.getStateKey();
+    const existingMember = this.getMember(userId);
+    if (existingMember) {
+        const existingMemberEvent = existingMember.events.member;
+        // ignore out of band members with events we are
+        // already aware of.
+        if (existingMemberEvent.getId() === stateEvent.getId()) {
+            return;
+        }
+        // this member was updated since we started
+        // loading the out of band members.
+        // Ignore the out of band member and clear
+        // the "supersedes" flag as the out of members are now loaded
+        if (existingMember.supersedesOutOfBand()) {
+            existingMember.clearSupersedesOutOfBand();
+            return;
+        }
+    }
+
+    const member =
+        existingMember ? existingMember : new RoomMember(this.roomId, userId);
+    member.setMembershipEvent(stateEvent);
+    // needed to know which members need to be stored seperately
+    // as the are not part of the sync accumulator
+    // this is cleared by setMembershipEvent so when it's updated through /sync
+    member.markOutOfBand();
+
     _updateDisplayNameCache(this, member.userId, member.name);
+
+    this._setStateEvent(stateEvent);
     this._updateMember(member);
 
-    if (preExistingMember) {
+    if (existingMember) {
         this.emit("RoomState.members", {}, this, member);
     } else {
         this.emit('RoomState.newMember', {}, this, member);

src/models/room.js

@@ -216,32 +216,38 @@ Room.prototype.getLiveTimeline = function() {
     return this.getUnfilteredTimelineSet().getLiveTimeline();
 };
 
-/**
- * Get the lazy loading state, whether loading is needed or not.
- * @return {bool} whether or not the members of this room need to be loaded
- */
-Room.prototype.membersNeedLoading = function() {
-    return this._membersNeedLoading;
+Room.prototype.getLastEventId = function() {
+    const liveEvents = this.getLiveTimeline().getEvents();
+    return liveEvents.length ? liveEvents[liveEvents.length - 1].getId() : undefined;
 };
 
 /**
- * Sets the lazily loaded members from the result of calling /joined_members
- * @param {Promise} membersPromise promise with array of {userId, avatarUrl, displayName, membership} tuples
+ * Get the out-of-band members loading state, whether loading is needed or not.
+ * Note that loading might be in progress and hence isn't needed.
+ * @return {bool} whether or not the members of this room need to be loaded
  */
-Room.prototype.setLazyLoadedMembers = async function(membersPromise) {
-    if (!this._membersNeedLoading) {
+Room.prototype.needsOutOfBandMembers = function() {
+    return this.currentState.needsOutOfBandMembers();
+};
+
+/**
+ * Loads the out-of-band members from the promise passed in
+ * @param {Promise} eventsPromise promise with array with state events
+ */
+Room.prototype.loadOutOfBandMembers = async function(eventsPromise) {
+    if (!this.membersNeedLoading()) {
         return;
     }
-    this._membersNeedLoading = false;
-    let members = null;
+    this.currentState.markOutOfBandMembersStarted();
+    let eventPojos = null;
     try {
-        members = await membersPromise;
+        eventPojos = await eventsPromise;
     } catch (err) {
-        this._membersNeedLoading = true;
+        this.currentState.markOutOfBandMembersFailed();
         throw err;  //rethrow so calling code is aware operation failed
     }
-    this.currentState.setLazyLoadedMembers(members);
-    this.emit('Room', this);
+    const events = eventPojos.map(this.client.getEventMapper());
+    this.currentState.setOutOfBandMembers(events);
 };
 
 /**