From 0b6f744a58b218d5b3ca1cae573d01ecf92428ca Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 3 Jul 2020 15:36:04 -0600 Subject: [PATCH 1/7] Wrap handleRoomUpdate in a lock Dev note: this is removed in a later commit --- src/stores/room-list/algorithms/Algorithm.ts | 118 ++++++++++--------- 1 file changed, 63 insertions(+), 55 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 36abf86975..b6e2a7c1f6 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -33,6 +33,7 @@ import { FILTER_CHANGED, FilterPriority, IFilterCondition } from "../filters/IFi import { EffectiveMembership, getEffectiveMembership, splitRoomsByMembership } from "../membership"; import { OrderingAlgorithm } from "./list-ordering/OrderingAlgorithm"; import { getListAlgorithmInstance } from "./list-ordering"; +import AwaitLock from "await-lock"; // TODO: Add locking support to avoid concurrent writes? https://github.com/vector-im/riot-web/issues/14235 @@ -66,6 +67,7 @@ export class Algorithm extends EventEmitter { } = {}; private allowedByFilter: Map = new Map(); private allowedRoomsByFilters: Set = new Set(); + private readonly lock = new AwaitLock(); public constructor() { super(); @@ -607,67 +609,73 @@ export class Algorithm extends EventEmitter { * processing. */ public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise { - if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from"); + try { + await this.lock.acquireAsync(); - if (cause === RoomUpdateCause.NewRoom) { - const roomTags = this.roomIdsToTags[room.roomId]; - if (roomTags && roomTags.length > 0) { - console.warn(`${room.roomId} is reportedly new but is already known - assuming TagChange instead`); - cause = RoomUpdateCause.PossibleTagChange; + if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from"); + + if (cause === RoomUpdateCause.NewRoom) { + const roomTags = this.roomIdsToTags[room.roomId]; + if (roomTags && roomTags.length > 0) { + console.warn(`${room.roomId} is reportedly new but is already known - assuming TagChange instead`); + cause = RoomUpdateCause.PossibleTagChange; + } } - } - if (cause === RoomUpdateCause.PossibleTagChange) { - // TODO: Be smarter and splice rather than regen the planet. https://github.com/vector-im/riot-web/issues/14035 - // TODO: No-op if no change. https://github.com/vector-im/riot-web/issues/14035 - await this.setKnownRooms(this.rooms); - return true; - } + if (cause === RoomUpdateCause.PossibleTagChange) { + // TODO: Be smarter and splice rather than regen the planet. https://github.com/vector-im/riot-web/issues/14035 + // TODO: No-op if no change. https://github.com/vector-im/riot-web/issues/14035 + await this.setKnownRooms(this.rooms); + return true; + } - // If the update is for a room change which might be the sticky room, prevent it. We - // need to make sure that the causes (NewRoom and RoomRemoved) are still triggered though - // as the sticky room relies on this. - if (cause !== RoomUpdateCause.NewRoom && cause !== RoomUpdateCause.RoomRemoved) { - if (this.stickyRoom === room) { - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.warn(`[RoomListDebug] Received ${cause} update for sticky room ${room.roomId} - ignoring`); + // If the update is for a room change which might be the sticky room, prevent it. We + // need to make sure that the causes (NewRoom and RoomRemoved) are still triggered though + // as the sticky room relies on this. + if (cause !== RoomUpdateCause.NewRoom && cause !== RoomUpdateCause.RoomRemoved) { + if (this.stickyRoom === room) { + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.warn(`[RoomListDebug] Received ${cause} update for sticky room ${room.roomId} - ignoring`); + return false; + } + } + + if (cause === RoomUpdateCause.NewRoom && !this.roomIdsToTags[room.roomId]) { + console.log(`[RoomListDebug] Updating tags for new room ${room.roomId} (${room.name})`); + + // Get the tags for the room and populate the cache + const roomTags = this.getTagsForRoom(room).filter(t => !isNullOrUndefined(this.cachedRooms[t])); + + // "This should never happen" condition - we specify DefaultTagID.Untagged in getTagsForRoom(), + // which means we should *always* have a tag to go off of. + if (!roomTags.length) throw new Error(`Tags cannot be determined for ${room.roomId}`); + + this.roomIdsToTags[room.roomId] = roomTags; + } + + let tags = this.roomIdsToTags[room.roomId]; + if (!tags) { + console.warn(`No tags known for "${room.name}" (${room.roomId})`); return false; } + + let changed = false; + for (const tag of tags) { + const algorithm: OrderingAlgorithm = this.algorithms[tag]; + if (!algorithm) throw new Error(`No algorithm for ${tag}`); + + await algorithm.handleRoomUpdate(room, cause); + this.cachedRooms[tag] = algorithm.orderedRooms; + + // Flag that we've done something + this.recalculateFilteredRoomsForTag(tag); // update filter to re-sort the list + this.recalculateStickyRoom(tag); // update sticky room to make sure it appears if needed + changed = true; + } + + return true; + } finally { + this.lock.release(); } - - if (cause === RoomUpdateCause.NewRoom && !this.roomIdsToTags[room.roomId]) { - console.log(`[RoomListDebug] Updating tags for new room ${room.roomId} (${room.name})`); - - // Get the tags for the room and populate the cache - const roomTags = this.getTagsForRoom(room).filter(t => !isNullOrUndefined(this.cachedRooms[t])); - - // "This should never happen" condition - we specify DefaultTagID.Untagged in getTagsForRoom(), - // which means we should *always* have a tag to go off of. - if (!roomTags.length) throw new Error(`Tags cannot be determined for ${room.roomId}`); - - this.roomIdsToTags[room.roomId] = roomTags; - } - - let tags = this.roomIdsToTags[room.roomId]; - if (!tags) { - console.warn(`No tags known for "${room.name}" (${room.roomId})`); - return false; - } - - let changed = false; - for (const tag of tags) { - const algorithm: OrderingAlgorithm = this.algorithms[tag]; - if (!algorithm) throw new Error(`No algorithm for ${tag}`); - - await algorithm.handleRoomUpdate(room, cause); - this.cachedRooms[tag] = algorithm.orderedRooms; - - // Flag that we've done something - this.recalculateFilteredRoomsForTag(tag); // update filter to re-sort the list - this.recalculateStickyRoom(tag); // update sticky room to make sure it appears if needed - changed = true; - } - - return true; } } From 18df29b62717620025bd1175206b9208228a1cf8 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 3 Jul 2020 15:36:17 -0600 Subject: [PATCH 2/7] Flag & add some debugging --- src/stores/room-list/algorithms/Algorithm.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index b6e2a7c1f6..254c75f055 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -641,11 +641,15 @@ export class Algorithm extends EventEmitter { } if (cause === RoomUpdateCause.NewRoom && !this.roomIdsToTags[room.roomId]) { + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[RoomListDebug] Updating tags for new room ${room.roomId} (${room.name})`); // Get the tags for the room and populate the cache const roomTags = this.getTagsForRoom(room).filter(t => !isNullOrUndefined(this.cachedRooms[t])); + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`[RoomListDebug] Updated tags for ${room.roomId}:`, roomTags); + // "This should never happen" condition - we specify DefaultTagID.Untagged in getTagsForRoom(), // which means we should *always* have a tag to go off of. if (!roomTags.length) throw new Error(`Tags cannot be determined for ${room.roomId}`); From 4345f972e0bbe19ccd23e9b6c2f753838b3869d5 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Jul 2020 17:49:18 -0600 Subject: [PATCH 3/7] Handle sticky room to avoid accidental removal Plus a bunch of logging. This fixes a case where switching rooms would cause the last room you were on to disappear due to an optimization where known NewRoom fires would be translated to tag change fires, which wouldn't re-add the room to the underlying tag algorithm. By tracking the last sticky room, we can identify when we're about to do this and avoid it. This commit also adds a check to ensure that we have the latest reference of a room stored as rooms changing from invite -> join change references. This commit additionally updates the PossibleTagChange handling to be faster and smarter, leading to a more stable generation of the room list. We convert the update cause to a Timeline update in order to indicate it is a change within the same tag rather than having to jump tags. This also means that PossibleTagChange should no longer make it as far as the underlying algorithm. New logging has also been added to aid debugging. --- src/stores/room-list/algorithms/Algorithm.ts | 87 ++++++++++++++++---- 1 file changed, 72 insertions(+), 15 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 254c75f055..51ab98fb0f 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -18,7 +18,7 @@ import { Room } from "matrix-js-sdk/src/models/room"; import { isNullOrUndefined } from "matrix-js-sdk/src/utils"; import DMRoomMap from "../../../utils/DMRoomMap"; import { EventEmitter } from "events"; -import { arrayHasDiff, ArrayUtil } from "../../../utils/arrays"; +import { arrayDiff, arrayHasDiff, ArrayUtil } from "../../../utils/arrays"; import { getEnumValues } from "../../../utils/enums"; import { DefaultTagID, RoomUpdateCause, TagID } from "../models"; import { @@ -58,6 +58,7 @@ export class Algorithm extends EventEmitter { private _cachedStickyRooms: ITagMap = {}; // a clone of the _cachedRooms, with the sticky room private filteredRooms: ITagMap = {}; private _stickyRoom: IStickyRoom = null; + private _lastStickyRoom: IStickyRoom = null; private sortAlgorithms: ITagSortingMap; private listAlgorithms: IListOrderingMap; private algorithms: IOrderingAlgorithmMap; @@ -172,6 +173,7 @@ export class Algorithm extends EventEmitter { if (this._stickyRoom) { const stickyRoom = this._stickyRoom.room; this._stickyRoom = null; // clear before we go to update the algorithm + this._lastStickyRoom = stickyRoom; // Lie to the algorithm and re-add the room to the algorithm await this.handleRoomUpdate(stickyRoom, RoomUpdateCause.NewRoom); @@ -198,7 +200,8 @@ export class Algorithm extends EventEmitter { // the same thing it no-ops. After we're done calling the algorithm, we'll issue // a new update for ourselves. const lastStickyRoom = this._stickyRoom; - this._stickyRoom = null; + this._stickyRoom = null; // clear before we update the algorithm + this._lastStickyRoom = lastStickyRoom; this.recalculateStickyRoom(); // When we do have the room, re-add the old room (if needed) to the algorithm @@ -562,7 +565,7 @@ export class Algorithm extends EventEmitter { /** * Updates the roomsToTags map */ - protected updateTagsFromCache() { + private updateTagsFromCache() { const newMap = {}; const tags = Object.keys(this.cachedRooms); @@ -609,24 +612,73 @@ export class Algorithm extends EventEmitter { * processing. */ public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise { + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.trace(`Handle room update for ${room.roomId} called with cause ${cause}`); try { await this.lock.acquireAsync(); if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from"); + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`[RoomListDebug] @@ ${room.roomId} with ${cause} for tags ${this.roomIdsToTags[room.roomId]} and sticky is currently ${this._stickyRoom && this._stickyRoom.room ? this._stickyRoom.room.roomId : ''} and last is ${this._lastStickyRoom && this._lastStickyRoom.room ? this._lastStickyRoom.room.roomId : ''}`); + if (cause === RoomUpdateCause.NewRoom) { + const isForLastSticky = this._lastStickyRoom && this._lastStickyRoom.room === room; const roomTags = this.roomIdsToTags[room.roomId]; - if (roomTags && roomTags.length > 0) { + const hasTags = roomTags && roomTags.length > 0; + + // Don't change the cause if the last sticky room is being re-added. If we fail to + // pass the cause through as NewRoom, we'll fail to lie to the algorithm and thus + // lose the room. + if (hasTags && !isForLastSticky) { console.warn(`${room.roomId} is reportedly new but is already known - assuming TagChange instead`); cause = RoomUpdateCause.PossibleTagChange; } + + // If we have tags for a room and don't have the room referenced, the room reference + // probably changed. We need to swap out the problematic reference. + if (hasTags && !this.rooms.includes(room)) { + console.warn(`${room.roomId} is missing from room array but is known - trying to find duplicate`); + this.rooms = this.rooms.map(r => r.roomId === room.roomId ? room : r); + + // Sanity check + if (!this.rooms.includes(room)) { + throw new Error(`Failed to replace ${room.roomId} with an updated reference`); + } + } } if (cause === RoomUpdateCause.PossibleTagChange) { - // TODO: Be smarter and splice rather than regen the planet. https://github.com/vector-im/riot-web/issues/14035 - // TODO: No-op if no change. https://github.com/vector-im/riot-web/issues/14035 - await this.setKnownRooms(this.rooms); - return true; + const oldTags = this.roomIdsToTags[room.roomId] || []; + const newTags = this.getTagsForRoom(room); + const diff = arrayDiff(oldTags, newTags); + if (diff.removed.length > 0 || diff.added.length > 0) { + for (const rmTag of diff.removed) { + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`Removing ${room.roomId} from ${rmTag}`); + const algorithm: OrderingAlgorithm = this.algorithms[rmTag]; + if (!algorithm) throw new Error(`No algorithm for ${rmTag}`); + await algorithm.handleRoomUpdate(room, RoomUpdateCause.RoomRemoved); + } + for (const addTag of diff.added) { + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`Adding ${room.roomId} to ${addTag}`); + const algorithm: OrderingAlgorithm = this.algorithms[addTag]; + if (!algorithm) throw new Error(`No algorithm for ${addTag}`); + await algorithm.handleRoomUpdate(room, RoomUpdateCause.NewRoom); + } + + // Update the tag map so we don't regen it in a moment + this.roomIdsToTags[room.roomId] = newTags; + + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`Changing update cause for ${room.roomId} to TIMELINE to sort rooms`); + cause = RoomUpdateCause.Timeline; + } else { + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.warn(`Received no-op update for ${room.roomId} - changing to TIMELINE update`); + cause = RoomUpdateCause.Timeline; + } } // If the update is for a room change which might be the sticky room, prevent it. We @@ -640,24 +692,27 @@ export class Algorithm extends EventEmitter { } } - if (cause === RoomUpdateCause.NewRoom && !this.roomIdsToTags[room.roomId]) { + if (!this.roomIdsToTags[room.roomId]) { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[RoomListDebug] Updating tags for new room ${room.roomId} (${room.name})`); + console.log(`[RoomListDebug] Updating tags for room ${room.roomId} (${room.name})`); // Get the tags for the room and populate the cache const roomTags = this.getTagsForRoom(room).filter(t => !isNullOrUndefined(this.cachedRooms[t])); - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[RoomListDebug] Updated tags for ${room.roomId}:`, roomTags); - // "This should never happen" condition - we specify DefaultTagID.Untagged in getTagsForRoom(), // which means we should *always* have a tag to go off of. if (!roomTags.length) throw new Error(`Tags cannot be determined for ${room.roomId}`); this.roomIdsToTags[room.roomId] = roomTags; + + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`[RoomListDebug] Updated tags for ${room.roomId}:`, roomTags); } - let tags = this.roomIdsToTags[room.roomId]; + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`[RoomListDebug] Reached algorithmic handling for ${room.roomId} and cause ${cause}`); + + const tags = this.roomIdsToTags[room.roomId]; if (!tags) { console.warn(`No tags known for "${room.name}" (${room.roomId})`); return false; @@ -677,7 +732,9 @@ export class Algorithm extends EventEmitter { changed = true; } - return true; + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`[RoomListDebug] Finished handling ${room.roomId} with cause ${cause} (changed=${changed})`); + return changed; } finally { this.lock.release(); } From dd833f4f2ffd78bfe4199a80ac1294d86536f45d Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Jul 2020 19:32:31 -0600 Subject: [PATCH 4/7] Ensure the sticky room changes if the tag changes This fixes a case where a user accepts an invite, which causes a tag change, but the room stays stuck in the invites list. The tag change additionally gets swallowed when the user moves away, causing the room to get lost. By moving it when we see it, potentially during a sticky room change itself (though extremely rare), we avoid having the room get lost in the wrong lists. A side effect of this is that accepting an invite puts it at the top of the tag it's going to (usually untagged), however this feels like the best option for the user. A rare case of a tag change happening during a sticky room change is when a leave event comes in for the sticky room, but because it's come through as a tag change it can get swallowed. If it does get swallowed and the user clicks away, the tag change will happen when the room is re-introduced to the list (fake NewRoom event). --- src/stores/room-list/algorithms/Algorithm.ts | 65 ++++++++++++++++++-- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 51ab98fb0f..4e06f93359 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -58,7 +58,7 @@ export class Algorithm extends EventEmitter { private _cachedStickyRooms: ITagMap = {}; // a clone of the _cachedRooms, with the sticky room private filteredRooms: ITagMap = {}; private _stickyRoom: IStickyRoom = null; - private _lastStickyRoom: IStickyRoom = null; + private _lastStickyRoom: IStickyRoom = null; // only not-null when changing the sticky room private sortAlgorithms: ITagSortingMap; private listAlgorithms: IListOrderingMap; private algorithms: IOrderingAlgorithmMap; @@ -165,15 +165,26 @@ export class Algorithm extends EventEmitter { } private async updateStickyRoom(val: Room) { + try { + return await this.doUpdateStickyRoom(val); + } finally { + this._lastStickyRoom = null; // clear to indicate we're done changing + } + } + + private async doUpdateStickyRoom(val: Room) { // Note throughout: We need async so we can wait for handleRoomUpdate() to do its thing, // otherwise we risk duplicating rooms. + // Set the last sticky room to indicate that we're in a change. The code throughout the + // class can safely handle a null room, so this should be safe to do as a backup. + this._lastStickyRoom = this._stickyRoom || {}; + // It's possible to have no selected room. In that case, clear the sticky room if (!val) { if (this._stickyRoom) { const stickyRoom = this._stickyRoom.room; this._stickyRoom = null; // clear before we go to update the algorithm - this._lastStickyRoom = stickyRoom; // Lie to the algorithm and re-add the room to the algorithm await this.handleRoomUpdate(stickyRoom, RoomUpdateCause.NewRoom); @@ -183,7 +194,7 @@ export class Algorithm extends EventEmitter { } // When we do have a room though, we expect to be able to find it - const tag = this.roomIdsToTags[val.roomId][0]; + let tag = this.roomIdsToTags[val.roomId][0]; if (!tag) throw new Error(`${val.roomId} does not belong to a tag and cannot be sticky`); // We specifically do NOT use the ordered rooms set as it contains the sticky room, which @@ -201,7 +212,6 @@ export class Algorithm extends EventEmitter { // a new update for ourselves. const lastStickyRoom = this._stickyRoom; this._stickyRoom = null; // clear before we update the algorithm - this._lastStickyRoom = lastStickyRoom; this.recalculateStickyRoom(); // When we do have the room, re-add the old room (if needed) to the algorithm @@ -214,6 +224,22 @@ export class Algorithm extends EventEmitter { // Lie to the algorithm and remove the room from it's field of view await this.handleRoomUpdate(val, RoomUpdateCause.RoomRemoved); + // Check for tag & position changes while we're here. We also check the room to ensure + // it is still the same room. + if (this._stickyRoom) { + if (this._stickyRoom.room !== val) { + // Check the room IDs just in case + if (this._stickyRoom.room.roomId === val.roomId) { + console.warn("Sticky room changed references"); + } else { + throw new Error("Sticky room changed while the sticky room was changing"); + } + } + + tag = this._stickyRoom.tag; + position = this._stickyRoom.position; + } + // Now that we're done lying to the algorithm, we need to update our position // marker only if the user is moving further down the same list. If they're switching // lists, or moving upwards, the position marker will splice in just fine but if @@ -619,6 +645,8 @@ export class Algorithm extends EventEmitter { if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from"); + const isSticky = this._stickyRoom && this._stickyRoom.room === room; + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`[RoomListDebug] @@ ${room.roomId} with ${cause} for tags ${this.roomIdsToTags[room.roomId]} and sticky is currently ${this._stickyRoom && this._stickyRoom.room ? this._stickyRoom.room.roomId : ''} and last is ${this._lastStickyRoom && this._lastStickyRoom.room ? this._lastStickyRoom.room.roomId : ''}`); @@ -637,7 +665,7 @@ export class Algorithm extends EventEmitter { // If we have tags for a room and don't have the room referenced, the room reference // probably changed. We need to swap out the problematic reference. - if (hasTags && !this.rooms.includes(room)) { + if (hasTags && !this.rooms.includes(room) && !isSticky) { console.warn(`${room.roomId} is missing from room array but is known - trying to find duplicate`); this.rooms = this.rooms.map(r => r.roomId === room.roomId ? room : r); @@ -646,9 +674,17 @@ export class Algorithm extends EventEmitter { throw new Error(`Failed to replace ${room.roomId} with an updated reference`); } } + + // Like above, update the reference to the sticky room if we need to + if (hasTags && isSticky) { + // Go directly in and set the sticky room's new reference, being careful not + // to trigger a sticky room update ourselves. + this._stickyRoom.room = room; + } } if (cause === RoomUpdateCause.PossibleTagChange) { + let didTagChange = false; const oldTags = this.roomIdsToTags[room.roomId] || []; const newTags = this.getTagsForRoom(room); const diff = arrayDiff(oldTags, newTags); @@ -674,11 +710,30 @@ export class Algorithm extends EventEmitter { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`Changing update cause for ${room.roomId} to TIMELINE to sort rooms`); cause = RoomUpdateCause.Timeline; + didTagChange = true; } else { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.warn(`Received no-op update for ${room.roomId} - changing to TIMELINE update`); cause = RoomUpdateCause.Timeline; } + + if (didTagChange && isSticky) { + // Manually update the tag for the sticky room without triggering a sticky room + // update. The update will be handled implicitly by the sticky room handling and + // requires no changes on our part, if we're in the middle of a sticky room change. + if (this._lastStickyRoom) { + this._stickyRoom = { + room, + tag: this.roomIdsToTags[room.roomId][0], + position: 0, // right at the top as it changed tags + }; + } else { + // We have to clear the lock as the sticky room change will trigger updates. + this.lock.release(); + await this.setStickyRoomAsync(room); + await this.lock.acquireAsync(); + } + } } // If the update is for a room change which might be the sticky room, prevent it. We From 70e5da677b70171a62052079ce9d5b0918ab38d3 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Jul 2020 19:35:58 -0600 Subject: [PATCH 5/7] Clean up debug logging --- src/stores/room-list/algorithms/Algorithm.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 4e06f93359..930759e68c 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -236,6 +236,9 @@ export class Algorithm extends EventEmitter { } } + console.warn(`Sticky room changed tag & position from ${tag} / ${position} ` + + `to ${this._stickyRoom.tag} / ${this._stickyRoom.position}`); + tag = this._stickyRoom.tag; position = this._stickyRoom.position; } @@ -639,17 +642,13 @@ export class Algorithm extends EventEmitter { */ public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.trace(`Handle room update for ${room.roomId} called with cause ${cause}`); + console.log(`Handle room update for ${room.roomId} called with cause ${cause}`); try { await this.lock.acquireAsync(); if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from"); const isSticky = this._stickyRoom && this._stickyRoom.room === room; - - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[RoomListDebug] @@ ${room.roomId} with ${cause} for tags ${this.roomIdsToTags[room.roomId]} and sticky is currently ${this._stickyRoom && this._stickyRoom.room ? this._stickyRoom.room.roomId : ''} and last is ${this._lastStickyRoom && this._lastStickyRoom.room ? this._lastStickyRoom.room.roomId : ''}`); - if (cause === RoomUpdateCause.NewRoom) { const isForLastSticky = this._lastStickyRoom && this._lastStickyRoom.room === room; const roomTags = this.roomIdsToTags[room.roomId]; @@ -708,12 +707,12 @@ export class Algorithm extends EventEmitter { this.roomIdsToTags[room.roomId] = newTags; // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`Changing update cause for ${room.roomId} to TIMELINE to sort rooms`); + console.log(`Changing update cause for ${room.roomId} to Timeline to sort rooms`); cause = RoomUpdateCause.Timeline; didTagChange = true; } else { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.warn(`Received no-op update for ${room.roomId} - changing to TIMELINE update`); + console.warn(`Received no-op update for ${room.roomId} - changing to Timeline update`); cause = RoomUpdateCause.Timeline; } From 34bd59c1519a57fa0921330db17d049a81c7ed46 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Jul 2020 19:37:57 -0600 Subject: [PATCH 6/7] Remove the lock around the algorithm This isn't needed --- src/stores/room-list/algorithms/Algorithm.ts | 248 +++++++++---------- 1 file changed, 119 insertions(+), 129 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index 930759e68c..d85ff4e2e1 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -33,7 +33,6 @@ import { FILTER_CHANGED, FilterPriority, IFilterCondition } from "../filters/IFi import { EffectiveMembership, getEffectiveMembership, splitRoomsByMembership } from "../membership"; import { OrderingAlgorithm } from "./list-ordering/OrderingAlgorithm"; import { getListAlgorithmInstance } from "./list-ordering"; -import AwaitLock from "await-lock"; // TODO: Add locking support to avoid concurrent writes? https://github.com/vector-im/riot-web/issues/14235 @@ -68,7 +67,6 @@ export class Algorithm extends EventEmitter { } = {}; private allowedByFilter: Map = new Map(); private allowedRoomsByFilters: Set = new Set(); - private readonly lock = new AwaitLock(); public constructor() { super(); @@ -643,154 +641,146 @@ export class Algorithm extends EventEmitter { public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log(`Handle room update for ${room.roomId} called with cause ${cause}`); - try { - await this.lock.acquireAsync(); + if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from"); - if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from"); + const isSticky = this._stickyRoom && this._stickyRoom.room === room; + if (cause === RoomUpdateCause.NewRoom) { + const isForLastSticky = this._lastStickyRoom && this._lastStickyRoom.room === room; + const roomTags = this.roomIdsToTags[room.roomId]; + const hasTags = roomTags && roomTags.length > 0; - const isSticky = this._stickyRoom && this._stickyRoom.room === room; - if (cause === RoomUpdateCause.NewRoom) { - const isForLastSticky = this._lastStickyRoom && this._lastStickyRoom.room === room; - const roomTags = this.roomIdsToTags[room.roomId]; - const hasTags = roomTags && roomTags.length > 0; + // Don't change the cause if the last sticky room is being re-added. If we fail to + // pass the cause through as NewRoom, we'll fail to lie to the algorithm and thus + // lose the room. + if (hasTags && !isForLastSticky) { + console.warn(`${room.roomId} is reportedly new but is already known - assuming TagChange instead`); + cause = RoomUpdateCause.PossibleTagChange; + } - // Don't change the cause if the last sticky room is being re-added. If we fail to - // pass the cause through as NewRoom, we'll fail to lie to the algorithm and thus - // lose the room. - if (hasTags && !isForLastSticky) { - console.warn(`${room.roomId} is reportedly new but is already known - assuming TagChange instead`); - cause = RoomUpdateCause.PossibleTagChange; - } + // If we have tags for a room and don't have the room referenced, the room reference + // probably changed. We need to swap out the problematic reference. + if (hasTags && !this.rooms.includes(room) && !isSticky) { + console.warn(`${room.roomId} is missing from room array but is known - trying to find duplicate`); + this.rooms = this.rooms.map(r => r.roomId === room.roomId ? room : r); - // If we have tags for a room and don't have the room referenced, the room reference - // probably changed. We need to swap out the problematic reference. - if (hasTags && !this.rooms.includes(room) && !isSticky) { - console.warn(`${room.roomId} is missing from room array but is known - trying to find duplicate`); - this.rooms = this.rooms.map(r => r.roomId === room.roomId ? room : r); - - // Sanity check - if (!this.rooms.includes(room)) { - throw new Error(`Failed to replace ${room.roomId} with an updated reference`); - } - } - - // Like above, update the reference to the sticky room if we need to - if (hasTags && isSticky) { - // Go directly in and set the sticky room's new reference, being careful not - // to trigger a sticky room update ourselves. - this._stickyRoom.room = room; + // Sanity check + if (!this.rooms.includes(room)) { + throw new Error(`Failed to replace ${room.roomId} with an updated reference`); } } - if (cause === RoomUpdateCause.PossibleTagChange) { - let didTagChange = false; - const oldTags = this.roomIdsToTags[room.roomId] || []; - const newTags = this.getTagsForRoom(room); - const diff = arrayDiff(oldTags, newTags); - if (diff.removed.length > 0 || diff.added.length > 0) { - for (const rmTag of diff.removed) { - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`Removing ${room.roomId} from ${rmTag}`); - const algorithm: OrderingAlgorithm = this.algorithms[rmTag]; - if (!algorithm) throw new Error(`No algorithm for ${rmTag}`); - await algorithm.handleRoomUpdate(room, RoomUpdateCause.RoomRemoved); - } - for (const addTag of diff.added) { - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`Adding ${room.roomId} to ${addTag}`); - const algorithm: OrderingAlgorithm = this.algorithms[addTag]; - if (!algorithm) throw new Error(`No algorithm for ${addTag}`); - await algorithm.handleRoomUpdate(room, RoomUpdateCause.NewRoom); - } - - // Update the tag map so we don't regen it in a moment - this.roomIdsToTags[room.roomId] = newTags; + // Like above, update the reference to the sticky room if we need to + if (hasTags && isSticky) { + // Go directly in and set the sticky room's new reference, being careful not + // to trigger a sticky room update ourselves. + this._stickyRoom.room = room; + } + } + if (cause === RoomUpdateCause.PossibleTagChange) { + let didTagChange = false; + const oldTags = this.roomIdsToTags[room.roomId] || []; + const newTags = this.getTagsForRoom(room); + const diff = arrayDiff(oldTags, newTags); + if (diff.removed.length > 0 || diff.added.length > 0) { + for (const rmTag of diff.removed) { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`Changing update cause for ${room.roomId} to Timeline to sort rooms`); - cause = RoomUpdateCause.Timeline; - didTagChange = true; + console.log(`Removing ${room.roomId} from ${rmTag}`); + const algorithm: OrderingAlgorithm = this.algorithms[rmTag]; + if (!algorithm) throw new Error(`No algorithm for ${rmTag}`); + await algorithm.handleRoomUpdate(room, RoomUpdateCause.RoomRemoved); + } + for (const addTag of diff.added) { + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`Adding ${room.roomId} to ${addTag}`); + const algorithm: OrderingAlgorithm = this.algorithms[addTag]; + if (!algorithm) throw new Error(`No algorithm for ${addTag}`); + await algorithm.handleRoomUpdate(room, RoomUpdateCause.NewRoom); + } + + // Update the tag map so we don't regen it in a moment + this.roomIdsToTags[room.roomId] = newTags; + + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`Changing update cause for ${room.roomId} to Timeline to sort rooms`); + cause = RoomUpdateCause.Timeline; + didTagChange = true; + } else { + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.warn(`Received no-op update for ${room.roomId} - changing to Timeline update`); + cause = RoomUpdateCause.Timeline; + } + + if (didTagChange && isSticky) { + // Manually update the tag for the sticky room without triggering a sticky room + // update. The update will be handled implicitly by the sticky room handling and + // requires no changes on our part, if we're in the middle of a sticky room change. + if (this._lastStickyRoom) { + this._stickyRoom = { + room, + tag: this.roomIdsToTags[room.roomId][0], + position: 0, // right at the top as it changed tags + }; } else { - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.warn(`Received no-op update for ${room.roomId} - changing to Timeline update`); - cause = RoomUpdateCause.Timeline; - } - - if (didTagChange && isSticky) { - // Manually update the tag for the sticky room without triggering a sticky room - // update. The update will be handled implicitly by the sticky room handling and - // requires no changes on our part, if we're in the middle of a sticky room change. - if (this._lastStickyRoom) { - this._stickyRoom = { - room, - tag: this.roomIdsToTags[room.roomId][0], - position: 0, // right at the top as it changed tags - }; - } else { - // We have to clear the lock as the sticky room change will trigger updates. - this.lock.release(); - await this.setStickyRoomAsync(room); - await this.lock.acquireAsync(); - } + // We have to clear the lock as the sticky room change will trigger updates. + await this.setStickyRoomAsync(room); } } + } - // If the update is for a room change which might be the sticky room, prevent it. We - // need to make sure that the causes (NewRoom and RoomRemoved) are still triggered though - // as the sticky room relies on this. - if (cause !== RoomUpdateCause.NewRoom && cause !== RoomUpdateCause.RoomRemoved) { - if (this.stickyRoom === room) { - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.warn(`[RoomListDebug] Received ${cause} update for sticky room ${room.roomId} - ignoring`); - return false; - } - } - - if (!this.roomIdsToTags[room.roomId]) { + // If the update is for a room change which might be the sticky room, prevent it. We + // need to make sure that the causes (NewRoom and RoomRemoved) are still triggered though + // as the sticky room relies on this. + if (cause !== RoomUpdateCause.NewRoom && cause !== RoomUpdateCause.RoomRemoved) { + if (this.stickyRoom === room) { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[RoomListDebug] Updating tags for room ${room.roomId} (${room.name})`); - - // Get the tags for the room and populate the cache - const roomTags = this.getTagsForRoom(room).filter(t => !isNullOrUndefined(this.cachedRooms[t])); - - // "This should never happen" condition - we specify DefaultTagID.Untagged in getTagsForRoom(), - // which means we should *always* have a tag to go off of. - if (!roomTags.length) throw new Error(`Tags cannot be determined for ${room.roomId}`); - - this.roomIdsToTags[room.roomId] = roomTags; - - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[RoomListDebug] Updated tags for ${room.roomId}:`, roomTags); - } - - // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[RoomListDebug] Reached algorithmic handling for ${room.roomId} and cause ${cause}`); - - const tags = this.roomIdsToTags[room.roomId]; - if (!tags) { - console.warn(`No tags known for "${room.name}" (${room.roomId})`); + console.warn(`[RoomListDebug] Received ${cause} update for sticky room ${room.roomId} - ignoring`); return false; } + } - let changed = false; - for (const tag of tags) { - const algorithm: OrderingAlgorithm = this.algorithms[tag]; - if (!algorithm) throw new Error(`No algorithm for ${tag}`); + if (!this.roomIdsToTags[room.roomId]) { + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`[RoomListDebug] Updating tags for room ${room.roomId} (${room.name})`); - await algorithm.handleRoomUpdate(room, cause); - this.cachedRooms[tag] = algorithm.orderedRooms; + // Get the tags for the room and populate the cache + const roomTags = this.getTagsForRoom(room).filter(t => !isNullOrUndefined(this.cachedRooms[t])); - // Flag that we've done something - this.recalculateFilteredRoomsForTag(tag); // update filter to re-sort the list - this.recalculateStickyRoom(tag); // update sticky room to make sure it appears if needed - changed = true; - } + // "This should never happen" condition - we specify DefaultTagID.Untagged in getTagsForRoom(), + // which means we should *always* have a tag to go off of. + if (!roomTags.length) throw new Error(`Tags cannot be determined for ${room.roomId}`); + + this.roomIdsToTags[room.roomId] = roomTags; // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 - console.log(`[RoomListDebug] Finished handling ${room.roomId} with cause ${cause} (changed=${changed})`); - return changed; - } finally { - this.lock.release(); + console.log(`[RoomListDebug] Updated tags for ${room.roomId}:`, roomTags); } + + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`[RoomListDebug] Reached algorithmic handling for ${room.roomId} and cause ${cause}`); + + const tags = this.roomIdsToTags[room.roomId]; + if (!tags) { + console.warn(`No tags known for "${room.name}" (${room.roomId})`); + return false; + } + + let changed = false; + for (const tag of tags) { + const algorithm: OrderingAlgorithm = this.algorithms[tag]; + if (!algorithm) throw new Error(`No algorithm for ${tag}`); + + await algorithm.handleRoomUpdate(room, cause); + this.cachedRooms[tag] = algorithm.orderedRooms; + + // Flag that we've done something + this.recalculateFilteredRoomsForTag(tag); // update filter to re-sort the list + this.recalculateStickyRoom(tag); // update sticky room to make sure it appears if needed + changed = true; + } + + // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 + console.log(`[RoomListDebug] Finished handling ${room.roomId} with cause ${cause} (changed=${changed})`); + return changed; } } From 8739e2f78120a83674ec9129b3003d9b3d2997d1 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 6 Jul 2020 20:12:25 -0600 Subject: [PATCH 7/7] Fix room duplication when the sticky room reference changes --- src/stores/room-list/algorithms/Algorithm.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index d85ff4e2e1..d5f2ed0053 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -215,7 +215,10 @@ export class Algorithm extends EventEmitter { // When we do have the room, re-add the old room (if needed) to the algorithm // and remove the sticky room from the algorithm. This is so the underlying // algorithm doesn't try and confuse itself with the sticky room concept. - if (lastStickyRoom) { + // We don't add the new room if the sticky room isn't changing because that's + // an easy way to cause duplication. We have to do room ID checks instead of + // referential checks as the references can differ through the lifecycle. + if (lastStickyRoom && lastStickyRoom.room && lastStickyRoom.room.roomId !== val.roomId) { // Lie to the algorithm and re-add the room to the algorithm await this.handleRoomUpdate(lastStickyRoom.room, RoomUpdateCause.NewRoom); } @@ -643,7 +646,8 @@ export class Algorithm extends EventEmitter { console.log(`Handle room update for ${room.roomId} called with cause ${cause}`); if (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from"); - const isSticky = this._stickyRoom && this._stickyRoom.room === room; + // Note: check the isSticky against the room ID just in case the reference is wrong + const isSticky = this._stickyRoom && this._stickyRoom.room && this._stickyRoom.room.roomId === room.roomId; if (cause === RoomUpdateCause.NewRoom) { const isForLastSticky = this._lastStickyRoom && this._lastStickyRoom.room === room; const roomTags = this.roomIdsToTags[room.roomId];