move to node-backoff in HttpClient

Author: Mark Cavage

CHANGES.md

@@ -2,7 +2,9 @@
 
 ## 2.0.5 (not yet released)
 
-- Support keep-alive by default in client
+- GH-291 fix overwriting `options.type` in createJSONClient (Trent Mick)
+- Move to `node-backoff` and rework retry logic in HttpClient
+- Support keep-alive by default in HttpClient
 
 ## 2.0.4
 

bin/report-latency

@@ -4,9 +4,9 @@
 var fs = require('fs');
 var path = require('path');
 var spawn = require('child_process').spawn;
+var readline = require('readline');
 var sprintf = require('util').format;
 
-var byline = require('byline');
 var nopt = require('nopt');
 
 
@@ -145,15 +145,17 @@ var buckets = {};
 
 var done = 0;
 parsed.argv.remain.forEach(function (f) {
-        var stream = byline(fs.createReadStream(f));
-        stream.on('data', function (l) {
+        var stream = readline.createInterface({
+                input: fs.createReadStream(f),
+                output: null
+        })
+        stream.on('line', function (l) {
                 var record;
                 var t = -1;
 
                 try {
                         record = JSON.parse(l);
                 } catch (e) {}
-                // console.error('%s: line was: "%s"', e, l);
 
                 if (!record)
                         return;

lib/clients/http_client.js

@@ -12,9 +12,10 @@ var util = require('util');
 var zlib = require('zlib');
 
 var assert = require('assert-plus');
+var backoff = require('backoff');
 var KeepAliveAgent = require('keep-alive-agent');
 var mime = require('mime');
-var retry = require('retry');
+var once = require('once');
 var semver = require('semver');
 var uuid = require('node-uuid');
 
@@ -42,38 +43,84 @@ function defaultUserAgent() {
 }
 
 
-function newRetryOperation(options) {
-        var operation;
-        if (options.retry !== false) {
-                operation = retry.operation(options.retry);
-        } else {
-                // Stub out node-retry so the code isn't fugly in the
-                // mainline client
-                operation = {
-                        attempt: function (callback) {
-                                return (callback(1));
-                        },
-                        retry: function (err) {
-                                operation._err = err;
-                                return (false);
-                        },
-                        mainError: function () {
-                                return (operation._err);
+function ConnectTimeoutError(ms) {
+        if (Error.captureStackTrace)
+                Error.captureStackTrace(this, ConnectTimeoutError);
+
+        this.message = 'connect timeout after ' + ms + 'ms';
+}
+util.inherits(ConnectTimeoutError, Error);
+
+
+function rawRequest(opts, cb) {
+        assert.object(opts, 'options');
+        assert.object(opts.log, 'options.log');
+        assert.func(cb, 'callback');
+
+        cb = once(cb);
+
+        var log = opts.log;
+        var proto = opts.protocol === 'https:' ? https : http;
+        var timer;
+
+        if (opts.cert && opts.key)
+                opts.agent = false;
+
+        if (opts.connectTimeout) {
+                timer = setTimeout(function connectTimeout() {
+                        timer = null;
+                        if (req) {
+                                req.abort();
                         }
-                };
+
+                        cb(new ConnectTimeoutError(opts.connectTimeout), req);
+                }, opts.connectTimeout);
         }
 
-        return (operation);
-}
+        var req = proto.request(opts, function onResponse(res) {
+                clearTimeout(timer);
+                log.trace({client_res: res}, 'Response received');
 
+                res.log = log;
 
-function ConnectTimeoutError() {
-        if (Error.captureStackTrace)
-                Error.captureStackTrace(this, ConnectTimeoutError);
+                var err;
+                if (res.statusCode >= 400)
+                        err = errors.codeToHttpError(res.statusCode);
 
-        this.message = util.format.apply(util, arguments);
-}
-util.inherits(ConnectTimeoutError, Error);
+                req.removeAllListeners('error');
+                req.removeAllListeners('socket');
+                req.emit('result', (err || null), res);
+        });
+        req.log = log;
+
+        req.on('error', function onError(err) {
+                log.trace({err: err}, 'Request failed');
+                clearTimeout(timer);
+
+                cb(err, req);
+                if (req) {
+                        process.nextTick(function () {
+                                req.emit('result', err, null);
+                        });
+                }
+        });
+
+        req.once('socket', function onSocket(socket) {
+                if (socket.writable && !socket._connecting) {
+                        clearTimeout(timer);
+                        cb(null, req);
+                        return;
+                }
+
+                socket.once('connect', function onConnect() {
+                        clearTimeout(timer);
+                        cb(null, req);
+                });
+        });
+
+        if (opts.signRequest)
+                opts.signRequest(req);
+} // end `rawRequest`
 
 
 
@@ -234,115 +281,39 @@ HttpClient.prototype.basicAuth = function basicAuth(username, password) {
 };
 
 
-HttpClient.prototype.request = function request(options, callback) {
-        var done = false;
-
-        function _callback(err, req) {
-                if (done)
-                        return (false);
-
-                done = true;
-                return (callback(err, req));
-        }
+HttpClient.prototype.request = function request(opts, cb) {
+        assert.object(opts, 'options');
+        assert.func(cb, 'callback');
 
-        // Don't rely on x-request-id, as the server may not accept it
-        var log = this.log;
-        var operation = newRetryOperation(options);
-        var proto = options.protocol === 'https:' ? https : http;
-        var signRequest = this.signRequest;
-
-        operation.attempt(function retryCallback(currentAttempt) {
-                var timer = false;
-                function clearTimer() {
-                        if (timer)
-                                clearTimeout(timer);
+        var self = this;
 
-                        timer = false;
+        // Make one request, then kick off the next call; note that
+        // node-backoff always imposes an initial time delay before
+        // starting, which we don't want.
+        rawRequest(opts, function _callback(err, req) {
+                if (!err || opts.retry === false) {
+                        cb(err, req);
+                        return;
                 }
 
-                log.trace({
-                        client_req: options,
-                        attempt: currentAttempt
-                }, 'sending request');
+                var retry = backoff.call(rawRequest, opts, cb);
+                retry.setStrategy(new backoff.ExponentialStrategy({
+                        initialDelay: self.retry.minTimeout || 1000,
+                        maxDelay: self.retry.maxTimeout || Infinity
+                }));
 
-                if (options.cert && options.key)
-                        options.agent = new proto.Agent(options);
-
-                var req = proto.request(options, function requestCallback(res) {
-                        clearTimer();
-                        log.trace({client_res: res}, 'Response received');
-
-                        res.log = log;
-
-                        var err = null;
-                        if (res.statusCode >= 400)
-                                err = errors.codeToHttpError(res.statusCode);
-
-                        req.emit('result', err, res);
-                });
-
-                req.log = log;
-                if (signRequest)
-                        signRequest(req);
-
-                req.on('error', function onError(err) {
-                        log.trace({err: err}, 'Request failed');
-                        clearTimer();
-                        if (!done) {
-                                if (!operation.retry(err)) {
-                                        err = operation.mainError() || err;
-                                        return (_callback(err));
-                                }
-                        } else {
-                                // Since we're changing subtly the way the
-                                // client interacts with callbacks to handle
-                                // socket connections, we have to handle the
-                                // case where the caller was told we're
-                                // "good to go", and instead mimic the behavior
-                                // of there being a bad http code. I.e., a
-                                // server may just TCP RST us instead of a 400
-                                req.emit('result', err, null);
-                        }
-
-                        return (false);
-                });
-
-                if (options.connectTimeout) {
-                        timer = setTimeout(function connectTimeout() {
-                                clearTimer();
-                                req.abort();
-
-                                var to = options.connectTimeout;
-                                var err = new ConnectTimeoutError('timeout ' +
-                                                                  'after ' +
-                                                                  to +
-                                                                  'ms');
-
-                                if (!operation.retry(err)) {
-                                        err = operation.mainError() || err;
-                                        return (_callback(err));
-                                }
-
-                                return (false);
-                        }, options.connectTimeout);
+                if (typeof (opts.retry.retries) === 'number') {
+                        if (opts.retry.retries !== Infinity)
+                                retry.failAfter(opts.retry.retries);
+                } else {
+                        retry.failAfter(4);
                 }
 
-                req.once('socket', function onSocket(socket) {
-                        if (socket.writable && !socket._connecting) {
-                                clearTimer();
-                                return (_callback(null, req));
-                        }
-
-                        return socket.once('connect', function onConnect() {
-                                clearTimer();
-                                return (_callback(null, req));
-                        });
-                });
+                retry.on('backoff', self.emit.bind(self, 'attempt'));
         });
 };
 
 
-
 HttpClient.prototype._options = function (method, options) {
         if (typeof (options) !== 'object')
                 options = { path: options };
@@ -354,9 +325,11 @@ HttpClient.prototype._options = function (method, options) {
                 connectTimeout: options.connectTimeout || self.connectTimeout,
                 headers: options.headers || {},
                 key: options.key || self.key,
+                log: options.log || self.log,
                 method: method,
                 path: options.path || self.path,
-                retry: options.retry || self.retry
+                retry: options.retry || self.retry,
+                signRequest: options.signRequest || self.signRequest
         };
 
         // Backwards compatibility with restify < 1.0

lib/index.js

@@ -89,7 +89,7 @@ function createServer(options) {
                         return (false);
                 }
 
-                res.send(new InternalError(e.message || 'unexpected error'));
+                res.send(new InternalError(e, e.message || 'unexpected error'));
                 return (true);
         });
 

package.json

@@ -37,20 +37,19 @@
         },
         "dependencies": {
                 "assert-plus": "0.1.2",
-                "bunyan": "0.16.8",
-                "clone": "git://github.com/pvorb/node-clone.git#341dd3f",
-                "dtrace-provider": "0.2.6",
-                "byline": "2.0.3",
+                "backoff": "1.2.0",
+                "bunyan": "0.17.0",
+                "clone": "0.1.5",
+                "dtrace-provider": "0.2.7",
                 "formidable": "1.0.11",
                 "http-signature": "0.9.9",
                 "keep-alive-agent": "0.0.1",
                 "lru-cache": "2.2.1",
                 "mime": "1.2.7",
                 "node-uuid": "1.4.0",
                 "once": "1.1.1",
-                "qs": "0.5.2",
-                "retry": "0.6.0",
-                "semver": "1.1.1",
+                "qs": "0.5.3",
+                "semver": "1.1.2",
                 "spdy": "1.3.9",
                 "verror": "1.3.5"
         },

test/client.test.js

@@ -76,12 +76,6 @@ before(function (callback) {
                         log: helper.getLog('server')
                 });
 
-                SERVER.on('clientError', function (err) {
-                        console.error('CLIENT ERROR');
-                        console.error(err.stack);
-                        // process.exit(1);
-                });
-
                 SERVER.use(restify.acceptParser(['json', 'text/plain']));
                 SERVER.use(restify.dateParser());
                 SERVER.use(restify.authorizationParser());
@@ -159,7 +153,7 @@ test('GET json', function (t) {
 test('GH-115 GET path with spaces', function (t) {
         JSON_CLIENT.get('/json/foo bar', function (err, req, res, obj) {
                 t.ok(err);
-                console.log(err);
+                t.equal(err.code, 'ECONNRESET');
                 t.end();
         });
 });
@@ -362,7 +356,6 @@ test('GH-20 connectTimeout', function (t) {
 
         client.get('/foo', function (err, req) {
                 t.ok(err);
-                t.notOk(req);
                 t.end();
         });
 });