Insert deprecation warnings and some minor refactoring

Author: Ruben Bridgewater

README.md

@@ -147,7 +147,7 @@ So please attach the error listener to node_redis.
 
 `client` will emit `end` when an established Redis server connection has closed.
 
-### "drain"
+### "drain" (deprecated)
 
 `client` will emit `drain` when the TCP connection to the Redis server has been buffering, but is now
 writable. This event can be used to stream commands in to Redis and adapt to backpressure.
@@ -162,20 +162,21 @@ If false is returned the stream had to buffer.
 
 `client` will emit `warning` when password was set but none is needed and if a deprecated option / function / similar is used.
 
-### "idle"
+### "idle" (deprecated)
 
 `client` will emit `idle` when there are no outstanding commands that are awaiting a response.
 
 ## redis.createClient()
 If you have `redis-server` running on the same computer as node, then the defaults for
 port and host are probably fine and you don't need to supply any arguments. `createClient()` returns a `RedisClient` object.
 
+If the redis server runs on the same machine as the client consider using unix sockets if possible to increase throughput.
+
 ### overloading
-* `redis.createClient()`
-* `redis.createClient(options)`
-* `redis.createClient(unix_socket, options)`
-* `redis.createClient(redis_url, options)`
-* `redis.createClient(port, host, options)`
+* `redis.createClient([options])`
+* `redis.createClient(unix_socket[, options])`
+* `redis.createClient(redis_url[, options])`
+* `redis.createClient(port[, host][, options])`
 
 #### `options` is an object with the following possible properties:
 * `host`: *127.0.0.1*; The host to connect to
@@ -204,10 +205,10 @@ This delay normally grows infinitely, but setting `retry_max_delay` limits it to
 * `connect_timeout`: *3600000*; Setting `connect_timeout` limits total time for client to connect and reconnect.
 The value is provided in milliseconds and is counted from the moment on a new client is created / a connection is lost. The last retry is going to happen exactly at the timeout time.
 Default is to try connecting until the default system socket timeout has been exceeded and to try reconnecting until 1h passed.
-* `max_attempts`: *0*; By default client will try reconnecting until connected. Setting `max_attempts`
+* `max_attempts`: *0*; (Deprecated, please use `retry_strategy` instead) By default client will try reconnecting until connected. Setting `max_attempts`
 limits total amount of connection tries. Setting this to 1 will prevent any reconnect tries.
 * `retry_unfulfilled_commands`: *false*; If set to true, all commands that were unfulfulled while the connection is lost will be retried after the connection has reestablished again. Use this with caution, if you use state altering commands (e.g. *incr*). This is especially useful if you use blocking commands.
-* `password`: *null*; If set, client will run redis auth command on connect. Alias `auth_pass`
+* `password`: *null*; If set, client will run redis auth command on connect. Alias `auth_pass` (node_redis < 2.5 have to use auth_pass)
 * `db`: *null*; If set, client will run redis select command on connect. This is [not recommended](https://groups.google.com/forum/#!topic/redis-db/vS5wX8X4Cjg).
 * `family`: *IPv4*; You can force using IPv6 if you set the family to 'IPv6'. See Node.js [net](https://nodejs.org/api/net.html) or [dns](https://nodejs.org/api/dns.html) modules how to use the family type.
 * `disable_resubscribing`: *false*; If set to `true`, a client won't resubscribe after disconnecting

index.js

@@ -51,20 +51,32 @@ function RedisClient (options) {
     for (var tls_option in options.tls) { // jshint ignore: line
         cnx_options[tls_option] = options.tls[tls_option];
     }
+    // Warn on misusing deprecated functions
+    if (typeof options.retry_strategy === 'function') {
+        if ('max_attempts' in options) {
+            self.warn('WARNING: You activated the retry_strategy and max_attempts at the same time. This is not possible and max_attempts will be ignored.');
+            // Do not print deprecation warnings twice
+            delete options.max_attempts;
+        }
+        if ('retry_max_delay' in options) {
+            self.warn('WARNING: You activated the retry_strategy and retry_max_delay at the same time. This is not possible and retry_max_delay will be ignored.');
+            // Do not print deprecation warnings twice
+            delete options.retry_max_delay;
+        }
+    }
+
     this.connection_options = cnx_options;
-    this.connection_id = ++connection_id;
+    this.connection_id = RedisClient.connection_id++;
     this.connected = false;
     this.ready = false;
     if (options.socket_nodelay === undefined) {
         options.socket_nodelay = true;
     } else if (!options.socket_nodelay) { // Only warn users with this set to false
-        process.nextTick(function () {
-            self.warn(
-                'socket_nodelay is deprecated and will be removed in v.3.0.0.\n' +
-                'Setting socket_nodelay to false likely results in a reduced throughput. Please use .batch to buffer commands and use pipelining.\n' +
-                'If you are sure you rely on the NAGLE-algorithm you can activate it by calling client.stream.setNoDelay(false) instead.'
-            );
-        });
+        self.warn(
+            'socket_nodelay is deprecated and will be removed in v.3.0.0.\n' +
+            'Setting socket_nodelay to false likely results in a reduced throughput. Please use .batch for pipelining instead.\n' +
+            'If you are sure you rely on the NAGLE-algorithm you can activate it by calling client.stream.setNoDelay(false) instead.'
+        );
     }
     if (options.socket_keepalive === undefined) {
         options.socket_keepalive = true;
@@ -76,9 +88,7 @@ function RedisClient (options) {
     options.detect_buffers = !!options.detect_buffers;
     // Override the detect_buffers setting if return_buffers is active and print a warning
     if (options.return_buffers && options.detect_buffers) {
-        process.nextTick(function () {
-            self.warn('WARNING: You activated return_buffers and detect_buffers at the same time. The return value is always going to be a buffer.');
-        });
+        self.warn('WARNING: You activated return_buffers and detect_buffers at the same time. The return value is always going to be a buffer.');
         options.detect_buffers = false;
     }
     if (options.detect_buffers) {
@@ -87,11 +97,27 @@ function RedisClient (options) {
     }
     this.should_buffer = false;
     this.max_attempts = options.max_attempts | 0;
+    if ('max_attempts' in options) {
+        self.warn(
+            'max_attempts is deprecated and will be removed in v.3.0.0.\n' +
+            'To reduce the amount of options and the improve the reconnection handling please use the new `retry_strategy` option instead.\n' +
+            'This replaces the max_attempts and retry_max_delay option.'
+        );
+    }
     this.command_queue = new Queue(); // Holds sent commands to de-pipeline them
     this.offline_queue = new Queue(); // Holds commands issued but not able to be sent
+    // ATTENTION: connect_timeout should change in v.3.0 so it does not count towards ending reconnection attempts after x seconds
+    // This should be done by the retry_strategy. Instead it should only be the timeout for connecting to redis
     this.connect_timeout = +options.connect_timeout || 3600000; // 60 * 60 * 1000 ms
     this.enable_offline_queue = options.enable_offline_queue === false ? false : true;
     this.retry_max_delay = +options.retry_max_delay || null;
+    if ('retry_max_delay' in options) {
+        self.warn(
+            'retry_max_delay is deprecated and will be removed in v.3.0.0.\n' +
+            'To reduce the amount of options and the improve the reconnection handling please use the new `retry_strategy` option instead.\n' +
+            'This replaces the max_attempts and retry_max_delay option.'
+        );
+    }
     this.initialize_retry_vars();
     this.pub_sub_mode = false;
     this.subscription_set = {};
@@ -123,8 +149,24 @@ function RedisClient (options) {
         name: options.parser
     });
     this.create_stream();
+    // The listeners will not be attached right away, so let's print the deprecation message while the listener is attached
+    this.on('newListener', function (event) {
+        if (event === 'idle') {
+            this.warn(
+                'The idle event listener is deprecated and will likely be removed in v.3.0.0.\n' +
+                'If you rely on this feature please open a new ticket in node_redis with your use case'
+            );
+        } else if (event === 'drain') {
+            this.warn(
+                'The drain event listener is deprecated and will be removed in v.3.0.0.\n' +
+                'If you want to keep on listening to this event please listen to the stream drain event directly.'
+            );
+        }
+    });
 }
-util.inherits(RedisClient, events.EventEmitter);
+util.inherits(RedisClient, EventEmitter);
+
+RedisClient.connection_id = 0;
 
 // Attention: the function name "create_stream" should not be changed, as other libraries need this to mock the stream (e.g. fakeredis)
 RedisClient.prototype.create_stream = function () {
@@ -238,19 +280,23 @@ RedisClient.prototype.unref = function () {
 };
 
 RedisClient.prototype.warn = function (msg) {
-    if (this.listeners('warning').length !== 0) {
-        this.emit('warning', msg);
-    } else {
-        console.warn('node_redis:', msg);
-    }
+    var self = this;
+    // Warn on the next tick. Otherwise no event listener can be added
+    // for warnings that are emitted in the redis client constructor
+    process.nextTick(function () {
+        if (self.listeners('warning').length !== 0) {
+            self.emit('warning', msg);
+        } else {
+            console.warn('node_redis:', msg);
+        }
+    });
 };
 
 // Flush provided queues, erroring any items with a callback first
 RedisClient.prototype.flush_and_error = function (error, queue_names) {
-    var command_obj;
     queue_names = queue_names || ['offline_queue', 'command_queue'];
     for (var i = 0; i < queue_names.length; i++) {
-        while (command_obj = this[queue_names[i]].shift()) {
+        for (var command_obj = this[queue_names[i]].shift(); command_obj; command_obj = this[queue_names[i]].shift()) {
             if (typeof command_obj.callback === 'function') {
                 error.command = command_obj.command.toUpperCase();
                 command_obj.callback(error);
@@ -419,9 +465,7 @@ RedisClient.prototype.ready_check = function () {
 };
 
 RedisClient.prototype.send_offline_queue = function () {
-    var command_obj;
-
-    while (command_obj = this.offline_queue.shift()) {
+    for (var command_obj = this.offline_queue.shift(); command_obj; command_obj = this.offline_queue.shift()) {
         debug('Sending offline command: ' + command_obj.command);
         this.send_command(command_obj.command, command_obj.args, command_obj.callback);
     }
@@ -805,8 +849,7 @@ RedisClient.prototype.writeDefault = RedisClient.prototype.writeStrings = functi
 };
 
 RedisClient.prototype.writeBuffers = function (data) {
-    var command;
-    while (command = this.pipeline_queue.shift()) {
+    for (var command = this.pipeline_queue.shift(); command; command = this.pipeline_queue.shift()) {
         this.stream.write(command);
     }
     this.should_buffer = !this.stream.write(data);

test/commands/blpop.spec.js

@@ -4,6 +4,7 @@ var assert = require("assert");
 var config = require("../lib/config");
 var helper = require("../helper");
 var redis = config.redis;
+var intercept = require('intercept-stdout');
 
 describe("The 'blpop' method", function () {
 
@@ -23,7 +24,14 @@ describe("The 'blpop' method", function () {
             it('pops value immediately if list contains values', function (done) {
                 bclient = redis.createClient.apply(redis.createClient, args);
                 redis.debug_mode = true;
+                var text = '';
+                var unhookIntercept = intercept(function(data) {
+                    text += data;
+                    return '';
+                });
                 client.rpush("blocking list", "initial value", helper.isNumber(1));
+                unhookIntercept();
+                assert(/^Send 127\.0\.0\.1:6379 id [0-9]+: \*3\r\n\$5\r\nrpush\r\n\$13\r\nblocking list\r\n\$13\r\ninitial value\r\n\n$/.test(text));
                 redis.debug_mode = false;
                 bclient.blpop("blocking list", 0, function (err, value) {
                     assert.strictEqual(value[0], "blocking list");

test/commands/get.spec.js

@@ -68,7 +68,6 @@ describe("The 'get' method", function () {
                     });
 
                     it("gets the value correctly", function (done) {
-                        client.GET(key, redis.print); // Use the utility function to print the result
                         client.GET(key, function (err, res) {
                             helper.isString(value)(err, res);
                             done(err);

test/commands/getset.spec.js

@@ -33,7 +33,6 @@ describe("The 'getset' method", function () {
                 });
 
                 it("reports an error", function (done) {
-                    client.GET(key, redis.print); // Use the utility function to print the error
                     client.get(key, function (err, res) {
                         assert(err.message.match(/The connection has already been closed/));
                         done();

test/connection.spec.js

@@ -4,6 +4,7 @@ var assert = require("assert");
 var config = require("./lib/config");
 var helper = require('./helper');
 var redis = config.redis;
+var intercept = require('intercept-stdout');
 
 describe("connection tests", function () {
     helper.allTests(function(parser, ip, args) {
@@ -91,6 +92,7 @@ describe("connection tests", function () {
 
                     client.on("reconnecting", function (params) {
                         client.end(true);
+                        assert.strictEqual(params.times_connected, 1);
                         setTimeout(done, 100);
                     });
                 });

test/lib/stunnel-process.js

@@ -2,11 +2,16 @@
 
 // helper to start and stop the stunnel process.
 var spawn = require('child_process').spawn;
-var EventEmitter = require('events').EventEmitter;
+var EventEmitter = require('events');
 var fs = require('fs');
 var path = require('path');
 var util = require('util');
 
+// Newer Node.js versions > 0.10 return the EventEmitter right away and using .EventEmitter was deprecated
+if (typeof EventEmitter !== 'function') {
+    EventEmitter = EventEmitter.EventEmitter;
+}
+
 function once(cb) {
     var called = false;
     return function() {

test/node_redis.spec.js

@@ -384,9 +384,18 @@ describe("The node_redis client", function () {
 
                 describe('idle', function () {
                     it('emits idle as soon as there are no outstanding commands', function (done) {
+                        var end = helper.callFuncAfter(done, 2);
+                        client.on('warning', function (msg) {
+                            assert.strictEqual(
+                                msg,
+                                'The idle event listener is deprecated and will likely be removed in v.3.0.0.\n' +
+                                'If you rely on this feature please open a new ticket in node_redis with your use case'
+                            );
+                            end();
+                        });
                         client.on('idle', function onIdle () {
                             client.removeListener("idle", onIdle);
-                            client.get('foo', helper.isString('bar', done));
+                            client.get('foo', helper.isString('bar', end));
                         });
                         client.set('foo', 'bar');
                     });
@@ -614,9 +623,17 @@ describe("The node_redis client", function () {
                             parser: parser,
                             no_ready_check: true
                         });
-                        var end = helper.callFuncAfter(done, 2);
+                        var end = helper.callFuncAfter(done, 3);
                         client.set('foo', 'bar');
                         client.get('foo', end);
+                        client.on('warning', function (msg) {
+                            assert.strictEqual(
+                                msg,
+                                'The drain event listener is deprecated and will be removed in v.3.0.0.\n' +
+                                'If you want to keep on listening to this event please listen to the stream drain event directly.'
+                            );
+                            end();
+                        });
                         client.on('drain', function() {
                             assert(client.offline_queue.length === 0);
                             end();
@@ -673,6 +690,9 @@ describe("The node_redis client", function () {
                         client.on('reconnecting', function(params) {
                             i++;
                             assert.equal(params.attempt, i);
+                            assert.strictEqual(params.times_connected, 0);
+                            assert(params.error instanceof Error);
+                            assert(typeof params.total_retry_time === 'number');
                             assert.strictEqual(client.offline_queue.length, 2);
                         });
 
@@ -706,10 +726,6 @@ describe("The node_redis client", function () {
                             helper.killConnection(client);
                         });
 
-                        client.on("reconnecting", function (params) {
-                            assert.equal(client.command_queue.length, 15);
-                        });
-
                         client.on('error', function(err) {
                             if (/uncertain state/.test(err.message)) {
                                 assert.equal(client.command_queue.length, 0);
@@ -787,10 +803,6 @@ describe("The node_redis client", function () {
                             helper.killConnection(client);
                         });
 
-                        client.on("reconnecting", function (params) {
-                            assert.equal(client.command_queue.length, 15);
-                        });
-
                         client.on('error', function(err) {
                             if (err.code === 'UNCERTAIN_STATE') {
                                 assert.equal(client.command_queue.length, 0);

test/utils.spec.js

@@ -3,6 +3,7 @@
 var assert = require('assert');
 var Queue = require('double-ended-queue');
 var utils = require('../lib/utils');
+var intercept = require('intercept-stdout');
 
 describe('utils.js', function () {
 
@@ -40,6 +41,30 @@ describe('utils.js', function () {
         });
     });
 
+    describe('print helper', function () {
+        it('callback with reply', function () {
+            var text = '';
+            var unhookIntercept = intercept(function(data) {
+                text += data;
+                return '';
+            });
+            utils.print(null, 'abc');
+            unhookIntercept();
+            assert.strictEqual(text, 'Reply: abc\n');
+        });
+
+        it('callback with error', function () {
+            var text = '';
+            var unhookIntercept = intercept(function(data) {
+                text += data;
+                return '';
+            });
+            utils.print(new Error('Wonderful exception'));
+            unhookIntercept();
+            assert.strictEqual(text, 'Error: Wonderful exception\n');
+        });
+    });
+
     describe('reply_in_order', function () {
 
         var err_count = 0;