From bdc2c3d9adee7b2d2aad4c4b1359b650776c3318 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Sat, 4 Jan 2020 20:04:57 +0100 Subject: [PATCH 1/2] cleanup: storage is not used in SyncWriter as the transaction is now always passed as an argument, it never creates one on its own. --- src/matrix/room/room.js | 2 +- src/matrix/room/timeline/persistence/SyncWriter.js | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/matrix/room/room.js b/src/matrix/room/room.js index a16816fb..387046fb 100644 --- a/src/matrix/room/room.js +++ b/src/matrix/room/room.js @@ -13,7 +13,7 @@ export default class Room extends EventEmitter { this._hsApi = hsApi; this._summary = new RoomSummary(roomId); this._fragmentIdComparer = new FragmentIdComparer([]); - this._syncWriter = new SyncWriter({roomId, storage, fragmentIdComparer: this._fragmentIdComparer}); + this._syncWriter = new SyncWriter({roomId, fragmentIdComparer: this._fragmentIdComparer}); this._emitCollectionChange = emitCollectionChange; this._sendQueue = new SendQueue({roomId, storage, sendScheduler, pendingEvents}); this._timeline = null; diff --git a/src/matrix/room/timeline/persistence/SyncWriter.js b/src/matrix/room/timeline/persistence/SyncWriter.js index 6e5701f2..5ba0de3f 100644 --- a/src/matrix/room/timeline/persistence/SyncWriter.js +++ b/src/matrix/room/timeline/persistence/SyncWriter.js @@ -18,9 +18,8 @@ function deduplicateEvents(events) { } export default class SyncWriter { - constructor({roomId, storage, fragmentIdComparer}) { + constructor({roomId, fragmentIdComparer}) { this._roomId = roomId; - this._storage = storage; this._fragmentIdComparer = fragmentIdComparer; this._lastLiveKey = null; } From 224d56698a711abf1dd2f8810405487201822783 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Sat, 4 Jan 2020 20:06:49 +0100 Subject: [PATCH 2/2] only set new live key when creating a fragment after the txn succeeds when doing a limited sync, and a new fragment is created, this._lastLiveKey is updated immediately. If the transaction would then fail, the fragmentId in this._lastLiveKey was incremented but the fragment wasn't written to the store, so if sync is resumed and would subsequently succeed, fragmentIds would be assigned to events that don't have a corresponding fragment in the timelineFragment store. This would throw errors when trying to load the timeline, breaking the whole app. This changes SyncWriter to only update this._lastLiveKey in the emit phase, when the transactions has been committed already. --- src/matrix/room/room.js | 7 +++--- .../room/timeline/persistence/SyncWriter.js | 24 +++++++++---------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/matrix/room/room.js b/src/matrix/room/room.js index 387046fb..eaed2378 100644 --- a/src/matrix/room/room.js +++ b/src/matrix/room/room.js @@ -22,19 +22,20 @@ export default class Room extends EventEmitter { async persistSync(roomResponse, membership, txn) { const summaryChanged = this._summary.applySync(roomResponse, membership, txn); - const newTimelineEntries = await this._syncWriter.writeSync(roomResponse, txn); + const {entries, newLiveKey} = await this._syncWriter.writeSync(roomResponse, txn); let removedPendingEvents; if (roomResponse.timeline && roomResponse.timeline.events) { removedPendingEvents = this._sendQueue.removeRemoteEchos(roomResponse.timeline.events, txn); } - return {summaryChanged, newTimelineEntries, removedPendingEvents}; + return {summaryChanged, newTimelineEntries: entries, newLiveKey, removedPendingEvents}; } - emitSync({summaryChanged, newTimelineEntries, removedPendingEvents}) { + emitSync({summaryChanged, newTimelineEntries, newLiveKey, removedPendingEvents}) { if (summaryChanged) { this.emit("change"); this._emitCollectionChange(this); } + this._syncWriter.setKeyOnCompleted(newLiveKey); if (this._timeline) { this._timeline.appendLiveEntries(newTimelineEntries); } diff --git a/src/matrix/room/timeline/persistence/SyncWriter.js b/src/matrix/room/timeline/persistence/SyncWriter.js index 5ba0de3f..537a495f 100644 --- a/src/matrix/room/timeline/persistence/SyncWriter.js +++ b/src/matrix/room/timeline/persistence/SyncWriter.js @@ -84,23 +84,23 @@ export default class SyncWriter { async writeSync(roomResponse, txn) { const entries = []; const timeline = roomResponse.timeline; - if (!this._lastLiveKey) { + let currentKey = this._lastLiveKey; + if (!currentKey) { // means we haven't synced this room yet (just joined or did initial sync) // as this is probably a limited sync, prev_batch should be there // (but don't fail if it isn't, we won't be able to back-paginate though) let liveFragment = await this._createLiveFragment(txn, timeline.prev_batch); - this._lastLiveKey = new EventKey(liveFragment.id, EventKey.defaultLiveKey.eventIndex); + currentKey = new EventKey(liveFragment.id, EventKey.defaultLiveKey.eventIndex); entries.push(FragmentBoundaryEntry.start(liveFragment, this._fragmentIdComparer)); } else if (timeline.limited) { // replace live fragment for limited sync, *only* if we had a live fragment already - const oldFragmentId = this._lastLiveKey.fragmentId; - this._lastLiveKey = this._lastLiveKey.nextFragmentKey(); - const {oldFragment, newFragment} = await this._replaceLiveFragment(oldFragmentId, this._lastLiveKey.fragmentId, timeline.prev_batch, txn); + const oldFragmentId = currentKey.fragmentId; + currentKey = currentKey.nextFragmentKey(); + const {oldFragment, newFragment} = await this._replaceLiveFragment(oldFragmentId, currentKey.fragmentId, timeline.prev_batch, txn); entries.push(FragmentBoundaryEntry.end(oldFragment, this._fragmentIdComparer)); entries.push(FragmentBoundaryEntry.start(newFragment, this._fragmentIdComparer)); } - let currentKey = this._lastLiveKey; if (timeline.events) { const events = deduplicateEvents(timeline.events); for(const event of events) { @@ -110,12 +110,6 @@ export default class SyncWriter { entries.push(new EventEntry(entry, this._fragmentIdComparer)); } } - // right thing to do? if the txn fails, not sure we'll continue anyways ... - // only advance the key once the transaction has succeeded - txn.complete().then(() => { - this._lastLiveKey = currentKey; - }) - // persist state const state = roomResponse.state; if (state.events) { @@ -132,7 +126,11 @@ export default class SyncWriter { } } - return entries; + return {entries, newLiveKey: currentKey}; + } + + setKeyOnCompleted(newLiveKey) { + this._lastLiveKey = newLiveKey; } }