Skip to content

Commit

Permalink
429 and Retry-After support for API requests
Browse files Browse the repository at this point in the history
  • Loading branch information
@dstillman
dstillman committed Apr 11, 2017
1 parent 64414e4 commit 5d6478e
Show file tree
Hide file tree
Showing 2 changed files with 160 additions and 33 deletions.
53 changes: 45 additions & 8 deletions chrome/content/zotero/xpcom/sync/syncAPIClient.js
View file
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
140 changes: 115 additions & 25 deletions test/tests/syncAPIClientTest.js
View file
Expand Up @@ -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",
Expand Down Expand Up @@ -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);
});
});
});
})

0 comments on commit 5d6478e

Please sign in to comment.