From c6e2607f1fd1c4efab5e34f55f903617179e6d11 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 27 May 2021 10:02:05 +0200 Subject: [PATCH] guard against updates emitted while populating during first subscription This came up now because Timeline uses a MappedList to map PendingEvents to PendingEventEntries. In the map function, we setup links between entries to support local echo for relations. When opening a timeline that has unsent relations, the initial populating of the MappedList will try to emit an update for the target entry in remoteEntries. This all happens while the ListView of the timeline is calling subscribe and all collections in the chain are populating themselves based on their sources. This usually entails calling subscribe on the source, and now you are subscribed, iterate over the source (as you're not allowed to query an unsubscribed observable collection, as it might not be populated yet, and even if it did, it wouldn't be guaranteed to be up to date as events aren't flowing yet). So in this concrete example, TilesCollection hadn't populated its tiles yet and when the update to the target of the unsent relation reached TilesCollection, the tiles array was still null and it crashed. I thought what would be the best way to fix this and have a solid model for observable collections to ensure they are always compatible with each other. I considered splitting up the subscription process in two steps where you'd first populate the source and then explicitly start events flowing. I didn't go with this way because it's really only updates that make sense to be emitted during setup. A missed update wouldn't usually bring the collections out of sync like a missed add or remove would. It would just mean the UI isn't updated (or any subsequent filtered collections are not updated), but this should be fine to ignore during setup, as you can rely on the subscribing collections down the chain picking up the update while populating. If we ever want to support add or remove events during setup, we would have to explicitly support them, but for now they are correct to throw. So for now, just ignore update events that happen during setup where needed. --- src/domain/session/room/timeline/TilesCollection.js | 4 ++++ src/observable/list/ConcatList.js | 10 ++++++---- src/observable/list/MappedList.js | 4 ++++ src/observable/list/SortedMapList.js | 4 ++++ src/observable/map/FilteredMap.js | 4 ++++ src/observable/map/JoinedMap.js | 4 ++++ src/observable/map/MappedMap.js | 4 ++++ 7 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/domain/session/room/timeline/TilesCollection.js b/src/domain/session/room/timeline/TilesCollection.js index 2a53c9d4..f8c55e3a 100644 --- a/src/domain/session/room/timeline/TilesCollection.js +++ b/src/domain/session/room/timeline/TilesCollection.js @@ -143,6 +143,10 @@ export class TilesCollection extends BaseObservableList { } onUpdate(index, entry, params) { + // if an update is emitted while calling source.subscribe() from onSubscribeFirst, ignore it + if (!this._tiles) { + return; + } const tileIdx = this._findTileIdx(entry); const tile = this._findTileAtIdx(entry, tileIdx); if (tile) { diff --git a/src/observable/list/ConcatList.js b/src/observable/list/ConcatList.js index a51ddd55..d395e807 100644 --- a/src/observable/list/ConcatList.js +++ b/src/observable/list/ConcatList.js @@ -33,10 +33,7 @@ export class ConcatList extends BaseObservableList { } onSubscribeFirst() { - this._sourceUnsubscribes = []; - for (const sourceList of this._sourceLists) { - this._sourceUnsubscribes.push(sourceList.subscribe(this)); - } + this._sourceUnsubscribes = this._sourceLists.map(sourceList => sourceList.subscribe(this)); } onUnsubscribeLast() { @@ -62,6 +59,11 @@ export class ConcatList extends BaseObservableList { } onUpdate(index, value, params, sourceList) { + // if an update is emitted while calling source.subscribe() from onSubscribeFirst, ignore it + // as we are not supposed to call `length` on any uninitialized list + if (!this._sourceUnsubscribes) { + return; + } this.emitUpdate(this._offsetForSource(sourceList) + index, value, params); } diff --git a/src/observable/list/MappedList.js b/src/observable/list/MappedList.js index 0bc4065a..1aed299f 100644 --- a/src/observable/list/MappedList.js +++ b/src/observable/list/MappedList.js @@ -48,6 +48,10 @@ export class MappedList extends BaseObservableList { } onUpdate(index, value, params) { + // if an update is emitted while calling source.subscribe() from onSubscribeFirst, ignore it + if (!this._mappedValues) { + return; + } const mappedValue = this._mappedValues[index]; if (this._updater) { this._updater(mappedValue, params, value); diff --git a/src/observable/list/SortedMapList.js b/src/observable/list/SortedMapList.js index 27003cba..babf5c35 100644 --- a/src/observable/list/SortedMapList.js +++ b/src/observable/list/SortedMapList.js @@ -70,6 +70,10 @@ export class SortedMapList extends BaseObservableList { } onUpdate(key, value, params) { + // if an update is emitted while calling source.subscribe() from onSubscribeFirst, ignore it + if (!this._sortedPairs) { + return; + } // TODO: suboptimal for performance, see above for idea with BST to speed this up if we need to const oldIdx = this._sortedPairs.findIndex(p => p.key === key); // neccesary to remove pair from array before diff --git a/src/observable/map/FilteredMap.js b/src/observable/map/FilteredMap.js index 71b7bbeb..8e936f5d 100644 --- a/src/observable/map/FilteredMap.js +++ b/src/observable/map/FilteredMap.js @@ -82,6 +82,10 @@ export class FilteredMap extends BaseObservableMap { } onUpdate(key, value, params) { + // if an update is emitted while calling source.subscribe() from onSubscribeFirst, ignore it + if (!this._included) { + return; + } if (this._filter) { const wasIncluded = this._included.get(key); const isIncluded = this._filter(value, key); diff --git a/src/observable/map/JoinedMap.js b/src/observable/map/JoinedMap.js index e5d0caa7..7db04be1 100644 --- a/src/observable/map/JoinedMap.js +++ b/src/observable/map/JoinedMap.js @@ -48,6 +48,10 @@ export class JoinedMap extends BaseObservableMap { } onUpdate(source, key, value, params) { + // if an update is emitted while calling source.subscribe() from onSubscribeFirst, ignore it + if (!this._subscriptions) { + return; + } if (!this._isKeyAtSourceOccluded(source, key)) { this.emitUpdate(key, value, params); } diff --git a/src/observable/map/MappedMap.js b/src/observable/map/MappedMap.js index 48a1d1ad..ec33c4dd 100644 --- a/src/observable/map/MappedMap.js +++ b/src/observable/map/MappedMap.js @@ -49,6 +49,10 @@ export class MappedMap extends BaseObservableMap { } onUpdate(key, value, params) { + // if an update is emitted while calling source.subscribe() from onSubscribeFirst, ignore it + if (!this._mappedValues) { + return; + } const mappedValue = this._mappedValues.get(key); if (mappedValue !== undefined) { // TODO: map params somehow if needed?