Add warnings and handle protocol errors gracefuly

README.md

@@ -118,7 +118,7 @@ Please be aware that sending null, undefined and Boolean values will result in t
 
 # API
 
-## Connection Events
+## Connection and other Events
 
 `client` will emit some events about the state of the connection to the Redis server.
 
@@ -155,9 +155,13 @@ writable. This event can be used to stream commands in to Redis and adapt to bac
 If the stream is buffering `client.should_buffer` is set to true. Otherwise the variable is always set to false.
 That way you can decide when to reduce your send rate and resume sending commands when you get `drain`.
 
-You can also check the return value of each command as it will also return the backpressure indicator.
+You can also check the return value of each command as it will also return the backpressure indicator (deprecated).
 If false is returned the stream had to buffer.
 
+### "warning"
+
+`client` will emit `warning` when password was set but none is needed and if a deprecated option / function / similar is used.
+
 ### "idle"
 
 `client` will emit `idle` when there are no outstanding commands that are awaiting a response.

changelog.md

@@ -1,18 +1,20 @@
 Changelog
 =========
 
-## v.2.5.0-0 - xx Dez, 2015
+## v.2.5.0-0 - xx Jan, 2015
 
 Features
 
 -  The parsers moved into the [redis-parser](https://github.com/NodeRedis/node-redis-parser) module and will be maintained in there from now on
  -  Improve js parser speed significantly for big SUNION/SINTER/LRANGE/ZRANGE
--  Improve redis-url parsing to also accept the database-number and options as query parameters as suggested in the [IANA](http://www.iana.org/assignments/uri-schemes/prov/redis)
+-  Improve redis-url parsing to also accept the database-number and options as query parameters as suggested in [IANA](http://www.iana.org/assignments/uri-schemes/prov/redis)
 -  Added a `retry_unfulfilled_commands` option
  -  Setting this to 'true' results in retrying all commands that were not fulfilled on a connection loss after the reconnect. Use with caution
 -  Added a `db` option to select the database while connecting (this is [not recommended](https://groups.google.com/forum/#!topic/redis-db/vS5wX8X4Cjg))
 -  Added a `password` option as alias for auth_pass
 -  The client.server_info is from now on updated while using the info command
+-  Gracefuly handle redis protocol errors from now on
+-  Added a `warning` emitter that receives deprecation messages and other node_redis warnings
 
 Bugfixes
 
@@ -31,7 +33,7 @@ Deprecations
  -  From v.3.0.0 on using a command with such an argument will return an error instead
  -  If you want to keep the old behavior please use a precheck in your code that converts the arguments to a string.
  -  Using SET or SETEX with a undefined or null value will from now on also result in converting the value to "null" / "undefined" to have a consistent behavior. This is not considered as breaking change, as it returned an error earlier.
--  Using .end(flush) without the flush parameter deprecated and the flush parameter should explicitly be used
+-  Using .end(flush) without the flush parameter is deprecated and the flush parameter should explicitly be used
  -  From v.3.0.0 on using .end without flush will result in an error
  -  Using .end without flush means that any command that did not yet return is going to silently fail. Therefor this is considered harmfull and you should explicitly silence such errors if you are sure you want this
 -  Depending on the return value of a command to detect the backpressure is deprecated

index.js

@@ -38,6 +38,7 @@ function RedisClient (options) {
     options = clone(options);
     events.EventEmitter.call(this);
     var cnx_options = {};
+    var self = this;
     if (options.path) {
         cnx_options.path = options.path;
         this.address = options.path;
@@ -58,11 +59,13 @@ function RedisClient (options) {
     if (options.socket_nodelay === undefined) {
         options.socket_nodelay = true;
     } else if (!options.socket_nodelay) { // Only warn users with this set to false
-        console.warn(
+        process.nextTick(function () {
-            'node_redis: socket_nodelay is deprecated and will be removed in v.3.0.0.\n' +
+            self.warn(
-            'Setting socket_nodelay to false likely results in a reduced throughput. Please use .batch to buffer commands and use pipelining.\n' +
+                'socket_nodelay is deprecated and will be removed in v.3.0.0.\n' +
-            'If you are sure you rely on the NAGLE-algorithm you can activate it by calling client.stream.setNoDelay(false) instead.'
+                '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.'
+            );
+        });
     }
     if (options.socket_keepalive === undefined) {
         options.socket_keepalive = true;
@@ -74,7 +77,9 @@ 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) {
-        console.warn('>> WARNING: You activated return_buffers and detect_buffers at the same time. The return value is always going to be a buffer.');
+        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.');
+        });
         options.detect_buffers = false;
     }
     if (options.detect_buffers) {
@@ -101,7 +106,6 @@ function RedisClient (options) {
     this.pipeline = 0;
     this.options = options;
     // Init parser
-    var self = this;
     this.reply_parser = new Parser({
         returnReply: function (data) {
             self.return_reply(data);
@@ -109,6 +113,12 @@ function RedisClient (options) {
         returnError: function (data) {
             self.return_error(data);
         },
+        returnFatalError: function (err) {
+            // Error out all fired commands. Otherwise they might rely on faulty data. We have to reconnect to get in a working state again
+            self.flush_and_error(err, ['command_queue']);
+            self.stream.destroy();
+            self.return_error(err);
+        },
         returnBuffers: options.return_buffers || options.detect_buffers,
         name: options.parser
     });
@@ -128,10 +138,10 @@ RedisClient.prototype.create_stream = function () {
 
     /* istanbul ignore if: travis does not work with stunnel atm. Therefor the tls tests are skipped on travis */
     if (this.options.tls) {
-	    this.stream = tls.connect(this.connection_options);
+        this.stream = tls.connect(this.connection_options);
     } else {
-	    this.stream = net.createConnection(this.connection_options);
+        this.stream = net.createConnection(this.connection_options);
-	}
+    }
 
     if (this.options.connect_timeout) {
         this.stream.setTimeout(this.connect_timeout, function () {
@@ -225,6 +235,14 @@ 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);
+    }
+};
+
 // Flush provided queues, erroring any items with a callback first
 RedisClient.prototype.flush_and_error = function (error, queue_names) {
     var command_obj;
@@ -665,10 +683,10 @@ RedisClient.prototype.send_command = function (command, args, callback) {
                 args[i] = args[i].toString();
                 // Add this to parse_arguments.
             } else if (args[i] === null) {
-                console.warn(
+                this.warn(
-                    'node_redis: Deprecated: The %s command contains a "null" argument.\n' +
+                    'Deprecated: The ' + command.toUpperCase() + ' command contains a "null" argument.\n' +
                     'This is converted to a "null" string now and will return an error from v.3.0 on.\n' +
-                    'Please handle this in your code to make sure everything works as you intended it to behave.', command.toUpperCase()
+                    'Please handle this in your code to make sure everything works as you intended it to.'
                 );
                 args[i] = 'null'; // Backwards compatible :/
             } else {
@@ -679,10 +697,10 @@ RedisClient.prototype.send_command = function (command, args, callback) {
                 }
             }
         } else if (typeof args[i] === 'undefined') {
-            console.warn(
+            this.warn(
-                'node_redis: Deprecated: The %s command contains a "undefined" argument.\n' +
+                'Deprecated: The ' + command.toUpperCase() + ' command contains a "undefined" argument.\n' +
                 'This is converted to a "undefined" string now and will return an error from v.3.0 on.\n' +
-                'Please handle this in your code to make sure everything works as you intended it to behave.', command.toUpperCase()
+                'Please handle this in your code to make sure everything works as you intended it to.'
             );
             args[i] = 'undefined'; // Backwards compatible :/
         } else {
@@ -834,8 +852,8 @@ RedisClient.prototype.end = function (flush) {
     if (flush) {
         this.flush_and_error(new Error("The command can't be processed. The connection has already been closed."));
     } else if (arguments.length === 0) {
-        console.warn(
+        this.warn(
-            'node_redis: Using .end() without the flush parameter is deprecated and throws from v.3.0.0 on.\n' +
+            'Using .end() without the flush parameter is deprecated and throws from v.3.0.0 on.\n' +
             'Please check the doku (https://github.com/NodeRedis/node_redis) and explictly use flush.'
         );
     }
@@ -859,7 +877,7 @@ function Multi(client, args) {
     this._client = client;
     this.queue = new Queue();
     var command, tmp_args;
-    if (Array.isArray(args)) {
+    if (args) { // Either undefined or an array. Fail hard if it's not an array
         for (var i = 0; i < args.length; i++) {
             command = args[i][0];
             tmp_args = args[i].slice(1);
@@ -1258,8 +1276,8 @@ var createClient = function (port_arg, host_arg, options) {
     } else if (typeof port_arg === 'string' || port_arg && port_arg.url) {
         options = clone(port_arg.url ? port_arg : host_arg || options);
         var parsed = URL.parse(port_arg.url || port_arg, true, true);
-        // [redis:]//[user][:password@][host][:port][/db-number][?db=db-number[&password=bar[&option=value]]]
+        // [redis:]//[[user][:password]@][host][:port][/db-number][?db=db-number[&password=bar[&option=value]]]
-        if (parsed.hostname) {
+        if (parsed.hostname || parsed.slashes) { // The host might be an empty string
             if (parsed.auth) {
                 options.password = parsed.auth.split(':')[1];
             }
@@ -1269,15 +1287,18 @@ var createClient = function (port_arg, host_arg, options) {
             if (parsed.pathname && parsed.pathname !== '/') {
                 options.db = parsed.pathname.substr(1);
             }
+            options.host = parsed.hostname;
+            options.port = parsed.port;
             if (parsed.search !== '') {
                 var elem;
                 for (elem in parsed.query) { // jshint ignore: line
                     // If options are passed twice, only the parsed options will be used
+                    if (options.hasOwnPropery(elem)) {
+                        RedisClient.warn('WARNING: You passed the ' + elem + ' option twice!');
+                    }
                     options[elem] = parsed.query[elem];
                 }
             }
-            options.host = parsed.hostname;
-            options.port = parsed.port;
         } else {
             options.path = port_arg;
         }

lib/utils.js

@@ -2,31 +2,23 @@
 
 // hgetall converts its replies to an Object. If the reply is empty, null is returned.
 function replyToObject(reply) {
-    var obj = {}, j, jl, key, val;
+    if (reply.length === 0 || !Array.isArray(reply)) { // TODO: Check why the isArray check is needed and what value reply has in that case
-
-    if (reply.length === 0 || !Array.isArray(reply)) {
         return null;
     }
-
+    var obj = {};
-    for (j = 0, jl = reply.length; j < jl; j += 2) {
+    for (var j = 0; j < reply.length; j += 2) {
-        key = reply[j].toString('binary');
+        obj[reply[j].toString('binary')] = reply[j + 1];
-        val = reply[j + 1];
-        obj[key] = val;
     }
-
     return obj;
 }
 
 function replyToStrings(reply) {
-    var i;
-
     if (Buffer.isBuffer(reply)) {
         return reply.toString();
     }
-
     if (Array.isArray(reply)) {
         var res = new Array(reply.length);
-        for (i = 0; i < reply.length; i++) {
+        for (var i = 0; i < reply.length; i++) {
             // Recusivly call the function as slowlog returns deep nested replies
             res[i] = replyToStrings(reply[i]);
         }

test/connection.spec.js

@@ -364,11 +364,20 @@ describe("connection tests", function () {
                 });
 
                 if (ip === 'IPv4') {
-                    it('allows connecting with the redis url and the default port', function (done) {
+                    it('allows connecting with the redis url to the default host and port, select db 3 and warn about duplicate db option', function (done) {
+                        client = redis.createClient('redis:///3?db=3');
+                        assert.strictEqual(client.selected_db, '3');
+                        client.on("ready", done);
+                    });
+
+                    it('allows connecting with the redis url and the default port and auth provided even though it is not required', function (done) {
                         client = redis.createClient('redis://:porkchopsandwiches@' + config.HOST[ip] + '/');
-                        client.on("ready", function () {
+                        var end = helper.callFuncAfter(done, 2);
-                            return done();
+                        client.on('warning', function (msg) {
+                            assert.strictEqual(msg, 'Warning: Redis server does not require a password, but a password was supplied.');
+                            end();
                         });
+                        client.on("ready", end);
                     });
 
                     it('allows connecting with the redis url as first parameter and the options as second parameter', function (done) {

test/helper.js

@@ -171,7 +171,10 @@ module.exports = {
     },
     callFuncAfter: function (func, max) {
         var i = 0;
-        return function () {
+        return function (err) {
+            if (err) {
+                throw err;
+            }
             i++;
             if (i === max) {
                 func();

test/node_redis.spec.js

@@ -550,6 +550,28 @@ describe("The node_redis client", function () {
                 });
             });
 
+            describe('protocol error', function () {
+
+                it("should gracefully recover and only fail on the already send commands", function (done) {
+                    client = redis.createClient.apply(redis.createClient, args);
+                    client.on('error', function(err) {
+                        assert.strictEqual(err.message, 'Protocol error, got "a" as reply type byte');
+                        // After the hard failure work properly again. The set should have been processed properly too
+                        client.get('foo', function (err, res) {
+                            assert.strictEqual(res, 'bar');
+                            done();
+                        });
+                    });
+                    client.once('ready', function () {
+                        client.set('foo', 'bar', function (err, res) {
+                            assert.strictEqual(err.message, 'Protocol error, got "a" as reply type byte');
+                        });
+                        // Fail the set answer. Has no corresponding command obj and will therefor land in the error handler and set
+                        client.reply_parser.execute(new Buffer('a*1\r*1\r$1`zasd\r\na'));
+                    });
+                });
+            });
+
             describe('enable_offline_queue', function () {
                 describe('true', function () {
                     it("should emit drain if offline queue is flushed and nothing to buffer", function (done) {

test/return_buffers.spec.js

@@ -18,17 +18,24 @@ describe("return_buffers", function () {
 
             beforeEach(function (done) {
                 client = redis.createClient.apply(redis.createClient, args);
+                var i = 1;
                 if (args[2].detect_buffers) {
                     // Test if detect_buffer option was deactivated
                     assert.strictEqual(client.options.detect_buffers, false);
                     args[2].detect_buffers = false;
+                    i++;
                 }
+                var end = helper.callFuncAfter(done, i);
+                client.on('warning', function (msg) {
+                    assert.strictEqual(msg, 'WARNING: You activated return_buffers and detect_buffers at the same time. The return value is always going to be a buffer.');
+                    end();
+                });
                 client.once("error", done);
                 client.once("connect", function () {
                     client.flushdb(function (err) {
                         client.hmset("hash key 2", "key 1", "val 1", "key 2", "val 2");
                         client.set("string key 1", "string value");
-                        return done(err);
+                        end(err);
                     });
                 });
             });