diff --git a/chrome/content/zotero/xpcom/sync/syncAPIClient.js b/chrome/content/zotero/xpcom/sync/syncAPIClient.js index ff281c858a..03b5c95ad2 100644 --- a/chrome/content/zotero/xpcom/sync/syncAPIClient.js +++ b/chrome/content/zotero/xpcom/sync/syncAPIClient.js @@ -40,6 +40,8 @@ Zotero.Sync.APIClient = function (options) { this.failureDelayIntervals = [2500, 5000, 10000, 20000, 40000, 60000, 120000, 240000, 300000]; this.failureDelayMax = 60 * 60 * 1000; // 1 hour + this.rateDelayIntervals = [30, 60, 300]; + this.rateDelayPosition = 0; } Zotero.Sync.APIClient.prototype = { @@ -626,16 +628,24 @@ Zotero.Sync.APIClient.prototype = { try { var xmlhttp = yield Zotero.HTTP.request(method, uri, opts); this._checkBackoff(xmlhttp); + this.rateDelayPosition = 0; return xmlhttp; } catch (e) { tries++; if (e instanceof Zotero.HTTP.UnexpectedStatusException) { this._checkConnection(e.xmlhttp, e.channel); - //this._checkRetry(e.xmlhttp); + if (this._check429(e.xmlhttp)) { + // Return false to keep retrying request + return false; + } if (e.is5xx()) { Zotero.logError(e); + if (e.xmlhttp.status == 503 && this._checkRetry(e.xmlhttp)) { + return false; + } + if (!failureDelayGenerator) { // Keep trying for up to an hour failureDelayGenerator = Zotero.Utilities.Internal.delayGenerator( @@ -814,17 +824,28 @@ Zotero.Sync.APIClient.prototype = { _checkBackoff: function (xmlhttp) { var backoff = xmlhttp.getResponseHeader("Backoff"); - if (backoff) { - // Sanity check -- don't wait longer than an hour - if (backoff > 3600) { - // TODO: Update status? - - this.caller.pause(backoff * 1000); - } + if (backoff && Number.isInteger(backoff)) { + // TODO: Update status? + this.caller.pause(backoff * 1000); } }, + _checkRetry: function (xmlhttp) { + var retryAfter = xmlhttp.getResponseHeader("Retry-After"); + var delay; + if (!retryAfter) return false; + if (!Number.isInteger(retryAfter)) { + Zotero.logError(`Invalid Retry-After delay ${retryAfter}`); + return false; + } + // TODO: Update status? + delay = retryAfter; + this.caller.pause(delay * 1000); + return true; + }, + + _check412: function (xmlhttp) { // Avoid logging error from Zotero.HTTP.request() in ConcurrentCaller if (xmlhttp.status == 412) { @@ -834,6 +855,22 @@ Zotero.Sync.APIClient.prototype = { }, + _check429: function (xmlhttp) { + if (xmlhttp.status != 429) return false; + + // If there's a Retry-After header, use that + if (this._checkRetry(xmlhttp)) { + return true; + } + + // Otherwise, pause for increasing amounts, or max amount if no more + var delay = this.rateDelayIntervals[this.rateDelayPosition++] + || this.rateDelayIntervals[this.rateDelayIntervals.length - 1]; + this.caller.pause(delay * 1000); + return true; + }, + + _getLastModifiedVersion: function (xmlhttp) { libraryVersion = xmlhttp.getResponseHeader('Last-Modified-Version'); if (!libraryVersion) { diff --git a/test/tests/syncAPIClientTest.js b/test/tests/syncAPIClientTest.js index fb972b9a13..eff73584d8 100644 --- a/test/tests/syncAPIClientTest.js +++ b/test/tests/syncAPIClientTest.js @@ -44,31 +44,6 @@ describe("Zotero.Sync.APIClient", function () { }) describe("#_checkConnection()", function () { - var spy; - - beforeEach(function () { - client.failureDelayIntervals = [10]; - client.failureDelayMax = 15; - }); - afterEach(function () { - if (spy) { - spy.restore(); - } - }); - - it("should retry on 500 error", function* () { - setResponse({ - method: "GET", - url: "error", - status: 500, - text: "" - }) - var spy = sinon.spy(Zotero.HTTP, "request"); - var e = yield getPromiseError(client.makeRequest("GET", baseURL + "error")); - assert.instanceOf(e, Zotero.HTTP.UnexpectedStatusException); - assert.isTrue(spy.calledTwice); - }) - it("should catch an interrupted connection", function* () { setResponse({ method: "GET", @@ -149,4 +124,119 @@ describe("Zotero.Sync.APIClient", function () { assert.sameMembers(results.map(o => o.id), [1, 2, 3, 4, 5]); }); }); + + + describe("Retries", function () { + var spy; + var delayStub; + + before(function () { + delayStub = sinon.stub(Zotero.Promise, "delay").returns(Zotero.Promise.resolve()); + }); + + beforeEach(function () { + client.failureDelayIntervals = [10, 20]; + client.failureDelayMax = 25; + client.rateDelayIntervals = [15, 25]; + }); + + afterEach(function () { + if (spy) { + spy.restore(); + } + delayStub.reset(); + }); + + after(function () { + delayStub.restore(); + }); + + + describe("#makeRequest()", function () { + it("should retry on 500 error", function* () { + setResponse({ + method: "GET", + url: "error", + status: 500, + text: "" + }); + spy = sinon.spy(Zotero.HTTP, "request"); + var e = yield getPromiseError(client.makeRequest("GET", baseURL + "error")); + assert.instanceOf(e, Zotero.HTTP.UnexpectedStatusException); + assert.isTrue(spy.calledTwice); + }); + + it("should obey Retry-After for 503", function* () { + var called = 0; + server.respond(function (req) { + if (req.method == "GET" && req.url == baseURL + "error") { + if (called < 1) { + req.respond( + 503, + { + "Retry-After": 5 + }, + "" + ); + } + else if (called < 2) { + req.respond( + 503, + { + "Retry-After": 10 + }, + "" + ); + } + else { + req.respond( + 200, + {}, + "" + ); + } + } + called++; + }); + spy = sinon.spy(Zotero.HTTP, "request"); + yield client.makeRequest("GET", baseURL + "error"); + assert.isTrue(spy.calledThrice); + // DEBUG: Why are these slightly off? + assert.approximately(delayStub.args[0][0], 5 * 1000, 5); + assert.approximately(delayStub.args[1][0], 10 * 1000, 5); + }); + }); + + + describe("#_check429()", function () { + it("should retry on 429 error", function* () { + var called = 0; + server.respond(function (req) { + if (req.method == "GET" && req.url == baseURL + "error") { + if (called < 2) { + req.respond( + 429, + {}, + "" + ); + } + else { + req.respond( + 200, + {}, + "" + ); + } + } + called++; + }); + spy = sinon.spy(Zotero.HTTP, "request"); + yield client.makeRequest("GET", baseURL + "error"); + assert.isTrue(spy.calledThrice); + // DEBUG: Why are these slightly off? + assert.approximately(delayStub.args[0][0], 15 * 1000, 5); + assert.approximately(delayStub.args[1][0], 25 * 1000, 5); + }); + }); + }); })