From 7ace5ea29e1ebb2382b51ea741bb29fbbf775de1 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 14 May 2021 03:26:14 -0400 Subject: [PATCH] Fix unnecessary sync looping after downloading items An extra sync loop would be performed for every object downloaded, so a download to an empty database could result in a huge number of unnecessary loops. This was a regression from 52932b6eb, which started queuing auto-syncs while a sync was in progress. The fix here is to skip auto-sync for all objects saved from a sync download. There are two new mechanisms involved: - Event-level notifier options that get passed to passed to notify() at the top level of extraData rather than being included with every object (e.g., because `skipAutoSync` should apply to an entire save transaction) - The ability to pass event-level notifier options when initializing a Zotero.Notifier.Queue, such as the one used for sync downloads --- .../content/zotero/xpcom/data/dataObject.js | 8 ++++- chrome/content/zotero/xpcom/notifier.js | 31 ++++++++++++++--- .../zotero/xpcom/sync/syncEventListeners.js | 19 +++++++---- chrome/content/zotero/xpcom/sync/syncLocal.js | 4 ++- test/tests/notifierTest.js | 34 +++++++++++++++++++ test/tests/syncLocalTest.js | 29 ++++++++++++++++ 6 files changed, 113 insertions(+), 12 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js index ec7ce06212..1b09572b37 100644 --- a/chrome/content/zotero/xpcom/data/dataObject.js +++ b/chrome/content/zotero/xpcom/data/dataObject.js @@ -918,13 +918,19 @@ Zotero.DataObject.prototype.save = Zotero.Promise.coroutine(function* (options = } env.notifierData = {}; - // Pass along any 'notifierData' values + // Pass along any 'notifierData' values, which become 'extraData' in notifier events if (env.options.notifierData) { Object.assign(env.notifierData, env.options.notifierData); } if (env.options.skipSelect) { env.notifierData.skipSelect = true; } + // Pass along event-level notifier options, which become top-level extraData properties + for (let option of Zotero.Notifier.EVENT_LEVEL_OPTIONS) { + if (env.options[option]) { + env.notifierData[option] = true; + } + } if (!env.isNew) { env.changed = this._previousData; } diff --git a/chrome/content/zotero/xpcom/notifier.js b/chrome/content/zotero/xpcom/notifier.js index 27c3d10868..59d7a0c56f 100644 --- a/chrome/content/zotero/xpcom/notifier.js +++ b/chrome/content/zotero/xpcom/notifier.js @@ -26,6 +26,9 @@ "use strict"; Zotero.Notifier = new function(){ + // Options that apply to an entire event, not a specific object + this.EVENT_LEVEL_OPTIONS = ['autoSyncDelay', 'skipAutoSync']; + var _observers = {}; var _types = [ 'collection', 'search', 'share', 'share-items', 'item', 'file', @@ -201,6 +204,14 @@ Zotero.Notifier = new function(){ // Merge extraData keys if (extraData) { + // Set event-level options as top-level properties in extraData + for (let option of Zotero.Notifier.EVENT_LEVEL_OPTIONS) { + if (extraData[option]) { + queue[type][event].data[option] = true; + delete extraData[option]; + } + } + // If just a single id, extra data can be keyed by id or passed directly if (ids.length == 1) { let id = ids[0]; @@ -300,11 +311,21 @@ Zotero.Notifier = new function(){ } var queue = {}; - for (let q of queues) { - q = q._queue; + for (let { _queue: q, options } of queues) { for (let type in q) { for (let event in q[type]) { - _mergeEvent(queue, event, type, q[type][event].ids, q[type][event].data); + let extraData = Object.assign({}, q[type][event].data); + // Add options from queue as top-level options in extraData + if (options) { + for (let option in options) { + if (!this.EVENT_LEVEL_OPTIONS.includes(option)) { + throw new Error(`Queue option '${option} must be an event-level option`); + } + extraData[option] = options[option]; + } + } + + _mergeEvent(queue, event, type, q[type][event].ids, extraData); } } } @@ -312,6 +333,7 @@ Zotero.Notifier = new function(){ else if (!_transactionID) { throw new Error("Can't commit outside of transaction"); } + // Use default queue else { var queue = _queue; } @@ -419,9 +441,10 @@ Zotero.Notifier = new function(){ } -Zotero.Notifier.Queue = function () { +Zotero.Notifier.Queue = function (options = {}) { this.id = Zotero.Utilities.randomString(); Zotero.debug("Creating notifier queue " + this.id); this._queue = {}; this.size = 0; + this.options = options; }; diff --git a/chrome/content/zotero/xpcom/sync/syncEventListeners.js b/chrome/content/zotero/xpcom/sync/syncEventListeners.js index 49fabcdb62..694b95f70c 100644 --- a/chrome/content/zotero/xpcom/sync/syncEventListeners.js +++ b/chrome/content/zotero/xpcom/sync/syncEventListeners.js @@ -131,6 +131,19 @@ Zotero.Sync.EventListeners.AutoSyncListener = { return; } + var autoSyncDelay = 0; + if (extraData) { + // Some events (e.g., writes from the server) skip auto-syncing altogether + if (extraData.skipAutoSync) { + return; + } + + // Use a different timeout if specified (e.g., for note editing) + if (extraData.autoSyncDelay) { + autoSyncDelay = extraData.autoSyncDelay; + } + } + // Determine affected libraries so only those can be synced let libraries = []; var fileLibraries = new Set(); @@ -175,13 +188,7 @@ Zotero.Sync.EventListeners.AutoSyncListener = { return; } - var autoSyncDelay = 0; if (type == 'item') { - // Use a different timeout if specified (e.g., for note editing) - if (extraData[ids[0]] && extraData[ids[0]].autoSyncDelay) { - autoSyncDelay = Math.max(autoSyncDelay, extraData[ids[0]].autoSyncDelay); - } - // Check whether file syncing or full-text syncing are necessary if (event == 'add' || event == 'modify' || event == 'index') { for (let id of ids) { diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 1c7aeebf84..4389c42dec 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -807,7 +807,9 @@ Zotero.Sync.Data.Local = { batchSize = options.getNotifierBatchSize() } } - let notifierQueue = new Zotero.Notifier.Queue; + let notifierQueue = new Zotero.Notifier.Queue({ + skipAutoSync: true + }); let jsonObject = json[i]; let jsonData = jsonObject.data; diff --git a/test/tests/notifierTest.js b/test/tests/notifierTest.js index c803370ad7..91bd6f3d05 100644 --- a/test/tests/notifierTest.js +++ b/test/tests/notifierTest.js @@ -78,4 +78,38 @@ describe("Zotero.Notifier", function () { assert.isTrue(promise2.isRejected()); }); }); + + describe("Queue", function () { + describe("#commit()", function () { + it("should add options from queue to extraData", async function () { + var called = false; + var data; + + var notifierID = Zotero.Notifier.registerObserver({ + notify: (event, type, ids, extraData) => { + called = true; + data = extraData; + } + }); + + var notifierQueue = new Zotero.Notifier.Queue({ + skipAutoSync: true + }); + + var item = createUnsavedDataObject('item'); + await item.saveTx({ + notifierQueue + }); + + assert.isFalse(called); + + await Zotero.Notifier.commit(notifierQueue); + + assert.isTrue(called); + assert.propertyVal(data, 'skipAutoSync', true); + + Zotero.Notifier.unregisterObserver(notifierID); + }); + }); + }); }); diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js index 9d9c4f8e56..bb15408f27 100644 --- a/test/tests/syncLocalTest.js +++ b/test/tests/syncLocalTest.js @@ -515,6 +515,35 @@ describe("Zotero.Sync.Data.Local", function() { }); }) + it("shouldn't trigger an auto-sync", async function () { + var libraryID = Zotero.Libraries.userLibraryID; + + var item = createUnsavedDataObject('item'); + let data = item.toJSON(); + data.key = Zotero.DataObjectUtilities.generateKey(); + data.version = 10; + let json = { + key: data.key, + version: 10, + data + }; + + // Make sure the pref in question is still disabled by default during tests + assert.isFalse(Zotero.Prefs.get('sync.autoSync')); + Zotero.Prefs.set('sync.autoSync', true); + var stub = sinon.stub(Zotero.Sync.Runner, 'setSyncTimeout'); + + await Zotero.Sync.Data.Local.processObjectsFromJSON( + 'item', libraryID, [json], { stopOnError: true } + ); + + // setSyncTimeout() shouldn't have been called at all + assert.isFalse(stub.called); + + stub.restore(); + Zotero.Prefs.set('sync.autoSync', false); + }); + it("should update local version number and mark as synced if remote version is identical", function* () { var libraryID = Zotero.Libraries.userLibraryID;