Don't do /keys/changes on incremental sync #625

dbkr · 2018-03-08T12:37:14Z

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 element-hq/element-web#6068

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 element-hq/element-web#6068

richvdh

looks generally good modulo concern about delay

richvdh · 2018-03-08T14:12:52Z

src/crypto/index.js

-        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


richvdh · 2018-03-08T14:13:40Z

src/crypto/DeviceList.js

        if (!this._dirty) return Promise.resolve(false);
+        if (delay === undefined) delay = 500;


I'm a bit concerned that, if there is already a save scheduled, it might not happen for 500ms even if delay is 0. can you reorganise this somehow so that it will happen immediately?

I was wondering whether this was worth worrying about. Have added it (although since Promise.delay can only be left to run or canceled outright, it's all had to change to a setTimeout)

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.

richvdh

lgtm modulo comment

richvdh · 2018-03-09T10:06:35Z

src/crypto/DeviceList.js


-        if (this._savePromise === null) {
+        if (this._saveTimer === null) {
            // Delay saves for a bit so we can aggregate multiple saves that happen


this comment probably needs to move up now

[Full Changelog](matrix-org/matrix-js-sdk@v0.9.2...v0.10.0-rc.1) * Fix duplicated state events in timeline from peek [\matrix-org#630](matrix-org#630) * Create indexeddb worker when starting the store [\matrix-org#627](matrix-org#627) * Fix indexeddb logging [\matrix-org#626](matrix-org#626) * Don't do /keys/changes on incremental sync [\matrix-org#625](matrix-org#625) * Don't mark devicelist dirty unnecessarily [\matrix-org#623](matrix-org#623) * Cache the joined member count for a room state [\matrix-org#619](matrix-org#619) * Fix JS doc [\matrix-org#618](matrix-org#618) * Precompute push actions for state events [\matrix-org#617](matrix-org#617) * Fix bug where global "Never send to unverified..." is ignored [\matrix-org#616](matrix-org#616) * Intern legacy top-level 'membership' field [\matrix-org#615](matrix-org#615) * Don't synthesize RR for m.room.redaction as causes the RR to go missing. [\matrix-org#598](matrix-org#598) * Make Events create Dates on demand [\matrix-org#613](matrix-org#613) * Stop cloning events when adding to state [\matrix-org#612](matrix-org#612) * De-dup code: use the initialiseState function [\matrix-org#611](matrix-org#611) * Create sentinel members on-demand [\matrix-org#610](matrix-org#610) * Some more doc on how sentinels work [\matrix-org#609](matrix-org#609) * Migrate room encryption store to crypto store [\matrix-org#597](matrix-org#597) * add parameter to getIdentityServerUrl to strip the protocol for invites [\matrix-org#600](matrix-org#600) * Move Device Tracking Data to Crypto Store [\matrix-org#594](matrix-org#594) * Optimise pushprocessor [\matrix-org#591](matrix-org#591) * Set event error before emitting [\matrix-org#592](matrix-org#592) * Add event type for stickers [WIP] [\matrix-org#590](matrix-org#590) * Migrate inbound sessions to cryptostore [\matrix-org#587](matrix-org#587) * Disambiguate names if they contain an mxid [\matrix-org#588](matrix-org#588) * Check for sessions in indexeddb before migrating [\matrix-org#585](matrix-org#585) * Emit an event for crypto store migration [\matrix-org#586](matrix-org#586) * Supporting fixes For making UnknownDeviceDialog not pop up automatically [\matrix-org#575](matrix-org#575) * Move sessions to the crypto store [\matrix-org#584](matrix-org#584) * Change crypto store transaction API [\matrix-org#582](matrix-org#582) * Add some missed copyright notices [\matrix-org#581](matrix-org#581) * Move Olm account to IndexedDB [\matrix-org#579](matrix-org#579) * Fix logging of DecryptionErrors to be more useful [\matrix-org#580](matrix-org#580) * [BREAKING] Change the behaviour of the unverfied devices blacklist flag [\matrix-org#568](matrix-org#568) * Support set_presence=offline for syncing [\matrix-org#557](matrix-org#557) * Consider cases where the sender may not redact their own event [\matrix-org#556](matrix-org#556)

dbkr added 3 commits March 8, 2018 12:33

lint

727ad57

fix tests

a0578ef

dbkr assigned richvdh Mar 8, 2018

richvdh requested changes Mar 8, 2018

View reviewed changes

dbkr added 2 commits March 8, 2018 14:26

One day I'll learn to spell guaranteed

3d1fcc6

DeviceList: bring save forward if necessary

facfcf6

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.

richvdh approved these changes Mar 9, 2018

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 9, 2018

Move comment up

5a23927

dbkr merged commit 3280cb6 into develop Mar 9, 2018

t3chguy deleted the dbkr/stop_spinner_of_doom branch May 10, 2022 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't do /keys/changes on incremental sync #625

Don't do /keys/changes on incremental sync #625

dbkr commented Mar 8, 2018

richvdh left a comment

richvdh Mar 8, 2018

dbkr Mar 8, 2018

richvdh Mar 8, 2018

dbkr Mar 8, 2018

richvdh left a comment

richvdh Mar 9, 2018

		if (!this._dirty) return Promise.resolve(false);
		if (delay === undefined) delay = 500;

Don't do /keys/changes on incremental sync #625

Don't do /keys/changes on incremental sync #625

Conversation

dbkr commented Mar 8, 2018

richvdh left a comment

Choose a reason for hiding this comment

richvdh Mar 8, 2018

Choose a reason for hiding this comment

dbkr Mar 8, 2018

Choose a reason for hiding this comment

richvdh Mar 8, 2018

Choose a reason for hiding this comment

dbkr Mar 8, 2018

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh Mar 9, 2018

Choose a reason for hiding this comment