feat: timeout option

can set connect timeout and response timeout seperately.
node-modules · Aug 4, 2016 · 2db3804 · 2db3804
1 parent d88b63b
commit 2db3804
Show file tree

Hide file tree

Showing 3 changed files with 107 additions and 30 deletions.
diff --git a/README.md b/README.md
@@ -129,7 +129,7 @@ httpclient.request('http://nodejs.org', function (err, body) {
     - ***dataType*** String - Type of response data. Could be `text` or `json`. If it's `text`, the `callback`ed `data` would be a String. If it's `json`, the `data` of callback would be a parsed JSON Object. Default `callback`ed `data` would be a `Buffer`.
     - **fixJSONCtlChars** Boolean - Fix the control characters (U+0000 through U+001F) before JSON parse response. Default is `false`.
     - ***headers*** Object - Request headers.
-    - ***timeout*** Number - Request timeout in milliseconds. Defaults to `exports.TIMEOUT`. Include remote server connecting timeout and response timeout. When timeout happen, will return `ConnectionTimeout` or `ResponseTimeout`.
+    - ***timeout*** Number | Array - Request timeout in milliseconds for connecting phase and response receiving phase. Defaults to `exports.TIMEOUT`, both are 5s. You can use `timeout: 5000` to tell urllib use same timeout on two phase or set them seperately such as `timeout: [3000, 5000]`, which will set connecting timeout to 3s and response 5s.
     - ***auth*** String - `username:password` used in HTTP Basic Authorization.
     - ***digestAuth*** String - `username:password` used in HTTP [Digest Authorization](http://en.wikipedia.org/wiki/Digest_access_authentication).
     - ***agent*** [http.Agent](http://nodejs.org/api/http.html#http_class_http_agent) - HTTP Agent object.

diff --git a/lib/urllib.js b/lib/urllib.js
@@ -35,7 +35,7 @@ exports.httpsAgent.maxSockets = 1000;
  * @const
  */
 
-exports.TIMEOUT = ms('5s');
+exports.TIMEOUT = [ms('5s'), ms('5s')];
 
 var REQUEST_ID = 0;
 var MAX_VALUE = Math.pow(2, 31) - 10;
@@ -315,17 +315,35 @@ exports.requestWithCallback = function (url, args, callback) {
     // you can use this hook to change every thing.
     args.beforeRequest(options);
   }
-  var timer = null;
+  var connectTimer = null;
+  var responseTimer = null;
   var __err = null;
   var connected = false; // socket connected or not
   var keepAliveSocket = false; // request with keepalive socket
   var responseSize = 0;
   var responseAborted = false;
-  var done = function (err, data, res) {
-    if (timer) {
-      clearTimeout(timer);
-      timer = null;
+
+
+  var cancelConnectTimer = function() {
+    if (connectTimer) {
+      clearTimeout(connectTimer);
+      connectTimer = null;
+    }
+
+  };
+  var cancelResponseTimer = function() {
+    if (responseTimer) {
+      clearTimeout(responseTimer);
+      responseTimer = null;
     }
+  };
+
+  var done = function (err, data, res) {
+    //if (timer) {
+      ////clearTimeout(timer);
+      //timer = null;
+    //}
+    cancelResponseTimer();
     if (!callback) {
       console.warn('[urllib:warn] [%s] [worker:%s] %s %s callback twice!!!',
         Date(), process.pid, options.method, url);
@@ -426,10 +444,11 @@ exports.requestWithCallback = function (url, args, callback) {
         var _url = urlutil.resolve(url, location);
         debug('Request#%d %s: `redirected` from %s to %s', reqId, options.path, url, _url);
         // make sure timer stop
-        if (timer) {
-          clearTimeout(timer);
-          timer = null;
-        }
+        //if (timer) {
+          //clearTimeout(timer);
+          //timer = null;
+        //}
+        cancelResponseTimer();
         // should clean up headers.Host on `location: http://other-domain/url`
         if (args.headers && args.headers.Host && PROTO_RE.test(location)) {
           args.headers.Host = null;
@@ -630,6 +649,49 @@ exports.requestWithCallback = function (url, args, callback) {
     });
   }
 
+  var connectTimeout, responseTimeout;
+  if (args.timeout.push) {  // isArray
+    connectTimeout = ms(args.timeout[0]);
+    responseTimeout = ms(args.timeout[1]);
+  } else {  // set both timeout equal
+    connectTimeout = responseTimeout = ms(args.timeout);
+  }
+  debug('ConnectTimeout: %d, ResponseTimeout: %d', connectTimeout, responseTimeout);
+
+  var startConnectTimer = function() {
+    debug('Connect timer ticking, timeout: %d', connectTimeout);
+    connectTimer = setTimeout(function () {
+      connectTimer = null;
+      var msg = 'Connect timeout for ' + connectTimeout + 'ms';
+      var errorName = 'ConnectionTimeoutError';
+      if (!req.socket) {
+        errorName = 'SocketAssignTimeoutError';
+        msg += ', working sockets is full';
+      }
+      __err = new Error(msg);
+      __err.name = errorName;
+      __err.requestId = reqId;
+      debug('connectTimer boom!!!!')
+      debug('Request#%d %s %s: %s, connected: %s', reqId, url, __err.name, msg, connected);
+      abortRequest();
+    }, connectTimeout);
+  };
+
+  var startResposneTimer = function() {
+    debug('Response timer ticking, timeout: %d', responseTimeout);
+    responseTimer = setTimeout(function () {
+      responseTimer = null;
+      var msg = 'Response timeout for ' + responseTimeout + 'ms';
+      var errorName = 'ResponseTimeoutError';
+      __err = new Error(msg);
+      __err.name = errorName;
+      __err.requestId = reqId;
+      debug('responseTimer boom!!!!')
+      debug('Request#%d %s %s: %s, connected: %s', reqId, url, __err.name, msg, connected);
+      abortRequest();
+    }, responseTimeout);
+  };
+
   var req;
   // request headers checker will throw error
   try {
@@ -648,27 +710,16 @@ exports.requestWithCallback = function (url, args, callback) {
     req.abort();
   };
 
-  var timeout = ms(args.timeout);
-
-  timer = setTimeout(function () {
-    timer = null;
-    var msg = 'Request timeout for ' + timeout + 'ms';
-    var errorName = connected ? 'ResponseTimeoutError' : 'ConnectionTimeoutError';
-    if (!req.socket) {
-      errorName = 'SocketAssignTimeoutError';
-      msg += ', working sockets is full';
-    }
-    __err = new Error(msg);
-    __err.name = errorName;
-    __err.requestId = reqId;
-    debug('Request#%d %s %s: %s, connected: %s', reqId, url, __err.name, msg, connected);
-    abortRequest();
-  }, timeout);
 
   req.once('socket', function (socket) {
     // https://github.com/iojs/io.js/blob/v1.x/lib/net.js#L342
     if (socket.readyState === 'opening') {
+      startConnectTimer();
       socket.once('connect', function () {
+        // cancel socket timer at first and start tick for TTFB
+        cancelConnectTimer();
+        startResposneTimer();
+
         debug('Request#%d %s new socket connected', reqId, url);
         connected = true;
       });

diff --git a/test/urllib.test.js b/test/urllib.test.js
@@ -257,10 +257,10 @@ describe('test/urllib.test.js', function () {
 
     describe('ConnectionTimeoutError and ResponseTimeoutError', function () {
       it('should connection timeout', function (done) {
-        urllib.request('http://npm.taobao.org', { timeout: 1 }, function (err, data, res) {
+        urllib.request('http://npm.taobao.net', { timeout: 1 }, function (err, data, res) {
           should.exist(err);
           err.name.should.equal('ConnectionTimeoutError');
-          err.message.should.match(/^Request timeout for 1ms\, GET http/);
+          err.message.should.match(/^Connect timeout for 1ms\, GET http/);
           err.requestId.should.be.instanceof(Number);
           should.not.exist(data);
           should.exist(res);
@@ -272,7 +272,33 @@ describe('test/urllib.test.js', function () {
         urllib.request(host + '/response_timeout', { timeout: 450 }, function (err, data, res) {
           should.exist(err);
           err.name.should.equal('ResponseTimeoutError');
-          err.message.should.match(/^Request timeout for 450ms\, GET http/);
+          err.message.should.match(/^Response timeout for 450ms\, GET http/);
+          err.requestId.should.be.instanceof(Number);
+          should.exist(data);
+          data.toString().should.equal('foo');
+          should.exist(res);
+          res.should.status(200);
+          done();
+        });
+      });
+
+      it('can pass two timeout seperately and get connect error', function (done) {
+        urllib.request('http://npm.taobao.net', { timeout: [1, 10000] }, function (err, data, res) {
+          should.exist(err);
+          err.name.should.equal('ConnectionTimeoutError');
+          err.message.should.match(/^Connect timeout for 1ms\, GET http/);
+          err.requestId.should.be.instanceof(Number);
+          should.not.exist(data);
+          should.exist(res);
+          done();
+        });
+      });
+
+      it('can pass two timeout seperately and get response error', function(done) {
+        urllib.request(host + '/response_timeout', { timeout: [1000, 450] }, function (err, data, res) {
+          should.exist(err);
+          err.name.should.equal('ResponseTimeoutError');
+          err.message.should.match(/^Response timeout for 450ms\, GET http/);
           err.requestId.should.be.instanceof(Number);
           should.exist(data);
           data.toString().should.equal('foo');