Merge pull request #261 from vector-im/bwindels/fix-192

Don't consider lazy-load member events for room membership changes
This commit is contained in:
Bruno Windels 2021-03-05 19:16:58 +00:00 committed by GitHub
commit 8791c0bf9c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 449 additions and 107 deletions

View File

@ -517,7 +517,7 @@ export function tests() {
}
}
};
const newSessionData = await session.writeSync({next_batch: "b"}, 6, syncTxn);
const newSessionData = await session.writeSync({next_batch: "b"}, 6, null, syncTxn, {});
assert(syncSet);
assert.equal(session.syncToken, "a");
assert.equal(session.syncFilterId, 5);

View File

@ -14,15 +14,16 @@ See the License for the specific language governing permissions and
limitations under the License.
*/
import {BaseLRUCache} from "../../../../utils/LRUCache.js";
const DEFAULT_CACHE_SIZE = 10;
/**
* Cache of unpickled inbound megolm session.
*/
export class SessionCache {
constructor(size) {
this._size = typeof size === "number" ? size : DEFAULT_CACHE_SIZE;
this._sessions = [];
export class SessionCache extends BaseLRUCache {
constructor(limit) {
limit = typeof limit === "number" ? limit : DEFAULT_CACHE_SIZE;
super(limit);
}
/**
@ -32,37 +33,28 @@ export class SessionCache {
* @return {SessionInfo?}
*/
get(roomId, senderKey, sessionId) {
const idx = this._sessions.findIndex(s => {
return this._get(s => {
return s.roomId === roomId &&
s.senderKey === senderKey &&
sessionId === s.session.session_id();
sessionId === s.sessionId;
});
if (idx !== -1) {
const sessionInfo = this._sessions[idx];
// move to top
if (idx > 0) {
this._sessions.splice(idx, 1);
this._sessions.unshift(sessionInfo);
}
return sessionInfo;
}
}
add(sessionInfo) {
sessionInfo.retain();
// add new at top
this._sessions.unshift(sessionInfo);
if (this._sessions.length > this._size) {
// free sessions we're about to remove
for (let i = this._size; i < this._sessions.length; i += 1) {
this._sessions[i].release();
}
this._sessions = this._sessions.slice(0, this._size);
}
this._set(sessionInfo, s => {
return s.roomId === sessionInfo.roomId &&
s.senderKey === sessionInfo.senderKey &&
s.sessionId === sessionInfo.sessionId;
});
}
_onEvictEntry(sessionInfo) {
sessionInfo.release();
}
dispose() {
for (const sessionInfo of this._sessions) {
for (const sessionInfo of this._entries) {
sessionInfo.release();
}
}

View File

@ -27,6 +27,10 @@ export class SessionInfo {
this._refCounter = 0;
}
get sessionId() {
return this.session?.session_id();
}
retain() {
this._refCounter += 1;
}

View File

@ -266,6 +266,9 @@ export class Room extends EventEmitter {
// fetch new members while we have txn open,
// but don't make any in-memory changes yet
let heroChanges;
// if any hero changes their display name, the summary in the room response
// is also updated, which will trigger a RoomSummary update
// and make summaryChanges non-falsy here
if (summaryChanges?.needsHeroes) {
// room name disappeared, open heroes
if (!this._heroes) {

View File

@ -24,7 +24,7 @@ export class RoomMember {
}
static fromMemberEvent(roomId, memberEvent) {
const userId = memberEvent && memberEvent.state_key;
const userId = memberEvent?.state_key;
if (typeof userId !== "string") {
return;
}
@ -103,36 +103,34 @@ export class RoomMember {
serialize() {
return this._data;
}
equals(other) {
const data = this._data;
const otherData = other._data;
return data.roomId === otherData.roomId &&
data.userId === otherData.userId &&
data.membership === otherData.membership &&
data.displayName === otherData.displayName &&
data.avatarUrl === otherData.avatarUrl;
}
}
export class MemberChange {
constructor(roomId, memberEvent) {
this._roomId = roomId;
this._memberEvent = memberEvent;
this._member = null;
}
get member() {
if (!this._member) {
this._member = RoomMember.fromMemberEvent(this._roomId, this._memberEvent);
}
return this._member;
constructor(member, previousMembership) {
this.member = member;
this.previousMembership = previousMembership;
}
get roomId() {
return this._roomId;
return this.member.roomId;
}
get userId() {
return this._memberEvent.state_key;
}
get previousMembership() {
return getPrevContentFromStateEvent(this._memberEvent)?.membership;
return this.member.userId;
}
get membership() {
return this._memberEvent.content?.membership;
return this.member.membership;
}
get hasLeft() {

View File

@ -112,10 +112,10 @@ export class GapWriter {
const event = events[i];
key = key.nextKeyForDirection(direction);
const eventStorageEntry = createEventEntry(key, this._roomId, event);
const memberData = this._findMemberData(event.sender, state, events, i, direction);
if (memberData) {
eventStorageEntry.displayName = memberData?.displayName;
eventStorageEntry.avatarUrl = memberData?.avatarUrl;
const member = this._findMember(event.sender, state, events, i, direction);
if (member) {
eventStorageEntry.displayName = member.displayName;
eventStorageEntry.avatarUrl = member.avatarUrl;
}
txn.timelineEvents.insert(eventStorageEntry);
const eventEntry = new EventEntry(eventStorageEntry, this._fragmentIdComparer);
@ -124,7 +124,7 @@ export class GapWriter {
return entries;
}
_findMemberData(userId, state, events, index, direction) {
_findMember(userId, state, events, index, direction) {
function isOurUser(event) {
return event.type === MEMBER_EVENT_TYPE && event.state_key === userId;
}
@ -133,7 +133,7 @@ export class GapWriter {
for (let i = index + inc; i >= 0 && i < events.length; i += inc) {
const event = events[i];
if (isOurUser(event)) {
return RoomMember.fromMemberEvent(this._roomId, event)?.serialize();
return RoomMember.fromMemberEvent(this._roomId, event);
}
}
// look into newer events, but using prev_content if found.
@ -143,14 +143,14 @@ export class GapWriter {
for (let i = index; i >= 0 && i < events.length; i -= inc) {
const event = events[i];
if (isOurUser(event)) {
return RoomMember.fromReplacingMemberEvent(this._roomId, event)?.serialize();
return RoomMember.fromReplacingMemberEvent(this._roomId, event);
}
}
// assuming the member hasn't changed within the chunk, just take it from state if it's there.
// Don't assume state is set though, as it can be empty at the top of the timeline in some circumstances
const stateMemberEvent = state?.find(isOurUser);
if (stateMemberEvent) {
return RoomMember.fromMemberEvent(this._roomId, stateMemberEvent)?.serialize();
return RoomMember.fromMemberEvent(this._roomId, stateMemberEvent);
}
}

View File

@ -0,0 +1,226 @@
/*
Copyright 2021 The Matrix.org Foundation C.I.C.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
import {MemberChange, RoomMember, EVENT_TYPE as MEMBER_EVENT_TYPE} from "../../members/RoomMember.js";
import {LRUCache} from "../../../../utils/LRUCache.js";
export class MemberWriter {
constructor(roomId) {
this._roomId = roomId;
this._cache = new LRUCache(5, member => member.userId);
}
writeTimelineMemberEvent(event, txn) {
return this._writeMemberEvent(event, false, txn);
}
writeStateMemberEvent(event, isLimited, txn) {
// member events in the state section when the room response
// is not limited must always be lazy loaded members.
// If they are not, they will be repeated in the timeline anyway.
return this._writeMemberEvent(event, !isLimited, txn);
}
async _writeMemberEvent(event, isLazyLoadingMember, txn) {
const userId = event.state_key;
if (!userId) {
return;
}
const member = RoomMember.fromMemberEvent(this._roomId, event);
if (!member) {
return;
}
let existingMember = this._cache.get(userId);
if (!existingMember) {
const memberData = await txn.roomMembers.get(this._roomId, userId);
if (memberData) {
existingMember = new RoomMember(memberData);
}
}
// either never heard of the member, or something changed
if (!existingMember || !existingMember.equals(member)) {
txn.roomMembers.set(member.serialize());
this._cache.set(member);
// we also return a member change for lazy loading members if something changed,
// so when the dupe timeline event comes and it doesn't see a diff
// with the cache, we already returned the event here.
//
// it's just important that we don't consider the first LL event
// for a user we see as a membership change, or we'll share keys with
// them, etc...
if (isLazyLoadingMember && !existingMember) {
// we don't have a previous member, but we know this is not a
// membership change as it's a lazy loaded
// member so take the membership from the member
return new MemberChange(member, member.membership);
}
return new MemberChange(member, existingMember?.membership);
}
}
async lookupMember(userId, timelineEvents, txn) {
let member = this._cache.get(userId);
if (!member) {
const memberData = await txn.roomMembers.get(this._roomId, userId);
if (memberData) {
member = new RoomMember(memberData);
this._cache.set(member);
}
}
if (!member) {
// sometimes the member event isn't included in state, but rather in the timeline,
// even if it is not the first event in the timeline. In this case, go look for the
// first occurence
const memberEvent = timelineEvents.find(e => {
return e.type === MEMBER_EVENT_TYPE && e.state_key === userId;
});
if (memberEvent) {
member = RoomMember.fromMemberEvent(this._roomId, memberEvent);
// adding it to the cache, but not storing it for now;
// we'll do that when we get to the event
this._cache.set(member);
}
}
return member;
}
}
export function tests() {
function createMemberEvent(membership, userId, displayName, avatarUrl) {
return {
content: {
membership,
"displayname": displayName,
"avatar_url": avatarUrl
},
sender: userId,
"state_key": userId,
type: "m.room.member"
};
}
function createStorage(initialMembers = []) {
const members = new Map();
for (const m of initialMembers) {
members.set(m.userId, m);
}
return {
members,
roomMembers: {
async get(_, userId) {
return members.get(userId);
},
set(member) {
members.set(member.userId, member);
}
}
}
}
function member(...args) {
return RoomMember.fromMemberEvent(roomId, createMemberEvent.apply(null, args));
}
const roomId = "abc";
const alice = "@alice:hs.tld";
const avatar = "mxc://hs.tld/def";
return {
"new join through state": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage();
const change = await writer.writeStateMemberEvent(createMemberEvent("join", alice), true, txn);
assert(change.hasJoined);
assert.equal(txn.members.get(alice).membership, "join");
},
"accept invite through state": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage([member("invite", alice)]);
const change = await writer.writeStateMemberEvent(createMemberEvent("join", alice), true, txn);
assert.equal(change.previousMembership, "invite");
assert(change.hasJoined);
assert.equal(txn.members.get(alice).membership, "join");
},
"change display name through timeline": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage([member("join", alice, "Alice")]);
const change = await writer.writeTimelineMemberEvent(createMemberEvent("join", alice, "Alies"), txn);
assert(!change.hasJoined);
assert.equal(change.member.displayName, "Alies");
assert.equal(txn.members.get(alice).displayName, "Alies");
},
"set avatar through timeline": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage([member("join", alice, "Alice")]);
const change = await writer.writeTimelineMemberEvent(createMemberEvent("join", alice, "Alice", avatar), txn);
assert(!change.hasJoined);
assert.equal(change.member.avatarUrl, avatar);
assert.equal(txn.members.get(alice).avatarUrl, avatar);
},
"ignore redundant member event": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage([member("join", alice, "Alice", avatar)]);
const change = await writer.writeTimelineMemberEvent(createMemberEvent("join", alice, "Alice", avatar), txn);
assert(!change);
},
"leave": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage([member("join", alice, "Alice")]);
const change = await writer.writeTimelineMemberEvent(createMemberEvent("leave", alice, "Alice"), txn);
assert(change.hasLeft);
assert(!change.hasJoined);
},
"ban": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage([member("join", alice, "Alice")]);
const change = await writer.writeTimelineMemberEvent(createMemberEvent("ban", alice, "Alice"), txn);
assert(change.hasLeft);
assert(!change.hasJoined);
},
"reject invite": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage([member("invite", alice, "Alice")]);
const change = await writer.writeTimelineMemberEvent(createMemberEvent("leave", alice, "Alice"), txn);
assert(!change.hasLeft);
assert(!change.hasJoined);
},
"lazy loaded member we already know about doens't return change": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage([member("join", alice, "Alice")]);
const change = await writer.writeStateMemberEvent(createMemberEvent("join", alice, "Alice"), false, txn);
assert(!change);
},
"lazy loaded member we already know about changes display name": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage([member("join", alice, "Alice")]);
const change = await writer.writeStateMemberEvent(createMemberEvent("join", alice, "Alies"), false, txn);
assert.equal(change.member.displayName, "Alies");
},
"unknown lazy loaded member returns change, but not considered a membership change": async assert => {
const writer = new MemberWriter(roomId);
const txn = createStorage();
const change = await writer.writeStateMemberEvent(createMemberEvent("join", alice, "Alice"), false, txn);
assert(!change.hasJoined);
assert(!change.hasLeft);
assert.equal(change.member.membership, "join");
assert.equal(txn.members.get(alice).displayName, "Alice");
},
};
}

View File

@ -1,5 +1,6 @@
/*
Copyright 2020 Bruno Windels <bruno@windels.cloud>
Copyright 2020 The Matrix.org Foundation C.I.C.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
@ -18,7 +19,8 @@ import {EventKey} from "../EventKey.js";
import {EventEntry} from "../entries/EventEntry.js";
import {FragmentBoundaryEntry} from "../entries/FragmentBoundaryEntry.js";
import {createEventEntry} from "./common.js";
import {MemberChange, RoomMember, EVENT_TYPE as MEMBER_EVENT_TYPE} from "../../members/RoomMember.js";
import {EVENT_TYPE as MEMBER_EVENT_TYPE} from "../../members/RoomMember.js";
import {MemberWriter} from "./MemberWriter.js";
// Synapse bug? where the m.room.create event appears twice in sync response
// when first syncing the room
@ -37,6 +39,7 @@ function deduplicateEvents(events) {
export class SyncWriter {
constructor({roomId, fragmentIdComparer}) {
this._roomId = roomId;
this._memberWriter = new MemberWriter(roomId);
this._fragmentIdComparer = fragmentIdComparer;
this._lastLiveKey = null;
}
@ -130,37 +133,19 @@ export class SyncWriter {
return currentKey;
}
_writeMember(event, txn) {
const userId = event.state_key;
if (userId) {
const memberChange = new MemberChange(this._roomId, event);
const {member} = memberChange;
if (member) {
// TODO: can we avoid writing redundant members here by checking
// if this is not a limited sync and the state is not in the timeline?
txn.roomMembers.set(member.serialize());
return memberChange;
}
}
}
_writeStateEvent(event, txn) {
if (event.type === MEMBER_EVENT_TYPE) {
return this._writeMember(event, txn);
} else {
txn.roomState.set(this._roomId, event);
}
}
_writeStateEvents(roomResponse, memberChanges, txn, log) {
async _writeStateEvents(roomResponse, memberChanges, isLimited, txn, log) {
// persist state
const {state} = roomResponse;
if (Array.isArray(state?.events)) {
log.set("stateEvents", state.events.length);
for (const event of state.events) {
const memberChange = this._writeStateEvent(event, txn);
if (memberChange) {
memberChanges.set(memberChange.userId, memberChange);
if (event.type === MEMBER_EVENT_TYPE) {
const memberChange = await this._memberWriter.writeStateMemberEvent(event, isLimited, txn);
if (memberChange) {
memberChanges.set(memberChange.userId, memberChange);
}
} else {
txn.roomState.set(this._roomId, event);
}
}
}
@ -177,20 +162,26 @@ export class SyncWriter {
// store event in timeline
currentKey = currentKey.nextKey();
const entry = createEventEntry(currentKey, this._roomId, event);
let memberData = await this._findMemberData(event.sender, events, txn);
if (memberData) {
entry.displayName = memberData.displayName;
entry.avatarUrl = memberData.avatarUrl;
let member = await this._memberWriter.lookupMember(event.sender, events, txn);
if (member) {
entry.displayName = member.displayName;
entry.avatarUrl = member.avatarUrl;
}
txn.timelineEvents.insert(entry);
entries.push(new EventEntry(entry, this._fragmentIdComparer));
// process live state events first, so new member info is available
// update state events after writing event, so for a member event,
// we only update the member info after having written the member event
// to the timeline, as we want that event to have the old profile info
if (typeof event.state_key === "string") {
timelineStateEventCount += 1;
const memberChange = this._writeStateEvent(event, txn);
if (memberChange) {
memberChanges.set(memberChange.userId, memberChange);
if (event.type === MEMBER_EVENT_TYPE) {
const memberChange = await this._memberWriter.writeTimelineMemberEvent(event, txn);
if (memberChange) {
memberChanges.set(memberChange.userId, memberChange);
}
} else {
txn.roomState.set(this._roomId, event);
}
}
}
@ -199,24 +190,6 @@ export class SyncWriter {
return currentKey;
}
async _findMemberData(userId, events, txn) {
// TODO: perhaps add a small cache here?
const memberData = await txn.roomMembers.get(this._roomId, userId);
if (memberData) {
return memberData;
} else {
// sometimes the member event isn't included in state, but rather in the timeline,
// even if it is not the first event in the timeline. In this case, go look for the
// first occurence
const memberEvent = events.find(e => {
return e.type === MEMBER_EVENT_TYPE && e.state_key === userId;
});
if (memberEvent) {
return RoomMember.fromMemberEvent(this._roomId, memberEvent)?.serialize();
}
}
}
/**
* @type {SyncWriterResult}
* @property {Array<BaseEntry>} entries new timeline entries written
@ -233,7 +206,7 @@ export class SyncWriter {
const memberChanges = new Map();
// important this happens before _writeTimeline so
// members are available in the transaction
this._writeStateEvents(roomResponse, memberChanges, txn, log);
await this._writeStateEvents(roomResponse, memberChanges, timeline?.limited, txn, log);
const currentKey = await this._writeTimeline(entries, timeline, this._lastLiveKey, memberChanges, txn, log);
log.set("memberChanges", memberChanges.size);
return {entries, newLiveKey: currentKey, memberChanges};

146
src/utils/LRUCache.js Normal file
View File

@ -0,0 +1,146 @@
/*
Copyright 2021 The Matrix.org Foundation C.I.C.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
/**
* Very simple least-recently-used cache implementation
* that should be fast enough for very small cache sizes
*/
export class BaseLRUCache {
constructor(limit) {
this._limit = limit;
this._entries = [];
}
_get(findEntryFn) {
const idx = this._entries.findIndex(findEntryFn);
if (idx !== -1) {
const entry = this._entries[idx];
// move to top
if (idx > 0) {
this._entries.splice(idx, 1);
this._entries.unshift(entry);
}
return entry;
}
}
_set(value, findEntryFn) {
let indexToRemove = this._entries.findIndex(findEntryFn);
this._entries.unshift(value);
if (indexToRemove === -1) {
if (this._entries.length > this._limit) {
indexToRemove = this._entries.length - 1;
}
} else {
// we added the entry at the start since we looked for the index
indexToRemove += 1;
}
if (indexToRemove !== -1) {
this._onEvictEntry(this._entries[indexToRemove]);
this._entries.splice(indexToRemove, 1);
}
}
_onEvictEntry() {}
}
export class LRUCache extends BaseLRUCache {
constructor(limit, keyFn) {
super(limit);
this._keyFn = keyFn;
}
get(key) {
return this._get(e => this._keyFn(e) === key);
}
set(value) {
const key = this._keyFn(value);
this._set(value, e => this._keyFn(e) === key);
}
}
export function tests() {
return {
"can retrieve added entries": assert => {
const cache = new LRUCache(2, e => e.id);
cache.set({id: 1, name: "Alice"});
cache.set({id: 2, name: "Bob"});
assert.equal(cache.get(1).name, "Alice");
assert.equal(cache.get(2).name, "Bob");
},
"first entry is evicted first": assert => {
const cache = new LRUCache(2, e => e.id);
cache.set({id: 1, name: "Alice"});
cache.set({id: 2, name: "Bob"});
cache.set({id: 3, name: "Charly"});
assert.equal(cache.get(1), undefined);
assert.equal(cache.get(2).name, "Bob");
assert.equal(cache.get(3).name, "Charly");
assert.equal(cache._entries.length, 2);
},
"second entry is evicted if first is requested": assert => {
const cache = new LRUCache(2, e => e.id);
cache.set({id: 1, name: "Alice"});
cache.set({id: 2, name: "Bob"});
cache.get(1);
cache.set({id: 3, name: "Charly"});
assert.equal(cache.get(1).name, "Alice");
assert.equal(cache.get(2), undefined);
assert.equal(cache.get(3).name, "Charly");
assert.equal(cache._entries.length, 2);
},
"setting an entry twice removes the first": assert => {
const cache = new LRUCache(2, e => e.id);
cache.set({id: 1, name: "Alice"});
cache.set({id: 2, name: "Bob"});
cache.set({id: 1, name: "Al Ice"});
cache.set({id: 3, name: "Charly"});
assert.equal(cache.get(1).name, "Al Ice");
assert.equal(cache.get(2), undefined);
assert.equal(cache.get(3).name, "Charly");
assert.equal(cache._entries.length, 2);
},
"evict callback is called": assert => {
let evictions = 0;
class CustomCache extends LRUCache {
_onEvictEntry(entry) {
assert.equal(entry.name, "Alice");
evictions += 1;
}
}
const cache = new CustomCache(2, e => e.id);
cache.set({id: 1, name: "Alice"});
cache.set({id: 2, name: "Bob"});
cache.set({id: 3, name: "Charly"});
assert.equal(evictions, 1);
},
"evict callback is called when replacing entry with same identity": assert => {
let evictions = 0;
class CustomCache extends LRUCache {
_onEvictEntry(entry) {
assert.equal(entry.name, "Alice");
evictions += 1;
}
}
const cache = new CustomCache(2, e => e.id);
cache.set({id: 1, name: "Alice"});
cache.set({id: 1, name: "Bob"});
assert.equal(evictions, 1);
},
};
}