From 5f12d858eb872724f79578e688692d215c9d94ef Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 6 Mar 2018 19:01:30 +0000 Subject: [PATCH 01/20] Keep a push processor and re-use it. (#622) Because it does some nice caching stuff but that's no good if we re-create a new one each time. --- src/client.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/client.js b/src/client.js index 19791a860dd..553f30462ad 100644 --- a/src/client.js +++ b/src/client.js @@ -187,6 +187,9 @@ function MatrixClient(opts) { // we still want to know which rooms are encrypted even if crypto is disabled: // we don't want to start sending unencrypted events to them. this._roomList = new RoomList(this._cryptoStore, this._sessionStore); + + // The pushprocessor caches useful things, so keep one and re-use it + this._pushProcessor = new PushProcessor(this); } utils.inherits(MatrixClient, EventEmitter); utils.extend(MatrixClient.prototype, MatrixBaseApis.prototype); @@ -1748,8 +1751,7 @@ function _membershipChange(client, roomId, userId, membership, reason, callback) */ MatrixClient.prototype.getPushActionsForEvent = function(event) { if (!event.getPushActions()) { - const pushProcessor = new PushProcessor(this); - event.setPushActions(pushProcessor.actionsForEvent(event)); + event.setPushActions(this._pushProcessor.actionsForEvent(event)); } return event.getPushActions(); }; From 66e2b3bb70e5131978f78bbdb179e22d12ed949f Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 7 Mar 2018 17:32:51 +0000 Subject: [PATCH 02/20] Don't mark devicelist dirty unnecessarily This was marking the device list as dirty even when nothing had actually changed, causing unnecessary saves. --- src/crypto/DeviceList.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/crypto/DeviceList.js b/src/crypto/DeviceList.js index d913b426a81..4263b39e9a2 100644 --- a/src/crypto/DeviceList.js +++ b/src/crypto/DeviceList.js @@ -418,11 +418,11 @@ export default class DeviceList { if (this._deviceTrackingStatus[userId]) { console.log('No longer tracking device list for ' + userId); this._deviceTrackingStatus[userId] = TRACKING_STATUS_NOT_TRACKED; - } - // we don't yet persist the tracking status, since there may be a lot - // of calls; we save all data together once the sync is done - this._dirty = true; + // we don't yet persist the tracking status, since there may be a lot + // of calls; we save all data together once the sync is done + this._dirty = true; + } } /** @@ -453,11 +453,11 @@ export default class DeviceList { if (this._deviceTrackingStatus[userId]) { console.log("Marking device list outdated for", userId); this._deviceTrackingStatus[userId] = TRACKING_STATUS_PENDING_DOWNLOAD; - } - // we don't yet persist the tracking status, since there may be a lot - // of calls; we save all data together once the sync is done - this._dirty = true; + // we don't yet persist the tracking status, since there may be a lot + // of calls; we save all data together once the sync is done + this._dirty = true; + } } /** From 4f173528588fcbdc1942c24057d9ff42803cbc37 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 8 Mar 2018 12:33:08 +0000 Subject: [PATCH 03/20] 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 04/20] 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 05/20] 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 06/20] 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 68b230a78fe6f458aed906ef19e6faaf685cdc6c Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 8 Mar 2018 15:01:01 +0000 Subject: [PATCH 07/20] Add function to cancel and resend key request (#624) --- src/client.js | 10 +++ src/crypto/OutgoingRoomKeyRequestManager.js | 82 ++++++++++++++++----- src/crypto/index.js | 6 +- src/models/event.js | 15 ++++ 4 files changed, 92 insertions(+), 21 deletions(-) diff --git a/src/client.js b/src/client.js index 553f30462ad..fe2dff69e16 100644 --- a/src/client.js +++ b/src/client.js @@ -623,6 +623,16 @@ MatrixClient.prototype.isEventSenderVerified = async function(event) { return device.isVerified(); }; +/** + * Cancel a room key request for this event if one is ongoing and resend the + * request. + * @param {MatrxEvent} event event of which to cancel and resend the room + * key request. + */ +MatrixClient.prototype.cancelAndResendEventRoomKeyRequest = function(event) { + event.cancelAndResendKeyRequest(this._crypto); +}; + /** * Enable end-to-end encryption for a room. * @param {string} roomId The room ID to enable encryption in. diff --git a/src/crypto/OutgoingRoomKeyRequestManager.js b/src/crypto/OutgoingRoomKeyRequestManager.js index 8bc860d62b9..75ab35454d5 100644 --- a/src/crypto/OutgoingRoomKeyRequestManager.js +++ b/src/crypto/OutgoingRoomKeyRequestManager.js @@ -35,13 +35,19 @@ const SEND_KEY_REQUESTS_DELAY_MS = 500; * * The state machine looks like: * - * | - * V (cancellation requested) - * UNSENT -----------------------------+ - * | | - * | (send successful) | - * V | - * SENT | + * | (cancellation sent) + * | .-------------------------------------------------. + * | | | + * V V (cancellation requested) | + * UNSENT -----------------------------+ | + * | | | + * | | | + * | (send successful) | CANCELLATION_PENDING_AND_WILL_RESEND + * V | Λ + * SENT | | + * |-------------------------------- | --------------' + * | | (cancellation requested with intent + * | | to resend the original request) * | | * | (cancellation requested) | * V | @@ -62,6 +68,12 @@ const ROOM_KEY_REQUEST_STATES = { /** reply received, cancellation not yet sent */ CANCELLATION_PENDING: 2, + + /** + * Cancellation not yet sent and will transition to UNSENT instead of + * being deleted once the cancellation has been sent. + */ + CANCELLATION_PENDING_AND_WILL_RESEND: 3, }; export default class OutgoingRoomKeyRequestManager { @@ -130,14 +142,16 @@ export default class OutgoingRoomKeyRequestManager { } /** - * Cancel room key requests, if any match the given details + * Cancel room key requests, if any match the given requestBody * * @param {module:crypto~RoomKeyRequestBody} requestBody + * @param {boolean} andResend if true, transition to UNSENT instead of + * deleting after sending cancellation. * * @returns {Promise} resolves when the request has been updated in our * pending list. */ - cancelRoomKeyRequest(requestBody) { + cancelRoomKeyRequest(requestBody, andResend=false) { return this._cryptoStore.getOutgoingRoomKeyRequest( requestBody, ).then((req) => { @@ -147,6 +161,7 @@ export default class OutgoingRoomKeyRequestManager { } switch (req.state) { case ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING: + case ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING_AND_WILL_RESEND: // nothing to do here return; @@ -166,11 +181,16 @@ export default class OutgoingRoomKeyRequestManager { req.requestId, ROOM_KEY_REQUEST_STATES.UNSENT, ); - case ROOM_KEY_REQUEST_STATES.SENT: + case ROOM_KEY_REQUEST_STATES.SENT: { + // If `andResend` is set, transition to UNSENT once the + // cancellation has successfully been sent. + const state = andResend ? + ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING_AND_WILL_RESEND : + ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING; // send a cancellation. return this._cryptoStore.updateOutgoingRoomKeyRequest( req.requestId, ROOM_KEY_REQUEST_STATES.SENT, { - state: ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING, + state, cancellationTxnId: this._baseApis.makeTxnId(), }, ).then((updatedReq) => { @@ -200,15 +220,23 @@ export default class OutgoingRoomKeyRequestManager { // do.) this._sendOutgoingRoomKeyRequestCancellation( updatedReq, + andResend, ).catch((e) => { console.error( "Error sending room key request cancellation;" + " will retry later.", e, ); this._startTimer(); - }).done(); + }).then(() => { + if (!andResend) return; + // The request has transitioned from + // CANCELLATION_PENDING_AND_WILL_RESEND to UNSENT. We + // still need to resend the request which is now UNSENT, so + // start the timer if it isn't already started. + this._startTimer(); + }); }); - + } default: throw new Error('unhandled state: ' + req.state); } @@ -258,6 +286,7 @@ export default class OutgoingRoomKeyRequestManager { return this._cryptoStore.getOutgoingRoomKeyRequestByState([ ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING, + ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING_AND_WILL_RESEND, ROOM_KEY_REQUEST_STATES.UNSENT, ]).then((req) => { if (!req) { @@ -267,10 +296,16 @@ export default class OutgoingRoomKeyRequestManager { } let prom; - if (req.state === ROOM_KEY_REQUEST_STATES.UNSENT) { - prom = this._sendOutgoingRoomKeyRequest(req); - } else { // must be a cancellation - prom = this._sendOutgoingRoomKeyRequestCancellation(req); + switch (req.state) { + case ROOM_KEY_REQUEST_STATES.UNSENT: + prom = this._sendOutgoingRoomKeyRequest(req); + break; + case ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING: + prom = this._sendOutgoingRoomKeyRequestCancellation(req); + break; + case ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING_AND_WILL_RESEND: + prom = this._sendOutgoingRoomKeyRequestCancellation(req, true); + break; } return prom.then(() => { @@ -309,8 +344,9 @@ export default class OutgoingRoomKeyRequestManager { }); } - // given a RoomKeyRequest, cancel it and delete the request record - _sendOutgoingRoomKeyRequestCancellation(req) { + // Given a RoomKeyRequest, cancel it and delete the request record unless + // andResend is set, in which case transition to UNSENT. + _sendOutgoingRoomKeyRequestCancellation(req, andResend) { console.log( `Sending cancellation for key request for ` + `${stringifyRequestBody(req.requestBody)} to ` + @@ -327,6 +363,14 @@ export default class OutgoingRoomKeyRequestManager { return this._sendMessageToDevices( requestMessage, req.recipients, req.cancellationTxnId, ).then(() => { + if (andResend) { + // We want to resend, so transition to UNSENT + return this._cryptoStore.updateOutgoingRoomKeyRequest( + req.requestId, + ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING_AND_WILL_RESEND, + { state: ROOM_KEY_REQUEST_STATES.UNSENT }, + ); + } return this._cryptoStore.deleteOutgoingRoomKeyRequest( req.requestId, ROOM_KEY_REQUEST_STATES.CANCELLATION_PENDING, ); diff --git a/src/crypto/index.js b/src/crypto/index.js index d773285dbed..c83b590d810 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -856,9 +856,11 @@ Crypto.prototype.requestRoomKey = function(requestBody, recipients) { * * @param {module:crypto~RoomKeyRequestBody} requestBody * parameters to match for cancellation + * @param {boolean} andResend + * if true, resend the key request after cancelling. */ -Crypto.prototype.cancelRoomKeyRequest = function(requestBody) { - this._outgoingRoomKeyRequestManager.cancelRoomKeyRequest(requestBody) +Crypto.prototype.cancelRoomKeyRequest = function(requestBody, andResend) { + this._outgoingRoomKeyRequestManager.cancelRoomKeyRequest(requestBody, andResend) .catch((e) => { console.warn("Error clearing pending room key requests", e); }).done(); diff --git a/src/models/event.js b/src/models/event.js index 26559c688cf..0bc2a3d4033 100644 --- a/src/models/event.js +++ b/src/models/event.js @@ -378,6 +378,21 @@ utils.extend(module.exports.MatrixEvent.prototype, { return this._decryptionPromise; }, + /** + * Cancel any room key request for this event and resend another. + * + * @param {module:crypto} crypto crypto module + */ + cancelAndResendKeyRequest: function(crypto) { + const wireContent = this.getWireContent(); + crypto.cancelRoomKeyRequest({ + algorithm: wireContent.algorithm, + room_id: this.getRoomId(), + session_id: wireContent.session_id, + sender_key: wireContent.sender_key, + }, true); + }, + _decryptionLoop: async function(crypto) { // make sure that this method never runs completely synchronously. // (doing so would mean that we would clear _decryptionPromise *before* From facfcf679d29c8c8a00811aba291495fbdbe6503 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 8 Mar 2018 15:35:35 +0000 Subject: [PATCH 08/20] 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 fbc43b0d589d461ab500cda27d6bbbe2d4b2e428 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Thu, 8 Mar 2018 23:58:39 +0000 Subject: [PATCH 09/20] stupid typo --- src/store/indexeddb-local-backend.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/store/indexeddb-local-backend.js b/src/store/indexeddb-local-backend.js index 453156134c5..2488341f7e0 100644 --- a/src/store/indexeddb-local-backend.js +++ b/src/store/indexeddb-local-backend.js @@ -361,7 +361,7 @@ LocalIndexedDBStoreBackend.prototype = { */ _loadSyncData: function() { console.log( - `LocalIndexedDBStoreBackend: loaded sync data`, + `LocalIndexedDBStoreBackend: loading sync data`, ); return Promise.try(() => { console.log( From beafd597ddbd30671ebfe4ff554cd701e0d44e43 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Fri, 9 Mar 2018 02:18:19 +0000 Subject: [PATCH 10/20] ensure indexeddb workers are never double-connected --- src/store/indexeddb-local-backend.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/store/indexeddb-local-backend.js b/src/store/indexeddb-local-backend.js index 2488341f7e0..5500fd95aea 100644 --- a/src/store/indexeddb-local-backend.js +++ b/src/store/indexeddb-local-backend.js @@ -101,6 +101,7 @@ const LocalIndexedDBStoreBackend = function LocalIndexedDBStoreBackend( this.indexedDB = indexedDBInterface; this._dbName = "matrix-js-sdk:" + (dbName || "default"); this.db = null; + this._disconnected = true; this._syncAccumulator = new SyncAccumulator(); }; @@ -112,13 +113,15 @@ LocalIndexedDBStoreBackend.prototype = { * @return {Promise} Resolves if successfully connected. */ connect: function() { - if (this.db) { + if (!this._disconnected) { console.log( - `LocalIndexedDBStoreBackend.connect: already connected`, + `LocalIndexedDBStoreBackend.connect: already connected or connecting`, ); return Promise.resolve(); } + this._disconnected = false; + console.log( `LocalIndexedDBStoreBackend.connect: connecting`, ); From 5a23927e5634c111512c1eb567a91b6a33c2d022 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 9 Mar 2018 10:09:36 +0000 Subject: [PATCH 11/20] 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(() => { From 8798bf42e64297ad16410f2d8a9c303e5a339acf Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 9 Mar 2018 10:16:32 +0000 Subject: [PATCH 12/20] Fix indexeddb logging 1. Fix double 'loaded' on sync data logging 2. Move the 'loaded' message into the bit where the data has actually loaded rather than the promise try block. 3. Add '...' to the 'loading' messages so they're easier to tell apart from the 'loaded' messages. --- src/store/indexeddb-local-backend.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/store/indexeddb-local-backend.js b/src/store/indexeddb-local-backend.js index 453156134c5..fcece5713f0 100644 --- a/src/store/indexeddb-local-backend.js +++ b/src/store/indexeddb-local-backend.js @@ -120,7 +120,7 @@ LocalIndexedDBStoreBackend.prototype = { } console.log( - `LocalIndexedDBStoreBackend.connect: connecting`, + `LocalIndexedDBStoreBackend.connect: connecting...`, ); const req = this.indexedDB.open(this._dbName, VERSION); req.onupgradeneeded = (ev) => { @@ -142,7 +142,7 @@ LocalIndexedDBStoreBackend.prototype = { }; console.log( - `LocalIndexedDBStoreBackend.connect: awaiting connection`, + `LocalIndexedDBStoreBackend.connect: awaiting connection...`, ); return promiseifyRequest(req).then((ev) => { console.log( @@ -341,16 +341,18 @@ LocalIndexedDBStoreBackend.prototype = { */ _loadAccountData: function() { console.log( - `LocalIndexedDBStoreBackend: loading account data`, + `LocalIndexedDBStoreBackend: loading account data...`, ); return Promise.try(() => { - console.log( - `LocalIndexedDBStoreBackend: loaded account data`, - ); const txn = this.db.transaction(["accountData"], "readonly"); const store = txn.objectStore("accountData"); return selectQuery(store, undefined, (cursor) => { return cursor.value; + }).then((result) => { + console.log( + `LocalIndexedDBStoreBackend: loaded account data`, + ); + return result; }); }); }, @@ -361,17 +363,17 @@ LocalIndexedDBStoreBackend.prototype = { */ _loadSyncData: function() { console.log( - `LocalIndexedDBStoreBackend: loaded sync data`, + `LocalIndexedDBStoreBackend: loading sync data...`, ); return Promise.try(() => { - console.log( - `LocalIndexedDBStoreBackend: loaded sync data`, - ); const txn = this.db.transaction(["sync"], "readonly"); const store = txn.objectStore("sync"); return selectQuery(store, undefined, (cursor) => { return cursor.value; }).then((results) => { + console.log( + `LocalIndexedDBStoreBackend: loaded sync data`, + ); if (results.length > 1) { console.warn("loadSyncData: More than 1 sync row found."); } From 8bd68e0f107cc241c17ea02b8a2eeeb3fcbc3ecf Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 9 Mar 2018 16:53:27 +0000 Subject: [PATCH 13/20] Create indexeddb worker when starting the store Rather than when creating it, otherwise we could potentially end up starting workers unnecessarily. --- src/store/indexeddb-remote-backend.js | 29 ++++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/store/indexeddb-remote-backend.js b/src/store/indexeddb-remote-backend.js index e5c1cdd8420..2221377db98 100644 --- a/src/store/indexeddb-remote-backend.js +++ b/src/store/indexeddb-remote-backend.js @@ -31,20 +31,16 @@ import Promise from 'bluebird'; const RemoteIndexedDBStoreBackend = function RemoteIndexedDBStoreBackend( workerScript, dbName, WorkerApi, ) { + this._workerScript = workerScript; this._dbName = dbName; - this._worker = new WorkerApi(workerScript); + this._workerApi = WorkerApi; + this._worker = null; this._nextSeq = 0; // The currently in-flight requests to the actual backend this._inFlight = { // seq: promise, }; - - this._worker.onmessage = this._onWorkerMessage.bind(this); - - // tell the worker the db name. - this._startPromise = this._doCmd('_setupWorker', [this._dbName]).then(() => { - console.log("IndexedDB worker is ready"); - }); + this._startPromise = null; }; @@ -55,7 +51,7 @@ RemoteIndexedDBStoreBackend.prototype = { * @return {Promise} Resolves if successfully connected. */ connect: function() { - return this._startPromise.then(() => this._doCmd('connect')); + return this._ensureStarted().then(() => this._doCmd('connect')); }, /** @@ -64,7 +60,7 @@ RemoteIndexedDBStoreBackend.prototype = { * @return {Promise} Resolved when the database is cleared. */ clearDatabase: function() { - return this._startPromise.then(() => this._doCmd('clearDatabase')); + return this._ensureStarted().then(() => this._doCmd('clearDatabase')); }, /** @@ -93,6 +89,19 @@ RemoteIndexedDBStoreBackend.prototype = { return this._doCmd('getUserPresenceEvents'); }, + _ensureStarted: function() { + if (this._startPromise === null) { + this._worker = new this._workerApi(this._workerScript); + this._worker.onmessage = this._onWorkerMessage.bind(this); + + // tell the worker the db name. + this._startPromise = this._doCmd('_setupWorker', [this._dbName]).then(() => { + console.log("IndexedDB worker is ready"); + }); + } + return this._startPromise; + }, + _doCmd: function(cmd, args) { // wrap in a q so if the postMessage throws, // the promise automatically gets rejected From 0be679de42fc41945c801fa7e6ccc6d246cd266d Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 9 Mar 2018 17:31:08 +0000 Subject: [PATCH 14/20] Make local variable look less like a global --- src/store/indexeddb-remote-backend.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/store/indexeddb-remote-backend.js b/src/store/indexeddb-remote-backend.js index 2221377db98..bcac2f78375 100644 --- a/src/store/indexeddb-remote-backend.js +++ b/src/store/indexeddb-remote-backend.js @@ -29,11 +29,11 @@ import Promise from 'bluebird'; * @param {Object} WorkerApi The web worker compatible interface object */ const RemoteIndexedDBStoreBackend = function RemoteIndexedDBStoreBackend( - workerScript, dbName, WorkerApi, + workerScript, dbName, workerApi, ) { this._workerScript = workerScript; this._dbName = dbName; - this._workerApi = WorkerApi; + this._workerApi = workerApi; this._worker = null; this._nextSeq = 0; // The currently in-flight requests to the actual backend From 5c2dfb138aea25cce8b9ea505910da7570a13994 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 9 Mar 2018 17:35:03 +0000 Subject: [PATCH 15/20] Hopefully clarify _startPromise --- src/store/indexeddb-remote-backend.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/store/indexeddb-remote-backend.js b/src/store/indexeddb-remote-backend.js index bcac2f78375..2191080f19e 100644 --- a/src/store/indexeddb-remote-backend.js +++ b/src/store/indexeddb-remote-backend.js @@ -40,6 +40,8 @@ const RemoteIndexedDBStoreBackend = function RemoteIndexedDBStoreBackend( this._inFlight = { // seq: promise, }; + // Once we start connecting, we keep the promise and re-use it + // if we try to connect again this._startPromise = null; }; From dc3ffb3b301ce45865f6258e4a45c988095126c4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 9 Mar 2018 17:39:02 +0000 Subject: [PATCH 16/20] Fix jsdoc --- src/store/indexeddb-remote-backend.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/store/indexeddb-remote-backend.js b/src/store/indexeddb-remote-backend.js index 2191080f19e..4decfc3f3b7 100644 --- a/src/store/indexeddb-remote-backend.js +++ b/src/store/indexeddb-remote-backend.js @@ -26,7 +26,7 @@ import Promise from 'bluebird'; * @param {string} workerScript URL to the worker script * @param {string=} dbName Optional database name. The same name must be used * to open the same database. - * @param {Object} WorkerApi The web worker compatible interface object + * @param {Object} workerApi The web worker compatible interface object */ const RemoteIndexedDBStoreBackend = function RemoteIndexedDBStoreBackend( workerScript, dbName, workerApi, From 64396de0dc709a34dbd00d62321420111cfaa03d Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 15 Mar 2018 17:35:18 +0000 Subject: [PATCH 17/20] Fix duplicated state events in timeline from peek When joining a room we were peeking into, we duplicated all the state events into the timeline. Put back the old behaviour of just setting them as state events, with copious commentary on how wrong this seems. --- spec/integ/matrix-client-syncing.spec.js | 6 +++++- src/sync.js | 13 ++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/spec/integ/matrix-client-syncing.spec.js b/spec/integ/matrix-client-syncing.spec.js index 9b737aaed12..a74eea53a88 100644 --- a/spec/integ/matrix-client-syncing.spec.js +++ b/spec/integ/matrix-client-syncing.spec.js @@ -437,7 +437,11 @@ describe("MatrixClient syncing", function() { }); }); - it("should correctly interpret state in incremental sync.", function() { + // XXX: This test asserts that the js-sdk obeys the spec and treats state + // events that arrive in the incremental sync as if they preceeded the + // timeline events, however this breaks peeking, so it's disabled + // (see sync.js) + xit("should correctly interpret state in incremental sync.", function() { httpBackend.when("GET", "/sync").respond(200, syncData); httpBackend.when("GET", "/sync").respond(200, nextSyncData); diff --git a/src/sync.js b/src/sync.js index a896490102a..0fe8f9123eb 100644 --- a/src/sync.js +++ b/src/sync.js @@ -1340,8 +1340,19 @@ SyncApi.prototype._processRoomEvents = function(room, stateEventList, // If the timeline wasn't empty, we process the state events here: they're // defined as updates to the state before the start of the timeline, so this // starts to roll the state forward. + // XXX: That's what we *should* do, but this can happen if we were previously + // peeking in a room, in which case we obviously do *not* want to add the + // state events here onto the end of the timeline. Historically, the js-sdk + // has just set these new state events on the old and new state. This seems + // very wrong because there could be events in the timeline that diverge the + // state, in which case this is going to leave things out of sync. However, + // for now I think it;s best to behave the same as the code has done previously. if (!timelineWasEmpty) { - room.addLiveEvents(stateEventList || []); + // XXX: As above, don't do this... + //room.addLiveEvents(stateEventList || []); + // Do this instead... + room.oldState.setStateEvents(stateEventList || []); + room.currentState.setStateEvents(stateEventList || []); } // execute the timeline events. This will continue to diverge the current state // if the timeline has any state events in it. From 16c062c069d905dbed171e30ea2e9f88c743727f Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 16 Mar 2018 15:22:06 +0000 Subject: [PATCH 18/20] Start first incremental sync request early (#629) * Start first incremental sync request early So it can run while we process our sync data. --- spec/unit/matrix-client.spec.js | 1 + src/store/indexeddb-local-backend.js | 5 + src/store/indexeddb-remote-backend.js | 5 + src/store/indexeddb-store-worker.js | 4 + src/store/indexeddb.js | 9 ++ src/store/memory.js | 9 ++ src/store/stub.js | 9 ++ src/sync-accumulator.js | 5 + src/sync.js | 221 +++++++++++++++----------- 9 files changed, 178 insertions(+), 90 deletions(-) diff --git a/spec/unit/matrix-client.spec.js b/spec/unit/matrix-client.spec.js index 6585bb5a73e..8ef22f6b018 100644 --- a/spec/unit/matrix-client.spec.js +++ b/spec/unit/matrix-client.spec.js @@ -137,6 +137,7 @@ describe("MatrixClient", function() { "getSyncAccumulator", "startup", "deleteAllData", ].reduce((r, k) => { r[k] = expect.createSpy(); return r; }, {}); store.getSavedSync = expect.createSpy().andReturn(Promise.resolve(null)); + store.getSavedSyncToken = expect.createSpy().andReturn(Promise.resolve(null)); store.setSyncData = expect.createSpy().andReturn(Promise.resolve(null)); client = new MatrixClient({ baseUrl: "https://my.home.server", diff --git a/src/store/indexeddb-local-backend.js b/src/store/indexeddb-local-backend.js index 33863169531..a928e1aa6bb 100644 --- a/src/store/indexeddb-local-backend.js +++ b/src/store/indexeddb-local-backend.js @@ -1,5 +1,6 @@ /* Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -243,6 +244,10 @@ LocalIndexedDBStoreBackend.prototype = { } }, + getNextBatchToken: function() { + return Promise.resolve(this._syncAccumulator.getNextBatchToken()); + }, + setSyncData: function(syncData) { return Promise.resolve().then(() => { this._syncAccumulator.accumulate(syncData); diff --git a/src/store/indexeddb-remote-backend.js b/src/store/indexeddb-remote-backend.js index 4decfc3f3b7..2221633e423 100644 --- a/src/store/indexeddb-remote-backend.js +++ b/src/store/indexeddb-remote-backend.js @@ -1,5 +1,6 @@ /* Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -74,6 +75,10 @@ RemoteIndexedDBStoreBackend.prototype = { return this._doCmd('getSavedSync'); }, + getNextBatchToken: function() { + return this._doCmd('getNextBatchToken'); + }, + setSyncData: function(syncData) { return this._doCmd('setSyncData', [syncData]); }, diff --git a/src/store/indexeddb-store-worker.js b/src/store/indexeddb-store-worker.js index 3dc16e85544..d32df7274e5 100644 --- a/src/store/indexeddb-store-worker.js +++ b/src/store/indexeddb-store-worker.js @@ -1,5 +1,6 @@ /* Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -88,6 +89,9 @@ class IndexedDBStoreWorker { case 'getUserPresenceEvents': prom = this.backend.getUserPresenceEvents(); break; + case 'getNextBatchToken': + prom = this.backend.getNextBatchToken(); + break; } if (prom === undefined) { diff --git a/src/store/indexeddb.js b/src/store/indexeddb.js index 133492065a1..d47e3554469 100644 --- a/src/store/indexeddb.js +++ b/src/store/indexeddb.js @@ -1,5 +1,6 @@ /* Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -145,6 +146,14 @@ IndexedDBStore.prototype.getSavedSync = function() { return this.backend.getSavedSync(); }; +/** + * @return {Promise} If there is a saved sync, the nextBatch token + * for this sync, otherwise null. + */ +IndexedDBStore.prototype.getSavedSyncToken = function() { + return this.backend.getNextBatchToken(); +}, + /** * Delete all data from this store. * @return {Promise} Resolves if the data was deleted from the database. diff --git a/src/store/memory.js b/src/store/memory.js index 8b6be29ae39..4f2f7c7546a 100644 --- a/src/store/memory.js +++ b/src/store/memory.js @@ -1,6 +1,7 @@ /* Copyright 2015, 2016 OpenMarket Ltd Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -346,6 +347,14 @@ module.exports.MatrixInMemoryStore.prototype = { return Promise.resolve(null); }, + /** + * @return {Promise} If there is a saved sync, the nextBatch token + * for this sync, otherwise null. + */ + getSavedSyncToken: function() { + return Promise.resolve(null); + }, + /** * Delete all data from this store. * @return {Promise} An immediately resolved promise. diff --git a/src/store/stub.js b/src/store/stub.js index f611be394cd..f41e0c5e058 100644 --- a/src/store/stub.js +++ b/src/store/stub.js @@ -1,6 +1,7 @@ /* Copyright 2015, 2016 OpenMarket Ltd Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -247,6 +248,14 @@ StubStore.prototype = { return Promise.resolve(null); }, + /** + * @return {Promise} If there is a saved sync, the nextBatch token + * for this sync, otherwise null. + */ + getSavedSyncToken: function() { + return Promise.resolve(null); + }, + /** * Delete all data from this store. Does nothing since this store * doesn't store anything. diff --git a/src/sync-accumulator.js b/src/sync-accumulator.js index c1266a9b01d..7369c9b8a89 100644 --- a/src/sync-accumulator.js +++ b/src/sync-accumulator.js @@ -1,5 +1,6 @@ /* Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -520,6 +521,10 @@ class SyncAccumulator { accountData: accData, }; } + + getNextBatchToken() { + return this.nextBatch; + } } function setState(eventMap, event) { diff --git a/src/sync.js b/src/sync.js index a896490102a..087f8c803f2 100644 --- a/src/sync.js +++ b/src/sync.js @@ -396,6 +396,17 @@ SyncApi.prototype.getSyncState = function() { return this._syncState; }; +SyncApi.prototype.recoverFromSyncStartupError = async function(savedSyncPromise, err) { + // Wait for the saved sync to complete - we send the pushrules and filter requests + // before the saved sync has finished so they can run in parallel, but only process + // the results after the saved sync is done. Equivalently, we wait for it to finish + // before reporting failures from these functions. + await savedSyncPromise; + const keepaliveProm = this._startKeepAlives(); + this._updateSyncState("ERROR", { error: err }); + await keepaliveProm; +}; + /** * Main entry point */ @@ -410,28 +421,32 @@ SyncApi.prototype.sync = function() { global.document.addEventListener("online", this._onOnlineBound, false); } + let savedSyncPromise = Promise.resolve(); + let savedSyncToken = null; + // We need to do one-off checks before we can begin the /sync loop. // These are: // 1) We need to get push rules so we can check if events should bing as we get // them from /sync. // 2) We need to get/create a filter which we can use for /sync. - function getPushRules() { - client.getPushRules().done((result) => { + async function getPushRules() { + try { + const result = await client.getPushRules(); debuglog("Got push rules"); client.pushRules = result; - - getFilter(); // Now get the filter and start syncing - }, (err) => { - self._startKeepAlives().done(() => { - getPushRules(); - }); - self._updateSyncState("ERROR", { error: err }); - }); + } catch (err) { + // wait for saved sync to complete before doing anything else, + // otherwise the sync state will end up being incorrect + await self.recoverFromSyncStartupError(savedSyncPromise, err); + getPushRules(); + return; + } + getFilter(); // Now get the filter and start syncing } - function getFilter() { + async function getFilter() { let filter; if (self.opts.filter) { filter = self.opts.filter; @@ -440,40 +455,55 @@ SyncApi.prototype.sync = function() { filter.setTimelineLimit(self.opts.initialSyncLimit); } - client.getOrCreateFilter( - getFilterName(client.credentials.userId), filter, - ).done((filterId) => { - // reset the notifications timeline to prepare it to paginate from - // the current point in time. - // The right solution would be to tie /sync pagination tokens into - // /notifications API somehow. - client.resetNotifTimelineSet(); - - self._sync({ filterId }); - }, (err) => { - self._startKeepAlives().done(function() { - getFilter(); - }); - self._updateSyncState("ERROR", { error: err }); - }); + let filterId; + try { + filterId = await client.getOrCreateFilter( + getFilterName(client.credentials.userId), filter, + ); + } catch (err) { + // wait for saved sync to complete before doing anything else, + // otherwise the sync state will end up being incorrect + await self.recoverFromSyncStartupError(savedSyncPromise, err); + getFilter(); + return; + } + // reset the notifications timeline to prepare it to paginate from + // the current point in time. + // The right solution would be to tie /sync pagination tokens into + // /notifications API somehow. + client.resetNotifTimelineSet(); + + if (self._currentSyncRequest === null) { + // Send this first sync request here so we can then wait for the saved + // sync data to finish processing before we process the results of this one. + console.log("Sending first sync request..."); + self._currentSyncRequest = self._doSyncRequest({ filterId }, savedSyncToken); + } + + // Now wait for the saved sync to finish... + await savedSyncPromise; + self._sync({ filterId }); } if (client.isGuest()) { // no push rules for guests, no access to POST filter for guests. self._sync({}); } else { - // Before fetching push rules, fetching the filter and syncing, check - // for persisted /sync data and use that if present. - client.store.getSavedSync().then((savedSync) => { + // Pull the saved sync token out first, before the worker starts sending + // all the sync data which could take a while. This will let us send our + // first incremental sync request before we've processed our saved data. + savedSyncPromise = client.store.getSavedSyncToken().then((tok) => { + savedSyncToken = tok; + return client.store.getSavedSync(); + }).then((savedSync) => { if (savedSync) { return self._syncFromCache(savedSync); } - }).then(() => { - // Get push rules and start syncing after getting the saved sync - // to handle the case where we needed the `nextBatch` token to - // start syncing from. - getPushRules(); }); + // Now start the first incremental sync request: this can also + // take a while so if we set it going now, we can wait for it + // to finish while we process our saved sync data. + getPushRules(); } }; @@ -565,70 +595,20 @@ SyncApi.prototype._sync = async function(syncOptions) { return; } - let filterId = syncOptions.filterId; - if (client.isGuest() && !filterId) { - filterId = this._getGuestFilter(); - } - const syncToken = client.store.getSyncToken(); - let pollTimeout = this.opts.pollTimeout; - - if (this.getSyncState() !== 'SYNCING' || this._catchingUp) { - // unless we are happily syncing already, we want the server to return - // as quickly as possible, even if there are no events queued. This - // serves two purposes: - // - // * When the connection dies, we want to know asap when it comes back, - // so that we can hide the error from the user. (We don't want to - // have to wait for an event or a timeout). - // - // * We want to know if the server has any to_device messages queued up - // for us. We do that by calling it with a zero timeout until it - // doesn't give us any more to_device messages. - this._catchingUp = true; - pollTimeout = 0; - } - - // normal timeout= plus buffer time - const clientSideTimeoutMs = pollTimeout + BUFFER_PERIOD_MS; - - const qps = { - filter: filterId, - timeout: pollTimeout, - }; - - if (this.opts.disablePresence) { - qps.set_presence = "offline"; - } - - if (syncToken) { - qps.since = syncToken; - } else { - // use a cachebuster for initialsyncs, to make sure that - // we don't get a stale sync - // (https://github.com/vector-im/vector-web/issues/1354) - qps._cacheBuster = Date.now(); - } - - if (this.getSyncState() == 'ERROR' || this.getSyncState() == 'RECONNECTING') { - // we think the connection is dead. If it comes back up, we won't know - // about it till /sync returns. If the timeout= is high, this could - // be a long time. Set it to 0 when doing retries so we don't have to wait - // for an event or a timeout before emiting the SYNCING event. - qps.timeout = 0; - } - let data; try { //debuglog('Starting sync since=' + syncToken); - this._currentSyncRequest = client._http.authedRequest( - undefined, "GET", "/sync", qps, undefined, clientSideTimeoutMs, - ); + if (this._currentSyncRequest === null) { + this._currentSyncRequest = this._doSyncRequest(syncOptions, syncToken); + } data = await this._currentSyncRequest; } catch (e) { this._onSyncError(e, syncOptions); return; + } finally { + this._currentSyncRequest = null; } //debuglog('Completed sync, next_batch=' + data.next_batch); @@ -699,6 +679,67 @@ SyncApi.prototype._sync = async function(syncOptions) { this._sync(syncOptions); }; +SyncApi.prototype._doSyncRequest = function(syncOptions, syncToken) { + const qps = this._getSyncParams(syncOptions, syncToken); + return this.client._http.authedRequest( + undefined, "GET", "/sync", qps, undefined, + qps.timeout + BUFFER_PERIOD_MS, + ); +}; + +SyncApi.prototype._getSyncParams = function(syncOptions, syncToken) { + let pollTimeout = this.opts.pollTimeout; + + if (this.getSyncState() !== 'SYNCING' || this._catchingUp) { + // unless we are happily syncing already, we want the server to return + // as quickly as possible, even if there are no events queued. This + // serves two purposes: + // + // * When the connection dies, we want to know asap when it comes back, + // so that we can hide the error from the user. (We don't want to + // have to wait for an event or a timeout). + // + // * We want to know if the server has any to_device messages queued up + // for us. We do that by calling it with a zero timeout until it + // doesn't give us any more to_device messages. + this._catchingUp = true; + pollTimeout = 0; + } + + let filterId = syncOptions.filterId; + if (this.client.isGuest() && !filterId) { + filterId = this._getGuestFilter(); + } + + const qps = { + filter: filterId, + timeout: pollTimeout, + }; + + if (this.opts.disablePresence) { + qps.set_presence = "offline"; + } + + if (syncToken) { + qps.since = syncToken; + } else { + // use a cachebuster for initialsyncs, to make sure that + // we don't get a stale sync + // (https://github.com/vector-im/vector-web/issues/1354) + qps._cacheBuster = Date.now(); + } + + if (this.getSyncState() == 'ERROR' || this.getSyncState() == 'RECONNECTING') { + // we think the connection is dead. If it comes back up, we won't know + // about it till /sync returns. If the timeout= is high, this could + // be a long time. Set it to 0 when doing retries so we don't have to wait + // for an event or a timeout before emiting the SYNCING event. + qps.timeout = 0; + } + + return qps; +}; + SyncApi.prototype._onSyncError = function(err, syncOptions) { if (!this._running) { debuglog("Sync no longer running: exiting"); From c18264c615f836f0ba6d705358091723315add57 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 19 Mar 2018 12:08:11 +0000 Subject: [PATCH 19/20] Prepare changelog for v0.10.0-rc.1 --- CHANGELOG.md | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57f9369a3fe..e26aa211b66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,78 @@ +Changes in [0.10.0-rc.1](https://github.com/matrix-org/matrix-js-sdk/releases/tag/v0.10.0-rc.1) (2018-03-19) +============================================================================================================ +[Full Changelog](https://github.com/matrix-org/matrix-js-sdk/compare/v0.9.2...v0.10.0-rc.1) + + * Fix duplicated state events in timeline from peek + [\#630](https://github.com/matrix-org/matrix-js-sdk/pull/630) + * Create indexeddb worker when starting the store + [\#627](https://github.com/matrix-org/matrix-js-sdk/pull/627) + * Fix indexeddb logging + [\#626](https://github.com/matrix-org/matrix-js-sdk/pull/626) + * Don't do /keys/changes on incremental sync + [\#625](https://github.com/matrix-org/matrix-js-sdk/pull/625) + * Don't mark devicelist dirty unnecessarily + [\#623](https://github.com/matrix-org/matrix-js-sdk/pull/623) + * Cache the joined member count for a room state + [\#619](https://github.com/matrix-org/matrix-js-sdk/pull/619) + * Fix JS doc + [\#618](https://github.com/matrix-org/matrix-js-sdk/pull/618) + * Precompute push actions for state events + [\#617](https://github.com/matrix-org/matrix-js-sdk/pull/617) + * Fix bug where global "Never send to unverified..." is ignored + [\#616](https://github.com/matrix-org/matrix-js-sdk/pull/616) + * Intern legacy top-level 'membership' field + [\#615](https://github.com/matrix-org/matrix-js-sdk/pull/615) + * Don't synthesize RR for m.room.redaction as causes the RR to go missing. + [\#598](https://github.com/matrix-org/matrix-js-sdk/pull/598) + * Make Events create Dates on demand + [\#613](https://github.com/matrix-org/matrix-js-sdk/pull/613) + * Stop cloning events when adding to state + [\#612](https://github.com/matrix-org/matrix-js-sdk/pull/612) + * De-dup code: use the initialiseState function + [\#611](https://github.com/matrix-org/matrix-js-sdk/pull/611) + * Create sentinel members on-demand + [\#610](https://github.com/matrix-org/matrix-js-sdk/pull/610) + * Some more doc on how sentinels work + [\#609](https://github.com/matrix-org/matrix-js-sdk/pull/609) + * Migrate room encryption store to crypto store + [\#597](https://github.com/matrix-org/matrix-js-sdk/pull/597) + * add parameter to getIdentityServerUrl to strip the protocol for invites + [\#600](https://github.com/matrix-org/matrix-js-sdk/pull/600) + * Move Device Tracking Data to Crypto Store + [\#594](https://github.com/matrix-org/matrix-js-sdk/pull/594) + * Optimise pushprocessor + [\#591](https://github.com/matrix-org/matrix-js-sdk/pull/591) + * Set event error before emitting + [\#592](https://github.com/matrix-org/matrix-js-sdk/pull/592) + * Add event type for stickers [WIP] + [\#590](https://github.com/matrix-org/matrix-js-sdk/pull/590) + * Migrate inbound sessions to cryptostore + [\#587](https://github.com/matrix-org/matrix-js-sdk/pull/587) + * Disambiguate names if they contain an mxid + [\#588](https://github.com/matrix-org/matrix-js-sdk/pull/588) + * Check for sessions in indexeddb before migrating + [\#585](https://github.com/matrix-org/matrix-js-sdk/pull/585) + * Emit an event for crypto store migration + [\#586](https://github.com/matrix-org/matrix-js-sdk/pull/586) + * Supporting fixes For making UnknownDeviceDialog not pop up automatically + [\#575](https://github.com/matrix-org/matrix-js-sdk/pull/575) + * Move sessions to the crypto store + [\#584](https://github.com/matrix-org/matrix-js-sdk/pull/584) + * Change crypto store transaction API + [\#582](https://github.com/matrix-org/matrix-js-sdk/pull/582) + * Add some missed copyright notices + [\#581](https://github.com/matrix-org/matrix-js-sdk/pull/581) + * Move Olm account to IndexedDB + [\#579](https://github.com/matrix-org/matrix-js-sdk/pull/579) + * Fix logging of DecryptionErrors to be more useful + [\#580](https://github.com/matrix-org/matrix-js-sdk/pull/580) + * [BREAKING] Change the behaviour of the unverfied devices blacklist flag + [\#568](https://github.com/matrix-org/matrix-js-sdk/pull/568) + * Support set_presence=offline for syncing + [\#557](https://github.com/matrix-org/matrix-js-sdk/pull/557) + * Consider cases where the sender may not redact their own event + [\#556](https://github.com/matrix-org/matrix-js-sdk/pull/556) + Changes in [0.9.2](https://github.com/matrix-org/matrix-js-sdk/releases/tag/v0.9.2) (2017-12-04) ================================================================================================ [Full Changelog](https://github.com/matrix-org/matrix-js-sdk/compare/v0.9.1...v0.9.2) From aa18eeb7d6250a534d79b08e9e71eb542e7c8097 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 19 Mar 2018 12:08:11 +0000 Subject: [PATCH 20/20] v0.10.0-rc.1 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e445b7e0291..bed513d95cb 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "matrix-js-sdk", - "version": "0.9.2", + "version": "0.10.0-rc.1", "description": "Matrix Client-Server SDK for Javascript", "main": "index.js", "scripts": {