From a08c3dee145afe0f6e2aee7a38402696c76c9fda Mon Sep 17 00:00:00 2001 From: Tom Najdek Date: Wed, 29 Sep 2021 18:17:40 +0200 Subject: [PATCH 1/2] Add tests for Zotero.File.download #2216 This test covers couple of scenarios downloading a 16MB file from a local http server (simple and concurrently) using Zotero.File.download. When running current implementation, this seems to fail persistently. In real-world usage, current implementation seems to work intermittently, failing more often for larger files. This intermittent behaviour is most likely cause of #2216. --- test/tests/fileTest.js | 75 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/test/tests/fileTest.js b/test/tests/fileTest.js index 491d7af052..a1b808b2a6 100644 --- a/test/tests/fileTest.js +++ b/test/tests/fileTest.js @@ -429,4 +429,79 @@ describe("Zotero.File", function () { throw new Error("Error not thrown"); }); }); + + describe('#download()', function () { + const sizeInMB = 16; // size of the generated text file + let port, httpd, baseURL; + + before(async function () { + // Real HTTP server + Components.utils.import("resource://zotero-unit/httpd.js"); + port = 16213; + httpd = new HttpServer(); + baseURL = `http://127.0.0.1:${port}`; + httpd.start(port); + httpd.registerPathHandler( + '/file1.txt', + { + handle: function (request, response) { + const text1KB = Array.from({ length: 64 }, _ => "lorem ipsum foo\n").join(''); + const text16MB = Array.from({ length: 1024 * sizeInMB }, _ => text1KB).join(''); + response.setStatusLine(null, 200, "OK"); + response.setHeader('Content-Type', 'text/plain', false); + response.write(text16MB); + } + } + ); + }); + + after(function* () { + var defer = new Zotero.Promise.defer(); + httpd.stop(() => defer.resolve()); + yield defer.promise; + }); + + it("should download a file", async function () { + const url = `${baseURL}/file1.txt`; + const path = OS.Path.join(Zotero.getTempDirectory().path, 'zotero.txt'); + await Zotero.File.download(url, path); + const fileSize = (await OS.File.stat(path)).size; + assert.equal(fileSize, 1024 * 1024 * sizeInMB); + }); + + it("should concurrently download three large files", async function () { + const url = `${baseURL}/file1.txt`; + + var caller = new ConcurrentCaller({ + numConcurrent: 3, + Promise: Zotero.Promise, + }); + + let failed = false; + + const fetchFile = async (srcUrl, targetPath) => { + try { + await Zotero.File.download(srcUrl, targetPath); + } + catch (e) { + failed = true; + throw e; + } + }; + + + caller.add(() => fetchFile(url, OS.Path.join(Zotero.getTempDirectory().path, 'zotero-1.txt'))); + caller.add(() => fetchFile(url, OS.Path.join(Zotero.getTempDirectory().path, 'zotero-2.txt'))); + caller.add(() => fetchFile(url, OS.Path.join(Zotero.getTempDirectory().path, 'zotero-3.txt'))); + + await caller.runAll(); + + assert.isFalse(failed); + for (let i = 1; i < 4; i++) { + const path = OS.Path.join(Zotero.getTempDirectory().path, `zotero-${i}.txt`); + const fileSize = (await OS.File.stat(path)).size; + assert.equal(fileSize, 1024 * 1024 * sizeInMB); + } + }); + }); }) From a745cde2cfd6c85520e59eff04bd39bc269cf9c0 Mon Sep 17 00:00:00 2001 From: Tom Najdek Date: Sat, 2 Oct 2021 00:34:51 +0200 Subject: [PATCH 2/2] New implementation of a download function #2216 This resolves a problem where, in certain scenarios, Zotero.file.download throws an exception even though file is successfully downloaded. Furthermore this new download function should be more memory-efficient, improving performance when dealing with large files. --- chrome/content/zotero/xpcom/file.js | 70 ++++++++++++++++++----------- 1 file changed, 45 insertions(+), 25 deletions(-) diff --git a/chrome/content/zotero/xpcom/file.js b/chrome/content/zotero/xpcom/file.js index 57320b2502..4e8a3031bb 100644 --- a/chrome/content/zotero/xpcom/file.js +++ b/chrome/content/zotero/xpcom/file.js @@ -445,9 +445,8 @@ Zotero.File = new function(){ }); }); }; - - - this.download = Zotero.Promise.coroutine(function* (uri, path) { + + this.download = async function (uri, path) { var uriStr = uri.spec || uri; Zotero.debug(`Saving ${uriStr} to ${path.pathQueryRef || path}`); @@ -459,31 +458,52 @@ Zotero.File = new function(){ } var deferred = Zotero.Promise.defer(); - NetUtil.asyncFetch(uri, function (is, status, request) { - if (!Components.isSuccessCode(status)) { - Zotero.logError(status); - let msg = Zotero.getString('sync.error.checkConnection'); - switch (status) { - case 2152398878: - // TODO: Localize - msg = "Server not found. Check your internet connection." - break; + const uri_ = NetUtil.ioService.newURI(uri); + const inputChannel = NetUtil.ioService.newChannelFromURI(uri_); + const outputChannel = FileUtils.openSafeFileOutputStream(new FileUtils.File(path)); + const pipe = Cc["@mozilla.org/pipe;1"].createInstance(Ci.nsIPipe); + pipe.init(true, true, 0, 0xffffffff, null); + + let listener = Cc[ + "@mozilla.org/network/simple-stream-listener;1" + ].createInstance(Ci.nsISimpleStreamListener); + + listener.init(pipe.outputStream, { + onStartRequest(request) { + // NOTE: This noop callback is required, do not remove. + }, + onStopRequest(request, status) { + const responseStatus = 'responseStatus' in request ? request.responseStatus : null; + pipe.outputStream.close(); + + if (!Components.isSuccessCode(status)) { + Zotero.logError(status); + let msg = Zotero.getString('sync.error.checkConnection'); + switch (status) { + case 2152398878: + // TODO: Localize + msg = "Server not found. Check your internet connection." + break; + } + deferred.reject(new Error(msg)); + return; + } + if (responseStatus != 200) { + let msg = `Download failed with response code ${responseStatus}`; + Zotero.logError(msg); + deferred.reject(new Error(msg)); + return; } - deferred.reject(new Error(msg)); - return; } - if (request.responseStatus != 200) { - let msg = `Download failed with response code ${request.responseStatus}`; - Zotero.logError(msg); - deferred.reject(new Error(msg)); - return; - } - deferred.resolve(is); }); - var is = yield deferred.promise; - yield Zotero.File.putContentsAsync(path, is); - }); - + + NetUtil.asyncCopy(pipe.inputStream, outputChannel, function(aResult) { + deferred.resolve(); + }); + inputChannel.asyncOpen(listener, null); + + return deferred.promise; + }; /** * Rename file within its parent directory