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; }, {}); diff --git a/src/crypto/DeviceList.js b/src/crypto/DeviceList.js index 4263b39e9a2..fa55f2fa65b 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; } /** @@ -146,25 +152,55 @@ 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); + // 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; + 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._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(() => { + if (this._saveTimer === null) { + 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, @@ -172,12 +208,12 @@ export default class DeviceList { syncToken: this._syncToken, }, txn); }, - ); - }).then(() => { - return true; - }); + ).then(() => { + resolveSavePromise(); + }); + }, delay); } - return this._savePromise; + return savePromise; } /** diff --git a/src/crypto/index.js b/src/crypto/index.js index d773285dbed..46b823b1ebe 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 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. + // 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..8b6be29ae39 100644 --- a/src/store/memory.js +++ b/src/store/memory.js @@ -315,6 +315,15 @@ module.exports.MatrixInMemoryStore.prototype = { return Promise.resolve(); }, + /** + * We never want to save becase we have nothing to save to. + * + * @return {boolean} If the store wants to save + */ + 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..f611be394cd 100644 --- a/src/store/stub.js +++ b/src/store/stub.js @@ -216,6 +216,15 @@ StubStore.prototype = { return Promise.resolve(); }, + /** + * We never want to save becase we have nothing to save to. + * + * @return {boolean} If the store wants to save + */ + 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);