diff --git a/chrome/content/zotero/xpcom/http.js b/chrome/content/zotero/xpcom/http.js index 629b04c4de..e1ad399350 100644 --- a/chrome/content/zotero/xpcom/http.js +++ b/chrome/content/zotero/xpcom/http.js @@ -1556,16 +1556,6 @@ Zotero.HTTP = new function () { return parseInt(retryAfter); } - async function _checkRetry(req) { - var retryAfter = _getRetryAfter(req); - if (retryAfter === null) { - return false; - } - Zotero.debug(`Delaying ${retryAfter} seconds for Retry-After`); - await Zotero.Promise.delay(retryAfter * 1000); - return true; - } - /** * Call `fn` and automatically retry on 429/5xx errors, honoring Retry-After @@ -1579,7 +1569,13 @@ Zotero.HTTP = new function () { * @return {Promise} - Result of fn() */ async function _retryOnServerError(fn, url, options) { - var errorDelayGenerator; + var errorDelayIntervals = (options.errorDelayIntervals || _errorDelayIntervals).slice(); + var errorDelayMax = options.errorDelayMax !== undefined + ? options.errorDelayMax + : _errorDelayMax; + var interval; + // Retry-After waits and backoff intervals share one budget + var totalDelay = 0; while (true) { try { @@ -1600,30 +1596,38 @@ Zotero.HTTP = new function () { if (e.status == 429 || e.is5xx()) { Zotero.logError(e); - // Check for Retry-After header on 429 or 503 - if ((e.status == 429 || e.status == 503) - && (await _checkRetry(e.xmlhttp))) { - continue; - } // Don't retry if errorDelayMax is 0 - if (options.errorDelayMax === 0 + if (errorDelayMax === 0 || Zotero.HTTP.disableErrorRetry) { throw e; } - // Automatically retry other 429/5xx errors by default - if (!errorDelayGenerator) { - // Keep trying for up to an hour - errorDelayGenerator - = Zotero.Utilities.Internal.delayGenerator( - options.errorDelayIntervals - || _errorDelayIntervals, - options.errorDelayMax !== undefined - ? options.errorDelayMax - : _errorDelayMax - ); + let retryAfter = (e.status == 429 || e.status == 503) + ? _getRetryAfter(e.xmlhttp) + : null; + let delay; + if (retryAfter !== null) { + // Wait at least a second, so that a Retry-After of 0 + // still uses up the budget + delay = Math.max(retryAfter, 1) * 1000; } - let delayPromise = errorDelayGenerator.next().value; - let keepGoing; + // Otherwise back off, repeating the last interval once + // the list is used up + else { + interval = errorDelayIntervals.shift() || interval; + delay = interval; + } + if (!delay || totalDelay + delay > errorDelayMax) { + Zotero.logError("Failed too many times"); + throw e; + } + totalDelay += delay; + if (retryAfter !== null) { + Zotero.debug(`Delaying ${retryAfter} seconds for Retry-After`); + } + else { + Zotero.debug(`Delaying ${delay} ms`); + } + let delayPromise = Zotero.Promise.delay(delay); // Provide caller with a callback to cancel // while waiting to retry if (options.cancellerReceiver) { @@ -1639,9 +1643,7 @@ Zotero.HTTP = new function () { ); options.cancellerReceiver(reject); try { - keepGoing = await Promise.race( - [delayPromise, cancelPromise] - ); + await Promise.race([delayPromise, cancelPromise]); } catch (e) { Zotero.debug("Request cancelled"); @@ -1650,11 +1652,7 @@ Zotero.HTTP = new function () { resolve(); } else { - keepGoing = await delayPromise; - } - if (!keepGoing) { - Zotero.logError("Failed too many times"); - throw e; + await delayPromise; } continue; } diff --git a/test/tests/httpTest.js b/test/tests/httpTest.js index 4c0c00d0d0..c0465e7b13 100644 --- a/test/tests/httpTest.js +++ b/test/tests/httpTest.js @@ -252,6 +252,91 @@ describe("Zotero.HTTP", function () { assert.isTrue(delayStub.notCalled); }); + it("shouldn't obey a Retry-After longer than errorDelayMax", async function () { + var called = 0; + server.respond(function (req) { + if (req.method == "GET" && req.url == baseURL + "error") { + if (called < 1) { + req.respond(503, { "Retry-After": "3600" }, ""); + } + else { + req.respond(200, {}, ""); + } + } + called++; + }); + spy = sinon.spy(Zotero.HTTP, "_requestInternal"); + var e = await getPromiseError( + Zotero.HTTP.request("GET", baseURL + "error", { errorDelayMax: 7500 }) + ); + assert.instanceOf(e, Zotero.HTTP.UnexpectedStatusException); + assert.isTrue(spy.calledOnce); + assert.isTrue(delayStub.notCalled); + }); + + it("should stop obeying Retry-After once errorDelayMax is used up", async function () { + server.respond(function (req) { + if (req.method == "GET" && req.url == baseURL + "error") { + req.respond(429, { "Retry-After": "3" }, ""); + } + }); + spy = sinon.spy(Zotero.HTTP, "_requestInternal"); + var e = await getPromiseError( + Zotero.HTTP.request("GET", baseURL + "error", { errorDelayMax: 7500 }) + ); + assert.instanceOf(e, Zotero.HTTP.UnexpectedStatusException); + // 3s + 3s fits within 7.5s; a third would not + assert.equal(spy.callCount, 3); + assert.isTrue(delayStub.calledTwice); + assert.equal(delayStub.args[0][0], 3000); + assert.equal(delayStub.args[1][0], 3000); + }); + + it("should wait at least a second for a Retry-After of 0", async function () { + server.respond(function (req) { + if (req.method == "GET" && req.url == baseURL + "error") { + req.respond(429, { "Retry-After": "0" }, ""); + } + }); + spy = sinon.spy(Zotero.HTTP, "_requestInternal"); + var e = await getPromiseError( + Zotero.HTTP.request("GET", baseURL + "error", { errorDelayMax: 2500 }) + ); + assert.instanceOf(e, Zotero.HTTP.UnexpectedStatusException); + assert.equal(spy.callCount, 3); + assert.deepEqual(delayStub.args.map(x => x[0]), [1000, 1000]); + }); + + it("should count Retry-After and backoff delays against the same errorDelayMax", async function () { + var called = 0; + server.respond(function (req) { + if (req.method == "GET" && req.url == baseURL + "error") { + if (called < 2) { + req.respond(500, {}, ""); + } + else { + req.respond(429, { "Retry-After": "3" }, ""); + } + } + called++; + }); + spy = sinon.spy(Zotero.HTTP, "_requestInternal"); + var e = await getPromiseError( + Zotero.HTTP.request( + "GET", + baseURL + "error", + { + errorDelayIntervals: [2500, 5000], + errorDelayMax: 7500 + } + ) + ); + assert.instanceOf(e, Zotero.HTTP.UnexpectedStatusException); + // 2.5s + 5s uses up the budget, so the Retry-After isn't honored + assert.equal(spy.callCount, 3); + assert.deepEqual(delayStub.args.map(x => x[0]), [2500, 5000]); + }); + it("should provide cancellerReceiver a callback to cancel while waiting to retry a 5xx error", async function () { delayStub.restore(); setResponse({