429 and Retry-After support for API requests

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) {
+		if (backoff && Number.isInteger(backoff)) {
-			// Sanity check -- don't wait longer than an hour
+			// TODO: Update status?
-			if (backoff > 3600) {
+			this.caller.pause(backoff * 1000);
-				// 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) {

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);
+			});
+		});
+	});
 })