Add locking to avoid index corruption

When a new room is added there's a fairly good chance that the other events being dispatched will happen in the middle of (for example) the room list being re-sorted. This commit wraps the entire handleRoomUpdate() function for the underlying algorithms in a lock so that if we're unlucky enough to get an update while we're sorting (as the ImportanceAlgorithm splices out what it is sorting) we won't scream about invalid index errors.

Author: turt2live

src/stores/room-list/algorithms/list-ordering/ImportanceAlgorithm.ts

@@ -179,45 +179,51 @@ export class ImportanceAlgorithm extends OrderingAlgorithm {
     }
 
     public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise<boolean> {
-        if (cause === RoomUpdateCause.NewRoom || cause === RoomUpdateCause.RoomRemoved) {
-            return this.handleSplice(room, cause);
-        }
+        try {
+            await this.updateLock.acquireAsync();
 
-        if (cause !== RoomUpdateCause.Timeline && cause !== RoomUpdateCause.ReadReceipt) {
-            throw new Error(`Unsupported update cause: ${cause}`);
-        }
+            if (cause === RoomUpdateCause.NewRoom || cause === RoomUpdateCause.RoomRemoved) {
+                return this.handleSplice(room, cause);
+            }
 
-        const category = this.getRoomCategory(room);
-        if (this.sortingAlgorithm === SortAlgorithm.Manual) {
-            return; // Nothing to do here.
-        }
+            if (cause !== RoomUpdateCause.Timeline && cause !== RoomUpdateCause.ReadReceipt) {
+                throw new Error(`Unsupported update cause: ${cause}`);
+            }
 
-        const roomIdx = this.getRoomIndex(room);
-        if (roomIdx === -1) {
-            throw new Error(`Room ${room.roomId} has no index in ${this.tagId}`);
-        }
+            const category = this.getRoomCategory(room);
+            if (this.sortingAlgorithm === SortAlgorithm.Manual) {
+                return; // Nothing to do here.
+            }
 
-        // Try to avoid doing array operations if we don't have to: only move rooms within
-        // the categories if we're jumping categories
-        const oldCategory = this.getCategoryFromIndices(roomIdx, this.indices);
-        if (oldCategory !== category) {
-            // Move the room and update the indices
-            this.moveRoomIndexes(1, oldCategory, category, this.indices);
-            this.cachedOrderedRooms.splice(roomIdx, 1); // splice out the old index (fixed position)
-            this.cachedOrderedRooms.splice(this.indices[category], 0, room); // splice in the new room (pre-adjusted)
-            // Note: if moveRoomIndexes() is called after the splice then the insert operation
-            // will happen in the wrong place. Because we would have already adjusted the index
-            // for the category, we don't need to determine how the room is moving in the list.
-            // If we instead tried to insert before updating the indices, we'd have to determine
-            // whether the room was moving later (towards IDLE) or earlier (towards RED) from its
-            // current position, as it'll affect the category's start index after we remove the
-            // room from the array.
-        }
+            const roomIdx = this.getRoomIndex(room);
+            if (roomIdx === -1) {
+                throw new Error(`Room ${room.roomId} has no index in ${this.tagId}`);
+            }
 
-        // Sort the category now that we've dumped the room in
-        await this.sortCategory(category);
+            // Try to avoid doing array operations if we don't have to: only move rooms within
+            // the categories if we're jumping categories
+            const oldCategory = this.getCategoryFromIndices(roomIdx, this.indices);
+            if (oldCategory !== category) {
+                // Move the room and update the indices
+                this.moveRoomIndexes(1, oldCategory, category, this.indices);
+                this.cachedOrderedRooms.splice(roomIdx, 1); // splice out the old index (fixed position)
+                this.cachedOrderedRooms.splice(this.indices[category], 0, room); // splice in the new room (pre-adjusted)
+                // Note: if moveRoomIndexes() is called after the splice then the insert operation
+                // will happen in the wrong place. Because we would have already adjusted the index
+                // for the category, we don't need to determine how the room is moving in the list.
+                // If we instead tried to insert before updating the indices, we'd have to determine
+                // whether the room was moving later (towards IDLE) or earlier (towards RED) from its
+                // current position, as it'll affect the category's start index after we remove the
+                // room from the array.
+            }
 
-        return true; // change made
+            // Sort the category now that we've dumped the room in
+            await this.sortCategory(category);
+
+            return true; // change made
+        } finally {
+            await this.updateLock.release();
+        }
     }
 
     private async sortCategory(category: Category) {

src/stores/room-list/algorithms/list-ordering/NaturalAlgorithm.ts

@@ -38,23 +38,29 @@ export class NaturalAlgorithm extends OrderingAlgorithm {
     }
 
     public async handleRoomUpdate(room, cause): Promise<boolean> {
-        const isSplice = cause === RoomUpdateCause.NewRoom || cause === RoomUpdateCause.RoomRemoved;
-        const isInPlace = cause === RoomUpdateCause.Timeline || cause === RoomUpdateCause.ReadReceipt;
-        if (!isSplice && !isInPlace) {
-            throw new Error(`Unsupported update cause: ${cause}`);
-        }
+        try {
+            await this.updateLock.acquireAsync();
 
-        if (cause === RoomUpdateCause.NewRoom) {
-            this.cachedOrderedRooms.push(room);
-        } else if (cause === RoomUpdateCause.RoomRemoved) {
-            const idx = this.cachedOrderedRooms.indexOf(room);
-            if (idx >= 0) this.cachedOrderedRooms.splice(idx, 1);
-        }
+            const isSplice = cause === RoomUpdateCause.NewRoom || cause === RoomUpdateCause.RoomRemoved;
+            const isInPlace = cause === RoomUpdateCause.Timeline || cause === RoomUpdateCause.ReadReceipt;
+            if (!isSplice && !isInPlace) {
+                throw new Error(`Unsupported update cause: ${cause}`);
+            }
 
-        // TODO: Optimize this to avoid useless operations: https://github.com/vector-im/riot-web/issues/14035
-        // For example, we can skip updates to alphabetic (sometimes) and manually ordered tags
-        this.cachedOrderedRooms = await sortRoomsWithAlgorithm(this.cachedOrderedRooms, this.tagId, this.sortingAlgorithm);
+            if (cause === RoomUpdateCause.NewRoom) {
+                this.cachedOrderedRooms.push(room);
+            } else if (cause === RoomUpdateCause.RoomRemoved) {
+                const idx = this.cachedOrderedRooms.indexOf(room);
+                if (idx >= 0) this.cachedOrderedRooms.splice(idx, 1);
+            }
 
-        return true;
+            // TODO: Optimize this to avoid useless operations: https://github.com/vector-im/riot-web/issues/14035
+            // For example, we can skip updates to alphabetic (sometimes) and manually ordered tags
+            this.cachedOrderedRooms = await sortRoomsWithAlgorithm(this.cachedOrderedRooms, this.tagId, this.sortingAlgorithm);
+
+            return true;
+        } finally {
+            await this.updateLock.release();
+        }
     }
 }

src/stores/room-list/algorithms/list-ordering/OrderingAlgorithm.ts

@@ -17,6 +17,7 @@ limitations under the License.
 import { Room } from "matrix-js-sdk/src/models/room";
 import { RoomUpdateCause, TagID } from "../../models";
 import { SortAlgorithm } from "../models";
+import AwaitLock from "await-lock";
 
 /**
  * Represents a list ordering algorithm. Subclasses should populate the
@@ -25,6 +26,7 @@ import { SortAlgorithm } from "../models";
 export abstract class OrderingAlgorithm {
     protected cachedOrderedRooms: Room[];
     protected sortingAlgorithm: SortAlgorithm;
+    protected readonly updateLock = new AwaitLock();
 
     protected constructor(protected tagId: TagID, initialSortingAlgorithm: SortAlgorithm) {
         // noinspection JSIgnoredPromiseFromCall