Response timeout

docs/index.md

@@ -353,12 +353,27 @@ For more information, see the Mozilla Developer Network [xhr.responseType docs](
 
   To abort requests simply invoke the `req.abort()` method.
 
-## Request timeouts
+## Timeouts
 
-  A timeout can be applied by invoking `req.timeout(ms)`, after which an error
-  will be triggered. To differentiate between other errors the `err.timeout` property
-  is set to the `ms` value. __NOTE__ that this is a timeout applied to the request
-  and all subsequent redirects, not per request.
+Sometimes networks and servers get "stuck" and never respond after accepting a request. Set timeouts to avoid requests waiting forever.
+
+  * `req.timeout({deadline:ms})` or `req.timeout(ms)` (where `ms` is a number of milliseconds > 0) sets a deadline for the entire request (including all redirects) to complete. If the response isn't fully downloaded within that time, the request will be aborted.
+
+  * `req.timeout({response:ms})` sets maximum time to wait for the first byte to arrive from the server, but it does not limit how long the entire download can take. Response timeout should be a few seconds longer than just the time it takes server to respond, because it also includes time to make DNS lookup, TCP/IP and TLS connections.
+
+You should use both `deadline` and `response` timeouts. This way you can use a short response timeout to detect unresponsive networks quickly, and a long deadline to give time for downloads on slow, but reliable, networks.
+
+    request
+      .get('/big-file?network=slow')
+      .timeout({
+        response: 5000,  // Wait 5 seconds for the server to start sending,
+        deadline: 60000, // but allow 1 minute for the file to finish loading.
+      })
+      .end(function(err, res){
+        if (err.timeout) { /* timed out! */ }
+      });
+
+Timeout errors have a `.timeout` property.
 
 ## Authentication
 

lib/client.js

@@ -690,7 +690,6 @@ Request.prototype._isHost = function _isHost(obj) {
 Request.prototype.end = function(fn){
   var self = this;
   var xhr = this.xhr = request.getXHR();
-  var timeout = this._timeout;
   var data = this._formData || this._data;
 
   if (this._endCalled) {
@@ -703,14 +702,20 @@ Request.prototype.end = function(fn){
 
   // state change
   xhr.onreadystatechange = function(){
-    if (4 != xhr.readyState) return;
+    var readyState = xhr.readyState;
+    if (readyState >= 2 && self._responseTimeoutTimer) {
+      clearTimeout(self._responseTimeoutTimer);
+    }
+    if (4 != readyState) {
+      return;
+    }
 
     // In IE9, reads to any property (e.g. status) off of an aborted XHR will
     // result in the error "Could not complete the operation due to error c00c023f"
     var status;
     try { status = xhr.status } catch(e) { status = 0; }
 
-    if (0 == status) {
+    if (!status) {
       if (self.timedout || self._aborted) return;
       return self.crossDomainError();
     }
@@ -738,16 +743,11 @@ Request.prototype.end = function(fn){
     }
   }
 
-  // timeout
-  if (timeout && !this._timer) {
-    this._timer = setTimeout(function(){
-      self._timeoutError();
-    }, timeout);
-  }
-
   // querystring
   this._appendQueryString();
 
+  this._setTimeouts();
+
   // initiate request
   if (this.username && this.password) {
     xhr.open(this.method, this.url, true, this.username, this.password);

lib/node/index.js

@@ -725,7 +725,6 @@ Request.prototype.end = function(fn){
   var req = this.request();
   var buffer = this._buffer;
   var method = this.method;
-  var timeout = this._timeout;
   debug('%s %s', this.method, this.url);
 
   if (this._endCalled) {
@@ -743,13 +742,7 @@ Request.prototype.end = function(fn){
     return this.callback(e);
   }
 
-  // timeout
-  if (timeout && !this._timer) {
-    debug('timeout %sms %s %s', timeout, this.method, this.url);
-    this._timer = setTimeout(function(){
-      self._timeoutError();
-    }, timeout);
-  }
+  this._setTimeouts();
 
   // body
   if ('HEAD' != method && !req._headerSent) {
@@ -775,6 +768,10 @@ Request.prototype.end = function(fn){
   req.once('response', function(res){
     debug('%s %s -> %s', self.method, self.url, res.statusCode);
 
+    if (self._responseTimeoutTimer) {
+      clearTimeout(self._responseTimeoutTimer);
+    }
+
     if (self.piped) {
       return;
     }

lib/request-base.js

@@ -43,7 +43,9 @@ function mixin(obj) {
 
 RequestBase.prototype.clearTimeout = function _clearTimeout(){
   this._timeout = 0;
+  this._responseTimeout = 0;
   clearTimeout(this._timer);
+  clearTimeout(this._responseTimeoutTimer);
   return this;
 };
 
@@ -76,15 +78,31 @@ RequestBase.prototype.serialize = function serialize(fn){
 };
 
 /**
- * Set timeout to `ms`.
+ * Set timeouts.
  *
- * @param {Number} ms
+ * - response timeout is time between sending request and receiving the first byte of the response. Includes DNS and connection time.
+ * - deadline is the time from start of the request to receiving response body in full. If the deadline is too short large files may not load at all on slow connections.
+ *
+ * Value of 0 or false means no timeout.
+ *
+ * @param {Number|Object} ms or {response, read, deadline}
  * @return {Request} for chaining
  * @api public
  */
 
-RequestBase.prototype.timeout = function timeout(ms){
-  this._timeout = ms;
+RequestBase.prototype.timeout = function timeout(options){
+  if (!options || 'object' !== typeof options) {
+    this._timeout = options;
+    this._responseTimeout = 0;
+    return this;
+  }
+
+  if ('undefined' !== typeof options.deadline) {
+    this._timeout = options.deadline;
+  }
+  if ('undefined' !== typeof options.response) {
+    this._responseTimeout = options.response;
+  }
   return this;
 };
 
@@ -445,13 +463,31 @@ RequestBase.prototype.sortQuery = function(sort) {
  * @api private
  */
 
-RequestBase.prototype._timeoutError = function(){
-
-  var timeout = this._timeout;
-  var err = new Error('timeout of ' + timeout + 'ms exceeded');
+RequestBase.prototype._timeoutError = function(reason, timeout){
+  if (this._aborted) {
+    return;
+  }
+  var err = new Error(reason + timeout + 'ms exceeded');
   err.timeout = timeout;
   err.code = 'ECONNABORTED';
   this.timedout = true;
   this.abort();
   this.callback(err);
 };
+
+RequestBase.prototype._setTimeouts = function() {
+  var self = this;
+
+  // deadline
+  if (this._timeout && !this._timer) {
+    this._timer = setTimeout(function(){
+      self._timeoutError('Timeout of ', self._timeout);
+    }, this._timeout);
+  }
+  // response timeout
+  if (this._responseTimeout && !this._responseTimeoutTimer) {
+    this._responseTimeoutTimer = setTimeout(function(){
+      self._timeoutError('Response timeout of ', self._responseTimeout);
+    }, this._responseTimeout);
+  }
+}

test/request.js

@@ -682,7 +682,7 @@ it('req.timeout(ms)', function(next){
     try {
     assert(err, 'error missing');
     assert.equal(1000, err.timeout, 'err.timeout missing');
-    assert.equal('timeout of 1000ms exceeded', err.message, 'err.message incorrect');
+    assert.equal('Timeout of 1000ms exceeded', err.message, 'err.message incorrect');
     assert.equal(null, res);
     assert(req.timedout, true);
     next();
@@ -698,7 +698,7 @@ it('req.timeout(ms) with redirect', function(next) {
     try {
     assert(err, 'error missing');
     assert.equal(1000, err.timeout, 'err.timeout missing');
-    assert.equal('timeout of 1000ms exceeded', err.message, 'err.message incorrect');
+    assert.equal('Timeout of 1000ms exceeded', err.message, 'err.message incorrect');
     assert.equal(null, res);
     assert(req.timedout, true);
     next();

test/support/server.js

@@ -162,6 +162,42 @@ app.get('/delay/const', function (req, res) {
   res.redirect('/delay/3000');
 });
 
+var slowBodyCallback;
+app.get('/delay/slowbody', function(req, res){
+  res.writeHead(200, {"Content-Type":"application/octet-stream"});
+
+  // Send lots of garbage data to overflow all buffers along the way,
+  // so that the browser gets some data before the request is done
+  var initialDataSent = new Promise(function(resolve){
+    res.write(new Buffer(4000), function(){
+      res.write(new Buffer(16000));
+      resolve();
+    });
+  });
+
+  // Make sure sending of request body takes over 1s,
+  // so that the test can't pass by accident.
+  var minimumTime = new Promise(function(resolve){setTimeout(resolve, 1001)});
+
+  new Promise(function(resolve){
+    // Waiting full 10 seconds for the test would be too annoying,
+    // so the remote callback is a hack to push the test forward
+    slowBodyCallback = resolve;
+    setTimeout(resolve, 10000);
+  })
+  .then(function(){
+    return Promise.all([initialDataSent, minimumTime]);
+  })
+  .then(function(){
+    res.end('bye');
+  });
+});
+
+app.get('/delay/slowbody/finish', function(req, res){
+  if (slowBodyCallback) slowBodyCallback();
+  res.sendStatus(204);
+});
+
 app.get('/delay/:ms', function(req, res){
   var ms = ~~req.params.ms;
   setTimeout(function(){

test/timeout.js

@@ -4,6 +4,8 @@ var assert = require('assert');
 var request = require('../');
 
 describe('.timeout(ms)', function(){
+  this.timeout(15000);
+
   describe('when timeout is exceeded', function(){
     it('should error', function(done){
       request
@@ -15,6 +17,53 @@ describe('.timeout(ms)', function(){
         assert.equal('ECONNABORTED', err.code, 'expected abort error code')
         done();
       });
-    })
+    });
+
+    it('should error on deadline', function(done){
+      request
+      .get(base + '/delay/500')
+      .timeout({deadline: 150})
+      .end(function(err, res){
+        assert(err, 'expected an error');
+        assert.equal('number', typeof err.timeout, 'expected an error with .timeout');
+        assert.equal('ECONNABORTED', err.code, 'expected abort error code')
+        done();
+      });
+    });
+
+    it('should support setting individual options', function(done){
+      request
+      .get(base + '/delay/500')
+      .timeout({deadline: 10})
+      .timeout({response: 99999})
+      .end(function(err, res){
+        assert(err, 'expected an error');
+        assert.equal('ECONNABORTED', err.code, 'expected abort error code')
+        done();
+      });
+    });
+
+    it('should error on response', function(done){
+      request
+      .get(base + '/delay/500')
+      .timeout({response: 150})
+      .end(function(err, res){
+        assert(err, 'expected an error');
+        assert.equal('number', typeof err.timeout, 'expected an error with .timeout');
+        assert.equal('ECONNABORTED', err.code, 'expected abort error code')
+        done();
+      });
+    });
+
+    it('should accept slow body with fast response', function(done){
+      request
+        .get(base + '/delay/slowbody')
+        .timeout({response: 1000})
+        .on('progress', function(){
+          // This only makes the test faster without relying on arbitrary timeouts
+          request.get(base + '/delay/slowbody/finish').end();
+        })
+        .end(done);
+    });
   })
 })