From dc12a2c95ac66936f56693fd5c98937556676df2 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Tue, 23 Mar 2021 03:05:34 -0400 Subject: [PATCH] Stop file upload queue after low-quota errors We weren't making actual upload requests after a quota error if the file would exceed the quota, but we were still going through all attachments to upload, which in some cases involves making stat() calls. We now just stop the queue immediately after a quota error or when starting a new background sync after a previous quota error. Closes #1255 --- .../zotero/xpcom/storage/storageEngine.js | 20 +++-- .../zotero/xpcom/storage/storageLocal.js | 4 + .../zotero/xpcom/storage/storageRequest.js | 5 +- chrome/content/zotero/xpcom/storage/zfs.js | 24 +++++- test/tests/zfsTest.js | 82 +++++++++++++++++-- 5 files changed, 122 insertions(+), 13 deletions(-) diff --git a/chrome/content/zotero/xpcom/storage/storageEngine.js b/chrome/content/zotero/xpcom/storage/storageEngine.js index 9ba53726b6..255d8d6d15 100644 --- a/chrome/content/zotero/xpcom/storage/storageEngine.js +++ b/chrome/content/zotero/xpcom/storage/storageEngine.js @@ -292,10 +292,19 @@ Zotero.Sync.Storage.Engine.prototype.start = Zotero.Promise.coroutine(function* }) -Zotero.Sync.Storage.Engine.prototype.stop = function () { - Zotero.debug("Stopping file sync for " + this.library.name); - for (let type in this.queues) { - this.queues[type].stop(); +/** + * @param {String} [queueToStop] - 'upload' or 'download'; if not specified, stop all queues + */ +Zotero.Sync.Storage.Engine.prototype.stop = function (queueToStop) { + if (queueToStop) { + Zotero.debug(`Stopping file sync ${queueToStop} queue for ` + this.library.name); + this.queues[queueToStop].stop(); + } + else { + Zotero.debug("Stopping file sync for " + this.library.name); + for (let type in this.queues) { + this.queues[type].stop(); + } } } @@ -330,9 +339,10 @@ Zotero.Sync.Storage.Engine.prototype.queueItem = Zotero.Promise.coroutine(functi this.queues[type].add(() => { var request = new Zotero.Sync.Storage.Request({ type, + engine: this, libraryID: this.libraryID, name: item.libraryKey, - onStart: request => this.controller[fn](request), + onStart: request => this.controller[fn](request, this), onStop: () => { this.requestsRemaining--; this.onProgress(this.numRequests - this.requestsRemaining, this.numRequests); diff --git a/chrome/content/zotero/xpcom/storage/storageLocal.js b/chrome/content/zotero/xpcom/storage/storageLocal.js index cf084196cb..4b50b767be 100644 --- a/chrome/content/zotero/xpcom/storage/storageLocal.js +++ b/chrome/content/zotero/xpcom/storage/storageLocal.js @@ -9,6 +9,10 @@ Zotero.Sync.Storage.Local = { SYNC_STATE_FORCE_DOWNLOAD: 4, SYNC_STATE_IN_CONFLICT: 5, + // If last-known remaining storage is below this number of megabytes, skip further upload + // attempts in various situations + STORAGE_REMAINING_MINIMUM: 5, + lastFullFileCheck: {}, uploadCheckFiles: [], storageRemainingForLibrary: new Map(), diff --git a/chrome/content/zotero/xpcom/storage/storageRequest.js b/chrome/content/zotero/xpcom/storage/storageRequest.js index fbbb7ebbaa..573b4ebb91 100644 --- a/chrome/content/zotero/xpcom/storage/storageRequest.js +++ b/chrome/content/zotero/xpcom/storage/storageRequest.js @@ -28,6 +28,7 @@ * Transfer request for storage sync * * @param {Object} options + * @param {Zotero.Sync.Storage.Engine} options.engine * @param {String} options.type * @param {Integer} options.libraryID * @param {String} options.name - Identifier for request (e.g., "[libraryID]/[key]") @@ -36,16 +37,18 @@ * @param {Function|Function[]} [options.onStop] */ Zotero.Sync.Storage.Request = function (options) { + if (!options.engine) throw new Error("engine must be provided"); if (!options.type) throw new Error("type must be provided"); if (!options.libraryID) throw new Error("libraryID must be provided"); if (!options.name) throw new Error("name must be provided"); - ['type', 'libraryID', 'name'].forEach(x => this[x] = options[x]); + ['engine', 'type', 'libraryID', 'name'].forEach(x => this[x] = options[x]); Zotero.debug(`Initializing ${this.type} request ${this.name}`); this.callbacks = ['onStart', 'onProgress', 'onStop']; this.Type = Zotero.Utilities.capitalize(this.type); + this.engine = options.engine; this.channel = null; this.queue = null; this.progress = 0; diff --git a/chrome/content/zotero/xpcom/storage/zfs.js b/chrome/content/zotero/xpcom/storage/zfs.js index 9d9990f9de..0d3e0a9f73 100644 --- a/chrome/content/zotero/xpcom/storage/zfs.js +++ b/chrome/content/zotero/xpcom/storage/zfs.js @@ -284,6 +284,11 @@ Zotero.Sync.Storage.Mode.ZFS.prototype = { } } if (skip) { + // Stop trying to upload files if there's very little storage remaining + if (remaining < Zotero.Sync.Storage.Local.STORAGE_REMAINING_MINIMUM) { + Zotero.debug(`${remaining} MB remaining in storage -- skipping further uploads`); + request.engine.stop('upload'); + } throw yield this._getQuotaError(item); } } @@ -294,7 +299,22 @@ Zotero.Sync.Storage.Mode.ZFS.prototype = { return new Zotero.Sync.Storage.Result; } } - return this._processUploadFile(request); + + try { + return yield this._processUploadFile(request); + } + catch (e) { + // Stop trying to upload files if we hit a quota error and there's very little space + // remaining. If there's more space, we keep going, because it might just be a big file. + if (e.error == Zotero.Error.ERROR_ZFS_OVER_QUOTA) { + let remaining = Zotero.Sync.Storage.Local.storageRemainingForLibrary.get(item.libraryID); + if (remaining < Zotero.Sync.Storage.Local.STORAGE_REMAINING_MINIMUM) { + Zotero.debug(`${remaining} MB remaining in storage -- skipping further uploads`); + request.engine.stop('upload'); + } + } + throw e; + } }), @@ -607,7 +627,7 @@ Zotero.Sync.Storage.Mode.ZFS.prototype = { } // Store the remaining space so that we can skip files bigger than that until the next - // manual sync + // manual sync. Values are in megabytes. let usage = req.getResponseHeader('Zotero-Storage-Usage'); let quota = req.getResponseHeader('Zotero-Storage-Quota'); Zotero.Sync.Storage.Local.storageRemainingForLibrary.set(item.libraryID, quota - usage); diff --git a/test/tests/zfsTest.js b/test/tests/zfsTest.js index e64597701c..afdfd2a2a6 100644 --- a/test/tests/zfsTest.js +++ b/test/tests/zfsTest.js @@ -74,9 +74,10 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () { var setup = Zotero.Promise.coroutine(function* (options = {}) { Components.utils.import("resource://zotero/concurrentCaller.js"); + var stopOnError = options.stopOnError !== undefined ? options.stopOnError : true; var caller = new ConcurrentCaller(1); caller.setLogger(msg => Zotero.debug(msg)); - caller.stopOnError = true; + caller.stopOnError = stopOnError; Components.utils.import("resource://zotero/config.js"); var client = new Zotero.Sync.APIClient({ @@ -93,7 +94,8 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () { apiClient: client, maxS3ConsecutiveFailures: 2 }), - stopOnError: true + background: options.background, + stopOnError }); return { engine, client, caller }; @@ -802,6 +804,75 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () { var result = yield engine.start(); assert.equal(called, 4); }); + + + it("should stop uploading files on quota error", async function () { + var { engine, client, caller } = await setup({ stopOnError: false }); + + var numItems = 4; + var items = []; + for (let i = 0; i < numItems; i++) { + let item = await importFileAttachment('test.png'); + item.version = 5; + item.synced = true; + await item.saveTx(); + items.push(item); + } + + var requests = 0; + server.respond(function (req) { + if (req.method == "POST" + && req.url.startsWith(`${baseURL}users/1/items/`) + && req.url.endsWith('/file') + && req.requestBody.indexOf('upload=') == -1 + && req.requestHeaders["If-None-Match"] == "*") { + requests++; + req.respond( + 413, + { + "Content-Type": "application/json", + "Last-Modified-Version": 10, + "Zotero-Storage-Usage": "300", + "Zotero-Storage-Quota": "300" + }, + "File would exceed quota (299.7 + 0.5 > 300)" + ); + } + }) + + await engine.start(); + assert.equal(requests, Zotero.Prefs.get('sync.storage.maxUploads')); + + Zotero.Sync.Storage.Local.storageRemainingForLibrary.delete(items[0].libraryID); + }); + + + // If there was a quota error in a previous run and remaining storage was determined to be + // very low, stop further file uploads for a background sync even when we bail without an + // HTTP request. A manual sync clears the remaining-storage value. + it("should stop uploading files for background sync if no storage remaining after previous quota error", async function () { + var { engine, client, caller } = await setup({ background: true, stopOnError: false }); + + var numItems = 4; + var items = []; + for (let i = 0; i < numItems; i++) { + let item = await importFileAttachment('test.png'); + item.version = 5; + item.synced = true; + await item.saveTx(); + items.push(item); + } + + Zotero.Sync.Storage.Local.storageRemainingForLibrary.set(items[0].libraryID, 0); + + var spy = sinon.spy(engine.controller, 'uploadFile'); + await engine.start() + + assert.equal(spy.callCount, Zotero.Prefs.get('sync.storage.maxUploads')); + spy.restore(); + + Zotero.Sync.Storage.Local.storageRemainingForLibrary.delete(items[0].libraryID); + }); }) @@ -830,7 +901,7 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () { var request = { name: item.libraryKey }; - var stub = sinon.stub(zfs, '_processUploadFile'); + var stub = sinon.stub(zfs, '_processUploadFile').returns(Zotero.Promise.resolve()); await zfs.uploadFile(request); var zipFile = OS.Path.join(Zotero.getTempDirectory().path, item.key + '.zip'); @@ -1057,7 +1128,7 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () { "Zotero-Storage-Usage": "300", "Zotero-Storage-Quota": "300" }, - "File would exceed quota (299.7 + 0.5 > 300)" + "File would exceed quota (299.7 + 0.5 > 300)" ); } }) @@ -1073,7 +1144,8 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () { // Try again var e = yield getPromiseError(zfs.uploadFile({ - name: item.libraryKey + name: item.libraryKey, + engine })); assert.ok(e); assert.equal(e.errorType, 'warning');