Lazy load room members - Part I #667

bwindels · 2018-07-10T15:47:39Z

Lazy loading of members, for now using the /joined_members endpoint until we have something better.

prototype lazily loaded members with /joined_members
pull code that assumes RoomMember.events... into RoomMember so it can transparently take lazily loaded members into account.
return fake sentinels with just user id when users haven't been loaded yet, better than breaking message continuation.
timeline reset should take into account lazily loaded members of previous timeline
use /members endpoint
fix rooms not visible on initial sync where user wasn't active recently
write/fix unit tests

Parent issue: element-hq/element-web#6611

emit individual RoomState.members/newMember events for each lazily loaded member as batch events are not a thing. This makes updating the memberlist work

… available

so MemberInfo can use it and take lazy loading into account

t3chguy · 2018-07-16T09:11:14Z

src/models/room-state.js

@@ -287,7 +321,7 @@ RoomState.prototype.getLastModifiedTime = function() {
 /**
 * Get user IDs with the specified display name.
 * @param {string} displayName The display name to get user IDs from.
- * @return {string[]} An array of user IDs or an empty array.
+ * @return {string[]} An array of user IDs or an empty array. 


seems like a spurious trailing whitespace was added for no reason?

for all timelines in all timeline sets

dont just rely on member events, but just copy the member

better than braking timeline continuation

In order for the lazy loading logic not to bleed into all corners of the JS SDK, I moved some of the state copying between timelines over to the RoomState and EventTimeLine class.

bwindels · 2018-07-17T16:29:36Z

want to do a general review if it's ready to merge, but all should be here for a first PR.
Note that the react SDK code won't call into lazy loading code if the feature is not enabled.

krombel

You need to fix the tests

krombel · 2018-07-18T16:02:59Z

src/models/room.js

+ */
+Room.prototype.setLazilyLoadedMembers = async function(joinedMembersPromise) {
+    this._membersNeedLoading = false;
+    const members = await joinedMembersPromise;


You might want to catch the case that this promise fails - e.g. when something is weird with the connection or server

Good point!

krombel · 2018-07-18T16:04:48Z

src/models/room.js

+    this._membersNeedLoading = false;
+    const members = await joinedMembersPromise;
+    //wait 10 seconds
+    await new Promise((resolve) => setTimeout(resolve, 5000));


Why should this wait? await joinedMembersPromise should make sure that it only continues executing once it is (successfully) done.
Besides that "5000" != 10 seconds ;)

…g flag

getSentinelMember now does return a member (with just the userid) when there is no corresponding member yet. With lazy loading it's perfectly possible the member is not available, and null breaks continuation in the timeline.

bwindels · 2018-07-20T11:49:59Z

@dbkr Right, the only edge case I can think off is when you'd back paginate before the LL members are loaded, but that's probably avoidable.

Adding support for lazy loading in back-pagination is both the plan for client and server before releasing this, see element-hq/element-web#6611 (adding this to the description here as well), so you're right we don't really need to correct any other room state but the forward, live one. I'll change it back.

Added a big XXX corners cut comment where we call /joined_members 👍

…meline since back-paginating will also support lazy loading the state needed to display that part of the timeline, and no user interaction is supposed to happen before the lazy loaded member are, well, loaded, applying the ll members to all timelines should not be neccessary.

when cloning the state, lazy loaded members are copied over with their rawDisplayName, which could originate from their userId if they don't have a displayname. the displayname algorithm would assume that the displayname is explicitly set, and see if we'd have to disambiguate. As a fix, if the display name is the same as the id, just return the id

bwindels · 2018-07-20T15:49:16Z

@dbkr @t3chguy ready for another look

dbkr · 2018-07-24T10:39:40Z

Yeah, this feels a lot safer. Once we have a /members API that can get state for any given point, it might make sense to track the membersNeedLoading per roomState so we can also load member lists for these historical states. This can come later though.

bwindels · 2018-07-24T11:44:31Z

Just had a look at the /members endpoint, and as it returns state events, not just profile information, some of the changes here could probably be avoided. So I'll make the change as part of this PR.

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.

…ership to determine where a room should show up in the room list, we need to know our membership type. But with lazy loading, we might not have our own member if we weren't recently active in the room. Using getSyncedMembership can be used to fallback if the users membership is not yet available.

bwindels · 2018-07-25T14:26:03Z

@dbkr ready for another look. This now includes calling /members instead of /joined_members

dbkr

OK - looks plausible: let's give it a go on the feature branch

bwindels added 4 commits July 10, 2018 17:15

add setter on RoomMember to update from lazily loaded member

32651da

Support for updating members in RoomState from lazily loaded members

a825ea0

prototype support for lazily loading members in matrixclient

8182777

emit individual events for lazily loaded members

24a2aaa

emit individual RoomState.members/newMember events for each lazily loaded member as batch events are not a thing. This makes updating the memberlist work

bwindels added the in progress label Jul 10, 2018

bwindels added 5 commits July 12, 2018 19:34

preserve member state event if available when lazy loading members

104ce78

keep is_direct checks inside RoomMember as events.member might not be…

0e49ff2

… available

return correct invite sender in case of a join

4224630

use more consistent naming

3614173

make method to get mxc avatar url public

202c131

so MemberInfo can use it and take lazy loading into account

bwindels mentioned this pull request Jul 16, 2018

Lazy load member list element-hq/element-web#6611

Closed

22 tasks

t3chguy reviewed Jul 16, 2018

View reviewed changes

bwindels added 2 commits July 16, 2018 12:45

set lazily loaded members on all RoomStates of a room

6165dfd

for all timelines in all timeline sets

make sentinels lazy loading compatible

6d017b1

dont just rely on member events, but just copy the member

bwindels mentioned this pull request Jul 16, 2018

Lazy loading room members matrix-org/matrix-react-sdk#2050

Closed

5 tasks

bwindels added 3 commits July 16, 2018 15:36

fix whitespace

e2c6eea

return sentinels with userid if members haven't been loaded yet,

0d55d4b

better than braking timeline continuation

make resetting the live timeline work with lazily loaded members

53e1203

In order for the lazy loading logic not to bleed into all corners of the JS SDK, I moved some of the state copying between timelines over to the RoomState and EventTimeLine class.

bwindels added 2 commits July 18, 2018 12:40

fix lint errors

eef07d5

fix one lint warning too many

ac07212

bwindels mentioned this pull request Jul 18, 2018

Lazy load room members - Part I matrix-org/matrix-react-sdk#2072

Merged

5 tasks

bwindels removed the in progress label Jul 18, 2018

bwindels requested review from dbkr and ara4n July 18, 2018 13:12

krombel suggested changes Jul 18, 2018

View reviewed changes

bwindels added 4 commits July 19, 2018 09:40

remove left-over test code, oops

6ae2871

improve return type comment

a5ae548

handle failed /joined_members call by logging to console and revertin…

a1776ef

…g flag

Fix tests

604a40e

getSentinelMember now does return a member (with just the userid) when there is no corresponding member yet. With lazy loading it's perfectly possible the member is not available, and null breaks continuation in the timeline.

bwindels added 2 commits July 20, 2018 13:33

actually, comment should be 1 line higher

f5613e5

clarify that we only get joined members for now

cc75b2b

bwindels added 6 commits July 20, 2018 14:05

some tests for room member + state, and some fixes to make them pass

92cb16d

test that modifications to clone'd() room state dont affect the old

d3b5e6d

room lazy loading tests + fix

8b3ded6

fix lint

1001f0f

bwindels removed the in progress label Jul 20, 2018

bwindels mentioned this pull request Jul 23, 2018

Summary api #670

Merged

5 tasks

bwindels added the in progress label Jul 24, 2018

bwindels added 7 commits July 24, 2018 18:55

fix lint & fix and add tests

ae6cf68

fix room not having access to event mapper + tests

89e8801

remove obsolete flag

7223149

move me || syncedmembership code into room.getmymembership

e478421

fix lint

2e8198e

bwindels changed the title ~~Lazy load room members~~ Lazy load room members - Part I Jul 25, 2018

bwindels removed the in progress label Jul 25, 2018

jsdoc doesn't like generic promise type annotations

66ec268

bwindels changed the base branch from develop to bwindels/feature_lazyloading July 25, 2018 15:19

dbkr approved these changes Jul 26, 2018

View reviewed changes

bwindels merged commit f9025c7 into bwindels/feature_lazyloading Jul 26, 2018

bwindels deleted the bwindels/lazy_load_members branch July 26, 2018 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy load room members - Part I #667

Lazy load room members - Part I #667

bwindels commented Jul 10, 2018 •

edited

Loading

t3chguy Jul 16, 2018

bwindels commented Jul 17, 2018

krombel left a comment

krombel Jul 18, 2018

bwindels Jul 19, 2018

krombel Jul 18, 2018

bwindels commented Jul 20, 2018

bwindels commented Jul 20, 2018

dbkr commented Jul 24, 2018

bwindels commented Jul 24, 2018

bwindels commented Jul 25, 2018

dbkr left a comment

Lazy load room members - Part I #667

Lazy load room members - Part I #667

Conversation

bwindels commented Jul 10, 2018 • edited Loading

t3chguy Jul 16, 2018

Choose a reason for hiding this comment

bwindels commented Jul 17, 2018

krombel left a comment

Choose a reason for hiding this comment

krombel Jul 18, 2018

Choose a reason for hiding this comment

bwindels Jul 19, 2018

Choose a reason for hiding this comment

krombel Jul 18, 2018

Choose a reason for hiding this comment

bwindels commented Jul 20, 2018

bwindels commented Jul 20, 2018

dbkr commented Jul 24, 2018

bwindels commented Jul 24, 2018

bwindels commented Jul 25, 2018

dbkr left a comment

Choose a reason for hiding this comment

bwindels commented Jul 10, 2018 •

edited

Loading