Reject request on XHR timeout (#2646)

Co-authored-by: Zachary Hamm <hamm.zachary@gmail.com>

request/request.js

@@ -79,7 +79,7 @@ module.exports = function($window, Promise, oncompletion) {
 			var assumeJSON = (args.serialize == null || args.serialize === JSON.serialize) && !(body instanceof $window.FormData)
 			var responseType = args.responseType || (typeof args.extract === "function" ? "" : "json")
 
-			var xhr = new $window.XMLHttpRequest(), aborted = false
+			var xhr = new $window.XMLHttpRequest(), aborted = false, isTimeout = false
 			var original = xhr, replacedAbort
 			var abort = xhr.abort
 
@@ -141,12 +141,25 @@ module.exports = function($window, Promise, oncompletion) {
 						}
 						if (success) resolve(response)
 						else {
-							try { message = ev.target.responseText }
+							var completeErrorResponse = function() {
-							catch (e) { message = response }
+								try { message = ev.target.responseText }
-							var error = new Error(message)
+								catch (e) { message = response }
-							error.code = ev.target.status
+								var error = new Error(message)
-							error.response = response
+								error.code = ev.target.status
-							reject(error)
+								error.response = response
+								reject(error)
+							}
+
+							if (xhr.status === 0) {
+								// Use setTimeout to push this code block onto the event queue
+								// This allows `xhr.ontimeout` to run in the case that there is a timeout
+								// Without this setTimeout, `xhr.ontimeout` doesn't have a chance to reject
+								// as `xhr.onreadystatechange` will run before it
+								setTimeout(function() {
+									if (isTimeout) return
+									completeErrorResponse()
+								})
+							} else completeErrorResponse()
 						}
 					}
 					catch (e) {
@@ -155,6 +168,13 @@ module.exports = function($window, Promise, oncompletion) {
 				}
 			}
 
+			xhr.ontimeout = function (ev) {
+				isTimeout = true
+				var error = new Error("Request timed out")
+				error.code = ev.target.status
+				reject(error)
+			}
+
 			if (typeof args.config === "function") {
 				xhr = args.config(xhr, args, url) || xhr
 

request/tests/test-request.js

@@ -677,6 +677,57 @@ o.spec("request", function() {
 				o(e instanceof Error).equals(true)
 			}).then(done)
 		})
+		o("rejects on request timeout", function(done) {
+			var timeout = 50
+			var timeToGetItem = timeout + 1
+
+			mock.$defineRoutes({
+				"GET /item": function() {
+					return new Promise(function(resolve) {
+						setTimeout(function() {
+							resolve({status: 200})
+						}, timeToGetItem)
+					})
+				}
+			})
+
+			request({
+				method: "GET", url: "/item",
+				timeout: timeout
+			}).catch(function(e) {
+				o(e instanceof Error).equals(true)
+				o(e.message).equals("Request timed out")
+				o(e.code).equals(0)
+			}).then(function() {
+				done()
+			})
+		})
+		o("does not reject when time to request resource does not exceed timeout", function(done) {
+			var timeout = 50
+			var timeToGetItem = timeout - 1
+			var isRequestRejected = false
+
+			mock.$defineRoutes({
+				"GET /item": function() {
+					return new Promise(function(resolve) {
+						setTimeout(function() {
+							resolve({status: 200})
+						}, timeToGetItem)
+					})
+				}
+			})
+
+			request({
+				method: "GET", url: "/item",
+				timeout: timeout
+			}).catch(function(e) {
+				isRequestRejected = true
+				o(e.message).notEquals("Request timed out")
+			}).then(function() {
+				o(isRequestRejected).equals(false)
+				done()
+			})
+		})
 		o("does not reject on status error code when extract provided", function(done) {
 			mock.$defineRoutes({
 				"GET /item": function() {

test-utils/xhrMock.js

@@ -46,6 +46,7 @@ module.exports = function() {
 			}
 			this.responseType = ""
 			this.response = null
+			this.timeout = 0
 			Object.defineProperty(this, "responseText", {get: function() {
 				if (this.responseType === "" || this.responseType === "text") {
 					return this.response
@@ -55,25 +56,50 @@ module.exports = function() {
 			}})
 			this.send = function(body) {
 				var self = this
-				if(!aborted) {
+				
-					var handler = routes[args.method + " " + args.pathname] || serverErrorHandler.bind(null, args.pathname)
+				var completeResponse = function (data) {
-					var data = handler({rawUrl: args.rawUrl, url: args.pathname, query: args.search || {}, body: body || null})
+					self._responseCompleted = true
-					self.status = data.status
+					if(!aborted) {
-					// Match spec
+						self.status = data.status
-					if (self.responseType === "json") {
+						// Match spec
-						try { self.response = JSON.parse(data.responseText) }
+						if (self.responseType === "json") {
-						catch (e) { /* ignore */ }
+							try { self.response = JSON.parse(data.responseText) }
+							catch (e) { /* ignore */ }
+						} else {
+							self.response = data.responseText
+						}
 					} else {
-						self.response = data.responseText
+						self.status = 0
+					}
+					self.readyState = 4
+					if (args.async === true) {
+						callAsync(function() {
+							if (typeof self.onreadystatechange === "function") self.onreadystatechange({target: self})
+						})
 					}
-				} else {
-					self.status = 0
 				}
-				self.readyState = 4
+
-				if (args.async === true) {
+				var data
-					callAsync(function() {
+				if (!aborted) {
-						if (typeof self.onreadystatechange === "function") self.onreadystatechange({target: self})
+					var handler = routes[args.method + " " + args.pathname] || serverErrorHandler.bind(null, args.pathname)
-					})
+					data = handler({rawUrl: args.rawUrl, url: args.pathname, query: args.search || {}, body: body || null})
+				}
+
+				if (typeof self.timeout === "number" && self.timeout > 0) {
+					setTimeout(function () {
+						if (self._responseCompleted) {
+							return
+						}
+
+						self.status = 0;
+						if (typeof self.ontimeout === "function") self.ontimeout({target: self, type:"timeout"})
+					}, self.timeout)
+				}
+
+				if (data instanceof Promise) {
+					data.then(completeResponse)
+				} else {
+					completeResponse(data)
 				}
 			}
 			this.abort = function() {