mirror of
https://github.com/zotero/zotero.git
synced 2026-10-09 03:18:01 +00:00
Stop retrying indefinitely on a repeated Retry-After
Retry-After was honored unconditionally with no attempt limit, so a server returning 429 or 503 with the header on every request retried forever. Retry-After waits and backoff intervals now share the errorDelayMax budget, with each Retry-After counted as at least a second so that a value of 0 can't loop forever.
This commit is contained in:
Dan Stillman
parent
d81484d2e8
commit
a6e814417f
2 changed files with 121 additions and 38 deletions
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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({
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue