add more context for request and connect timeout errors

lib/HttpClient.js

@@ -2,6 +2,7 @@
 
 'use strict';
 
+// core modules
 var EventEmitter = require('events').EventEmitter;
 var fs = require('fs');
 var http = require('http');
@@ -12,19 +13,23 @@ var querystring = require('querystring');
 var url = require('url');
 var util = require('util');
 
+// external modules
+var _ = require('lodash');
 var assert = require('assert-plus');
 var backoff = require('backoff');
+var errors = require('restify-errors');
 var mime = require('mime');
 var once = require('once');
+var semver = require('semver');
 var tunnelAgent = require('tunnel-agent');
+var verror = require('verror');
 
+// local globals
 var bunyanHelper = require('./helpers/bunyan');
 var dtrace = require('./helpers/dtrace');
 var auditor = require('./helpers/auditor');
-var errors = require('restify-errors');
 
 // Use native KeepAlive in Node as of 0.11.6
-var semver = require('semver');
 var nodeVersion = process.version;
 var nativeKeepAlive = semver.satisfies(nodeVersion, '>=0.11.6');
 var KeepAliveAgent;
@@ -46,11 +51,20 @@ if (!nativeKeepAlive) {
     httpsMaxSockets = Math.min(httpsMaxSockets, 1024);
 }
 
+// require('http').STATUS_CODES has a http 429 error that in < node 4 was known
+// as TooManyRedirects. This has been rectified as TooManyRequests.
+// in the case of restify-clients, we will retain this error type given that
+// users can specify maximum number of redirects to follow.
+if (!errors.TooManyRedirectsError) {
+    errors.makeConstructor('TooManyRedirectsError');
+}
+
+
 // --- Globals
 
-// jscs:disable maximumLineLength
-var VERSION = JSON.parse(fs.readFileSync(path.join(__dirname, './../package.json'), 'utf8')).version;
-// jscs:enable maximumLineLength
+var VERSION = JSON.parse(fs.readFileSync(
+    path.join(__dirname, './../package.json'), 'utf8'
+)).version;
 
 var REDIRECT_CODES = [301, 302, 303, 307];
 
@@ -87,29 +101,89 @@ function defaultUserAgent() {
     return (UA);
 }
 
-if (!errors.TooManyRedirectsError) {
-    errors.makeConstructor('TooManyRedirectsError');
+
+/**
+ * build a request timeout error based for a request.
+ * @private
+ * @function buildRequestTimeoutError
+ * @param {Object} opts options object for a request
+ * @returns {Object} RequestTimeoutError
+ */
+function buildRequestTimeoutError(opts) {
+    assert.object(opts, 'opts');
+
+    var fullUrl = fullRequestUrl(opts);
+    var errMsg = [
+        opts.method,
+        'request to',
+        fullUrl,
+        'failed to complete within',
+        opts.requestTimeout + 'ms'
+    ].join(' ');
+
+    return new verror.VError({
+        name: 'RequestTimeoutError',
+        info: {
+            fullUrl: fullUrl,
+            requestTimeout: opts.requestTimeout
+        }
+    }, errMsg);
 }
 
-function ConnectTimeoutError(ms) {
-    if (Error.captureStackTrace) {
-        Error.captureStackTrace(this, ConnectTimeoutError);
-    }
 
-    this.message = 'connect timeout after ' + ms + 'ms';
-    this.name = 'ConnectTimeoutError';
+/**
+ * build a connect timeout error based for a request.
+ * @private
+ * @function buildConnectionTimeoutError
+ * @param {Object} opts options object for a request
+ * @returns {Object} ConnectionTimeoutError
+ */
+function buildConnectTimeoutError(opts) {
+    assert.object(opts, 'opts');
+
+    var fullUrl = fullRequestUrl(opts);
+    var errMsg = [
+        opts.method,
+        'request to',
+        fullUrl,
+        'failed to obtain connected socket within',
+        opts.connectTimeout + 'ms'
+    ].join(' ');
+
+    return new verror.VError({
+        name: 'ConnectTimeoutError',
+        info: {
+            fullUrl: fullUrl,
+            connectTimeout: opts.connectTimeout
+        }
+    }, errMsg);
 }
-util.inherits(ConnectTimeoutError, Error);
 
-function RequestTimeoutError(ms) {
-    if (Error.captureStackTrace) {
-        Error.captureStackTrace(this, RequestTimeoutError);
-    }
 
-    this.message = 'request timeout after ' + ms + 'ms';
-    this.name = 'RequestTimeoutError';
+/**
+ * given options object for a request, build the fully qualified url. this
+ * method needs to exist primarily a result of the inconsistencies in core API
+ * make this a little bit more confusing than it needs to be. in short,
+ * http.request()'s path is not the same as url.parse()'s path, so some
+ * normalization needs to happen here.
+ * https://nodejs.org/api/url.html#url_url_strings_and_url_objects
+ * @private
+ * @function fullRequestUrl
+ * @param {Object} opts request options
+ * @returns {String}
+ */
+function fullRequestUrl(opts) {
+    assert.object(opts, 'opts');
+
+    var urlObj = _.assign({}, url.parse(opts.path), {
+        protocol: opts.protocol,
+        host: opts.host,
+        port: opts.port
+    });
+
+    return url.format(urlObj);
 }
-util.inherits(RequestTimeoutError, Error);
+
 
 function rawRequest(opts, cb) {
     assert.object(opts, 'options');
@@ -139,7 +213,9 @@ function rawRequest(opts, cb) {
                 req.abort();
             }
 
-            var err = new ConnectTimeoutError(opts.connectTimeout);
+            // build connect timeout error using current request options
+            var err = buildConnectTimeoutError(opts);
+
             dtrace._rstfy_probes['client-error'].fire(function () {
                 return ([id, err.toString()]);
             });
@@ -290,7 +366,8 @@ function rawRequest(opts, cb) {
                 requestTimer = setTimeout(function requestTimeout() {
                     requestTimer = null;
 
-                    var err = new RequestTimeoutError(opts.requestTimeout);
+                    var err = buildRequestTimeoutError(opts);
+
                     dtrace._rstfy_probes['client-error'].fire(function () {
                         return ([id, err.toString()]);
                     });
@@ -738,6 +815,10 @@ HttpClient.prototype._options = function (method, options) {
         opts.maxRedirects = this.maxRedirects;
     }
 
+    // this.url is an object created from core's url.parse() method. these are
+    // like the "default" options for the client. merge all properties
+    // from this object into the options object IFF they haven't already been
+    // set.
     Object.keys(this.url).forEach(function (k) {
         if (!opts[k]) {
             opts[k] = self.url[k];

package.json

@@ -48,7 +48,7 @@
     "coveralls": "^2.11.4",
     "eslint": "^3.1.0",
     "istanbul": "^0.4.0",
-    "jscs": "^3.0.0",
+    "jscs": "^2.11.0",
     "json": "^9.0.4",
     "mkdirp": "^0.5.1",
     "mocha": "^2.3.3",
@@ -70,6 +70,7 @@
     "restify-errors": "^3.1.0",
     "semver": "^5.0.1",
     "tunnel-agent": "^0.4.0",
-    "uuid": "^3.0.1"
+    "uuid": "^3.0.1",
+    "verror": "^1.9.0"
   }
 }

test/index.js

@@ -9,9 +9,10 @@ var assert = require('chai').assert;
 var bunyan = require('bunyan');
 var crypto = require('crypto');
 var format = require('util').format;
+var restify = require('restify');
 var uuid   = require('uuid');
+var verror = require('verror');
 
-var restify = require('restify');
 var clients = require('../lib');
 var auditor = require('../lib/helpers/auditor');
 var pkgJson = require('../package');
@@ -706,18 +707,40 @@ describe('restify-client tests', function () {
             agent: false
         });
 
-        client.get('/foo', function (err, req) {
-            assert.ok(err);
+        client.get({
+            path: '/foo',
+            query: { a: 1 }
+        }, function (err, req) {
+            assert.isTrue(err instanceof Error);
             assert.equal(err.name, 'ConnectTimeoutError');
+            assert.equal(
+                err.message,
+                'GET request to http://169.254.1.10/foo?a=1 failed to ' +
+                'obtain connected socket within 100ms'
+            );
+            assert.deepEqual(verror.info(err), {
+                fullUrl: 'http://169.254.1.10/foo?a=1',
+                connectTimeout: 100
+            });
             done();
         });
     });
 
     it('requestTimeout', function (done) {
         TIMEOUT_CLIENT.get('/str/request_timeout',
             function (err, req, res, obj) {
-            assert.ok(err);
+            assert.isTrue(err instanceof Error);
             assert.equal(err.name, 'RequestTimeoutError');
+            assert.equal(
+                err.message,
+                'GET request to ' +
+                'http://127.0.0.1:' + PORT + '/str/request_timeout ' +
+                'failed to complete within 150ms'
+            );
+            assert.deepEqual(verror.info(err), {
+                fullUrl: 'http://127.0.0.1:' + PORT + '/str/request_timeout',
+                requestTimeout: 150
+            });
             done();
         });
     });

tools/nspBadge.js

@@ -6,13 +6,11 @@ var fs = require('fs');
 var path = require('path');
 
 var README_PATH = path.join(__dirname, '../README.md');
-/* jscs:disable maximumLineLength */
 var FAIL_BADGE = 'vulnerabilities%20found-red';
 var SUCCESS_BADGE = 'no%20vulnerabilities-green';
 var NSP_LINE_ID = '[NSP Status]';
-/* jscs:enable maximumLineLength */
 
-process.stdin.on('data', function(exitCodeBuf) {
+process.stdin.on('data', function (exitCodeBuf) {
 
     var nspExitCode = parseInt(exitCodeBuf.toString(), 10);
 
@@ -32,7 +30,7 @@ function processLines(exitCode, readmeStr) {
     var lines = readmeStr.toString().split('\n');
     var outLines = '';
 
-    lines.forEach(function(line) {
+    lines.forEach(function (line) {
         if (line.indexOf(NSP_LINE_ID) > -1) {
             if (exitCode === 0) {
                 outLines += line.replace(FAIL_BADGE, SUCCESS_BADGE) + '\n';