From ec7c1ab9f0423921d4fbab2c40070fc703dd943b Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 18 May 2021 15:40:09 -0600 Subject: [PATCH] Revert "Try putting room list handling behind a lock" --- src/stores/room-list/algorithms/Algorithm.ts | 325 +++++++++---------- src/utils/MultiLock.ts | 30 -- 2 files changed, 155 insertions(+), 200 deletions(-) delete mode 100644 src/utils/MultiLock.ts diff --git a/src/stores/room-list/algorithms/Algorithm.ts b/src/stores/room-list/algorithms/Algorithm.ts index c50db43d08..024c484c41 100644 --- a/src/stores/room-list/algorithms/Algorithm.ts +++ b/src/stores/room-list/algorithms/Algorithm.ts @@ -34,7 +34,6 @@ import { OrderingAlgorithm } from "./list-ordering/OrderingAlgorithm"; import { getListAlgorithmInstance } from "./list-ordering"; import SettingsStore from "../../../settings/SettingsStore"; import { VisibilityProvider } from "../filters/VisibilityProvider"; -import { MultiLock } from "../../../utils/MultiLock"; /** * Fired when the Algorithm has determined a list has been updated. @@ -78,7 +77,6 @@ export class Algorithm extends EventEmitter { } = {}; private allowedByFilter: Map = new Map(); private allowedRoomsByFilters: Set = new Set(); - private handlerLock = new MultiLock(); /** * Set to true to suspend emissions of algorithm updates. @@ -681,204 +679,191 @@ export class Algorithm extends EventEmitter { public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise { if (SettingsStore.getValue("advancedRoomListLogging")) { // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - console.log(`Acquiring lock for ${room.roomId} with cause ${cause}`); + console.log(`Handle room update for ${room.roomId} called with cause ${cause}`); } - const release = await this.handlerLock.acquire(room.roomId); - try { - if (SettingsStore.getValue("advancedRoomListLogging")) { - // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - 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"); + + // 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]; + 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 (!this.algorithms) throw new Error("Not ready: no algorithms to determine tags from"); - // 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]; - 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; - } - - // Check to see if the room is known first - let knownRoomRef = this.rooms.includes(room); - if (hasTags && !knownRoomRef) { - console.warn(`${room.roomId} might be a reference change - attempting to update reference`); - this.rooms = this.rooms.map(r => r.roomId === room.roomId ? room : r); - knownRoomRef = this.rooms.includes(room); - if (!knownRoomRef) { - console.warn(`${room.roomId} is still not referenced. It may be sticky.`); - } - } - - // If we have tags for a room and don't have the room referenced, something went horribly - // wrong - the reference should have been updated above. - if (hasTags && !knownRoomRef && !isSticky) { - throw new Error(`${room.roomId} is missing from room array but is known`); - } - - // 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 after all that we're still a NewRoom update, add the room if applicable. - // We don't do this for the sticky room (because it causes duplication issues) - // or if we know about the reference (as it should be replaced). - if (cause === RoomUpdateCause.NewRoom && !isSticky && !knownRoomRef) { - this.rooms.push(room); + // Check to see if the room is known first + let knownRoomRef = this.rooms.includes(room); + if (hasTags && !knownRoomRef) { + console.warn(`${room.roomId} might be a reference change - attempting to update reference`); + this.rooms = this.rooms.map(r => r.roomId === room.roomId ? room : r); + knownRoomRef = this.rooms.includes(room); + if (!knownRoomRef) { + console.warn(`${room.roomId} is still not referenced. It may be sticky.`); } } - let didTagChange = false; - if (cause === RoomUpdateCause.PossibleTagChange) { - 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) { - if (SettingsStore.getValue("advancedRoomListLogging")) { - // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - 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); - this._cachedRooms[rmTag] = algorithm.orderedRooms; - this.recalculateFilteredRoomsForTag(rmTag); // update filter to re-sort the list - this.recalculateStickyRoom(rmTag); // update sticky room to make sure it moves if needed - } - for (const addTag of diff.added) { - if (SettingsStore.getValue("advancedRoomListLogging")) { - // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - 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); - this._cachedRooms[addTag] = algorithm.orderedRooms; - } + // If we have tags for a room and don't have the room referenced, something went horribly + // wrong - the reference should have been updated above. + if (hasTags && !knownRoomRef && !isSticky) { + throw new Error(`${room.roomId} is missing from room array but is known - trying to find duplicate`); + } - // 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 after all that we're still a NewRoom update, add the room if applicable. + // We don't do this for the sticky room (because it causes duplication issues) + // or if we know about the reference (as it should be replaced). + if (cause === RoomUpdateCause.NewRoom && !isSticky && !knownRoomRef) { + this.rooms.push(room); + } + } + + let didTagChange = false; + if (cause === RoomUpdateCause.PossibleTagChange) { + 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) { if (SettingsStore.getValue("advancedRoomListLogging")) { // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - console.log(`Changing update cause for ${room.roomId} to Timeline to sort rooms`); + console.log(`Removing ${room.roomId} from ${rmTag}`); } - cause = RoomUpdateCause.Timeline; - didTagChange = true; + const algorithm: OrderingAlgorithm = this.algorithms[rmTag]; + if (!algorithm) throw new Error(`No algorithm for ${rmTag}`); + await algorithm.handleRoomUpdate(room, RoomUpdateCause.RoomRemoved); + this._cachedRooms[rmTag] = algorithm.orderedRooms; + this.recalculateFilteredRoomsForTag(rmTag); // update filter to re-sort the list + this.recalculateStickyRoom(rmTag); // update sticky room to make sure it moves if needed + } + for (const addTag of diff.added) { + if (SettingsStore.getValue("advancedRoomListLogging")) { + // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 + 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); + this._cachedRooms[addTag] = algorithm.orderedRooms; + } + + // Update the tag map so we don't regen it in a moment + this.roomIdsToTags[room.roomId] = newTags; + + if (SettingsStore.getValue("advancedRoomListLogging")) { + // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 + console.log(`Changing update cause for ${room.roomId} to Timeline to sort rooms`); + } + cause = RoomUpdateCause.Timeline; + didTagChange = true; + } else { + if (SettingsStore.getValue("advancedRoomListLogging")) { + // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 + console.log(`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 { - if (SettingsStore.getValue("advancedRoomListLogging")) { - // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - console.log(`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. - await this.setStickyRoom(room); - } + // We have to clear the lock as the sticky room change will trigger updates. + await this.setStickyRoom(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) { - if (SettingsStore.getValue("advancedRoomListLogging")) { - // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - console.warn( - `[RoomListDebug] Received ${cause} update for sticky room ${room.roomId} - ignoring`, - ); - } - return false; - } - } - - if (!this.roomIdsToTags[room.roomId]) { - if (CAUSES_REQUIRING_ROOM.includes(cause)) { - if (SettingsStore.getValue("advancedRoomListLogging")) { - // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - console.warn(`Skipping tag update for ${room.roomId} because we don't know about the room`); - } - return false; - } - + // 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) { if (SettingsStore.getValue("advancedRoomListLogging")) { // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - console.log(`[RoomListDebug] Updating tags for room ${room.roomId} (${room.name})`); + console.warn(`[RoomListDebug] Received ${cause} update for sticky room ${room.roomId} - ignoring`); } + return false; + } + } - // 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; - + if (!this.roomIdsToTags[room.roomId]) { + if (CAUSES_REQUIRING_ROOM.includes(cause)) { if (SettingsStore.getValue("advancedRoomListLogging")) { // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - console.log(`[RoomListDebug] Updated tags for ${room.roomId}:`, roomTags); + console.warn(`Skipping tag update for ${room.roomId} because we don't know about the room`); } - } - - if (SettingsStore.getValue("advancedRoomListLogging")) { - // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - 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 = didTagChange; - 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; + if (SettingsStore.getValue("advancedRoomListLogging")) { + // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 + 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; + if (SettingsStore.getValue("advancedRoomListLogging")) { // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 - console.log( - `[RoomListDebug] Finished handling ${room.roomId} with cause ${cause} (changed=${changed})`, - ); + console.log(`[RoomListDebug] Updated tags for ${room.roomId}:`, roomTags); } - return changed; - } finally { - release(); } + + if (SettingsStore.getValue("advancedRoomListLogging")) { + // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 + 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 = didTagChange; + 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; + } + + if (SettingsStore.getValue("advancedRoomListLogging")) { + // TODO: Remove debug: https://github.com/vector-im/element-web/issues/14602 + console.log(`[RoomListDebug] Finished handling ${room.roomId} with cause ${cause} (changed=${changed})`); + } + return changed; } } diff --git a/src/utils/MultiLock.ts b/src/utils/MultiLock.ts deleted file mode 100644 index 507a924dda..0000000000 --- a/src/utils/MultiLock.ts +++ /dev/null @@ -1,30 +0,0 @@ -/* -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 { EnhancedMap } from "./maps"; -import AwaitLock from "await-lock"; - -export type DoneFn = () => void; - -export class MultiLock { - private locks = new EnhancedMap(); - - public async acquire(key: string): Promise { - const lock = this.locks.getOrCreate(key, new AwaitLock()); - await lock.acquireAsync(); - return () => lock.release(); - } -}