Merge pull request #625 from matrix-org/dbkr/stop_spinner_of_doom

Don't do /keys/changes on incremental sync

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; }, {});

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,38 +152,68 @@ 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<bool>} 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,
                             trackingStatus: this._deviceTrackingStatus,
                             syncToken: this._syncToken,
                         }, txn);
                     },
-                );
-            }).then(() => {
-                return true;
-            });
+                ).then(() => {
+                    resolveSavePromise();
+                });
+            }, delay);
         }
-        return this._savePromise;
+        return savePromise;
     }
 
     /**

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

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

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.
      */

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.
      */

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