Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Improve room safety in the new room list #4905

Merged
merged 7 commits into from
Jul 7, 2020
Merged

Improve room safety in the new room list #4905

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0b6f744
Wrap handleRoomUpdate in a lock
turt2live Jul 3, 2020
18df29b
Flag & add some debugging
turt2live Jul 3, 2020
4345f97
Handle sticky room to avoid accidental removal
turt2live Jul 6, 2020
dd833f4
Ensure the sticky room changes if the tag changes
turt2live Jul 7, 2020
70e5da6
Clean up debug logging
turt2live Jul 7, 2020
34bd59c
Remove the lock around the algorithm
turt2live Jul 7, 2020
8739e2f
Fix room duplication when the sticky room reference changes
turt2live Jul 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 131 additions & 14 deletions src/stores/room-list/algorithms/Algorithm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -57,6 +57,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; // only not-null when changing the sticky room
private sortAlgorithms: ITagSortingMap;
private listAlgorithms: IListOrderingMap;
private algorithms: IOrderingAlgorithmMap;
Expand Down Expand Up @@ -162,9 +163,21 @@ 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 || <IStickyRoom>{};

// It's possible to have no selected room. In that case, clear the sticky room
if (!val) {
if (this._stickyRoom) {
Expand All @@ -179,7 +192,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
Expand All @@ -196,19 +209,41 @@ 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.recalculateStickyRoom();

// 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);
}
// 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");
}
}

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;
}

// 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
Expand Down Expand Up @@ -560,7 +595,7 @@ export class Algorithm extends EventEmitter {
/**
* Updates the roomsToTags map
*/
protected updateTagsFromCache() {
private updateTagsFromCache() {
const newMap = {};

const tags = Object.keys(this.cachedRooms);
Expand Down Expand Up @@ -607,21 +642,94 @@ export class Algorithm extends EventEmitter {
* processing.
*/
public async handleRoomUpdate(room: Room, cause: RoomUpdateCause): Promise<boolean> {
// TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035
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];
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) && !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;
}
}

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;
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;

// 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.
await this.setStickyRoomAsync(room);
}
}
}

// If the update is for a room change which might be the sticky room, prevent it. We
Expand All @@ -635,8 +743,9 @@ export class Algorithm extends EventEmitter {
}
}

if (cause === RoomUpdateCause.NewRoom && !this.roomIdsToTags[room.roomId]) {
console.log(`[RoomListDebug] Updating tags for new room ${room.roomId} (${room.name})`);
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})`);

// Get the tags for the room and populate the cache
const roomTags = this.getTagsForRoom(room).filter(t => !isNullOrUndefined(this.cachedRooms[t]));
Expand All @@ -646,9 +755,15 @@ export class Algorithm extends EventEmitter {
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;
Expand All @@ -668,6 +783,8 @@ 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;
}
}