diff --git a/chrome/content/zotero/xpcom/attachments.js b/chrome/content/zotero/xpcom/attachments.js index 4a65ee755f..0a5a10f0f5 100644 --- a/chrome/content/zotero/xpcom/attachments.js +++ b/chrome/content/zotero/xpcom/attachments.js @@ -1170,6 +1170,8 @@ Zotero.Attachments = new function () { * @param {String} [options.referrer] * @param {Boolean} [options.enforceFileType] - Delete file if not one of SUPPORTED_FILE_TYPES * @param {Boolean} [options.shouldDisplayCaptcha] + * @param {Number} [options.errorDelayMax] - Passed to Zotero.HTTP.download() + * @param {Boolean} [options.noRetryOnThrottle] - Passed to Zotero.HTTP.download() */ this.downloadFile = async function (url, path, options = {}) { Zotero.debug(`Downloading file from ${url}`); @@ -1184,6 +1186,8 @@ Zotero.Attachments = new function () { path, { headers, + errorDelayMax: options.errorDelayMax, + noRetryOnThrottle: options.noRetryOnThrottle, } ); // Check that the downloaded file is the expected type @@ -1733,7 +1737,10 @@ Zotero.Attachments = new function () { // Retry-After if (status == 429 || status == 503) { - let retryAfter = e.xmlhttp.getResponseHeader('Retry-After'); + // xmlhttp is a fetch Response for downloads + let retryAfter = e.xmlhttp.headers?.get + ? e.xmlhttp.headers.get('Retry-After') + : e.xmlhttp.getResponseHeader('Retry-After'); if (retryAfter) { Zotero.debug("Got Retry-After: " + retryAfter); if (parseInt(retryAfter) == retryAfter) { @@ -1745,7 +1752,7 @@ Zotero.Attachments = new function () { return true; } else if (Zotero.Date.isHTTPDate(retryAfter)) { - let d = new Date(val); + let d = new Date(retryAfter); if (d > Date.now() + maxDelay * 1000) { Zotero.debug("Retry-After is too long -- skipping request"); return false; @@ -2086,7 +2093,14 @@ Zotero.Attachments = new function () { while (tries-- > 0) { try { await beforeRequest(url); - await this.downloadFile(url, path, options); + // Waiting here blocks every other item in the queue, so + // keep it short and leave longer backoff and Retry-After + // to the loop below + await this.downloadFile( + url, + path, + { ...options, errorDelayMax: 7500, noRetryOnThrottle: true } + ); afterRequest(url); return { url, props: urlResolver }; } diff --git a/test/tests/attachmentsTest.js b/test/tests/attachmentsTest.js index 40eb18c112..b14df7bf81 100644 --- a/test/tests/attachmentsTest.js +++ b/test/tests/attachmentsTest.js @@ -1524,6 +1524,179 @@ describe("Zotero.Attachments", function () { assert.equal(requests, 1); }); + it("should give up on a file URL that keeps returning a server error", async function () { + var doi = doi4; + var item = createUnsavedDataObject('item', { itemType: 'journalArticle' }); + item.setField('title', 'Test'); + item.setField('DOI', doi); + await item.saveTx(); + + var requests = 0; + httpd.registerPathHandler( + "/failing-pdf", + { + handle: function (request, response) { + requests++; + response.setStatusLine(null, 502, "Bad Gateway"); + } + } + ); + httpd.registerPathHandler( + "/failing-pdf-page/" + doi, + { + handle: function (request, response) { + response.setStatusLine(null, 200, "OK"); + response.write( + `PDF` + + `` + ); + } + } + ); + + var resolvers = [{ + name: 'Custom', + method: 'get', + url: baseURL + "failing-pdf-page/{doi}", + mode: 'html', + selector: '#pdf-link', + attribute: 'href' + }]; + Zotero.Prefs.set('findPDFs.resolvers', JSON.stringify(resolvers)); + + var delayStub = sinon.stub(Zotero.Promise, "delay").returns(Promise.resolve()); + try { + var attachment = await Zotero.Attachments.addAvailableFile(item); + } + finally { + delayStub.restore(); + } + + assert.isFalse(attachment); + // Initial request plus two retries within errorDelayMax + assert.equal(requests, 3); + }); + + it("should not wait on a Retry-After from a file URL", async function () { + var doi = doi4; + var item = createUnsavedDataObject('item', { itemType: 'journalArticle' }); + item.setField('title', 'Test'); + item.setField('DOI', doi); + await item.saveTx(); + + var requests = 0; + httpd.registerPathHandler( + "/throttled-pdf", + { + handle: function (request, response) { + requests++; + if (requests == 1) { + response.setStatusLine(null, 429, "Too Many Requests"); + response.setHeader("Retry-After", "1", false); + } + else { + response.setStatusLine(null, 404, "Not Found"); + } + } + } + ); + httpd.registerPathHandler( + "/throttled-pdf-page/" + doi, + { + handle: function (request, response) { + response.setStatusLine(null, 200, "OK"); + response.write( + `PDF` + + `` + ); + } + } + ); + + var resolvers = [{ + name: 'Custom', + method: 'get', + url: baseURL + "throttled-pdf-page/{doi}", + mode: 'html', + selector: '#pdf-link', + attribute: 'href' + }]; + Zotero.Prefs.set('findPDFs.resolvers', JSON.stringify(resolvers)); + + var attachment = await Zotero.Attachments.addAvailableFile(item); + + assert.isFalse(attachment); + assert.equal(requests, 1); + }); + + async function testThrottledFileURL(retryAfter) { + var doi = doi4; + var item = createUnsavedDataObject('item', { itemType: 'journalArticle' }); + item.setField('title', 'Test'); + item.setField('DOI', doi); + await item.saveTx(); + + var requestTimes = []; + httpd.registerPathHandler( + "/throttled-pdf", + { + handle: function (request, response) { + requestTimes.push(Date.now()); + if (requestTimes.length == 1) { + response.setStatusLine(null, 429, "Too Many Requests"); + response.setHeader("Retry-After", retryAfter(), false); + } + else { + response.setStatusLine(null, 302, "Found"); + response.setHeader("Location", pdfURL, false); + } + } + } + ); + httpd.registerPathHandler( + "/throttled-pdf-page/" + doi, + { + handle: function (request, response) { + response.setStatusLine(null, 200, "OK"); + response.write( + // Use an address that isn't exempt from per-domain delays + `PDF` + + `` + ); + } + } + ); + + var resolvers = [{ + name: 'Custom', + method: 'get', + url: baseURL + "throttled-pdf-page/{doi}", + mode: 'html', + selector: '#pdf-link', + attribute: 'href' + }]; + Zotero.Prefs.set('findPDFs.resolvers', JSON.stringify(resolvers)); + + await Zotero.Attachments.addAvailableFiles([item]); + + assert.equal(item.numAttachments(), 1); + assert.lengthOf(requestTimes, 2); + return requestTimes[1] - requestTimes[0]; + } + + it("should retry a file URL after a Retry-After in seconds when finding files for multiple items", async function () { + var elapsed = await testThrottledFileURL(() => "1"); + assert.isAtLeast(elapsed, 950); + }); + + it("should retry a file URL after a Retry-After date when finding files for multiple items", async function () { + var elapsed = await testThrottledFileURL( + () => new Date(Date.now() + 2000).toUTCString() + ); + // HTTP dates have one-second resolution + assert.isAtLeast(elapsed, 950); + }); + it("should not honor Retry-After from a custom resolver", async function () { var doi = doi4; var item = createUnsavedDataObject('item', { itemType: 'journalArticle' });