From a78f923a727cab99f09dd29b2e316af88e5e567a Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Fri, 6 May 2016 03:08:22 -0400
Subject: [PATCH] Sync engine cleanup
- Use custom exception for user-initiated sync cancellations, which can bubble
up to the sync runner -- this should help with a sync stop button (#915)
- Separate out deletions-downloading code
- Refactor delay generator handling on library version mismatch
- Clearer variable names
---
.../content/zotero/xpcom/sync/syncEngine.js | 339 +++++++++---------
.../content/zotero/xpcom/sync/syncRunner.js | 14 +-
components/zotero-service.js | 1 +
test/tests/syncEngineTest.js | 8 +-
test/tests/syncRunnerTest.js | 45 +++
5 files changed, 235 insertions(+), 172 deletions(-)
diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js
index d1d64cff0..49f90f49e 100644
--- a/chrome/content/zotero/xpcom/sync/syncEngine.js
+++ b/chrome/content/zotero/xpcom/sync/syncEngine.js
@@ -75,7 +75,6 @@ Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CONTINUE = 1;
Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CHANGES_TO_UPLOAD = 2;
Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_NO_CHANGES_TO_UPLOAD = 3;
Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_RESTART = 4;
-Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CANCEL = 5;
Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_SUCCESS = 1;
Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_NOTHING_TO_UPLOAD = 2;
@@ -151,9 +150,6 @@ Zotero.Sync.Data.Engine.prototype.start = Zotero.Promise.coroutine(function* ()
case this.UPLOAD_RESULT_LIBRARY_CONFLICT:
downloadResult = yield this._startDownload();
Zotero.debug("Download result is " + downloadResult, 4);
- if (downloadResult == this.DOWNLOAD_RESULT_CANCEL) {
- break sync;
- }
if (!gen) {
var gen = Zotero.Utilities.Internal.delayGenerator(
@@ -200,17 +196,17 @@ Zotero.Sync.Data.Engine.prototype.stop = function () {
Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(function* () {
var localChanges = false;
var libraryVersion = this.library.libraryVersion;
- var lastLibraryVersion;
+ var newLibraryVersion;
- var gen = Zotero.Utilities.Internal.delayGenerator(
+ this.downloadDelayGenerator = Zotero.Utilities.Internal.delayGenerator(
Zotero.Sync.Data.delayIntervals, 60 * 60 * 1000
);
loop:
while (true) {
// Get synced settings first, since they affect how other data is displayed
- lastLibraryVersion = yield this._downloadSettings(libraryVersion);
- if (lastLibraryVersion === false) {
+ newLibraryVersion = yield this._downloadSettings(libraryVersion);
+ if (newLibraryVersion === false) {
break;
}
@@ -222,14 +218,13 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
// For items, fetch top-level items first
//
- // The next run will then see the same items in the non-top versions request,
+ // The next run below will then see the same items in the non-top versions request,
// but they'll have been downloaded already and will be skipped.
if (objectType == 'item') {
let result = yield this._downloadUpdatedObjects(
objectType,
libraryVersion,
- lastLibraryVersion,
- gen,
+ newLibraryVersion,
{
top: true
}
@@ -237,164 +232,28 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
if (result == this.DOWNLOAD_RESULT_RESTART) {
continue loop;
}
- else if (result == this.DOWNLOAD_RESULT_CANCEL) {
- return this.DOWNLOAD_RESULT_CANCEL;
- }
}
let result = yield this._downloadUpdatedObjects(
objectType,
libraryVersion,
- lastLibraryVersion,
- gen
+ newLibraryVersion
);
if (result == this.DOWNLOAD_RESULT_RESTART) {
continue loop;
}
- else if (result == this.DOWNLOAD_RESULT_CANCEL) {
- return this.DOWNLOAD_RESULT_CANCEL;
- }
}
- //
- // Get deleted objects
- //
- let results = yield this.apiClient.getDeleted(
- this.library.libraryType,
- this.libraryTypeID,
- libraryVersion
- );
- if (lastLibraryVersion) {
- // If something else modified the remote library while we were getting updates,
- // wait for increasing amounts of time before trying again, and then start from
- // the beginning
- if (lastLibraryVersion != results.libraryVersion) {
- Zotero.logError("Library version changed since last download -- restarting sync");
- let keepGoing = yield gen.next();
- if (!keepGoing) {
- throw new Error("Could not update " + this.library.name + " -- library in use");
- }
- continue loop;
- }
- }
- else {
- lastLibraryVersion = results.libraryVersion;
- }
-
- var numObjects = Object.keys(results.deleted).reduce((n, k) => n + results.deleted[k].length, 0);
- if (numObjects) {
- Zotero.debug(numObjects + " objects deleted remotely since last check");
-
- // Process deletions
- for (let objectTypePlural in results.deleted) {
- let objectType = Zotero.DataObjectUtilities.getObjectTypeSingular(objectTypePlural);
- let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType);
- let toDelete = [];
- let conflicts = [];
- for (let key of results.deleted[objectTypePlural]) {
- // TODO: Remove from request?
- if (objectType == 'tag') {
- continue;
- }
-
- if (objectType == 'setting') {
- let meta = Zotero.SyncedSettings.getMetadata(this.libraryID, key);
- if (!meta) {
- continue;
- }
- if (meta.synced) {
- yield Zotero.SyncedSettings.clear(this.libraryID, key, {
- skipDeleteLog: true
- });
- }
-
- // Ignore setting if changed locally
- continue;
- }
-
- let obj = objectsClass.getByLibraryAndKey(this.libraryID, key);
- if (!obj) {
- continue;
- }
- if (obj.synced) {
- toDelete.push(obj);
- }
- // Conflict resolution
- else if (objectType == 'item') {
- conflicts.push({
- left: obj.toJSON(),
- right: {
- deleted: true
- }
- });
- }
-
- // Ignore deletion if collection/search changed locally
- }
-
- if (conflicts.length) {
- conflicts.sort(function (a, b) {
- var d1 = a.left.dateModified;
- var d2 = b.left.dateModified;
- if (d1 > d2) {
- return 1
- }
- if (d1 < d2) {
- return -1;
- }
- return 0;
- });
- var mergeData = Zotero.Sync.Data.Local.showConflictResolutionWindow(conflicts);
- if (!mergeData) {
- Zotero.debug("Cancelling sync");
- return this.DOWNLOAD_RESULT_CANCEL;
- }
- let concurrentObjects = 50;
- yield Zotero.Utilities.Internal.forEachChunkAsync(
- mergeData,
- concurrentObjects,
- function (chunk) {
- return Zotero.DB.executeTransaction(function* () {
- for (let json of chunk) {
- if (!json.deleted) continue;
- let obj = objectsClass.getByLibraryAndKey(
- this.libraryID, json.key
- );
- if (!obj) {
- Zotero.logError("Remotely deleted " + objectType
- + " didn't exist after conflict resolution");
- continue;
- }
- yield obj.erase({
- skipEditCheck: true
- });
- }
- }.bind(this));
- }.bind(this)
- );
- }
-
- if (toDelete.length) {
- yield Zotero.DB.executeTransaction(function* () {
- for (let obj of toDelete) {
- yield obj.erase({
- skipEditCheck: true,
- skipDeleteLog: true
- });
- }
- });
- }
- }
- }
- else {
- Zotero.debug("No objects deleted remotely since last check");
+ let deletionsResult = yield this._downloadDeletions(libraryVersion, newLibraryVersion);
+ if (deletionsResult == this.DOWNLOAD_RESULT_RESTART) {
+ continue loop;
}
break;
}
- if (lastLibraryVersion) {
- yield Zotero.Libraries.setVersion(this.libraryID, lastLibraryVersion);
+ if (newLibraryVersion) {
+ yield Zotero.Libraries.setVersion(this.libraryID, newLibraryVersion);
}
return localChanges
@@ -404,15 +263,15 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
/**
- * @param {Integer} libraryVersion - Last library version
+ * @param {Integer} since - Last-known library version; get changes since this version
* @return {Integer|Boolean} - Library version returned from server, or false if no changes since
* specified version
*/
-Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(function* (libraryVersion) {
+Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(function* (since) {
let results = yield this.apiClient.getSettings(
this.library.libraryType,
this.libraryTypeID,
- libraryVersion
+ since
);
// If library version hasn't changed remotely, the local library is up-to-date and we
// can skip all remaining downloads
@@ -444,9 +303,14 @@ Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(f
/**
* Get versions of objects updated remotely since the last sync time and kick off object downloading
*
+ * @param {String} objectType
+ * @param {Integer} since - Last-known library version; get changes sinces this version
+ * @param {Integer} newLibraryVersion - Last library version seen in this sync process; if newer version
+ * is seen, restart the sync
+ * @param {Object} [options]
* @return {Promise<Integer>} - A download result code (this.DOWNLOAD_RESULT_*)
*/
-Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.coroutine(function* (objectType, libraryVersion, lastLibraryVersion, delayGenerator, options = {}) {
+Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.coroutine(function* (objectType, since, newLibraryVersion, options = {}) {
var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType);
var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType);
@@ -454,8 +318,8 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou
Zotero.debug(`Checking for updated ${options.top ? 'top-level ' : ''}`
+ `${objectTypePlural} in ${this.library.name}`);
var queryParams = {};
- if (libraryVersion) {
- queryParams.since = libraryVersion;
+ if (since) {
+ queryParams.since = since;
}
if (options.top) {
queryParams.top = true;
@@ -473,13 +337,8 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou
// If something else modified the remote library while we were getting updates,
// wait for increasing amounts of time before trying again, and then start from
// the beginning
- if (lastLibraryVersion != results.libraryVersion) {
- Zotero.logError("Library version changed since last download -- restarting sync");
- let keepGoing = yield delayGenerator.next();
- if (!keepGoing) {
- throw new Error("Could not update " + this.library.name + " -- library in use");
- }
- return this.DOWNLOAD_RESULT_RESTART;
+ if (newLibraryVersion != results.libraryVersion) {
+ return this._onLibraryVersionChange();
}
@@ -708,7 +567,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
// Keys can be unprocessed if conflict resolution is cancelled
let keys = results.filter(x => x.processed).map(x => x.key);
if (!keys.length) {
- return this.DOWNLOAD_RESULT_CANCEL;
+ throw new Zotero.Sync.UserCancelledException();
}
yield Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, keys);
}
@@ -717,6 +576,152 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
});
+/**
+ * Get deleted objects from the API and process them
+ *
+ * @param {Integer} since - Last-known library version; get changes sinces this version
+ * @param {Integer} newLibraryVersion - Newest library version seen in this sync process; if newer version
+ * is seen, restart the sync
+ * @return {Promise<Integer>} - A download result code (this.DOWNLOAD_RESULT_*)
+ */
+Zotero.Sync.Data.Engine.prototype._downloadDeletions = Zotero.Promise.coroutine(function* (since, newLibraryVersion) {
+ let results = yield this.apiClient.getDeleted(
+ this.library.libraryType,
+ this.libraryTypeID,
+ since
+ );
+ if (newLibraryVersion != results.libraryVersion) {
+ return this._onLibraryVersionChange();
+ }
+
+ var numObjects = Object.keys(results.deleted).reduce((n, k) => n + results.deleted[k].length, 0);
+ if (!numObjects) {
+ Zotero.debug("No objects deleted remotely since last check");
+ return this.DOWNLOAD_RESULT_CONTINUE;
+ }
+
+ Zotero.debug(numObjects + " objects deleted remotely since last check");
+
+ // Process deletions
+ for (let objectTypePlural in results.deleted) {
+ let objectType = Zotero.DataObjectUtilities.getObjectTypeSingular(objectTypePlural);
+ let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType);
+ let toDelete = [];
+ let conflicts = [];
+ for (let key of results.deleted[objectTypePlural]) {
+ // TODO: Remove from request?
+ if (objectType == 'tag') {
+ continue;
+ }
+
+ if (objectType == 'setting') {
+ let meta = Zotero.SyncedSettings.getMetadata(this.libraryID, key);
+ if (!meta) {
+ continue;
+ }
+ if (meta.synced) {
+ yield Zotero.SyncedSettings.clear(this.libraryID, key, {
+ skipDeleteLog: true
+ });
+ }
+
+ // Ignore setting if changed locally
+ continue;
+ }
+
+ let obj = objectsClass.getByLibraryAndKey(this.libraryID, key);
+ if (!obj) {
+ continue;
+ }
+ if (obj.synced) {
+ toDelete.push(obj);
+ }
+ // Conflict resolution
+ else if (objectType == 'item') {
+ conflicts.push({
+ left: obj.toJSON(),
+ right: {
+ deleted: true
+ }
+ });
+ }
+
+ // Ignore deletion if collection/search changed locally
+ }
+
+ if (conflicts.length) {
+ // Sort conflicts by Date Modified
+ conflicts.sort(function (a, b) {
+ var d1 = a.left.dateModified;
+ var d2 = b.left.dateModified;
+ if (d1 > d2) {
+ return 1
+ }
+ if (d1 < d2) {
+ return -1;
+ }
+ return 0;
+ });
+ var mergeData = Zotero.Sync.Data.Local.showConflictResolutionWindow(conflicts);
+ if (!mergeData) {
+ Zotero.debug("Cancelling sync");
+ throw new Zotero.Sync.UserCancelledException();
+ }
+ let concurrentObjects = 50;
+ yield Zotero.Utilities.Internal.forEachChunkAsync(
+ mergeData,
+ concurrentObjects,
+ function (chunk) {
+ return Zotero.DB.executeTransaction(function* () {
+ for (let json of chunk) {
+ if (!json.deleted) continue;
+ let obj = objectsClass.getByLibraryAndKey(
+ this.libraryID, json.key
+ );
+ if (!obj) {
+ Zotero.logError("Remotely deleted " + objectType
+ + " didn't exist after conflict resolution");
+ continue;
+ }
+ yield obj.erase({
+ skipEditCheck: true
+ });
+ }
+ }.bind(this));
+ }.bind(this)
+ );
+ }
+
+ if (toDelete.length) {
+ yield Zotero.DB.executeTransaction(function* () {
+ for (let obj of toDelete) {
+ yield obj.erase({
+ skipEditCheck: true,
+ skipDeleteLog: true
+ });
+ }
+ });
+ }
+ }
+
+ return this.DOWNLOAD_RESULT_CONTINUE;
+});
+
+
+/**
+ * If something else modified the remote library while we were getting updates, wait for increasing
+ * amounts of time before trying again, and then start from the beginning
+ */
+Zotero.Sync.Data.Engine.prototype._onLibraryVersionChange = Zotero.Promise.coroutine(function* (mode) {
+ Zotero.logError("Library version changed since last download -- restarting sync");
+ let keepGoing = yield this.downloadDelayGenerator.next();
+ if (!keepGoing) {
+ throw new Error("Could not update " + this.library.name + " -- library in use");
+ }
+ return this.DOWNLOAD_RESULT_RESTART;
+});
+
+
/**
* Get unsynced objects, build upload JSON, and start API requests
*
diff --git a/chrome/content/zotero/xpcom/sync/syncRunner.js b/chrome/content/zotero/xpcom/sync/syncRunner.js
index 188ea1da5..2a3bdf527 100644
--- a/chrome/content/zotero/xpcom/sync/syncRunner.js
+++ b/chrome/content/zotero/xpcom/sync/syncRunner.js
@@ -213,7 +213,10 @@ Zotero.Sync.Runner_Module = function (options = {}) {
}
}
catch (e) {
- if (options.onError) {
+ if (e instanceof Zotero.Sync.UserCancelledException) {
+ Zotero.debug("Sync was cancelled");
+ }
+ else if (options.onError) {
options.onError(e);
}
else {
@@ -509,6 +512,15 @@ Zotero.Sync.Runner_Module = function (options = {}) {
successfulLibraries.push(libraryID);
}
catch (e) {
+ if (e instanceof Zotero.Sync.UserCancelledException) {
+ if (e.advanceToNextLibrary) {
+ Zotero.debug("Sync cancelled for library " + libraryID + " -- "
+ + "advancing to next library");
+ continue;
+ }
+ throw e;
+ }
+
Zotero.debug("Sync failed for library " + libraryID);
Zotero.logError(e);
this.checkError(e);
diff --git a/components/zotero-service.js b/components/zotero-service.js
index eb73167e7..8ddd7afd6 100644
--- a/components/zotero-service.js
+++ b/components/zotero-service.js
@@ -104,6 +104,7 @@ const xpcomFilesLocal = [
'sync',
'sync/syncAPIClient',
'sync/syncEngine',
+ 'sync/syncExceptions',
'sync/syncEventListeners',
'sync/syncFullTextEngine',
'sync/syncLocal',
diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js
index cca9ddc3f..9d6ee64f4 100644
--- a/test/tests/syncEngineTest.js
+++ b/test/tests/syncEngineTest.js
@@ -1631,8 +1631,8 @@ describe("Zotero.Sync.Data.Engine", function () {
var wizard = doc.documentElement;
wizard.getButton('cancel').click();
})
- var downloadResult = yield engine._startDownload();
- assert.equal(downloadResult, engine.DOWNLOAD_RESULT_CANCEL);
+ var e = yield getPromiseError(engine._startDownload());
+ assert.isTrue(e instanceof Zotero.Sync.UserCancelledException);
// Non-conflicted item should be saved
assert.ok(Zotero.Items.getIDFromLibraryAndKey(library.id, "AAAAAAAA"));
@@ -1725,8 +1725,8 @@ describe("Zotero.Sync.Data.Engine", function () {
var wizard = doc.documentElement;
wizard.getButton('cancel').click();
})
- var downloadResult = yield engine._startDownload();
- assert.equal(downloadResult, engine.DOWNLOAD_RESULT_CANCEL);
+ var e = yield getPromiseError(engine._startDownload());
+ assert.isTrue(e instanceof Zotero.Sync.UserCancelledException);
// Conflicted items should still exists
assert.isTrue(Zotero.Items.exists(itemID1));
diff --git a/test/tests/syncRunnerTest.js b/test/tests/syncRunnerTest.js
index 2fe130641..c90be9eaa 100644
--- a/test/tests/syncRunnerTest.js
+++ b/test/tests/syncRunnerTest.js
@@ -670,6 +670,51 @@ describe("Zotero.Sync.Runner", function () {
assert.isAbove(lastSyncTime, new Date().getTime() - 1000);
assert.isBelow(lastSyncTime, new Date().getTime());
})
+
+
+ it("should handle user-initiated cancellation", function* () {
+ setResponse('keyInfo.fullAccess');
+ setResponse('userGroups.groupVersions');
+ setResponse('groups.ownerGroup');
+ setResponse('groups.memberGroup');
+
+ var stub = sinon.stub(Zotero.Sync.Data.Engine.prototype, "start");
+
+ stub.onCall(0).returns(Zotero.Promise.resolve());
+ var e = new Zotero.Sync.UserCancelledException();
+ e.handledRejection = true;
+ stub.onCall(1).returns(Zotero.Promise.reject(e));
+ // Shouldn't be reached
+ stub.onCall(2).throws();
+
+ yield runner.sync({
+ onError: e => { throw e },
+ });
+
+ stub.restore();
+ });
+
+
+ it("should handle user-initiated cancellation for current library", function* () {
+ setResponse('keyInfo.fullAccess');
+ setResponse('userGroups.groupVersions');
+ setResponse('groups.ownerGroup');
+ setResponse('groups.memberGroup');
+
+ var stub = sinon.stub(Zotero.Sync.Data.Engine.prototype, "start");
+
+ stub.returns(Zotero.Promise.resolve());
+ var e = new Zotero.Sync.UserCancelledException(true);
+ e.handledRejection = true;
+ stub.onCall(1).returns(Zotero.Promise.reject(e));
+
+ yield runner.sync({
+ onError: e => { throw e },
+ });
+
+ assert.equal(stub.callCount, 4);
+ stub.restore();
+ });
})