From 4f173528588fcbdc1942c24057d9ff42803cbc37 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 8 Mar 2018 12:33:08 +0000 Subject: [PATCH 1/6] Don't do /keys/changes on incremental sync Remove the call to /keys/changes when we do an incremental syn where the old sync token doesn't match the one in the device list store. To allow us to do this, always save the device list store before saving the sync data, so we can safely assume the device list store is at least as fresh as the sync token in the sync store. Thread save functions through to allow this, add a delay parameter so the sync can save the device list immediately and skip the wait, and add a wantsSave() method so the sync can skip saving the device list if the sync store isn't going to save anyway. Fixes https://github.com/vector-im/riot-web/issues/6068 --- src/crypto/DeviceList.js | 9 ++++++-- src/crypto/index.js | 45 +++++++++++++++++++++++----------------- src/store/indexeddb.js | 20 +++++++++++++++--- src/store/memory.js | 7 +++++++ src/store/stub.js | 7 +++++++ src/sync.js | 15 ++++++++++++-- 6 files changed, 77 insertions(+), 26 deletions(-) diff --git a/src/crypto/DeviceList.js b/src/crypto/DeviceList.js index 4263b39e9a2..e30133ca4ef 100644 --- a/src/crypto/DeviceList.js +++ b/src/crypto/DeviceList.js @@ -146,18 +146,23 @@ export default class DeviceList { * The actual save will be delayed by a short amount of time to * aggregate multiple writes to the database. * + * @param {integer} delay Time in ms before which the save actually happens. + * By default, the save is delayed for a short period in order to batch + * multiple writes, but this behaviour can be disabled by passing 0. + * * @return {Promise} true if the data was saved, false if * it was not (eg. because no changes were pending). The promise * will only resolve once the data is saved, so may take some time * to resolve. */ - async saveIfDirty() { + async saveIfDirty(delay) { if (!this._dirty) return Promise.resolve(false); + if (delay === undefined) delay = 500; if (this._savePromise === null) { // Delay saves for a bit so we can aggregate multiple saves that happen // in quick succession (eg. when a whole room's devices are marked as known) - this._savePromise = Promise.delay(500).then(() => { + this._savePromise = Promise.delay(delay).then(() => { console.log('Saving device tracking data at token ' + this._syncToken); // null out savePromise now (after the delay but before the write), // otherwise we could return the existing promise when the save has diff --git a/src/crypto/index.js b/src/crypto/index.js index d773285dbed..cf2e1cd2cbf 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -444,6 +444,22 @@ Crypto.prototype.getStoredDevice = function(userId, deviceId) { return this._deviceList.getStoredDevice(userId, deviceId); }; +/** + * Save the device list, if necessary + * + * @param {integer} delay Time in ms before which the save actually happens. + * By default, the save is delayed for a short period in order to batch + * multiple writes, but this behaviour can be disabled by passing 0. + * + * @return {Promise} true if the data was saved, false if + * it was not (eg. because no changes were pending). The promise + * will only resolve once the data is saved, so may take some time + * to resolve. + */ +Crypto.prototype.saveDeviceList = function(delay) { + return this._deviceList.saveIfDirty(delay); +}; + /** * Update the blocked/verified state of the given device * @@ -811,27 +827,18 @@ Crypto.prototype.decryptEvent = function(event) { */ Crypto.prototype.handleDeviceListChanges = async function(syncData, syncDeviceLists) { // Initial syncs don't have device change lists. We'll either get the complete list - // of changes for the interval or invalidate everything in onSyncComplete + // of changes for the interval or will have invalidated everything in willProcessSync if (!syncData.oldSyncToken) return; - if (syncData.oldSyncToken === this._deviceList.getSyncToken()) { - // the point the db is at matches where the sync started from, so - // we can safely write the changes - this._evalDeviceListChanges(syncDeviceLists); - } else { - // the db is at a different point to where this sync started from, so - // additionally fetch the changes between where the db is and where the - // sync started - console.log( - "Device list sync gap detected - fetching key changes between " + - this._deviceList.getSyncToken() + " and " + syncData.oldSyncToken, - ); - const gapDeviceLists = await this._baseApis.getKeyChanges( - this._deviceList.getSyncToken(), syncData.oldSyncToken, - ); - this._evalDeviceListChanges(gapDeviceLists); - this._evalDeviceListChanges(syncDeviceLists); - } + // Here, we're relying on the fact that we only ever save the sync data after + // sucessfully saving the device list data, so we're guarenteed that the device + // list store is at least as fresh as the sync token from the sync store, ie. + // any device changes received in sync tokens prior to the 'next' token here + // have been processed and are reflected in the current device list. + // If we didn't make this assumption, we'd have to use the /keys/changes API + // to get key changes between the sync token in the device list and the 'old' + // sync token used here to make sure we didn't miss any. + this._evalDeviceListChanges(syncDeviceLists); }; /** diff --git a/src/store/indexeddb.js b/src/store/indexeddb.js index 335f496d4c8..133492065a1 100644 --- a/src/store/indexeddb.js +++ b/src/store/indexeddb.js @@ -159,13 +159,27 @@ IndexedDBStore.prototype.deleteAllData = function() { }); }; +/** + * Whether this store would like to save its data + * Note that obviously whether the store wants to save or + * not could change between calling this function and calling + * save(). + * + * @return {boolean} True if calling save() will actually save + * (at the time this function is called). + */ +IndexedDBStore.prototype.wantsSave = function() { + const now = Date.now(); + return now - this._syncTs > WRITE_DELAY_MS; +}; + /** * Possibly write data to the database. - * @return {Promise} Promise resolves after the write completes. + * @return {Promise} Promise resolves after the write completes + * (or immediately if no write is performed) */ IndexedDBStore.prototype.save = function() { - const now = Date.now(); - if (now - this._syncTs > WRITE_DELAY_MS) { + if (this.wantsSave()) { return this._reallySave(); } return Promise.resolve(); diff --git a/src/store/memory.js b/src/store/memory.js index d25d2ea8960..81c96993b94 100644 --- a/src/store/memory.js +++ b/src/store/memory.js @@ -315,6 +315,13 @@ module.exports.MatrixInMemoryStore.prototype = { return Promise.resolve(); }, + /** + * We never want to save becase we have nothing to save to. + */ + wantsSave: function() { + return false; + }, + /** * Save does nothing as there is no backing data store. */ diff --git a/src/store/stub.js b/src/store/stub.js index 964ca4a815a..779c56ca2d5 100644 --- a/src/store/stub.js +++ b/src/store/stub.js @@ -216,6 +216,13 @@ StubStore.prototype = { return Promise.resolve(); }, + /** + * We never want to save becase we have nothing to save to. + */ + wantsSave: function() { + return false; + }, + /** * Save does nothing as there is no backing data store. */ diff --git a/src/sync.js b/src/sync.js index 3928875f2bb..a896490102a 100644 --- a/src/sync.js +++ b/src/sync.js @@ -681,8 +681,19 @@ SyncApi.prototype._sync = async function(syncOptions) { // keep emitting SYNCING -> SYNCING for clients who want to do bulk updates this._updateSyncState("SYNCING", syncEventData); - // tell databases that everything is now in a consistent state and can be saved. - client.store.save(); + if (client.store.wantsSave()) { + // We always save the device list (if it's dirty) before saving the sync data: + // this means we know the saved device list data is at least as fresh as the + // stored sync data which means we don't have to worry that we may have missed + // device changes. We can also skip the delay since we're not calling this very + // frequently (and we don't really want to delay the sync for it). + if (this.opts.crypto) { + await this.opts.crypto.saveDeviceList(0); + } + + // tell databases that everything is now in a consistent state and can be saved. + client.store.save(); + } // Begin next sync this._sync(syncOptions); From 727ad5755edcf3b287c4ac1cf95e40df8c3d8ba9 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 8 Mar 2018 12:40:01 +0000 Subject: [PATCH 2/6] lint --- src/store/memory.js | 2 ++ src/store/stub.js | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/store/memory.js b/src/store/memory.js index 81c96993b94..8b6be29ae39 100644 --- a/src/store/memory.js +++ b/src/store/memory.js @@ -317,6 +317,8 @@ module.exports.MatrixInMemoryStore.prototype = { /** * We never want to save becase we have nothing to save to. + * + * @return {boolean} If the store wants to save */ wantsSave: function() { return false; diff --git a/src/store/stub.js b/src/store/stub.js index 779c56ca2d5..f611be394cd 100644 --- a/src/store/stub.js +++ b/src/store/stub.js @@ -218,6 +218,8 @@ StubStore.prototype = { /** * We never want to save becase we have nothing to save to. + * + * @return {boolean} If the store wants to save */ wantsSave: function() { return false; From a0578efeb9bddb4c7a32505e3005de8235b1bb69 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 8 Mar 2018 13:52:48 +0000 Subject: [PATCH 3/6] fix tests --- spec/unit/matrix-client.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/matrix-client.spec.js b/spec/unit/matrix-client.spec.js index c7bc164284f..6585bb5a73e 100644 --- a/spec/unit/matrix-client.spec.js +++ b/spec/unit/matrix-client.spec.js @@ -132,7 +132,7 @@ describe("MatrixClient", function() { ].reduce((r, k) => { r[k] = expect.createSpy(); return r; }, {}); store = [ "getRoom", "getRooms", "getUser", "getSyncToken", "scrollback", - "save", "setSyncToken", "storeEvents", "storeRoom", "storeUser", + "save", "wantsSave", "setSyncToken", "storeEvents", "storeRoom", "storeUser", "getFilterIdByName", "setFilterIdByName", "getFilter", "storeFilter", "getSyncAccumulator", "startup", "deleteAllData", ].reduce((r, k) => { r[k] = expect.createSpy(); return r; }, {}); From 3d1fcc6f83b1919842bea445bcc4594be5f8a4c2 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 8 Mar 2018 14:26:48 +0000 Subject: [PATCH 4/6] One day I'll learn to spell guaranteed --- src/crypto/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/index.js b/src/crypto/index.js index cf2e1cd2cbf..46b823b1ebe 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -831,7 +831,7 @@ Crypto.prototype.handleDeviceListChanges = async function(syncData, syncDeviceLi if (!syncData.oldSyncToken) return; // Here, we're relying on the fact that we only ever save the sync data after - // sucessfully saving the device list data, so we're guarenteed that the device + // sucessfully saving the device list data, so we're guaranteed that the device // list store is at least as fresh as the sync token from the sync store, ie. // any device changes received in sync tokens prior to the 'next' token here // have been processed and are reflected in the current device list. From facfcf679d29c8c8a00811aba291495fbdbe6503 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 8 Mar 2018 15:35:35 +0000 Subject: [PATCH 5/6] DeviceList: bring save forward if necessary If save is called with a delay that would want the save to happen sooner then the save we currently have scheduled, cancel the current save and schedule a new one for the sooner time. --- src/crypto/DeviceList.js | 47 +++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/src/crypto/DeviceList.js b/src/crypto/DeviceList.js index e30133ca4ef..f0c0c752529 100644 --- a/src/crypto/DeviceList.js +++ b/src/crypto/DeviceList.js @@ -92,6 +92,12 @@ export default class DeviceList { // Promise resolved when device data is saved this._savePromise = null; + // Function that resolves the save promise + this._resolveSavePromise = null; + // The time the save is scheduled for + this._savePromiseTime = null; + // The timer used to delay the save + this._saveTimer = null; } /** @@ -159,17 +165,42 @@ export default class DeviceList { if (!this._dirty) return Promise.resolve(false); if (delay === undefined) delay = 500; - if (this._savePromise === null) { + const targetTime = Date.now + delay; + if (this._savePromiseTime && targetTime < this._savePromiseTime) { + // There's a save scheduled but for after we would like: cancel + // it & schedule one for the time we want + clearTimeout(this._saveTimer); + this._saveTimer = null; + this._savePromiseTime = null; + // (but keep the save promise since whatever called save before + // will still want to know when the save is done) + } + + let savePromise = this._savePromise; + if (savePromise === null) { + savePromise = new Promise((resolve, reject) => { + this._resolveSavePromise = resolve; + }); + this._savePromise = savePromise; + } + + if (this._saveTimer === null) { // Delay saves for a bit so we can aggregate multiple saves that happen // in quick succession (eg. when a whole room's devices are marked as known) - this._savePromise = Promise.delay(delay).then(() => { + const resolveSavePromise = this._resolveSavePromise; + this._savePromiseTime = targetTime; + this._saveTimer = setTimeout(() => { console.log('Saving device tracking data at token ' + this._syncToken); // null out savePromise now (after the delay but before the write), // otherwise we could return the existing promise when the save has // actually already happened. Likewise for the dirty flag. + this._savePromiseTime = null; + this._saveTimer = null; this._savePromise = null; + this._resolveSavePromise = null; + this._dirty = false; - return this._cryptoStore.doTxn( + this._cryptoStore.doTxn( 'readwrite', [IndexedDBCryptoStore.STORE_DEVICE_DATA], (txn) => { this._cryptoStore.storeEndToEndDeviceData({ devices: this._devices, @@ -177,12 +208,12 @@ export default class DeviceList { syncToken: this._syncToken, }, txn); }, - ); - }).then(() => { - return true; - }); + ).then(() => { + resolveSavePromise(); + }); + }, delay); } - return this._savePromise; + return savePromise; } /** From 5a23927e5634c111512c1eb567a91b6a33c2d022 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 9 Mar 2018 10:09:36 +0000 Subject: [PATCH 6/6] Move comment up --- src/crypto/DeviceList.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/crypto/DeviceList.js b/src/crypto/DeviceList.js index f0c0c752529..fa55f2fa65b 100644 --- a/src/crypto/DeviceList.js +++ b/src/crypto/DeviceList.js @@ -163,6 +163,8 @@ export default class DeviceList { */ async saveIfDirty(delay) { if (!this._dirty) return Promise.resolve(false); + // Delay saves for a bit so we can aggregate multiple saves that happen + // in quick succession (eg. when a whole room's devices are marked as known) if (delay === undefined) delay = 500; const targetTime = Date.now + delay; @@ -185,8 +187,6 @@ export default class DeviceList { } if (this._saveTimer === null) { - // Delay saves for a bit so we can aggregate multiple saves that happen - // in quick succession (eg. when a whole room's devices are marked as known) const resolveSavePromise = this._resolveSavePromise; this._savePromiseTime = targetTime; this._saveTimer = setTimeout(() => {