From 8dcf06754df226f73fd485a542fb0bdf0d89d9f8 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Jan 2016 22:28:26 +0100 Subject: [PATCH 01/28] Add warnings and handle protocol errors gracefuly --- README.md | 8 +++-- changelog.md | 8 +++-- index.js | 67 ++++++++++++++++++++++++------------- lib/utils.js | 18 +++------- test/connection.spec.js | 15 +++++++-- test/helper.js | 5 ++- test/node_redis.spec.js | 22 ++++++++++++ test/return_buffers.spec.js | 9 ++++- 8 files changed, 106 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index a88332e..1de7668 100644 --- a/README.md +++ b/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. diff --git a/changelog.md b/changelog.md index e615e8d..95ce30e 100644 --- a/changelog.md +++ b/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 diff --git a/index.js b/index.js index cfb123e..83694f2 100644 --- a/index.js +++ b/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( - 'node_redis: 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.' - ); + 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.' + ); + }); } 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( - 'node_redis: Deprecated: The %s command contains a "null" argument.\n' + + this.warn( + '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( - 'node_redis: Deprecated: The %s command contains a "undefined" argument.\n' + + this.warn( + '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( - 'node_redis: Using .end() without the flush parameter is deprecated and throws from v.3.0.0 on.\n' + + this.warn( + '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]]] - if (parsed.hostname) { + // [redis:]//[[user][:password]@][host][:port][/db-number][?db=db-number[&password=bar[&option=value]]] + 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; } diff --git a/lib/utils.js b/lib/utils.js index e8328fc..9fbb22c 100644 --- a/lib/utils.js +++ b/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)) { + if (reply.length === 0 || !Array.isArray(reply)) { // TODO: Check why the isArray check is needed and what value reply has in that case return null; } - - for (j = 0, jl = reply.length; j < jl; j += 2) { - key = reply[j].toString('binary'); - val = reply[j + 1]; - obj[key] = val; + var obj = {}; + for (var j = 0; j < reply.length; j += 2) { + obj[reply[j].toString('binary')] = reply[j + 1]; } - 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]); } diff --git a/test/connection.spec.js b/test/connection.spec.js index d731fee..2f05329 100644 --- a/test/connection.spec.js +++ b/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 () { - return done(); + var end = helper.callFuncAfter(done, 2); + 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) { diff --git a/test/helper.js b/test/helper.js index 2cb6694..72ce1d5 100644 --- a/test/helper.js +++ b/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(); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index e4ef75f..1210799 100644 --- a/test/node_redis.spec.js +++ b/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) { diff --git a/test/return_buffers.spec.js b/test/return_buffers.spec.js index 3258284..029103a 100644 --- a/test/return_buffers.spec.js +++ b/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); }); }); }); From 470ccf632da05fa03f4e53ccef8d226167df1c28 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Jan 2016 22:29:40 +0100 Subject: [PATCH 02/28] Improve benchmark by using a higher precision and fix js parser benchmark for buffers --- benchmarks/multi_bench.js | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/benchmarks/multi_bench.js b/benchmarks/multi_bench.js index 6d5b0e6..1c3c08a 100644 --- a/benchmarks/multi_bench.js +++ b/benchmarks/multi_bench.js @@ -25,10 +25,8 @@ var num_clients = returnArg('clients', 1); var run_time = returnArg('time', 2500); // ms var versions_logged = false; var client_options = { - return_buffers: false, - max_attempts: 4, - parser: returnArg('parser', 'hiredis') - }; + parser: returnArg('parser', 'hiredis') +}; var small_str, large_str, small_buf, large_buf, very_large_str, very_large_buf; function lpad(input, len, chr) { @@ -42,7 +40,7 @@ function lpad(input, len, chr) { metrics.Histogram.prototype.print_line = function () { var obj = this.printObj(); - return lpad(obj.min, 4) + '/' + lpad(obj.max, 4) + '/' + lpad(obj.mean.toFixed(2), 7); + return lpad((obj.min / 1000000).toFixed(2), 6) + '/' + lpad((obj.max / 1000000).toFixed(2), 6) + '/' + lpad((obj.mean / 1000000).toFixed(2), 6); }; function Test(args) { @@ -54,7 +52,8 @@ function Test(args) { this.commands_completed = 0; this.max_pipeline = this.args.pipeline || 50; this.batch_pipeline = this.args.batch || 0; - this.client_options = args.client_options || client_options; + this.client_options = args.client_options || {}; + this.client_options.parser = client_options.parser; this.connect_latency = new metrics.Histogram(); this.ready_latency = new metrics.Histogram(); this.command_latency = new metrics.Histogram(); @@ -131,14 +130,6 @@ Test.prototype.fill_pipeline = function () { return; } - if (this.clients[0].should_buffer) { - var self = this; - setTimeout(function() { - self.fill_pipeline(); - }, 1); - return; - } - if (this.batch_pipeline) { this.batch(); } else { @@ -153,7 +144,7 @@ Test.prototype.fill_pipeline = function () { Test.prototype.batch = function () { var self = this, cur_client = client_nr++ % this.clients.length, - start = Date.now(), + start = process.hrtime(), i = 0, batch = this.clients[cur_client].batch(); @@ -167,7 +158,7 @@ Test.prototype.batch = function () { throw err; } self.commands_completed += res.length; - self.command_latency.update(Date.now() - start); + self.command_latency.update(process.hrtime(start)[1]); self.fill_pipeline(); }); }; @@ -189,14 +180,14 @@ Test.prototype.stop_clients = function () { Test.prototype.send_next = function () { var self = this, cur_client = this.commands_sent % this.clients.length, - start = Date.now(); + start = process.hrtime(); this.clients[cur_client][this.args.command](this.args.args, function (err, res) { if (err) { throw err; } self.commands_completed++; - self.command_latency.update(Date.now() - start); + self.command_latency.update(process.hrtime(start)[1]); self.fill_pipeline(); }); }; From d739ff3a762e566eba7891407e70b703b4ba2523 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Jan 2016 22:31:17 +0100 Subject: [PATCH 03/28] Fix test race condition --- test/connection.spec.js | 45 +++++++++++------------------------------ 1 file changed, 12 insertions(+), 33 deletions(-) diff --git a/test/connection.spec.js b/test/connection.spec.js index 2f05329..5f5892d 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -133,6 +133,7 @@ describe("connection tests", function () { it("emit an error after the socket timeout exceeded the connect_timeout time", function (done) { var connect_timeout = 1000; // in ms + var time = Date.now(); client = redis.createClient({ parser: parser, host: '192.168.74.167', // Should be auto detected as ipv4 @@ -143,7 +144,6 @@ describe("connection tests", function () { }); assert.strictEqual(client.address, '192.168.74.167:6379'); assert.strictEqual(client.connection_options.family, 4); - var time = Date.now(); client.on("reconnecting", function (params) { throw new Error('No reconnect, since no connection was ever established'); @@ -152,7 +152,7 @@ describe("connection tests", function () { client.on('error', function(err) { assert(/Redis connection in broken state: connection timeout.*?exceeded./.test(err.message)); assert(Date.now() - time < connect_timeout + 50); - assert(Date.now() - time >= connect_timeout); + assert(Date.now() - time >= connect_timeout - 50); // Somehow this is triggered to early at times done(); }); }); @@ -192,9 +192,7 @@ describe("connection tests", function () { connect_timeout: 1000 }); - client.once('ready', function() { - done(); - }); + client.once('ready', done); }); if (process.platform !== 'win32') { @@ -207,10 +205,7 @@ describe("connection tests", function () { var end = helper.callFuncAfter(done, 2); - client.once('ready', function() { - end(); - }); - + client.once('ready', end); client.set('foo', 'bar', end); }); } @@ -221,9 +216,7 @@ describe("connection tests", function () { client.once("ready", function () { client.removeListener("error", done); - client.get("recon 1", function (err, res) { - done(err); - }); + client.get("recon 1", done); }); }); @@ -233,9 +226,7 @@ describe("connection tests", function () { client.once("ready", function () { client.removeListener("error", done); - client.get("recon 1", function (err, res) { - done(err); - }); + client.get("recon 1", done); }); }); @@ -246,9 +237,7 @@ describe("connection tests", function () { client.once("ready", function () { client.removeListener("error", done); - client.get("recon 1", function (err, res) { - done(err); - }); + client.get("recon 1", done); }); }); @@ -258,9 +247,7 @@ describe("connection tests", function () { client.once("ready", function () { client.removeListener("error", done); - client.get("recon 1", function (err, res) { - done(err); - }); + client.get("recon 1", done); }); }); @@ -385,9 +372,7 @@ describe("connection tests", function () { connect_timeout: 1000 }); assert.strictEqual(client.options.connect_timeout, 1000); - client.on('ready', function () { - done(); - }); + client.on('ready', done); }); it('allows connecting with the redis url in the options object and works with protocols other than the redis protocol (e.g. http)', function (done) { @@ -398,9 +383,7 @@ describe("connection tests", function () { assert.strictEqual(+client.selected_db, 3); assert(!client.options.port); assert.strictEqual(client.options.host, config.HOST[ip]); - client.on("ready", function () { - return done(); - }); + client.on("ready", done); }); it('allows connecting with the redis url and no auth and options as second parameter', function (done) { @@ -409,18 +392,14 @@ describe("connection tests", function () { }; client = redis.createClient('redis://' + config.HOST[ip] + ':' + config.PORT, options); assert.strictEqual(Object.keys(options).length, 1); - client.on("ready", function () { - return done(); - }); + client.on("ready", done); }); it('allows connecting with the redis url and no auth and options as third parameter', function (done) { client = redis.createClient('redis://' + config.HOST[ip] + ':' + config.PORT, null, { detect_buffers: false }); - client.on("ready", function () { - return done(); - }); + client.on("ready", done); }); } From 60eee34de1b57ca987a396cbff954fc23755634c Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Jan 2016 22:35:40 +0100 Subject: [PATCH 04/28] Unify command handling --- changelog.md | 1 + index.js | 279 ++++++++++++++++++---------------- test/batch.spec.js | 13 +- test/commands/get.spec.js | 7 - test/commands/hgetall.spec.js | 4 +- test/commands/mset.spec.js | 2 +- test/multi.spec.js | 8 +- 7 files changed, 166 insertions(+), 148 deletions(-) diff --git a/changelog.md b/changelog.md index 95ce30e..5c1a01b 100644 --- a/changelog.md +++ b/changelog.md @@ -42,6 +42,7 @@ Deprecations - If you want to buffer commands you should use [.batch or .multi](./README.md) instead. This is necessary to reduce the amount of different options and this is very likely reducing your throughput if set to false. - If you are sure you want to activate the NAGLE algorithm you can still activate it by using client.stream.setNoDelay(false) - Redis < v. 2.6.11 is not supported anymore and will not work in all cases. Please update to a newer redis version +- Removed non documented command syntax (adding the callback to an arguments array instead of passing it as individual argument) ## v.2.4.2 - 27 Nov, 2015 diff --git a/index.js b/index.js index 83694f2..811312d 100644 --- a/index.js +++ b/index.js @@ -651,45 +651,44 @@ RedisClient.prototype.send_command = function (command, args, callback) { command_str = '', buffer_args = false, big_data = false, - prefix_keys; + prefix_keys, + len, args_copy; if (typeof args === 'undefined') { args = []; - } else if (typeof callback === 'undefined') { - if (typeof args[args.length - 1] === 'function') { - callback = args.pop(); - } else if (typeof args[args.length - 1] === 'undefined') { - args.pop(); - } } - if (callback && process.domain) { callback = process.domain.bind(callback); } - for (i = 0; i < args.length; i += 1) { + len = args.length; + args_copy = new Array(len); + + for (i = 0; i < len; i += 1) { if (typeof args[i] === 'string') { // 30000 seemed to be a good value to switch to buffers after testing and checking the pros and cons if (args[i].length > 30000) { big_data = true; - args[i] = new Buffer(args[i], 'utf8'); + args_copy[i] = new Buffer(args[i], 'utf8'); if (this.pipeline !== 0) { this.pipeline += 2; this.writeDefault = this.writeBuffers; } + } else { + args_copy[i] = args[i]; } } else if (typeof args[i] === 'object') { // Checking for object instead of Buffer.isBuffer helps us finding data types that we can't handle properly if (args[i] instanceof Date) { // Accept dates as valid input - args[i] = args[i].toString(); - // Add this to parse_arguments. + args_copy[i] = args[i].toString(); } else if (args[i] === null) { this.warn( '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.' ); - args[i] = 'null'; // Backwards compatible :/ + args_copy[i] = 'null'; // Backwards compatible :/ } else { + args_copy[i] = args[i]; buffer_args = true; if (this.pipeline !== 0) { this.pipeline += 2; @@ -702,13 +701,13 @@ RedisClient.prototype.send_command = function (command, args, callback) { '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.' ); - args[i] = 'undefined'; // Backwards compatible :/ + args_copy[i] = 'undefined'; // Backwards compatible :/ } else { - args[i] = String(args[i]); + args_copy[i] = String(args[i]); } } - command_obj = new Command(command, args, buffer_args, callback); + command_obj = new Command(command, args_copy, buffer_args, callback); // TODO: Replace send_anyway with `commands.hasFlag(command, 'loading') === false` as soon as pub_sub is handled in the result handler if (this.ready === false && this.send_anyway === false || !stream.writable) { @@ -746,20 +745,20 @@ RedisClient.prototype.send_command = function (command, args, callback) { command = this.options.rename_commands[command]; } if (this.options.prefix) { - prefix_keys = commands.getKeyIndexes(command, args); + prefix_keys = commands.getKeyIndexes(command, args_copy); i = prefix_keys.pop(); while (i !== undefined) { - args[i] = this.options.prefix + args[i]; + args_copy[i] = this.options.prefix + args_copy[i]; i = prefix_keys.pop(); } } // Always use 'Multi bulk commands', but if passed any Buffer args, then do multiple writes, one for each arg. // This means that using Buffers in commands is going to be slower, so use Strings if you don't already have a Buffer. - command_str = '*' + (args.length + 1) + '\r\n$' + command.length + '\r\n' + command + '\r\n'; + command_str = '*' + (len + 1) + '\r\n$' + command.length + '\r\n' + command + '\r\n'; if (buffer_args === false && big_data === false) { // Build up a string and send entire command in one write - for (i = 0; i < args.length; i += 1) { - arg = args[i]; + for (i = 0; i < len; i += 1) { + arg = args_copy[i]; command_str += '$' + Buffer.byteLength(arg) + '\r\n' + arg + '\r\n'; } debug('Send ' + this.address + ' id ' + this.connection_id + ': ' + command_str); @@ -768,8 +767,8 @@ RedisClient.prototype.send_command = function (command, args, callback) { debug('Send command (' + command_str + ') has Buffer arguments'); this.write(command_str); - for (i = 0; i < args.length; i += 1) { - arg = args[i]; + for (i = 0; i < len; i += 1) { + arg = args_copy[i]; if (typeof arg !== 'object') { // string; number; boolean this.write('$' + Buffer.byteLength(arg) + '\r\n' + arg + '\r\n'); } else { // buffer @@ -892,40 +891,64 @@ function Multi(client, args) { commands.list.forEach(function (command) { - RedisClient.prototype[command.toUpperCase()] = RedisClient.prototype[command] = function (key, arg, callback) { - if (Array.isArray(key)) { - return this.send_command(command, key, arg); - } else if (Array.isArray(arg)) { - arg = [key].concat(arg); - return this.send_command(command, arg, callback); - } - // This has to be inlined, otherwise the arguments leak - var len = arguments.length; - var arr = new Array(len); - for (var i = 0; i < len; i += 1) { - arr[i] = arguments[i]; + RedisClient.prototype[command.toUpperCase()] = RedisClient.prototype[command] = function () { + var arr, + len = 0, + callback, + i = 0; + if (arguments[0] instanceof Array) { + arr = arguments[0]; + callback = arguments[1]; // It does not matter if it exists or not + } else if (arguments[1] instanceof Array) { + len = arguments[1].length; + arr = new Array(len + 1); + arr[0] = arguments[0]; + for (; i < len; i += 1) { + arr[i + 1] = arguments[1][i]; + } + callback = arguments[2]; + } else { + len = arguments.length; + arr = new Array(len); + for (; i < len; i += 1) { + arr[i] = arguments[i]; + } + // The later should not be the average use case + if (typeof arr[i - 1] === 'function' || typeof arr[i - 1] === 'undefined') { + callback = arr.pop(); + } } - return this.send_command(command, arr); + return this.send_command(command, arr, callback); }; - Multi.prototype[command.toUpperCase()] = Multi.prototype[command] = function (key, arg, callback) { - if (Array.isArray(key)) { - if (arg) { - key = key.concat([arg]); - } - this.queue.push([command].concat(key)); - } else if (Array.isArray(arg)) { - if (callback) { - arg = arg.concat([callback]); + + Multi.prototype[command.toUpperCase()] = Multi.prototype[command] = function () { + var arr, + len = 0, + callback, + i = 0; + if (arguments[0] instanceof Array) { + arr = arguments[0]; + callback = arguments[1]; + } else if (arguments[1] instanceof Array) { + len = arguments[1].length; + arr = new Array(len + 1); + arr[0] = arguments[0]; + for (; i < len; i += 1) { + arr[i + 1] = arguments[1][i]; } - this.queue.push([command, key].concat(arg)); + callback = arguments[2]; } else { - var len = arguments.length; - var arr = new Array(len); - for (var i = 0; i < len; i += 1) { + len = arguments.length; + arr = new Array(len); + for (; i < len; i += 1) { arr[i] = arguments[i]; } - this.queue.push([command].concat(arr)); + // The later should not be the average use case + if (typeof arr[i - 1] === 'function' || typeof arr[i - 1] === 'undefined') { + callback = arr.pop(); + } } + this.queue.push([command, arr, callback]); return this; }; }); @@ -1028,63 +1051,76 @@ RedisClient.prototype.auth = RedisClient.prototype.AUTH = function (pass, callba return tmp; }; -RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function (key, args, callback) { - var field, tmp_args; - if (Array.isArray(key)) { - return this.send_command('hmset', key, args); - } - if (Array.isArray(args)) { - return this.send_command('hmset', [key].concat(args), callback); - } - if (typeof args === 'object' && (typeof callback === 'function' || typeof callback === 'undefined')) { - // User does: client.hmset(key, {key1: val1, key2: val2}) - // assuming key is a string, i.e. email address - tmp_args = [key]; - var fields = Object.keys(args); - while (field = fields.shift()) { - tmp_args.push(field, args[field]); +RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function () { + var arr, + len = 0, + callback, + i = 0; + if (arguments[0] instanceof Array) { + arr = arguments[0]; + callback = arguments[1]; + } else if (arguments[1] instanceof Array) { + len = arguments[1].length; + arr = new Array(len + 1); + arr[0] = arguments[0]; + for (; i < len; i += 1) { + arr[i + 1] = arguments[1][i]; + } + callback = arguments[2]; + } else if (typeof arguments[1] === 'object' && (typeof arguments[2] === 'function' || typeof arguments[2] === 'undefined')) { + arr = [arguments[0]]; + for (var field in arguments[1]) { // jshint ignore: line + arr.push(field, arguments[1][field]); + } + callback = arguments[2]; + } else { + len = arguments.length; + arr = new Array(len); + for (; i < len; i += 1) { + arr[i] = arguments[i]; + } + // The later should not be the average use case + if (typeof arr[i - 1] === 'function' || typeof arr[i - 1] === 'undefined') { + callback = arr.pop(); } - return this.send_command('hmset', tmp_args, callback); - } - var len = arguments.length; - tmp_args = new Array(len); - for (var i = 0; i < len; i += 1) { - tmp_args[i] = arguments[i]; } - return this.send_command('hmset', tmp_args); + return this.send_command('hmset', arr, callback); }; -Multi.prototype.hmset = Multi.prototype.HMSET = function (key, args, callback) { - var tmp_args, field; - if (Array.isArray(key)) { - if (args) { - key = key.concat([args]); - } - tmp_args = ['hmset'].concat(key); - } else if (Array.isArray(args)) { - if (callback) { - args = args.concat([callback]); +Multi.prototype.hmset = Multi.prototype.HMSET = function () { + var arr, + len = 0, + callback, + i = 0; + if (arguments[0] instanceof Array) { + arr = arguments[0]; + callback = arguments[1]; + } else if (arguments[1] instanceof Array) { + len = arguments[1].length; + arr = new Array(len + 1); + arr[0] = arguments[0]; + for (; i < len; i += 1) { + arr[i + 1] = arguments[1][i]; } - tmp_args = ['hmset', key].concat(args); - } else if (typeof args === 'object' && (typeof callback === 'function' || typeof callback === 'undefined')) { - tmp_args = ['hmset', key]; - var fields = Object.keys(args); - while (field = fields.shift()) { - tmp_args.push(field); - tmp_args.push(args[field]); - } - if (callback) { - tmp_args.push(callback); + callback = arguments[2]; + } else if (typeof arguments[1] === 'object' && (typeof arguments[2] === 'function' || typeof arguments[2] === 'undefined')) { + arr = [arguments[0]]; + for (var field in arguments[1]) { // jshint ignore: line + arr.push(field, arguments[1][field]); } + callback = arguments[2]; } else { - var len = arguments.length; - tmp_args = new Array(len); - tmp_args[0] = 'hmset'; - for (var i = 0; i < len; i += 1) { - tmp_args[i + 1] = arguments[i]; + len = arguments.length; + arr = new Array(len); + for (; i < len; i += 1) { + arr[i] = arguments[i]; + } + // The later should not be the average use case + if (typeof arr[i - 1] === 'function' || typeof arr[i - 1] === 'undefined') { + callback = arr.pop(); } } - this.queue.push(tmp_args); + this.queue.push(['hmset', arr, callback]); return this; }; @@ -1111,8 +1147,6 @@ Multi.prototype.exec_atomic = function (callback) { Multi.prototype.exec_transaction = function (callback) { var self = this; var len = this.queue.length; - var cb; - var args_len = 1; this.errors = []; this.callback = callback; this._client.cork(len + 2); @@ -1120,26 +1154,20 @@ Multi.prototype.exec_transaction = function (callback) { this.send_command('multi', []); // Drain queue, callback will catch 'QUEUED' or error for (var index = 0; index < len; index++) { - var args = this.queue.get(index).slice(0); - var command = args.shift(); - args_len = args.length - 1; - if (typeof args[args_len] === 'function' || typeof args[args_len] === 'undefined') { - cb = args.pop(); - } else { - // Explicitly set the callback to undefined. Otherwise we might have the callback from the command earlier - cb = undefined; - } + var args = this.queue.get(index); + var command = args[0]; + var cb = args[2]; // Keep track of who wants buffer responses: if (this._client.options.detect_buffers) { this.wants_buffers[index] = false; - for (var i = 0; i < args.length; i += 1) { - if (Buffer.isBuffer(args[i])) { + for (var i = 0; i < args[1].length; i += 1) { + if (Buffer.isBuffer(args[1][i])) { this.wants_buffers[index] = true; break; } } } - this.send_command(command, args, index, cb); + this.send_command(command, args[1], index, cb); } this._client.send_command('exec', [], function(err, replies) { @@ -1168,7 +1196,6 @@ Multi.prototype.execute_callback = function (err, replies) { if (replies) { while (args = this.queue.shift()) { - // If we asked for strings, even in detect_buffers mode, then return strings: if (replies[i] instanceof Error) { var match = replies[i].message.match(utils.err_code); // LUA script could return user errors that don't behave like all other errors! @@ -1176,14 +1203,13 @@ Multi.prototype.execute_callback = function (err, replies) { replies[i].code = match[1]; } replies[i].command = args[0].toUpperCase(); - } else if (replies[i]) { - replies[i] = this._client.handle_reply(replies[i], args[0], this.wants_buffers[i]); - } - - if (typeof args[args.length - 1] === 'function') { - if (replies[i] instanceof Error) { + if (typeof args[args.length - 1] === 'function') { args[args.length - 1](replies[i]); - } else { + } + } else { + // If we asked for strings, even in detect_buffers mode, then return strings: + replies[i] = this._client.handle_reply(replies[i], args[0], this.wants_buffers[i]); + if (typeof args[args.length - 1] === 'function') { args[args.length - 1](null, replies[i]); } } @@ -1246,21 +1272,18 @@ Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = funct this.results = []; this._client.cork(len); while (args = this.queue.shift()) { - var command = args.shift(); + var command = args[0]; var cb; - args_len = args.length - 1; - if (typeof args[args_len] === 'function') { - cb = this.callback(args.pop(), index); + args_len = args[1].length - 1; + if (typeof args[2] === 'function') { + cb = this.callback(args[2], index); } else { cb = callback_without_own_cb; - if (typeof args[args_len] === 'undefined') { - args.pop(); - } } if (callback && index === len - 1) { cb = last_callback(cb); } - this._client.send_command(command, args, cb); + this._client.send_command(command, args[1], cb); index++; } this._client.uncork(); diff --git a/test/batch.spec.js b/test/batch.spec.js index 1c8b51b..a99cb82 100644 --- a/test/batch.spec.js +++ b/test/batch.spec.js @@ -199,8 +199,8 @@ describe("The 'batch' method", function () { arr4, [["mset", "batchfoo2", "batchbar2", "batchfoo3", "batchbar3"], helper.isString('OK')], ["hmset", arr], - [["hmset", "batchhmset2", "batchbar2", "batchfoo3", "batchbar3", "test", helper.isString('OK')]], - ["hmset", ["batchhmset", "batchbar", "batchfoo", helper.isString('OK')]], + [["hmset", "batchhmset2", "batchbar2", "batchfoo3", "batchbar3", "test"], helper.isString('OK')], + ["hmset", ["batchhmset", "batchbar", "batchfoo"], helper.isString('OK')], ["hmset", arr3, helper.isString('OK')], ['hmset', now, {123456789: "abcdefghij", "some manner of key": "a type of value", "otherTypes": 555}], ['hmset', 'key2', {"0123456789": "abcdefghij", "some manner of key": "a type of value", "otherTypes": 999}, helper.isString('OK')], @@ -211,8 +211,9 @@ describe("The 'batch' method", function () { .hmget('key2', arr2, function noop() {}) .hmget(['batchhmset2', 'some manner of key', 'batchbar3']) .mget('batchfoo2', ['batchfoo3', 'batchfoo'], function(err, res) { - assert(res[0], 'batchfoo3'); - assert(res[1], 'batchfoo'); + assert.strictEqual(res[0], 'batchbar2'); + assert.strictEqual(res[1], 'batchbar3'); + assert.strictEqual(res[2], null); }) .exec(function (err, replies) { assert.equal(arr.length, 3); @@ -273,7 +274,7 @@ describe("The 'batch' method", function () { it('allows multiple commands to work the same as normal to be performed using a chaining API', function (done) { client.batch() .mset(['some', '10', 'keys', '20']) - .incr(['some', helper.isNumber(11)]) + .incr('some', helper.isNumber(11)) .incr(['keys'], helper.isNumber(21)) .mget('some', 'keys') .exec(function (err, replies) { @@ -290,7 +291,7 @@ describe("The 'batch' method", function () { it('allows multiple commands to work the same as normal to be performed using a chaining API promisified', function () { return client.batch() .mset(['some', '10', 'keys', '20']) - .incr(['some', helper.isNumber(11)]) + .incr('some', helper.isNumber(11)) .incr(['keys'], helper.isNumber(21)) .mget('some', 'keys') .execAsync() diff --git a/test/commands/get.spec.js b/test/commands/get.spec.js index f51201b..522872e 100644 --- a/test/commands/get.spec.js +++ b/test/commands/get.spec.js @@ -75,13 +75,6 @@ describe("The 'get' method", function () { }); }); - it("gets the value correctly with array syntax and the callback being in the array", function (done) { - client.GET([key, function (err, res) { - helper.isString(value)(err, res); - done(err); - }]); - }); - it("should not throw on a get without callback (even if it's not useful)", function (done) { client.GET(key); client.on('error', function(err) { diff --git a/test/commands/hgetall.spec.js b/test/commands/hgetall.spec.js index 4e21932..d531703 100644 --- a/test/commands/hgetall.spec.js +++ b/test/commands/hgetall.spec.js @@ -64,7 +64,7 @@ describe("The 'hgetall' method", function () { it('returns binary results', function (done) { client.hmset(["bhosts", "mjr", "1", "another", "23", "home", "1234", new Buffer([0xAA, 0xBB, 0x00, 0xF0]), new Buffer([0xCC, 0xDD, 0x00, 0xF0])], helper.isString("OK")); - client.HGETALL(["bhosts", function (err, obj) { + client.HGETALL("bhosts", function (err, obj) { assert.strictEqual(4, Object.keys(obj).length); assert.strictEqual("1", obj.mjr.toString()); assert.strictEqual("23", obj.another.toString()); @@ -72,7 +72,7 @@ describe("The 'hgetall' method", function () { assert.strictEqual((new Buffer([0xAA, 0xBB, 0x00, 0xF0])).toString('binary'), Object.keys(obj)[3]); assert.strictEqual((new Buffer([0xCC, 0xDD, 0x00, 0xF0])).toString('binary'), obj[(new Buffer([0xAA, 0xBB, 0x00, 0xF0])).toString('binary')].toString('binary')); return done(err); - }]); + }); }); }); diff --git a/test/commands/mset.spec.js b/test/commands/mset.spec.js index c61c3d7..c5d754f 100644 --- a/test/commands/mset.spec.js +++ b/test/commands/mset.spec.js @@ -89,7 +89,7 @@ describe("The 'mset' method", function () { it("sets the value correctly with array syntax", function (done) { client.mset([key, value2, key2, value]); - client.get([key, helper.isString(value2)]); + client.get(key, helper.isString(value2)); client.get(key2, helper.isString(value, done)); }); }); diff --git a/test/multi.spec.js b/test/multi.spec.js index 86dfe93..4d17c92 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -330,8 +330,8 @@ describe("The 'multi' method", function () { arr4, [["mset", "multifoo2", "multibar2", "multifoo3", "multibar3"], helper.isString('OK')], ["hmset", arr], - [["hmset", "multihmset2", "multibar2", "multifoo3", "multibar3", "test", helper.isString('OK')]], - ["hmset", ["multihmset", "multibar", "multifoo", helper.isString('OK')]], + [["hmset", "multihmset2", "multibar2", "multifoo3", "multibar3", "test"], helper.isString('OK')], + ["hmset", ["multihmset", "multibar", "multifoo"], helper.isString('OK')], ["hmset", arr3, helper.isString('OK')], ['hmset', now, {123456789: "abcdefghij", "some manner of key": "a type of value", "otherTypes": 555}], ['hmset', 'key2', {"0123456789": "abcdefghij", "some manner of key": "a type of value", "otherTypes": 999}, helper.isString('OK')], @@ -399,7 +399,7 @@ describe("The 'multi' method", function () { it('allows multiple commands to work the same as normal to be performed using a chaining API', function (done) { client.multi() .mset(['some', '10', 'keys', '20']) - .incr(['some', helper.isNumber(11)]) + .incr('some', helper.isNumber(11)) .incr(['keys'], helper.isNumber(21)) .mget('some', 'keys') .exec(function (err, replies) { @@ -416,7 +416,7 @@ describe("The 'multi' method", function () { it('allows multiple commands to work the same as normal to be performed using a chaining API promisified', function () { return client.multi() .mset(['some', '10', 'keys', '20']) - .incr(['some', helper.isNumber(11)]) + .incr('some', helper.isNumber(11)) .incr(['keys'], helper.isNumber(21)) .mget('some', 'keys') .execAsync() From 518e46dcc736fad7538b8ec59c47f31d41d12eb6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Jan 2016 22:36:07 +0100 Subject: [PATCH 05/28] Use a own clone function instead of using JSON.parse(JSON.stringify()) This will also clone functions --- index.js | 13 ++++++------- lib/utils.js | 29 ++++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/index.js b/index.js index 811312d..f0c220b 100644 --- a/index.js +++ b/index.js @@ -15,7 +15,6 @@ var default_port = 6379; var default_host = '127.0.0.1'; function noop () {} -function clone (obj) { return JSON.parse(JSON.stringify(obj || {})); } function debug (msg) { if (exports.debug_mode) { console.error(msg); } } function handle_detect_buffers_reply (reply, command, buffer_args) { @@ -35,7 +34,7 @@ exports.debug_mode = /\bredis\b/i.test(process.env.NODE_DEBUG); function RedisClient (options) { // Copy the options so they are not mutated - options = clone(options); + options = utils.clone(options); events.EventEmitter.call(this); var cnx_options = {}; var self = this; @@ -205,8 +204,8 @@ RedisClient.prototype.cork = noop; RedisClient.prototype.uncork = noop; RedisClient.prototype.duplicate = function (options) { - var existing_options = clone(this.options); - options = clone(options); + var existing_options = utils.clone(this.options); + options = utils.clone(options); for (var elem in options) { // jshint ignore: line existing_options[elem] = options[elem]; } @@ -1293,11 +1292,11 @@ Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = funct var createClient = function (port_arg, host_arg, options) { if (typeof port_arg === 'number' || typeof port_arg === 'string' && /^\d+$/.test(port_arg)) { - options = clone(options); + options = utils.clone(options); options.host = host_arg; options.port = port_arg; } else if (typeof port_arg === 'string' || port_arg && port_arg.url) { - options = clone(port_arg.url ? port_arg : host_arg || options); + options = utils.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]]] if (parsed.hostname || parsed.slashes) { // The host might be an empty string @@ -1326,7 +1325,7 @@ var createClient = function (port_arg, host_arg, options) { options.path = port_arg; } } else if (typeof port_arg === 'object' || port_arg === undefined) { - options = clone(port_arg || options); + options = utils.clone(port_arg || options); options.host = options.host || host_arg; } if (!options) { diff --git a/lib/utils.js b/lib/utils.js index 9fbb22c..8cdfb1f 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -38,9 +38,36 @@ function print (err, reply) { var redisErrCode = /^([A-Z]+)\s+(.+)$/; +// Deep clone arbitrary objects with arrays. Can't handle cyclic structures (results in a range error) +function clone (obj) { + if (obj) { + var copy; + if (obj.constructor === Array) { + copy = new Array(obj.length); + for (var i = 0; i < obj.length; i++) { + copy[i] = clone(obj[i]); + } + return copy; + } + if (obj.constructor === Object) { + copy = {}; + for (var elem in obj) { + if (!obj.hasOwnProperty(elem)) { + // Do not add non own properties to the cloned object + continue; + } + copy[elem] = clone(obj[elem]); + } + return copy; + } + } + return obj; +} + module.exports = { reply_to_strings: replyToStrings, reply_to_object: replyToObject, print: print, - err_code: redisErrCode + err_code: redisErrCode, + clone: clone }; From fb0eaf4d41462c724b978efb4813d364fb8c640e Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Jan 2016 22:37:34 +0100 Subject: [PATCH 06/28] Unify auth handling --- index.js | 58 +++++++++++++++++++---------------------------- test/auth.spec.js | 4 ++-- 2 files changed, 25 insertions(+), 37 deletions(-) diff --git a/index.js b/index.js index f0c220b..481fd1f 100644 --- a/index.js +++ b/index.js @@ -188,8 +188,8 @@ RedisClient.prototype.create_stream = function () { } // Fire the command before redis is connected to be sure it's the first fired command - if (typeof this.auth_pass === 'string') { - this.do_auth(); + if (this.auth_pass !== undefined) { + this.auth(this.auth_pass); } }; @@ -274,29 +274,6 @@ RedisClient.prototype.on_error = function (err) { this.connection_gone('error'); }; -var noPasswordIsSet = /no password is set/; - -RedisClient.prototype.do_auth = function () { - var self = this; - debug('Sending auth to ' + self.address + ' id ' + self.connection_id); - - this.send_anyway = true; - this.send_command('auth', [this.auth_pass], function (err, res) { - if (err) { - if (noPasswordIsSet.test(err.message)) { - debug('Warning: Redis server does not require a password, but a password was supplied.'); - err = null; - res = 'OK'; - } else { - self.emit('error', err); - } - } else { - debug('Auth succeeded ' + self.address + ' id ' + self.connection_id); - } - }); - this.send_anyway = false; -}; - RedisClient.prototype.on_connect = function () { debug('Stream connected ' + this.address + ' id ' + this.connection_id); @@ -1034,18 +1011,29 @@ RedisClient.prototype.callback_emit_error = function (callback, err) { } }; -// Stash auth for connect and reconnect. Send immediately if already connected. -RedisClient.prototype.auth = RedisClient.prototype.AUTH = function (pass, callback) { - if (typeof pass !== 'string') { - var err = new Error('The password has to be of type "string"'); - err.command = 'AUTH'; - this.callback_emit_error(callback, err); - return true; - } +var noPasswordIsSet = /no password is set/; + +RedisClient.prototype.auth = function (pass, callback) { + var self = this; + debug('Sending auth to ' + self.address + ' id ' + self.connection_id); + + // Stash auth for connect and reconnect. this.auth_pass = pass; - debug('Saving auth as ' + this.auth_pass); this.send_anyway = true; - var tmp = this.send_command('auth', [pass], callback); + var tmp = this.send_command('auth', [pass], function (err, res) { + if (err) { + if (noPasswordIsSet.test(err.message)) { + self.warn('Warning: Redis server does not require a password, but a password was supplied.'); + err = null; + res = 'OK'; + } else if (!callback) { + self.emit('error', err); + } + } + if (callback) { + callback(err, res); + } + }); this.send_anyway = false; return tmp; }; diff --git a/test/auth.spec.js b/test/auth.spec.js index 9e648d6..5f18f35 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -161,7 +161,7 @@ describe("client authentication", function () { client = redis.createClient.apply(redis.createClient, args); var async = true; client.auth(undefined, function(err, res) { - assert.strictEqual(err.message, 'The password has to be of type "string"'); + assert.strictEqual(err.message, 'ERR invalid password'); assert.strictEqual(err.command, 'AUTH'); assert.strictEqual(res, undefined); async = false; @@ -175,7 +175,7 @@ describe("client authentication", function () { client = redis.createClient.apply(redis.createClient, args); client.on('error', function (err) { - assert.strictEqual(err.message, 'The password has to be of type "string"'); + assert.strictEqual(err.message, 'ERR invalid password'); assert.strictEqual(err.command, 'AUTH'); done(); }); From 32172cd29120811b4e3c899a3df00ff40bea5297 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Jan 2016 22:38:05 +0100 Subject: [PATCH 07/28] Use instanceof Array instead of Array.isArray The reply is being done with a regular array and therefor will be the same array instance --- index.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 481fd1f..7b3db91 100644 --- a/index.js +++ b/index.js @@ -535,8 +535,6 @@ RedisClient.prototype.drain = function () { RedisClient.prototype.emit_idle = function (queue_len) { if (this.pub_sub_mode === false && queue_len === 0) { - // Free the queue capacity memory by using a new queue - this.command_queue = new Queue(); this.emit('idle'); } }; @@ -547,7 +545,7 @@ RedisClient.prototype.return_reply = function (reply) { // If the 'reply' here is actually a message received asynchronously due to a // pubsub subscription, don't pop the command queue as we'll only be consuming // the head command prematurely. - if (this.pub_sub_mode && Array.isArray(reply) && reply[0]) { + if (this.pub_sub_mode && reply instanceof Array && reply[0]) { type = reply[0].toString(); } @@ -571,12 +569,13 @@ RedisClient.prototype.return_reply = function (reply) { debug('No callback for reply'); } } else if (this.pub_sub_mode || command_obj && command_obj.sub_command) { - if (Array.isArray(reply)) { + if (reply instanceof Array) { if ((!command_obj || command_obj.buffer_args === false) && !this.options.return_buffers) { reply = utils.reply_to_strings(reply); } type = reply[0].toString(); + // TODO: Add buffer emiters (we have to get all pubsub messages as buffers back in that case) if (type === 'message') { this.emit('message', reply[1], reply[2]); // channel, message } else if (type === 'pmessage') { From 92be1e93c53f0a27978dfa708b1f3c0b9e87b2a6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 31 Jan 2016 14:57:07 +0100 Subject: [PATCH 08/28] Remove buffer bench This benchmark should not be relied on. v8 is going to inline code and the benchmark expactations won't be met anymore --- benchmarks/buffer_bench.js | 91 -------------------------------------- 1 file changed, 91 deletions(-) delete mode 100644 benchmarks/buffer_bench.js diff --git a/benchmarks/buffer_bench.js b/benchmarks/buffer_bench.js deleted file mode 100644 index 63dae1e..0000000 --- a/benchmarks/buffer_bench.js +++ /dev/null @@ -1,91 +0,0 @@ -'use strict'; - -var source = new Buffer(100), - dest = new Buffer(100), i, j, k, tmp, count = 1000000, bytes = 100; - -for (i = 99 ; i >= 0 ; i--) { - source[i] = 120; -} - -var str = 'This is a nice String.', - buf = new Buffer('This is a lovely Buffer.'); - -var start = new Date(); -for (i = count * 100; i > 0 ; i--) { - if (Buffer.isBuffer(str)) {} -} -var end = new Date(); -console.log('Buffer.isBuffer(str) ' + (end - start) + ' ms'); - -var start = new Date(); -for (i = count * 100; i > 0 ; i--) { - if (Buffer.isBuffer(buf)) {} -} -var end = new Date(); -console.log('Buffer.isBuffer(buf) ' + (end - start) + ' ms'); - -var start = new Date(); -for (i = count * 100; i > 0 ; i--) { - if (str instanceof Buffer) {} -} -var end = new Date(); -console.log('str instanceof Buffer ' + (end - start) + ' ms'); - -var start = new Date(); -for (i = count * 100; i > 0 ; i--) { - if (buf instanceof Buffer) {} -} -var end = new Date(); -console.log('buf instanceof Buffer ' + (end - start) + ' ms'); - -for (i = bytes ; i > 0 ; i --) { - var start = new Date(); - for (j = count ; j > 0; j--) { - tmp = source.toString('ascii', 0, bytes); - } - var end = new Date(); - console.log('toString() ' + i + ' bytes ' + (end - start) + ' ms'); -} - -for (i = bytes ; i > 0 ; i --) { - var start = new Date(); - for (j = count ; j > 0; j--) { - tmp = ''; - for (k = 0; k <= i ; k++) { - tmp += String.fromCharCode(source[k]); - } - } - var end = new Date(); - console.log('manual string ' + i + ' bytes ' + (end - start) + ' ms'); -} - -for (i = bytes ; i > 0 ; i--) { - var start = new Date(); - for (j = count ; j > 0 ; j--) { - for (k = i ; k > 0 ; k--) { - dest[k] = source[k]; - } - } - var end = new Date(); - console.log('Manual copy ' + i + ' bytes ' + (end - start) + ' ms'); -} - -for (i = bytes ; i > 0 ; i--) { - var start = new Date(); - for (j = count ; j > 0 ; j--) { - for (k = i ; k > 0 ; k--) { - dest[k] = 120; - } - } - var end = new Date(); - console.log('Direct assignment ' + i + ' bytes ' + (end - start) + ' ms'); -} - -for (i = bytes ; i > 0 ; i--) { - var start = new Date(); - for (j = count ; j > 0 ; j--) { - source.copy(dest, 0, 0, i); - } - var end = new Date(); - console.log('Buffer.copy() ' + i + ' bytes ' + (end - start) + ' ms'); -} From f2ca678afed5da4ae54f1dbd36ddb7c2b1a34511 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 31 Jan 2016 14:59:45 +0100 Subject: [PATCH 09/28] Update benchmark to also signal tcp / socket connections --- benchmarks/multi_bench.js | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/benchmarks/multi_bench.js b/benchmarks/multi_bench.js index 1c3c08a..f8b9299 100644 --- a/benchmarks/multi_bench.js +++ b/benchmarks/multi_bench.js @@ -25,7 +25,8 @@ var num_clients = returnArg('clients', 1); var run_time = returnArg('time', 2500); // ms var versions_logged = false; var client_options = { - parser: returnArg('parser', 'hiredis') + parser: returnArg('parser', 'hiredis'), + path: returnArg('socket') // '/tmp/redis.sock' }; var small_str, large_str, small_buf, large_buf, very_large_str, very_large_buf; @@ -40,7 +41,7 @@ function lpad(input, len, chr) { metrics.Histogram.prototype.print_line = function () { var obj = this.printObj(); - return lpad((obj.min / 1000000).toFixed(2), 6) + '/' + lpad((obj.max / 1000000).toFixed(2), 6) + '/' + lpad((obj.mean / 1000000).toFixed(2), 6); + return lpad((obj.min / 1e6).toFixed(2), 6) + '/' + lpad((obj.max / 1e6).toFixed(2), 6) + '/' + lpad((obj.mean / 1e6).toFixed(2), 6); }; function Test(args) { @@ -54,6 +55,10 @@ function Test(args) { this.batch_pipeline = this.args.batch || 0; this.client_options = args.client_options || {}; this.client_options.parser = client_options.parser; + this.client_options.connect_timeout = 1000; + if (client_options.path) { + this.client_options.path = client_options.path; + } this.connect_latency = new metrics.Histogram(); this.ready_latency = new metrics.Histogram(); this.command_latency = new metrics.Histogram(); @@ -78,9 +83,14 @@ Test.prototype.new_client = function (id) { }); new_client.on('ready', function () { - if (! versions_logged) { - console.log('Client count: ' + num_clients + ', node version: ' + process.versions.node + ', server version: ' + - new_client.server_info.redis_version + ', parser: ' + new_client.reply_parser.name); + if (!versions_logged) { + console.log( + 'clients: ' + num_clients + + ', NodeJS: ' + process.versions.node + + ', Redis: ' + new_client.server_info.redis_version + + ', parser: ' + client_options.parser + + ', connected by: ' + (client_options.path ? 'socket' : 'tcp') + ); versions_logged = true; } self.ready_latency.update(Date.now() - new_client.create_time); @@ -197,7 +207,7 @@ Test.prototype.print_stats = function () { totalTime += duration; console.log('min/max/avg: ' + this.command_latency.print_line() + ' ' + lpad(duration, 6) + 'ms total, ' + - lpad((this.commands_completed / (duration / 1000)).toFixed(2), 9) + ' ops/sec'); + lpad(Math.round(this.commands_completed / (duration / 1000)), 7) + ' ops/sec'); }; small_str = '1234'; @@ -208,71 +218,54 @@ very_large_str = (new Array((4 * 1024 * 1024) + 1).join('-')); very_large_buf = new Buffer(very_large_str); tests.push(new Test({descr: 'PING', command: 'ping', args: [], pipeline: 1})); -// tests.push(new Test({descr: 'PING', command: 'ping', args: [], pipeline: 50})); tests.push(new Test({descr: 'PING', command: 'ping', args: [], batch: 50})); tests.push(new Test({descr: 'SET 4B str', command: 'set', args: ['foo_rand000000000000', small_str], pipeline: 1})); -// tests.push(new Test({descr: 'SET 4B str', command: 'set', args: ['foo_rand000000000000', small_str], pipeline: 50})); tests.push(new Test({descr: 'SET 4B str', command: 'set', args: ['foo_rand000000000000', small_str], batch: 50})); tests.push(new Test({descr: 'SET 4B buf', command: 'set', args: ['foo_rand000000000000', small_buf], pipeline: 1})); -// tests.push(new Test({descr: 'SET 4B buf', command: 'set', args: ['foo_rand000000000000', small_buf], pipeline: 50})); tests.push(new Test({descr: 'SET 4B buf', command: 'set', args: ['foo_rand000000000000', small_buf], batch: 50})); tests.push(new Test({descr: 'GET 4B str', command: 'get', args: ['foo_rand000000000000'], pipeline: 1})); -// tests.push(new Test({descr: 'GET 4B str', command: 'get', args: ['foo_rand000000000000'], pipeline: 50})); tests.push(new Test({descr: 'GET 4B str', command: 'get', args: ['foo_rand000000000000'], batch: 50})); tests.push(new Test({descr: 'GET 4B buf', command: 'get', args: ['foo_rand000000000000'], pipeline: 1, client_opts: { return_buffers: true} })); -// tests.push(new Test({descr: 'GET 4B buf', command: 'get', args: ['foo_rand000000000000'], pipeline: 50, client_opts: { return_buffers: true} })); tests.push(new Test({descr: 'GET 4B buf', command: 'get', args: ['foo_rand000000000000'], batch: 50, client_opts: { return_buffers: true} })); tests.push(new Test({descr: 'SET 4KiB str', command: 'set', args: ['foo_rand000000000001', large_str], pipeline: 1})); -// tests.push(new Test({descr: 'SET 4KiB str', command: 'set', args: ['foo_rand000000000001', large_str], pipeline: 50})); tests.push(new Test({descr: 'SET 4KiB str', command: 'set', args: ['foo_rand000000000001', large_str], batch: 50})); tests.push(new Test({descr: 'SET 4KiB buf', command: 'set', args: ['foo_rand000000000001', large_buf], pipeline: 1})); -// tests.push(new Test({descr: 'SET 4KiB buf', command: 'set', args: ['foo_rand000000000001', large_buf], pipeline: 50})); tests.push(new Test({descr: 'SET 4KiB buf', command: 'set', args: ['foo_rand000000000001', large_buf], batch: 50})); tests.push(new Test({descr: 'GET 4KiB str', command: 'get', args: ['foo_rand000000000001'], pipeline: 1})); -// tests.push(new Test({descr: 'GET 4KiB str', command: 'get', args: ['foo_rand000000000001'], pipeline: 50})); tests.push(new Test({descr: 'GET 4KiB str', command: 'get', args: ['foo_rand000000000001'], batch: 50})); tests.push(new Test({descr: 'GET 4KiB buf', command: 'get', args: ['foo_rand000000000001'], pipeline: 1, client_opts: { return_buffers: true} })); -// tests.push(new Test({descr: 'GET 4KiB buf', command: 'get', args: ['foo_rand000000000001'], pipeline: 50, client_opts: { return_buffers: true} })); tests.push(new Test({descr: 'GET 4KiB buf', command: 'get', args: ['foo_rand000000000001'], batch: 50, client_opts: { return_buffers: true} })); tests.push(new Test({descr: 'INCR', command: 'incr', args: ['counter_rand000000000000'], pipeline: 1})); -// tests.push(new Test({descr: 'INCR', command: 'incr', args: ['counter_rand000000000000'], pipeline: 50})); tests.push(new Test({descr: 'INCR', command: 'incr', args: ['counter_rand000000000000'], batch: 50})); tests.push(new Test({descr: 'LPUSH', command: 'lpush', args: ['mylist', small_str], pipeline: 1})); -// tests.push(new Test({descr: 'LPUSH', command: 'lpush', args: ['mylist', small_str], pipeline: 50})); tests.push(new Test({descr: 'LPUSH', command: 'lpush', args: ['mylist', small_str], batch: 50})); tests.push(new Test({descr: 'LRANGE 10', command: 'lrange', args: ['mylist', '0', '9'], pipeline: 1})); -// tests.push(new Test({descr: 'LRANGE 10', command: 'lrange', args: ['mylist', '0', '9'], pipeline: 50})); tests.push(new Test({descr: 'LRANGE 10', command: 'lrange', args: ['mylist', '0', '9'], batch: 50})); tests.push(new Test({descr: 'LRANGE 100', command: 'lrange', args: ['mylist', '0', '99'], pipeline: 1})); -// tests.push(new Test({descr: 'LRANGE 100', command: 'lrange', args: ['mylist', '0', '99'], pipeline: 50})); tests.push(new Test({descr: 'LRANGE 100', command: 'lrange', args: ['mylist', '0', '99'], batch: 50})); tests.push(new Test({descr: 'SET 4MiB str', command: 'set', args: ['foo_rand000000000002', very_large_str], pipeline: 1})); -// tests.push(new Test({descr: 'SET 4MiB str', command: 'set', args: ['foo_rand000000000002', very_large_str], pipeline: 20})); tests.push(new Test({descr: 'SET 4MiB str', command: 'set', args: ['foo_rand000000000002', very_large_str], batch: 20})); tests.push(new Test({descr: 'SET 4MiB buf', command: 'set', args: ['foo_rand000000000002', very_large_buf], pipeline: 1})); -// tests.push(new Test({descr: 'SET 4MiB buf', command: 'set', args: ['foo_rand000000000002', very_large_buf], pipeline: 20})); tests.push(new Test({descr: 'SET 4MiB buf', command: 'set', args: ['foo_rand000000000002', very_large_buf], batch: 20})); tests.push(new Test({descr: 'GET 4MiB str', command: 'get', args: ['foo_rand000000000002'], pipeline: 1})); -// tests.push(new Test({descr: 'GET 4MiB str', command: 'get', args: ['foo_rand000000000002'], pipeline: 20})); tests.push(new Test({descr: 'GET 4MiB str', command: 'get', args: ['foo_rand000000000002'], batch: 20})); tests.push(new Test({descr: 'GET 4MiB buf', command: 'get', args: ['foo_rand000000000002'], pipeline: 1, client_opts: { return_buffers: true} })); -// tests.push(new Test({descr: 'GET 4MiB buf', command: 'get', args: ['foo_rand000000000002'], pipeline: 20, client_opts: { return_buffers: true} })); tests.push(new Test({descr: 'GET 4MiB buf', command: 'get', args: ['foo_rand000000000002'], batch: 20, client_opts: { return_buffers: true} })); function next() { From ce80569bfed54eee070cd36b46f0816a17c69c06 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 31 Jan 2016 15:00:27 +0100 Subject: [PATCH 10/28] Update npm ignore file --- .npmignore | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/.npmignore b/.npmignore index 886d9ea..b0238e0 100644 --- a/.npmignore +++ b/.npmignore @@ -1,9 +1,10 @@ examples/ -benches/ +benchmarks/ test/ -diff_multi_bench_output.js -generate_commands.js -multi_bench.js -test-unref.js -changelog.md -*.log \ No newline at end of file +.nyc_output/ +coverage/ +.tern-port +*.log +*.rdb +*.out +*.yml From 614e35ab57eabe55267e59ef8c625fbbdda54eca Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 22 Feb 2016 19:38:07 +0100 Subject: [PATCH 11/28] Move multi; commands; createClient code into separate files --- lib/command.js | 4 +- lib/commands.js | 86 +++++++++++++++ lib/createClient.js | 67 ++++++++++++ lib/debug.js | 11 ++ lib/individualCommands.js | 137 +++++++++++++++++++++++ lib/multi.js | 224 ++++++++++++++++++++++++++++++++++++++ lib/utils.js | 83 ++++++++++---- 7 files changed, 586 insertions(+), 26 deletions(-) create mode 100644 lib/commands.js create mode 100644 lib/createClient.js create mode 100644 lib/debug.js create mode 100644 lib/individualCommands.js create mode 100644 lib/multi.js diff --git a/lib/command.js b/lib/command.js index d04052f..0c01976 100644 --- a/lib/command.js +++ b/lib/command.js @@ -2,10 +2,10 @@ // This Command constructor is ever so slightly faster than using an object literal, but more importantly, using // a named constructor helps it show up meaningfully in the V8 CPU profiler and in heap snapshots. -function Command(command, args, buffer_args, callback) { +function Command(command, args, callback) { this.command = command; this.args = args; - this.buffer_args = buffer_args; + this.buffer_args = false; this.callback = callback; } diff --git a/lib/commands.js b/lib/commands.js new file mode 100644 index 0000000..734e7b7 --- /dev/null +++ b/lib/commands.js @@ -0,0 +1,86 @@ +'use strict'; + +var commands = require('redis-commands'); +var Multi = require('./multi'); +var RedisClient = require('../').RedisClient; + +// TODO: Rewrite this including the invidual commands into a Commands class +// that provided a functionality to add new commands to the client + +commands.list.forEach(function (command) { + + // Do not override existing functions + if (!RedisClient.prototype[command]) { + RedisClient.prototype[command.toUpperCase()] = RedisClient.prototype[command] = function () { + var arr; + var len = arguments.length; + var callback; + var i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + if (len === 2) { + callback = arguments[1]; + } + } else if (len > 1 && Array.isArray(arguments[1])) { + if (len === 3) { + callback = arguments[2]; + } + len = arguments[1].length; + arr = new Array(len + 1); + arr[0] = arguments[0]; + for (; i < len; i += 1) { + arr[i + 1] = arguments[1][i]; + } + } else { + // The later should not be the average use case + if (len !== 0 && (typeof arguments[len - 1] === 'function' || typeof arguments[len - 1] === 'undefined')) { + len--; + callback = arguments[len]; + } + arr = new Array(len); + for (; i < len; i += 1) { + arr[i] = arguments[i]; + } + } + return this.send_command(command, arr, callback); + }; + } + + // Do not override existing functions + if (!Multi.prototype[command]) { + Multi.prototype[command.toUpperCase()] = Multi.prototype[command] = function () { + var arr; + var len = arguments.length; + var callback; + var i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + if (len === 2) { + callback = arguments[1]; + } + } else if (len > 1 && Array.isArray(arguments[1])) { + if (len === 3) { + callback = arguments[2]; + } + len = arguments[1].length; + arr = new Array(len + 1); + arr[0] = arguments[0]; + for (; i < len; i += 1) { + arr[i + 1] = arguments[1][i]; + } + } else { + // The later should not be the average use case + if (len !== 0 && (typeof arguments[len - 1] === 'function' || typeof arguments[len - 1] === 'undefined')) { + len--; + callback = arguments[len]; + } + arr = new Array(len); + for (; i < len; i += 1) { + arr[i] = arguments[i]; + } + } + this.queue.push([command, arr, callback]); + return this; + }; + } +}); diff --git a/lib/createClient.js b/lib/createClient.js new file mode 100644 index 0000000..3b14ef0 --- /dev/null +++ b/lib/createClient.js @@ -0,0 +1,67 @@ +'use strict'; + +var utils = require('./utils'); +var URL = require('url'); + +module.exports = function createClient (port_arg, host_arg, options) { + + if (typeof port_arg === 'number' || typeof port_arg === 'string' && /^\d+$/.test(port_arg)) { + + var host; + if (typeof host_arg === 'string') { + host = host_arg; + } else { + options = options || host_arg; + } + options = utils.clone(options); + options.host = host || options.host; + options.port = port_arg; + + } else if (typeof port_arg === 'string' || port_arg && port_arg.url) { + + options = utils.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]]] + if (parsed.hostname || parsed.slashes) { // The host might be an empty string + if (parsed.auth) { + options.password = parsed.auth.split(':')[1]; + } + if (!/^([a-z]+:)?\/\//i.test(parsed.href)) { + throw new Error('Connection string must use the "redis:" protocol or begin with slashes //'); + } + 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 (elem in options) { + if (options[elem] === parsed.query[elem]) { + console.warn('node_redis: WARNING: You passed the ' + elem + ' option twice!'); + } else { + throw new Error('The ' + elem + ' option is added twice and does not match'); + } + } + options[elem] = parsed.query[elem]; + } + } + } else { + options.path = port_arg; + } + + } else if (typeof port_arg === 'object' || port_arg === undefined) { + options = utils.clone(port_arg || options); + options.host = options.host || host_arg; + } + + if (!options) { + throw new Error('Unknown type of connection in createClient()'); + } + + return options; +}; diff --git a/lib/debug.js b/lib/debug.js new file mode 100644 index 0000000..3f9d482 --- /dev/null +++ b/lib/debug.js @@ -0,0 +1,11 @@ +'use strict'; + +var index = require('../'); + +function debug (msg) { + if (index.debug_mode) { + console.error(msg); + } +} + +module.exports = debug; diff --git a/lib/individualCommands.js b/lib/individualCommands.js new file mode 100644 index 0000000..b162238 --- /dev/null +++ b/lib/individualCommands.js @@ -0,0 +1,137 @@ +'use strict'; + +var utils = require('./utils'); +var debug = require('./debug'); +var Multi = require('./multi'); +var no_password_is_set = /no password is set/; +var RedisClient = require('../').RedisClient; + +/******************************** +Replace built-in redis functions +********************************/ + +RedisClient.prototype.multi = RedisClient.prototype.MULTI = function multi (args) { + var multi = new Multi(this, args); + multi.exec = multi.EXEC = multi.exec_transaction; + return multi; +}; + +// ATTENTION: This is not a native function but is still handled as a individual command as it behaves just the same as multi +RedisClient.prototype.batch = RedisClient.prototype.BATCH = function batch (args) { + return new Multi(this, args); +}; + +// Store db in this.select_db to restore it on reconnect +RedisClient.prototype.select = RedisClient.prototype.SELECT = function select (db, callback) { + var self = this; + return this.send_command('select', [db], function (err, res) { + if (err === null) { + self.selected_db = db; + } + utils.callback_or_emit(self, callback, err, res); + }); +}; + +// Store info in this.server_info after each call +RedisClient.prototype.info = RedisClient.prototype.INFO = function info (callback) { + var self = this; + var ready = this.ready; + this.ready = ready || this.offline_queue.length === 0; // keep the execution order intakt + var tmp = this.send_command('info', [], function (err, res) { + if (res) { + var obj = {}; + var lines = res.toString().split('\r\n'); + var line, parts, sub_parts; + + for (var i = 0; i < lines.length; i++) { + parts = lines[i].split(':'); + if (parts[1]) { + if (parts[0].indexOf('db') === 0) { + sub_parts = parts[1].split(','); + obj[parts[0]] = {}; + while (line = sub_parts.pop()) { + line = line.split('='); + obj[parts[0]][line[0]] = +line[1]; + } + } else { + obj[parts[0]] = parts[1]; + } + } + } + obj.versions = []; + /* istanbul ignore else: some redis servers do not send the version */ + if (obj.redis_version) { + obj.redis_version.split('.').forEach(function (num) { + obj.versions.push(+num); + }); + } + // Expose info key/vals to users + self.server_info = obj; + } else { + self.server_info = {}; + } + utils.callback_or_emit(self, callback, err, res); + }); + this.ready = ready; + return tmp; +}; + +RedisClient.prototype.auth = RedisClient.prototype.AUTH = function auth (pass, callback) { + var self = this; + var ready = this.ready; + debug('Sending auth to ' + self.address + ' id ' + self.connection_id); + + // Stash auth for connect and reconnect. + this.auth_pass = pass; + this.ready = this.offline_queue.length === 0; // keep the execution order intakt + var tmp = this.send_command('auth', [pass], function (err, res) { + if (err && no_password_is_set.test(err.message)) { + self.warn('Warning: Redis server does not require a password, but a password was supplied.'); + err = null; + res = 'OK'; + } + + utils.callback_or_emit(self, callback, err, res); + }); + this.ready = ready; + return tmp; +}; + +RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function hmset () { + var arr, + len = arguments.length, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } else if (Array.isArray(arguments[1])) { + if (len === 3) { + callback = arguments[2]; + } + len = arguments[1].length; + arr = new Array(len + 1); + arr[0] = arguments[0]; + for (; i < len; i += 1) { + arr[i + 1] = arguments[1][i]; + } + } else if (typeof arguments[1] === 'object' && (arguments.length === 2 || arguments.length === 3 && typeof arguments[2] === 'function' || typeof arguments[2] === 'undefined')) { + arr = [arguments[0]]; + for (var field in arguments[1]) { // jshint ignore: line + arr.push(field, arguments[1][field]); + } + callback = arguments[2]; + } else { + len = arguments.length; + // The later should not be the average use case + if (len !== 0 && (typeof arguments[len - 1] === 'function' || typeof arguments[len - 1] === 'undefined')) { + len--; + callback = arguments[len]; + } + arr = new Array(len); + for (; i < len; i += 1) { + arr[i] = arguments[i]; + } + } + return this.send_command('hmset', arr, callback); +}; diff --git a/lib/multi.js b/lib/multi.js new file mode 100644 index 0000000..5a06e10 --- /dev/null +++ b/lib/multi.js @@ -0,0 +1,224 @@ +'use strict'; + +var Queue = require('double-ended-queue'); +var utils = require('./utils'); + +function Multi(client, args) { + this._client = client; + this.queue = new Queue(); + var command, tmp_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); + if (Array.isArray(command)) { + this[command[0]].apply(this, command.slice(1).concat(tmp_args)); + } else { + this[command].apply(this, tmp_args); + } + } + } +} + +Multi.prototype.hmset = Multi.prototype.HMSET = function hmset () { + var arr, + len = 0, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } else if (Array.isArray(arguments[1])) { + len = arguments[1].length; + arr = new Array(len + 1); + arr[0] = arguments[0]; + for (; i < len; i += 1) { + arr[i + 1] = arguments[1][i]; + } + callback = arguments[2]; + } else if (typeof arguments[1] === 'object' && (typeof arguments[2] === 'function' || typeof arguments[2] === 'undefined')) { + arr = [arguments[0]]; + for (var field in arguments[1]) { // jshint ignore: line + arr.push(field, arguments[1][field]); + } + callback = arguments[2]; + } else { + len = arguments.length; + // The later should not be the average use case + if (len !== 0 && (typeof arguments[len - 1] === 'function' || typeof arguments[len - 1] === 'undefined')) { + len--; + callback = arguments[len]; + } + arr = new Array(len); + for (; i < len; i += 1) { + arr[i] = arguments[i]; + } + } + this.queue.push(['hmset', arr, callback]); + return this; +}; + +function pipeline_transaction_command (self, command, args, index, cb) { + self._client.send_command(command, args, function (err, reply) { + if (err) { + if (cb) { + cb(err); + } + err.position = index; + self.errors.push(err); + } + }); +} + +Multi.prototype.exec_atomic = function exec_atomic (callback) { + if (this.queue.length < 2) { + return this.exec_batch(callback); + } + return this.exec(callback); +}; + +function multi_callback (self, err, replies) { + var i = 0, args; + + if (err) { + // The errors would be circular + var connection_error = ['CONNECTION_BROKEN', 'UNCERTAIN_STATE'].indexOf(err.code) !== -1; + err.errors = connection_error ? [] : self.errors; + if (self.callback) { + self.callback(err); + // Exclude connection errors so that those errors won't be emitted twice + } else if (!connection_error) { + self._client.emit('error', err); + } + return; + } + + if (replies) { + while (args = self.queue.shift()) { + if (replies[i] instanceof Error) { + var match = replies[i].message.match(utils.err_code); + // LUA script could return user errors that don't behave like all other errors! + if (match) { + replies[i].code = match[1]; + } + replies[i].command = args[0].toUpperCase(); + if (typeof args[2] === 'function') { + args[2](replies[i]); + } + } else { + // If we asked for strings, even in detect_buffers mode, then return strings: + replies[i] = self._client.handle_reply(replies[i], args[0], self.wants_buffers[i]); + if (typeof args[2] === 'function') { + args[2](null, replies[i]); + } + } + i++; + } + } + + if (self.callback) { + self.callback(null, replies); + } +} + +Multi.prototype.exec_transaction = function exec_transaction (callback) { + var self = this; + var len = self.queue.length; + self.errors = []; + self.callback = callback; + self._client.cork(len + 2); + self.wants_buffers = new Array(len); + pipeline_transaction_command(self, 'multi', []); + // Drain queue, callback will catch 'QUEUED' or error + for (var index = 0; index < len; index++) { + var args = self.queue.get(index); + var command = args[0]; + var cb = args[2]; + // Keep track of who wants buffer responses: + if (self._client.options.detect_buffers) { + self.wants_buffers[index] = false; + for (var i = 0; i < args[1].length; i += 1) { + if (args[1][i] instanceof Buffer) { + self.wants_buffers[index] = true; + break; + } + } + } + pipeline_transaction_command(self, command, args[1], index, cb); + } + + self._client.send_command('exec', [], function(err, replies) { + multi_callback(self, err, replies); + }); + self._client.uncork(); + self._client.writeDefault = self._client.writeStrings; + return !self._client.should_buffer; +}; + +function batch_callback (self, cb, i) { + return function batch_callback (err, res) { + if (err) { + self.results[i] = err; + // Add the position to the error + self.results[i].position = i; + } else { + self.results[i] = res; + } + cb(err, res); + }; +} + +Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = function exec_batch (callback) { + var self = this; + var len = self.queue.length; + var index = 0; + var args; + var args_len = 1; + var callback_without_own_cb = function (err, res) { + if (err) { + self.results.push(err); + // Add the position to the error + var i = self.results.length - 1; + self.results[i].position = i; + } else { + self.results.push(res); + } + // Do not emit an error here. Otherwise each error would result in one emit. + // The errors will be returned in the result anyway + }; + var last_callback = function (cb) { + return function (err, res) { + cb(err, res); + callback(null, self.results); + }; + }; + if (len === 0) { + if (callback) { + utils.reply_in_order(self._client, callback, null, []); + } + return true; + } + self.results = []; + self._client.cork(len); + while (args = self.queue.shift()) { + var command = args[0]; + var cb; + args_len = args[1].length - 1; + if (typeof args[2] === 'function') { + cb = batch_callback(self, args[2], index); + } else { + cb = callback_without_own_cb; + } + if (callback && index === len - 1) { + cb = last_callback(cb); + } + self._client.send_command(command, args[1], cb); + index++; + } + self.queue = new Queue(); + self._client.uncork(); + self._client.writeDefault = self._client.writeStrings; + return !self._client.should_buffer; +}; + +module.exports = Multi; diff --git a/lib/utils.js b/lib/utils.js index 8cdfb1f..89d8590 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -1,22 +1,24 @@ 'use strict'; // hgetall converts its replies to an Object. If the reply is empty, null is returned. +// These function are only called with internal data and have therefor always the same instanceof X function replyToObject(reply) { - if (reply.length === 0 || !Array.isArray(reply)) { // TODO: Check why the isArray check is needed and what value reply has in that case + // The reply might be a string or a buffer if this is called in a transaction (multi) + if (reply.length === 0 || !(reply instanceof Array)) { return null; } var obj = {}; - for (var j = 0; j < reply.length; j += 2) { - obj[reply[j].toString('binary')] = reply[j + 1]; + for (var i = 0; i < reply.length; i += 2) { + obj[reply[i].toString('binary')] = reply[i + 1]; } return obj; } function replyToStrings(reply) { - if (Buffer.isBuffer(reply)) { + if (reply instanceof Buffer) { return reply.toString(); } - if (Array.isArray(reply)) { + if (reply instanceof Array) { var res = new Array(reply.length); for (var i = 0; i < reply.length; i++) { // Recusivly call the function as slowlog returns deep nested replies @@ -39,35 +41,68 @@ function print (err, reply) { var redisErrCode = /^([A-Z]+)\s+(.+)$/; // Deep clone arbitrary objects with arrays. Can't handle cyclic structures (results in a range error) +// Any attribute with a non primitive value besides object and array will be passed by reference (e.g. Buffers, Maps, Functions) function clone (obj) { - if (obj) { - var copy; - if (obj.constructor === Array) { - copy = new Array(obj.length); - for (var i = 0; i < obj.length; i++) { - copy[i] = clone(obj[i]); - } - return copy; + var copy; + if (Array.isArray(obj)) { + copy = new Array(obj.length); + for (var i = 0; i < obj.length; i++) { + copy[i] = clone(obj[i]); } - if (obj.constructor === Object) { - copy = {}; - for (var elem in obj) { - if (!obj.hasOwnProperty(elem)) { - // Do not add non own properties to the cloned object - continue; - } - copy[elem] = clone(obj[elem]); - } - return copy; + return copy; + } + if (Object.prototype.toString.call(obj) === '[object Object]') { + copy = {}; + var elems = Object.keys(obj); + var elem; + while (elem = elems.pop()) { + copy[elem] = clone(obj[elem]); } + return copy; } return obj; } +function convenienceClone (obj) { + return clone(obj) || {}; +} + +function callbackOrEmit (self, callback, err, res) { + if (callback) { + callback(err, res); + } else if (err) { + self.emit('error', err); + } +} + +function replyInOrder (self, callback, err, res) { + var command_obj = self.command_queue.peekBack() || self.offline_queue.peekBack(); + if (!command_obj) { + process.nextTick(function () { + callbackOrEmit(self, callback, err, res); + }); + } else { + var tmp = command_obj.callback; + command_obj.callback = tmp ? + function (e, r) { + tmp(e, r); + callbackOrEmit(self, callback, err, res); + } : + function (e, r) { + if (e) { + self.emit('error', e); + } + callbackOrEmit(self, callback, err, res); + }; + } +} + module.exports = { reply_to_strings: replyToStrings, reply_to_object: replyToObject, print: print, err_code: redisErrCode, - clone: clone + clone: convenienceClone, + callback_or_emit: callbackOrEmit, + reply_in_order: replyInOrder }; From 4f3c4a2ef672af2a7182b72aa92dca0310084932 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 25 Feb 2016 02:37:42 +0100 Subject: [PATCH 12/28] Add more tests Add execution order tests Fix flaky test Add utils tests Improve other tests --- test/batch.spec.js | 10 ++- test/commands/set.spec.js | 11 +++- test/connection.spec.js | 15 ++--- test/multi.spec.js | 3 +- test/node_redis.spec.js | 35 +++++++++++ test/rename.spec.js | 19 ++++++ test/utils.spec.js | 129 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 210 insertions(+), 12 deletions(-) create mode 100644 test/utils.spec.js diff --git a/test/batch.spec.js b/test/batch.spec.js index a99cb82..ba4c902 100644 --- a/test/batch.spec.js +++ b/test/batch.spec.js @@ -63,11 +63,16 @@ describe("The 'batch' method", function () { client.end(true); }); - it("returns an empty array", function (done) { + it("returns an empty array and keep the execution order in takt", function (done) { + var called = false; + client.set('foo', 'bar', function (err, res) { + called = true; + }); var batch = client.batch(); batch.exec(function (err, res) { assert.strictEqual(err, null); assert.strictEqual(res.length, 0); + assert(called); done(); }); }); @@ -328,10 +333,11 @@ describe("The 'batch' method", function () { .exec(done); }); - it("should work without any callback", function (done) { + it("should work without any callback or arguments", function (done) { var batch = client.batch(); batch.set("baz", "binary"); batch.set("foo", "bar"); + batch.ping(); batch.exec(); client.get('foo', helper.isString('bar', done)); diff --git a/test/commands/set.spec.js b/test/commands/set.spec.js index c486629..3235d63 100644 --- a/test/commands/set.spec.js +++ b/test/commands/set.spec.js @@ -66,14 +66,21 @@ describe("The 'set' method", function () { }); }); - describe("with undefined 'key' and missing 'value' parameter", function () { - it("reports an error", function (done) { + describe("reports an error with invalid parameters", function () { + it("undefined 'key' and missing 'value' parameter", function (done) { client.set(undefined, function (err, res) { helper.isError()(err, null); assert.equal(err.command, 'SET'); done(); }); }); + + it("empty array as second parameter", function (done) { + client.set('foo', [], function (err, res) { + assert.strictEqual(err.message, "ERR wrong number of arguments for 'set' command"); + done(); + }); + }); }); }); diff --git a/test/connection.spec.js b/test/connection.spec.js index 5f5892d..c047788 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -136,13 +136,14 @@ describe("connection tests", function () { var time = Date.now(); client = redis.createClient({ parser: parser, - host: '192.168.74.167', // Should be auto detected as ipv4 + // Auto detect ipv4 and use non routable ip to trigger the timeout + host: '10.255.255.1', connect_timeout: connect_timeout }); process.nextTick(function() { - assert(client.stream._events.timeout); + assert.strictEqual(client.stream.listeners('timeout').length, 1); }); - assert.strictEqual(client.address, '192.168.74.167:6379'); + assert.strictEqual(client.address, '10.255.255.1:6379'); assert.strictEqual(client.connection_options.family, 4); client.on("reconnecting", function (params) { @@ -151,8 +152,8 @@ describe("connection tests", function () { client.on('error', function(err) { assert(/Redis connection in broken state: connection timeout.*?exceeded./.test(err.message)); - assert(Date.now() - time < connect_timeout + 50); - assert(Date.now() - time >= connect_timeout - 50); // Somehow this is triggered to early at times + assert(Date.now() - time < connect_timeout + 25); + assert(Date.now() - time >= connect_timeout - 3); // Timers sometimes trigger early (e.g. 1ms to early) done(); }); }); @@ -165,7 +166,7 @@ describe("connection tests", function () { assert.strictEqual(client.address, '2001:db8::ff00:42:8329:6379'); assert.strictEqual(client.connection_options.family, 6); process.nextTick(function() { - assert.strictEqual(client.stream._events.timeout, undefined); + assert.strictEqual(client.stream.listeners('timeout').length, 0); }); }); @@ -179,7 +180,7 @@ describe("connection tests", function () { }); client.on('connect', function () { assert.strictEqual(client.stream._idleTimeout, -1); - assert.strictEqual(client.stream._events.timeout, undefined); + assert.strictEqual(client.stream.listeners('timeout').length, 0); client.on('ready', done); }); }); diff --git a/test/multi.spec.js b/test/multi.spec.js index 4d17c92..766d579 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -241,6 +241,7 @@ describe("The 'multi' method", function () { multi1.set("m1", "123"); multi1.get('m1'); multi2.get('m2'); + multi2.ping(); multi1.exec(end); multi2.exec(function(err, res) { @@ -538,7 +539,7 @@ describe("The 'multi' method", function () { client.get('foo', helper.isString('bar', done)); }); - it("should not use a transaction with exec_atomic if only no command is used", function () { + it("should not use a transaction with exec_atomic if no command is used", function () { var multi = client.multi(); var test = false; multi.exec_batch = function () { diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 1210799..ef5090c 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -423,6 +423,41 @@ describe("The node_redis client", function () { }); }); + describe('execution order / fire query while loading', function () { + it('keep execution order for commands that may fire while redis is still loading', function (done) { + client = redis.createClient.apply(null, args); + var fired = false; + client.set('foo', 'bar', function (err, res) { + assert(fired === false); + done(); + }); + client.info(function (err, res) { + fired = true; + }); + }); + + it('should fire early', function (done) { + client = redis.createClient.apply(null, args); + var fired = false; + client.info(function (err, res) { + fired = true; + }); + client.set('foo', 'bar', function (err, res) { + assert(fired); + done(); + }); + assert.strictEqual(client.offline_queue.length, 1); + assert.strictEqual(client.command_queue.length, 1); + client.on('connect', function () { + assert.strictEqual(client.offline_queue.length, 1); + assert.strictEqual(client.command_queue.length, 1); + }); + client.on('ready', function () { + assert.strictEqual(client.offline_queue.length, 0); + }); + }); + }); + describe('socket_nodelay', function () { describe('true', function () { var args = config.configureClient(parser, ip, { diff --git a/test/rename.spec.js b/test/rename.spec.js index 5ac7435..9607210 100644 --- a/test/rename.spec.js +++ b/test/rename.spec.js @@ -109,6 +109,25 @@ describe("rename commands", function () { }); }); + it("should also work prefixed commands", function (done) { + if (helper.redisProcess().spawnFailed()) this.skip(); + + client.end(true); + client = redis.createClient({ + rename_commands: { + set: '807081f5afa96845a02816a28b7258c3' + }, + parser: parser, + prefix: 'baz' + }); + client.set('foo', 'bar'); + client.keys('*', function(err, reply) { + assert.strictEqual(reply[0], 'bazfoo'); + assert.strictEqual(err, null); + done(); + }); + }); + }); }); diff --git a/test/utils.spec.js b/test/utils.spec.js new file mode 100644 index 0000000..d75e035 --- /dev/null +++ b/test/utils.spec.js @@ -0,0 +1,129 @@ +'use strict'; + +var assert = require('assert'); +var Queue = require('double-ended-queue'); +var utils = require('../lib/utils'); + +describe('utils.js', function () { + + describe('clone', function () { + it('ignore the object prototype and clone a nested array / object', function () { + var obj = { + a: [null, 'foo', ['bar'], { + "I'm special": true + }], + number: 5, + fn: function noop () {} + }; + var clone = utils.clone(obj); + assert.deepEqual(clone, obj); + assert.strictEqual(obj.fn, clone.fn); + assert(typeof clone.fn === 'function'); + }); + + it('replace faulty values with an empty object as return value', function () { + var a = utils.clone(); + var b = utils.clone(null); + assert.strictEqual(Object.keys(a).length, 0); + assert.strictEqual(Object.keys(b).length, 0); + }); + + it('throws on circular data', function () { + try { + var a = {}; + a.b = a; + utils.clone(a); + throw new Error('failed'); + } catch (e) { + assert(e.message !== 'failed'); + } + }); + }); + + describe('reply_in_order', function () { + + var err_count = 0; + var res_count = 0; + var emitted = false; + var clientMock = { + emit: function () { emitted = true; }, + offline_queue: new Queue(), + command_queue: new Queue() + }; + var create_command_obj = function () { + return { + callback: function (err, res) { + if (err) err_count++; + else res_count++; + } + }; + }; + + beforeEach(function () { + clientMock.offline_queue.clear(); + clientMock.command_queue.clear(); + err_count = 0; + res_count = 0; + emitted = false; + }); + + it('no elements in either queue. Reply in the next tick', function (done) { + var called = false; + utils.reply_in_order(clientMock, function () { + called = true; + done(); + }, null, null); + assert(!called); + }); + + it('no elements in either queue. Reply in the next tick', function (done) { + assert(!emitted); + utils.reply_in_order(clientMock, null, new Error('tada')); + assert(!emitted); + setTimeout(function () { + assert(emitted); + done(); + }, 1); + }); + + it('elements in the offline queue. Reply after the offline queue is empty and respect the command_obj callback', function (done) { + clientMock.offline_queue.push(create_command_obj(), create_command_obj()); + utils.reply_in_order(clientMock, function () { + assert.strictEqual(clientMock.offline_queue.length, 0); + assert.strictEqual(res_count, 2); + done(); + }, null, null); + while (clientMock.offline_queue.length) clientMock.offline_queue.shift().callback(null, 'foo'); + }); + + it('elements in the offline queue. Reply after the offline queue is empty and respect the command_obj error emit', function (done) { + clientMock.command_queue.push({}, create_command_obj(), {}); + utils.reply_in_order(clientMock, function () { + assert.strictEqual(clientMock.command_queue.length, 0); + assert(emitted); + assert.strictEqual(err_count, 1); + assert.strictEqual(res_count, 0); + done(); + }, null, null); + while (clientMock.command_queue.length) { + var command_obj = clientMock.command_queue.shift(); + if (command_obj.callback) { + command_obj.callback(new Error('tada')); + } + } + }); + + it('elements in the offline queue. Reply after the offline queue is empty and respect the command_obj', function (done) { + clientMock.command_queue.push(create_command_obj(), {}); + utils.reply_in_order(clientMock, function () { + assert.strictEqual(clientMock.command_queue.length, 0); + assert(!emitted); + assert.strictEqual(res_count, 1); + done(); + }, null, null); + while (clientMock.command_queue.length) { + clientMock.command_queue.shift().callback(null, 'bar'); + } + }); + }); +}); From 1a8a72ddf36d15d7850fc7a6d616e11510e16b77 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 16:32:15 +0100 Subject: [PATCH 13/28] Improve tls tests Removed redundant tests. Skip the tests on windows instead of not even showing them. Add a faulty cert to check proper cert validation. Reject unauthorized certs --- test/conf/faulty.cert | 19 ++++ test/tls.spec.js | 251 +++++++++++++----------------------------- 2 files changed, 98 insertions(+), 172 deletions(-) create mode 100644 test/conf/faulty.cert diff --git a/test/conf/faulty.cert b/test/conf/faulty.cert new file mode 100644 index 0000000..30c9879 --- /dev/null +++ b/test/conf/faulty.cert @@ -0,0 +1,19 @@ +-----BEGIN CERTIFICATE----- +MIIDATCCAemgAwIBAgIJALkMmVkQOERnMA0GCSqGSIb3DQEBBQUAMBcxFTATBgNV +BAMMDHJlZGlzLmpzLm9yZzAeFw0xNTEwMTkxMjIzMjRaFw0yNTEwMTYxMjIzMjRa +MBcxFTATBgNVBAMMDHJlZGlzLmpzLm9yZzCCASIwDQYJKoZIhvcNAQEBBQADggEP +ADCCAQoCggEBAJ/DmMTJHf7kyspxI1A/JmOc+KI9vxEcN5qn7IiZuGN7ghE43Q3q +XB2GUkMAuW1POkmM5yi3SuT1UXDR/4Gk7KlbHKMs37AV6PgJXX6oX0zu12LTAT7V +5byNrYtehSo42l1188dGEMCGaaf0cDntc7A3aW0ZtzrJt+2pu31Uatl2SEJCMra6 ++v6O0c9aHMF1cArKeawGqR+jHw6vXFZQbUd06nW5nQlUA6wVt1JjlLPwBwYsWLsi +YQxMC8NqpgAIg5tULSCpKwx5isL/CeotVVGDNZ/G8R1nTrxuygPlc3Qskj57hmV4 +tZK4JJxQFi7/9ehvjAvHohKrEPeqV5XL87cCAwEAAaNQME4wHQYDVR0OBBYEFCn/ +5hB+XY4pVOnaqvrmZMxrLFjLMB8GA1UdIwQYMBaAFCn/5hB+XY4pVOnaqvrmZMxr +LFjLMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEFBQADggEBAEduPyTHpXkCVZRQ +v6p+Ug4iVeXpxGCVr34y7EDUMgmuDdqsz1SrmqeDd0VmjZT8htbWw7QBKDPEBsbi +wl606aAn01iM+oUrwbtXxid1xfZj/j6pIhQVkGu7e/8A7Pr4QOP4OMdHB7EmqkAo +d/OLHa9LdKv2UtJHD6U7oVQbdBHrRV62125GMmotpQuSkEfZM6edKNzHPlqV/zJc +2kGCw3lZC21mTrsSMIC/FQiobPnig4kAvfh0of2rK/XAntlwT8ie1v1aK+jERsfm +uzMihl6XXBdzheq6KdIlf+5STHBIIRcvBoRKr5Va7EhnO03tTzeJowtqDv47yPC6 +w4kLcP8= +-----END CERTIFICATE----- diff --git a/test/tls.spec.js b/test/tls.spec.js index 68d12d7..20cd246 100644 --- a/test/tls.spec.js +++ b/test/tls.spec.js @@ -6,25 +6,33 @@ var fs = require('fs'); var helper = require('./helper'); var path = require('path'); var redis = config.redis; +var utils = require('../lib/utils'); var tls_options = { servername: "redis.js.org", - rejectUnauthorized: false, + rejectUnauthorized: true, ca: [ String(fs.readFileSync(path.resolve(__dirname, "./conf/redis.js.org.cert"))) ] }; var tls_port = 6380; - -if (process.platform === 'win32') { - return; -} +// Use skip instead of returning to indicate what tests really got skipped +var skip = false; // Wait until stunnel4 is in the travis whitelist // Check: https://github.com/travis-ci/apt-package-whitelist/issues/403 // If this is merged, remove the travis env checks describe("TLS connection tests", function () { + before(function (done) { - if (process.env.TRAVIS === 'true') { + // Print the warning when the tests run instead of while starting mocha + if (process.platform === 'win32') { + skip = true; + console.warn('\nStunnel tests do not work on windows atm. If you think you can fix that, it would be warmly welcome.\n'); + } else if (process.env.TRAVIS === 'true') { + skip = true; + console.warn('\nTravis does not support stunnel right now. Skipping tests.\nCheck: https://github.com/travis-ci/apt-package-whitelist/issues/403\n'); + } + if (skip) { done(); return; } @@ -34,188 +42,87 @@ describe("TLS connection tests", function () { }); after(function (done) { - if (process.env.TRAVIS === 'true') { + if (skip) { done(); return; } helper.stopStunnel(done); }); - helper.allTests(function(parser, ip, args) { + var client; - describe("using " + parser + " and " + ip, function () { - - var client; + afterEach(function () { + client.end(true); + }); - afterEach(function () { - client.end(true); + describe("on lost connection", function () { + it("emit an error after max retry timeout and do not try to reconnect afterwards", function (done) { + if (skip) this.skip(); + var connect_timeout = 500; // in ms + client = redis.createClient({ + connect_timeout: connect_timeout, + port: tls_port, + tls: tls_options }); + var time = 0; - describe("on lost connection", function () { - it("emit an error after max retry attempts and do not try to reconnect afterwards", function (done) { - if (process.env.TRAVIS === 'true') this.skip(); - var max_attempts = 4; - var options = { - parser: parser, - max_attempts: max_attempts, - port: tls_port, - tls: tls_options - }; - client = redis.createClient(options); - var calls = 0; - - client.once('ready', function() { - helper.killConnection(client); - }); - - client.on("reconnecting", function (params) { - calls++; - }); - - client.on('error', function(err) { - if (/Redis connection in broken state: maximum connection attempts.*?exceeded./.test(err.message)) { - setTimeout(function () { - assert.strictEqual(calls, max_attempts - 1); - done(); - }, 500); - } - }); - }); - - it("emit an error after max retry timeout and do not try to reconnect afterwards", function (done) { - if (process.env.TRAVIS === 'true') this.skip(); - var connect_timeout = 500; // in ms - client = redis.createClient({ - parser: parser, - connect_timeout: connect_timeout, - port: tls_port, - tls: tls_options - }); - var time = 0; - - client.once('ready', function() { - helper.killConnection(client); - }); - - client.on("reconnecting", function (params) { - time += params.delay; - }); - - client.on('error', function(err) { - if (/Redis connection in broken state: connection timeout.*?exceeded./.test(err.message)) { - setTimeout(function () { - assert(time === connect_timeout); - done(); - }, 500); - } - }); - }); - - it("end connection while retry is still ongoing", function (done) { - if (process.env.TRAVIS === 'true') this.skip(); - var connect_timeout = 1000; // in ms - client = redis.createClient({ - parser: parser, - connect_timeout: connect_timeout, - port: tls_port, - tls: tls_options - }); - - client.once('ready', function() { - helper.killConnection(client); - }); - - client.on("reconnecting", function (params) { - client.end(true); - setTimeout(done, 100); - }); - }); - - it("can not connect with wrong host / port in the options object", function (done) { - if (process.env.TRAVIS === 'true') this.skip(); - var options = { - host: 'somewhere', - max_attempts: 1, - port: tls_port, - tls: tls_options - }; - client = redis.createClient(options); - var end = helper.callFuncAfter(done, 2); - - client.on('error', function (err) { - assert(/CONNECTION_BROKEN|ENOTFOUND|EAI_AGAIN/.test(err.code)); - end(); - }); - - }); + client.once('ready', function() { + helper.killConnection(client); }); - describe("when not connected", function () { - - it("connect with host and port provided in the options object", function (done) { - if (process.env.TRAVIS === 'true') this.skip(); - client = redis.createClient({ - host: 'localhost', - parser: parser, - connect_timeout: 1000, - port: tls_port, - tls: tls_options - }); + client.on("reconnecting", function (params) { + time += params.delay; + }); - client.once('ready', function() { + client.on('error', function(err) { + if (/Redis connection in broken state: connection timeout.*?exceeded./.test(err.message)) { + setTimeout(function () { + assert(time === connect_timeout); done(); - }); - }); - - it("connects correctly with args", function (done) { - if (process.env.TRAVIS === 'true') this.skip(); - var args_host = args[1]; - var args_options = args[2] || {}; - args_options.tls = tls_options; - client = redis.createClient(tls_port, args_host, args_options); - client.on("error", done); - - client.once("ready", function () { - client.removeListener("error", done); - client.get("recon 1", function (err, res) { - done(err); - }); - }); - }); - - if (ip === 'IPv4') { - it('allows connecting with the redis url and no auth and options as second parameter', function (done) { - if (process.env.TRAVIS === 'true') this.skip(); - var options = { - detect_buffers: false, - magic: Math.random(), - port: tls_port, - tls: tls_options - }; - client = redis.createClient('redis://' + config.HOST[ip] + ':' + tls_port, options); - // verify connection is using TCP, not UNIX socket - assert.strictEqual(client.connection_options.host, config.HOST[ip]); - assert.strictEqual(client.connection_options.port, tls_port); - assert(typeof client.stream.getCipher === 'function'); - // verify passed options are in use - assert.strictEqual(client.options.magic, options.magic); - client.on("ready", function () { - return done(); - }); - }); - - it('allows connecting with the redis url and no auth and options as third parameter', function (done) { - if (process.env.TRAVIS === 'true') this.skip(); - client = redis.createClient('redis://' + config.HOST[ip] + ':' + tls_port, null, { - detect_buffers: false, - tls: tls_options - }); - client.on("ready", function () { - return done(); - }); - }); + }, 100); } }); }); }); + + describe("when not connected", function () { + + it("connect with host and port provided in the options object", function (done) { + if (skip) this.skip(); + client = redis.createClient({ + host: 'localhost', + connect_timeout: 1000, + port: tls_port, + tls: tls_options + }); + + // verify connection is using TCP, not UNIX socket + assert.strictEqual(client.connection_options.host, 'localhost'); + assert.strictEqual(client.connection_options.port, tls_port); + assert(client.stream.encrypted); + + client.set('foo', 'bar'); + client.get('foo', helper.isString('bar', done)); + }); + + it('fails to connect because the cert is not correct', function (done) { + if (skip) this.skip(); + var faulty_cert = utils.clone(tls_options); + faulty_cert.ca = [ String(fs.readFileSync(path.resolve(__dirname, "./conf/faulty.cert"))) ]; + client = redis.createClient({ + host: 'localhost', + connect_timeout: 1000, + port: tls_port, + tls: faulty_cert + }); + client.on('error', function (err) { + assert.strictEqual(err.code, 'DEPTH_ZERO_SELF_SIGNED_CERT'); + client.end(true); + }); + client.set('foo', 'bar', function (err, res) { + done(res); + }); + }); + + }); }); From 0a636f95ccc6879dca153feeb24be4fe7f417e53 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 16:36:48 +0100 Subject: [PATCH 14/28] Signal test failures due to used ports and accept individual ports to be provided --- test/lib/redis-process.js | 49 ++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/test/lib/redis-process.js b/test/lib/redis-process.js index 465afdb..a6e9784 100644 --- a/test/lib/redis-process.js +++ b/test/lib/redis-process.js @@ -5,30 +5,51 @@ var config = require('./config'); var fs = require('fs'); var path = require('path'); var spawn = require('win-spawn'); -var spawnFailed = false; var tcpPortUsed = require('tcp-port-used'); +var bluebird = require('bluebird'); // wait for redis to be listening in // all three modes (ipv4, ipv6, socket). -function waitForRedis (available, cb) { +function waitForRedis (available, cb, port) { if (process.platform === 'win32') return cb(); - var ipV4 = false; + var time = Date.now(); + var running = false; + var socket = '/tmp/redis.sock'; + if (port) { + // We have to distinguishe the redis sockets if we have more than a single redis instance running + socket = '/tmp/redis' + port + '.sock'; + } + port = port || config.PORT; var id = setInterval(function () { - tcpPortUsed.check(config.PORT, '127.0.0.1').then(function (_ipV4) { - ipV4 = _ipV4; - return tcpPortUsed.check(config.PORT, '::1'); - }).then(function (ipV6) { - if (ipV6 === available && ipV4 === available && fs.existsSync('/tmp/redis.sock') === available) { - clearInterval(id); - return cb(); + if (running) return; + running = true; + bluebird.join( + tcpPortUsed.check(port, '127.0.0.1'), + tcpPortUsed.check(port, '::1'), + function (ipV4, ipV6) { + if (ipV6 === available && ipV4 === available) { + if (fs.existsSync(socket) === available) { + clearInterval(id); + return cb(); + } + // The same message applies for can't stop but we ignore that case + throw new Error('Port ' + port + ' is already in use. Tests can\'t start.\n'); } + if (Date.now() - time > 6000) { + throw new Error('Redis could not start on port ' + (port || config.PORT) + '\n'); + } + running = false; + }).catch(function (err) { + console.error('\x1b[31m' + err.stack + '\x1b[0m\n'); + process.exit(1); }); }, 100); } module.exports = { - start: function (done, conf) { + start: function (done, conf, port) { + var spawnFailed = false; // spawn redis with our testing configuration. var confFile = conf || path.resolve(__dirname, '../conf/redis.conf'); var rp = spawn("redis-server", [confFile], {}); @@ -53,15 +74,15 @@ module.exports = { rp.once("exit", function (code) { var error = null; if (code !== null && code !== 0) { - error = Error('Redis shutdown failed with code ' + code); + error = new Error('Redis shutdown failed with code ' + code); } waitForRedis(false, function () { return done(error); - }); + }, port); }); rp.kill("SIGTERM"); } }); - }); + }, port); } }; From 6013ee7f900769f70124999df28bfa3e782bd255 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 16:37:32 +0100 Subject: [PATCH 15/28] callFuncAfter should fire the passed fn everytime called above the minimum Also pass individual ports to the redis process through --- test/helper.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/helper.js b/test/helper.js index 72ce1d5..7408577 100644 --- a/test/helper.js +++ b/test/helper.js @@ -8,11 +8,11 @@ var StunnelProcess = require("./lib/stunnel-process"); var rp; var stunnel_process; -function startRedis (conf, done) { +function startRedis (conf, done, port) { RedisProcess.start(function (err, _rp) { rp = _rp; return done(err); - }, path.resolve(__dirname, conf)); + }, path.resolve(__dirname, conf), port); } function startStunnel(done) { @@ -176,7 +176,7 @@ module.exports = { throw err; } i++; - if (i === max) { + if (i >= max) { func(); return true; } From 711d51c387e729c49c376e7d603cbef54410c46f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 16:39:52 +0100 Subject: [PATCH 16/28] Add unify_options / createClient tests --- test/connection.spec.js | 24 ---- test/unify_options.spec.js | 241 +++++++++++++++++++++++++++++++++++++ 2 files changed, 241 insertions(+), 24 deletions(-) create mode 100644 test/unify_options.spec.js diff --git a/test/connection.spec.js b/test/connection.spec.js index c047788..906ae02 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -327,30 +327,6 @@ describe("connection tests", function () { assert(create_stream_string === String(redis.RedisClient.prototype.create_stream)); }); - it("throws on strange connection info", function () { - client = { - end: function() {} - }; - try { - redis.createClient(true); - throw new Error('failed'); - } catch (err) { - assert.equal(err.message, 'Unknown type of connection in createClient()'); - } - }); - - it("throws on protocol other than redis in the redis url", function () { - client = { - end: function() {} - }; - try { - redis.createClient(config.HOST[ip] + ':' + config.PORT); - throw new Error('failed'); - } catch (err) { - assert.equal(err.message, 'Connection string must use the "redis:" protocol or begin with slashes //'); - } - }); - if (ip === 'IPv4') { 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'); diff --git a/test/unify_options.spec.js b/test/unify_options.spec.js new file mode 100644 index 0000000..975ce2b --- /dev/null +++ b/test/unify_options.spec.js @@ -0,0 +1,241 @@ +'use strict'; + +var assert = require('assert'); +var unifyOptions = require('../lib/createClient'); +var intercept = require('intercept-stdout'); + +describe('createClient options', function () { + + describe('port as first parameter', function () { + it('pass the options in the second parameter after a port', function () { + var options = unifyOptions(1234, { + option1: true, + option2: function () {} + }); + assert.strictEqual(Object.keys(options).length, 4); + assert(options.option1); + assert.strictEqual(options.port, 1234); + assert.strictEqual(options.host, undefined); + assert.strictEqual(typeof options.option2, 'function'); + }); + + it('pass the options in the third parameter after a port and host being set to null', function () { + var options = unifyOptions(1234, null, { + option1: true, + option2: function () {} + }); + assert.strictEqual(Object.keys(options).length, 4); + assert(options.option1); + assert.strictEqual(options.port, 1234); + assert.strictEqual(options.host, undefined); + assert.strictEqual(typeof options.option2, 'function'); + }); + + it('pass the options in the third parameter after a port and host being set to undefined', function () { + var options = unifyOptions(1234, undefined, { + option1: true, + option2: function () {} + }); + assert.strictEqual(Object.keys(options).length, 4); + assert(options.option1); + assert.strictEqual(options.port, 1234); + assert.strictEqual(options.host, undefined); + assert.strictEqual(typeof options.option2, 'function'); + }); + + it('pass the options in the third parameter after a port and host', function () { + var options = unifyOptions('1234', 'localhost', { + option1: true, + option2: function () {} + }); + assert.strictEqual(Object.keys(options).length, 4); + assert(options.option1); + assert.strictEqual(options.port, '1234'); + assert.strictEqual(options.host, 'localhost'); + assert.strictEqual(typeof options.option2, 'function'); + }); + + it('should throw with three parameters all set to a truthy value', function () { + try { + unifyOptions(1234, {}, {}); + throw new Error('failed'); + } catch (err) { + assert.strictEqual(err.message, 'Unknown type of connection in createClient()'); + } + }); + }); + + describe('unix socket as first parameter', function () { + it('pass the options in the second parameter after a port', function () { + var options = unifyOptions('/tmp/redis.sock', { + option1: true, + option2: function () {}, + option3: [1, 2, 3] + }); + assert.strictEqual(Object.keys(options).length, 4); + assert(options.option1); + assert.strictEqual(options.path, '/tmp/redis.sock'); + assert.strictEqual(typeof options.option2, 'function'); + assert.strictEqual(options.option3.length, 3); + }); + + it('pass the options in the third parameter after a port and host being set to null', function () { + var options = unifyOptions('/tmp/redis.sock', null, { + option1: true, + option2: function () {} + }); + assert.strictEqual(Object.keys(options).length, 3); + assert(options.option1); + assert.strictEqual(options.path, '/tmp/redis.sock'); + assert.strictEqual(typeof options.option2, 'function'); + }); + }); + + describe('redis url as first parameter', function () { + it('empty redis url including options as second parameter', function () { + var options = unifyOptions('redis://', { + option: [1, 2, 3] + }); + assert.strictEqual(Object.keys(options).length, 1); + assert.strictEqual(options.option.length, 3); + }); + + it('begin with two slashes including options as third parameter', function () { + var options = unifyOptions('//:abc@/3?port=123', { + option: [1, 2, 3] + }); + assert.strictEqual(Object.keys(options).length, 4); + assert.strictEqual(options.option.length, 3); + assert.strictEqual(options.port, '123'); + assert.strictEqual(options.db, '3'); + assert.strictEqual(options.password, 'abc'); + }); + + it('duplicated, identical query options including options obj', function () { + var text = ''; + var unhookIntercept = intercept(function(data) { + text += data; + return ''; + }); + var options = unifyOptions('//:abc@localhost:123/3?db=3&port=123&password=abc', null, { + option: [1, 2, 3] + }); + unhookIntercept(); + assert.strictEqual(text, + 'node_redis: WARNING: You passed the db option twice!\n' + + 'node_redis: WARNING: You passed the port option twice!\n' + + 'node_redis: WARNING: You passed the password option twice!\n' + ); + assert.strictEqual(Object.keys(options).length, 5); + assert.strictEqual(options.option.length, 3); + assert.strictEqual(options.host, 'localhost'); + assert.strictEqual(options.port, '123'); + assert.strictEqual(options.db, '3'); + assert.strictEqual(options.password, 'abc'); + }); + + it('should throw on duplicated, non-identical query options', function () { + try { + unifyOptions('//:abc@localhost:1234/3?port=123&password=abc'); + throw new Error('failed'); + } catch (err) { + assert.equal(err.message, 'The port option is added twice and does not match'); + } + }); + + it('should throw without protocol slashes', function () { + try { + unifyOptions('redis:abc@localhost:123/3?db=3&port=123&password=abc'); + throw new Error('failed'); + } catch (err) { + assert.equal(err.message, 'The redis url must begin with slashes "//" or contain slashes after the redis protocol'); + } + }); + + it("warns on protocol other than redis in the redis url", function () { + var text = ''; + var unhookIntercept = intercept(function (data) { + text += data; + return ''; + }); + var options = unifyOptions('http://abc'); + unhookIntercept(); + assert.strictEqual(Object.keys(options).length, 1); + assert.strictEqual(options.host, 'abc'); + assert.strictEqual(text, 'node_redis: WARNING: You passed "http" as protocol instead of the "redis" protocol!\n'); + }); + }); + + describe('no parameters or set to null / undefined', function () { + it('no parameters', function () { + var options = unifyOptions(); + assert.strictEqual(Object.keys(options).length, 1); + assert.strictEqual(options.host, undefined); + }); + + it('set to null', function () { + var options = unifyOptions(null, null); + assert.strictEqual(Object.keys(options).length, 1); + assert.strictEqual(options.host, null); + }); + + it('set to undefined', function () { + var options = unifyOptions(undefined, undefined); + assert.strictEqual(Object.keys(options).length, 1); + assert.strictEqual(options.host, undefined); + }); + }); + + describe('only an options object is passed', function () { + it('with options', function () { + var options = unifyOptions({ + option: true + }); + assert.strictEqual(Object.keys(options).length, 2); + assert.strictEqual(options.host, undefined); + assert.strictEqual(options.option, true); + }); + + it('without options', function () { + var options = unifyOptions({}); + assert.strictEqual(Object.keys(options).length, 1); + assert.strictEqual(options.host, undefined); + }); + + it('should throw with more parameters', function () { + try { + unifyOptions({ + option: true + }, undefined); + throw new Error('failed'); + } catch (err) { + assert.strictEqual(err.message, 'To many arguments passed to createClient. Please only pass the options object'); + } + }); + + it('including url as option', function () { + var options = unifyOptions({ + option: [1, 2, 3], + url: '//hm:abc@localhost:123/3' + }); + assert.strictEqual(Object.keys(options).length, 6); + assert.strictEqual(options.option.length, 3); + assert.strictEqual(options.host, 'localhost'); + assert.strictEqual(options.port, '123'); + assert.strictEqual(options.db, '3'); + assert.strictEqual(options.url, '//hm:abc@localhost:123/3'); + assert.strictEqual(options.password, 'abc'); + }); + }); + + describe('faulty data', function () { + it("throws on strange connection info", function () { + try { + unifyOptions(true); + throw new Error('failed'); + } catch (err) { + assert.equal(err.message, 'Unknown type of connection in createClient()'); + } + }); + }); +}); From 290bf1d65105ae800755c94fd689560eba3f5524 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 16:42:15 +0100 Subject: [PATCH 17/28] Small utils improvements Don't write "Error:" infront of errors --- lib/utils.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/utils.js b/lib/utils.js index 89d8590..069bf30 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -32,7 +32,8 @@ function replyToStrings(reply) { function print (err, reply) { if (err) { - console.log('Error: ' + err); + // A error always begins with Error: + console.log(err.toString()); } else { console.log('Reply: ' + reply); } From 5e8a87b4dcdb5161b938f567f6bcd37dd9869962 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 16:43:46 +0100 Subject: [PATCH 18/28] Improve createClient function to detect faulty input and throw --- lib/createClient.js | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/createClient.js b/lib/createClient.js index 3b14ef0..f97823e 100644 --- a/lib/createClient.js +++ b/lib/createClient.js @@ -11,6 +11,9 @@ module.exports = function createClient (port_arg, host_arg, options) { if (typeof host_arg === 'string') { host = host_arg; } else { + if (options && host_arg) { + throw new Error('Unknown type of connection in createClient()'); + } options = options || host_arg; } options = utils.clone(options); @@ -23,19 +26,22 @@ module.exports = function createClient (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]]] - if (parsed.hostname || parsed.slashes) { // The host might be an empty string + if (parsed.slashes) { // We require slashes if (parsed.auth) { options.password = parsed.auth.split(':')[1]; } - if (!/^([a-z]+:)?\/\//i.test(parsed.href)) { - throw new Error('Connection string must use the "redis:" protocol or begin with slashes //'); + if (parsed.protocol && parsed.protocol !== 'redis:') { + console.warn('node_redis: WARNING: You passed "' + parsed.protocol.substring(0, parsed.protocol.length - 1) + '" as protocol instead of the "redis" protocol!'); } if (parsed.pathname && parsed.pathname !== '/') { options.db = parsed.pathname.substr(1); } - options.host = parsed.hostname; - options.port = parsed.port; - + if (parsed.hostname) { + options.host = parsed.hostname; + } + if (parsed.port) { + options.port = parsed.port; + } if (parsed.search !== '') { var elem; for (elem in parsed.query) { // jshint ignore: line @@ -50,6 +56,8 @@ module.exports = function createClient (port_arg, host_arg, options) { options[elem] = parsed.query[elem]; } } + } else if (parsed.hostname) { + throw new Error('The redis url must begin with slashes "//" or contain slashes after the redis protocol'); } else { options.path = port_arg; } @@ -57,6 +65,10 @@ module.exports = function createClient (port_arg, host_arg, options) { } else if (typeof port_arg === 'object' || port_arg === undefined) { options = utils.clone(port_arg || options); options.host = options.host || host_arg; + + if (port_arg && arguments.length !== 1) { + throw new Error('To many arguments passed to createClient. Please only pass the options object'); + } } if (!options) { From cc540dbc3c90e24098be71cf180567b8001f4ce2 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 16:55:12 +0100 Subject: [PATCH 19/28] Implement retry_strategy and add more info to the reconnect event --- index.js | 54 ++++++++++++++++++++++++++++--------- test/connection.spec.js | 59 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 13 deletions(-) diff --git a/index.js b/index.js index 7b3db91..57d0c65 100644 --- a/index.js +++ b/index.js @@ -103,6 +103,7 @@ function RedisClient (options) { this.old_state = null; this.send_anyway = false; this.pipeline = 0; + this.times_connected = 0; this.options = options; // Init parser this.reply_parser = new Parser({ @@ -145,14 +146,15 @@ RedisClient.prototype.create_stream = function () { if (this.options.connect_timeout) { this.stream.setTimeout(this.connect_timeout, function () { self.retry_totaltime = self.connect_timeout; - self.connection_gone('timeout'); + self.connection_gone('timeout', new Error('Redis connection gone from timeout event')); }); } /* istanbul ignore next: travis does not work with stunnel atm. Therefor the tls tests are skipped on travis */ - var connect_event = this.options.tls ? "secureConnect" : "connect"; + var connect_event = this.options.tls ? 'secureConnect' : 'connect'; this.stream.once(connect_event, function () { - this.removeAllListeners("timeout"); + this.removeAllListeners('timeout'); + self.times_connected++; self.on_connect(); }); @@ -166,17 +168,18 @@ RedisClient.prototype.create_stream = function () { self.on_error(err); }); - /* istanbul ignore next: travis does not work with stunnel atm. Therefor the tls tests are skipped on travis */ + /* istanbul ignore next: difficult to test and not important as long as we keep this listener */ this.stream.on('clientError', function (err) { + debug('clientError occured'); self.on_error(err); }); this.stream.once('close', function () { - self.connection_gone('close'); + self.connection_gone('close', new Error('Stream connection closed')); }); this.stream.once('end', function () { - self.connection_gone('end'); + self.connection_gone('end', new Error('Stream connection ended')); }); this.stream.on('drain', function () { @@ -268,10 +271,14 @@ RedisClient.prototype.on_error = function (err) { this.connected = false; this.ready = false; - this.emit('error', err); + + // Only emit the error if the retry_stategy option is not set + if (!this.options.retry_strategy) { + this.emit('error', err); + } // 'error' events get turned into exceptions if they aren't listened for. If the user handled this error // then we should try to reconnect. - this.connection_gone('error'); + this.connection_gone('error', err); }; RedisClient.prototype.on_connect = function () { @@ -417,12 +424,15 @@ RedisClient.prototype.send_offline_queue = function () { this.offline_queue = new Queue(); }; -var retry_connection = function (self) { +var retry_connection = function (self, error) { debug('Retrying connection...'); self.emit('reconnecting', { delay: self.retry_delay, - attempt: self.attempts + attempt: self.attempts, + error: error, + times_connected: self.times_connected, + total_retry_time: self.retry_totaltime }); self.retry_totaltime += self.retry_delay; @@ -432,8 +442,7 @@ var retry_connection = function (self) { self.retry_timer = null; }; -RedisClient.prototype.connection_gone = function (why) { - var error; +RedisClient.prototype.connection_gone = function (why, error) { // If a retry is already in progress, just let that happen if (this.retry_timer) { return; @@ -469,6 +478,25 @@ RedisClient.prototype.connection_gone = function (why) { return; } + if (typeof this.options.retry_strategy === 'function') { + this.retry_delay = this.options.retry_strategy({ + attempt: this.attempts, + error: error, + total_retry_time: this.retry_totaltime, + times_connected: this.times_connected + }); + if (typeof this.retry_delay !== 'number') { + // Pass individual error through + if (this.retry_delay instanceof Error) { + error = this.retry_delay; + } + this.flush_and_error(error); + this.emit('error', error); + this.end(false); + return; + } + } + if (this.max_attempts !== 0 && this.attempts >= this.max_attempts || this.retry_totaltime >= this.connect_timeout) { var message = this.retry_totaltime >= this.connect_timeout ? 'connection timeout exceeded.' : @@ -502,7 +530,7 @@ RedisClient.prototype.connection_gone = function (why) { debug('Retry connection in ' + this.retry_delay + ' ms'); - this.retry_timer = setTimeout(retry_connection, this.retry_delay, this); + this.retry_timer = setTimeout(retry_connection, this.retry_delay, this, error); }; RedisClient.prototype.return_error = function (err) { diff --git a/test/connection.spec.js b/test/connection.spec.js index 906ae02..507089c 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -127,6 +127,65 @@ describe("connection tests", function () { client.stream.destroy(); }); }); + + it("retry_strategy used to reconnect with individual error", function (done) { + var text = ''; + var unhookIntercept = intercept(function (data) { + text += data; + return ''; + }); + var end = helper.callFuncAfter(done, 2); + client = redis.createClient({ + retry_strategy: function (options) { + if (options.total_retry_time > 150) { + client.set('foo', 'bar', function (err, res) { + assert.strictEqual(err.message, 'Connection timeout'); + end(); + }); + // Pass a individual error message to the error handler + return new Error('Connection timeout'); + } + return Math.min(options.attempt * 25, 200); + }, + max_attempts: 5, + retry_max_delay: 123, + port: 9999 + }); + + client.on('error', function(err) { + unhookIntercept(); + assert.strictEqual( + text, + 'node_redis: WARNING: You activated the retry_strategy and max_attempts at the same time. This is not possible and max_attempts will be ignored.\n' + + 'node_redis: 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.\n' + ); + assert.strictEqual(err.message, 'Connection timeout'); + assert(!err.code); + end(); + }); + }); + + it("retry_strategy used to reconnect", function (done) { + var end = helper.callFuncAfter(done, 2); + client = redis.createClient({ + retry_strategy: function (options) { + if (options.total_retry_time > 150) { + client.set('foo', 'bar', function (err, res) { + assert.strictEqual(err.code, 'ECONNREFUSED'); + end(); + }); + return false; + } + return Math.min(options.attempt * 25, 200); + }, + port: 9999 + }); + + client.on('error', function(err) { + assert.strictEqual(err.code, 'ECONNREFUSED'); + end(); + }); + }); }); describe("when not connected", function () { From 89209b8adc48cd15cc6784605c22a01b1edb3002 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 16:57:32 +0100 Subject: [PATCH 20/28] Handle very big pipelines without crashing --- index.js | 11 ++++++++--- test/multi.spec.js | 22 ++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 57d0c65..ba2a78e 100644 --- a/index.js +++ b/index.js @@ -106,7 +106,7 @@ function RedisClient (options) { this.times_connected = 0; this.options = options; // Init parser - this.reply_parser = new Parser({ + this.reply_parser = Parser({ returnReply: function (data) { self.return_reply(data); }, @@ -786,8 +786,13 @@ RedisClient.prototype.send_command = function (command, args, callback) { }; RedisClient.prototype.writeDefault = RedisClient.prototype.writeStrings = function (data) { - var command, str = ''; - while (command = this.pipeline_queue.shift()) { + var str = ''; + for (var command = this.pipeline_queue.shift(); command; command = this.pipeline_queue.shift()) { + // Write to stream if the string is bigger than 4mb. The biggest string may be Math.pow(2, 28) - 15 chars long + if (str.length + command.length > 4 * 1024 * 1024) { + this.stream.write(str); + str = ''; + } str += command; } this.should_buffer = !this.stream.write(str + data); diff --git a/test/multi.spec.js b/test/multi.spec.js index 766d579..181cdf8 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -70,6 +70,28 @@ describe("The 'multi' method", function () { }); }); + describe('pipeline limit', function () { + + it('do not exceed maximum string size', function (done) { + this.timeout(12000); // Windows tests on 0.10 are slow + // Triggers a RangeError: Invalid string length if not handled properly + client = redis.createClient(); + var multi = client.multi(); + var i = Math.pow(2, 28); + while (i > 0) { + i -= 10230; + multi.set('foo' + i, 'bar' + new Array(1024).join('1234567890')); + } + client.on('ready', function () { + multi.exec(function (err, res) { + assert.strictEqual(res.length, 26241); + }); + client.flushdb(done); + }); + }); + + }); + helper.allTests(function(parser, ip, args) { describe("using " + parser + " and " + ip, function () { From 19ea518b36ae55cef8fc9630e74df8974566e886 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 16:58:42 +0100 Subject: [PATCH 21/28] Do not emit ready if the slave is still syncing with master / master being down --- index.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index ba2a78e..d5c0c01 100644 --- a/index.js +++ b/index.js @@ -389,9 +389,15 @@ RedisClient.prototype.on_info_cmd = function (err, res) { } if (!this.server_info.loading || this.server_info.loading === '0') { - debug('Redis server ready.'); - this.on_ready(); - return; + // If the master_link_status exists but the link is not up, try again after 50 ms + if (this.server_info.master_link_status && this.server_info.master_link_status !== 'up') { + this.server_info.loading_eta_seconds = 0.05; + } else { + // Eta loading should change + debug('Redis server ready.'); + this.on_ready(); + return; + } } var retry_time = +this.server_info.loading_eta_seconds * 1000; From 575ad7357b5f51bc02209e7fecdedb26f7883024 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 17:11:02 +0100 Subject: [PATCH 22/28] Insert deprecation warnings and some minor refactoring --- README.md | 19 ++++---- index.js | 91 ++++++++++++++++++++++++++---------- test/commands/blpop.spec.js | 8 ++++ test/commands/get.spec.js | 1 - test/commands/getset.spec.js | 1 - test/connection.spec.js | 2 + test/lib/stunnel-process.js | 7 ++- test/node_redis.spec.js | 32 +++++++++---- test/utils.spec.js | 25 ++++++++++ 9 files changed, 140 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 1de7668..c48054b 100644 --- a/README.md +++ b/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,7 +162,7 @@ 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. @@ -170,12 +170,13 @@ If false is returned the stream had to buffer. 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 diff --git a/index.js b/index.js index d5c0c01..8f755cc 100644 --- a/index.js +++ b/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); diff --git a/test/commands/blpop.spec.js b/test/commands/blpop.spec.js index fc022a9..4105dcd 100644 --- a/test/commands/blpop.spec.js +++ b/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"); diff --git a/test/commands/get.spec.js b/test/commands/get.spec.js index 522872e..7f1f0cd 100644 --- a/test/commands/get.spec.js +++ b/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); diff --git a/test/commands/getset.spec.js b/test/commands/getset.spec.js index 11465bf..b39b028 100644 --- a/test/commands/getset.spec.js +++ b/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(); diff --git a/test/connection.spec.js b/test/connection.spec.js index 507089c..262face 100644 --- a/test/connection.spec.js +++ b/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); }); }); diff --git a/test/lib/stunnel-process.js b/test/lib/stunnel-process.js index f20bc22..f75049d 100644 --- a/test/lib/stunnel-process.js +++ b/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() { diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index ef5090c..527633e 100644 --- a/test/node_redis.spec.js +++ b/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); diff --git a/test/utils.spec.js b/test/utils.spec.js index d75e035..c886648 100644 --- a/test/utils.spec.js +++ b/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; From 535db5231edb64570eafe1629a24a708b2e6af63 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 17:13:31 +0100 Subject: [PATCH 23/28] Fix rename command not working together with key prefixes --- index.js | 10 ++++------ test/rename.spec.js | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/index.js b/index.js index 8f755cc..4263786 100644 --- a/index.js +++ b/index.js @@ -794,17 +794,15 @@ RedisClient.prototype.send_command = function (command, args, callback) { } this.command_queue.push(command_obj); - if (typeof this.options.rename_commands !== 'undefined' && this.options.rename_commands[command]) { - command = this.options.rename_commands[command]; - } if (this.options.prefix) { prefix_keys = commands.getKeyIndexes(command, args_copy); - i = prefix_keys.pop(); - while (i !== undefined) { + for (i = prefix_keys.pop(); i !== undefined; i = prefix_keys.pop()) { args_copy[i] = this.options.prefix + args_copy[i]; - i = prefix_keys.pop(); } } + if (typeof this.options.rename_commands !== 'undefined' && this.options.rename_commands[command]) { + command = this.options.rename_commands[command]; + } // Always use 'Multi bulk commands', but if passed any Buffer args, then do multiple writes, one for each arg. // This means that using Buffers in commands is going to be slower, so use Strings if you don't already have a Buffer. command_str = '*' + (len + 1) + '\r\n$' + command.length + '\r\n' + command + '\r\n'; diff --git a/test/rename.spec.js b/test/rename.spec.js index 9607210..e829323 100644 --- a/test/rename.spec.js +++ b/test/rename.spec.js @@ -27,7 +27,7 @@ describe("rename commands", function () { }); client.on('ready', function () { - done(); + client.flushdb(done); }); }); From c2e25a7f7157431597651ebf6866bdbe3c061134 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 17:14:57 +0100 Subject: [PATCH 24/28] Move lots code into separate files and split big functions party into smaller ones Also refactor small stuff here Removed the .send_anyway boolean and use .ready instead --- index.js | 735 ++++++++-------------------------------- package.json | 3 +- test/multi.spec.js | 2 +- test/node_redis.spec.js | 2 +- 4 files changed, 150 insertions(+), 592 deletions(-) diff --git a/index.js b/index.js index 4263786..b59e69d 100644 --- a/index.js +++ b/index.js @@ -2,20 +2,22 @@ var net = require('net'); var tls = require('tls'); -var URL = require('url'); var util = require('util'); var utils = require('./lib/utils'); var Queue = require('double-ended-queue'); var Command = require('./lib/command'); -var events = require('events'); +var EventEmitter = require('events'); var Parser = require('redis-parser'); var commands = require('redis-commands'); -var connection_id = 0; -var default_port = 6379; -var default_host = '127.0.0.1'; +var debug = require('./lib/debug'); +var unifyOptions = require('./lib/createClient'); + +// Newer Node.js versions > 0.10 return the EventEmitter right away and using .EventEmitter was deprecated +if (typeof EventEmitter !== 'function') { + EventEmitter = EventEmitter.EventEmitter; +} function noop () {} -function debug (msg) { if (exports.debug_mode) { console.error(msg); } } function handle_detect_buffers_reply (reply, command, buffer_args) { if (buffer_args === false) { @@ -35,15 +37,15 @@ exports.debug_mode = /\bredis\b/i.test(process.env.NODE_DEBUG); function RedisClient (options) { // Copy the options so they are not mutated options = utils.clone(options); - events.EventEmitter.call(this); + EventEmitter.call(this); var cnx_options = {}; var self = this; if (options.path) { cnx_options.path = options.path; this.address = options.path; } else { - cnx_options.port = +options.port || default_port; - cnx_options.host = options.host || default_host; + cnx_options.port = +options.port || 6379; + cnx_options.host = options.host || '127.0.0.1'; cnx_options.family = (!options.family && net.isIP(cnx_options.host)) || (options.family === 'IPv6' ? 6 : 4); this.address = cnx_options.host + ':' + cnx_options.port; } @@ -459,9 +461,12 @@ RedisClient.prototype.on_info_cmd = function (err, res) { RedisClient.prototype.ready_check = function () { var self = this; debug('Checking server ready state...'); + // Always fire this info command as first command even if other commands are already queued up + this.ready = true; this.info(function (err, res) { self.on_info_cmd(err, res); }); + this.ready = false; }; RedisClient.prototype.send_offline_queue = function () { @@ -599,11 +604,7 @@ RedisClient.prototype.return_error = function (err) { this.emit_idle(queue_len); - if (command_obj && command_obj.callback) { - command_obj.callback(err); - } else { - this.emit('error', err); - } + utils.callback_or_emit(this, command_obj && command_obj.callback, err); }; RedisClient.prototype.drain = function () { @@ -612,13 +613,78 @@ RedisClient.prototype.drain = function () { }; RedisClient.prototype.emit_idle = function (queue_len) { - if (this.pub_sub_mode === false && queue_len === 0) { + if (queue_len === 0 && this.pub_sub_mode === false) { this.emit('idle'); } }; +/* istanbul ignore next: this is a safety check that we should not be able to trigger */ +function queue_state_error (self, command_obj) { + var err = new Error('node_redis command queue state error. If you can reproduce this, please report it.'); + err.command_obj = command_obj; + self.emit('error', err); +} + +function monitor (self, reply) { + if (typeof reply !== 'string') { + reply = reply.toString(); + } + // If in monitoring mode only two commands are valid ones: AUTH and MONITOR wich reply with OK + var len = reply.indexOf(' '); + var timestamp = reply.slice(0, len); + var argindex = reply.indexOf('"'); + var args = reply.slice(argindex + 1, -1).split('" "').map(function (elem) { + return elem.replace(/\\"/g, '"'); + }); + self.emit('monitor', timestamp, args); +} + +function normal_reply (self, reply, command_obj) { + if (typeof command_obj.callback === 'function') { + if ('exec' !== command_obj.command) { + reply = self.handle_reply(reply, command_obj.command, command_obj.buffer_args); + } + command_obj.callback(null, reply); + } else { + debug('No callback for reply'); + } +} + +function return_pub_sub (self, reply, command_obj) { + if (reply instanceof Array) { + if ((!command_obj || command_obj.buffer_args === false) && !self.options.return_buffers) { + reply = utils.reply_to_strings(reply); + } + var type = reply[0].toString(); + + // TODO: Add buffer emiters (we have to get all pubsub messages as buffers back in that case) + if (type === 'message') { + self.emit('message', reply[1], reply[2]); // channcel, message + } else if (type === 'pmessage') { + self.emit('pmessage', reply[1], reply[2], reply[3]); // pattern, channcel, message + } else if (type === 'subscribe' || type === 'unsubscribe' || type === 'psubscribe' || type === 'punsubscribe') { + if (reply[2].toString() === '0') { + self.pub_sub_mode = false; + debug('All subscriptions removed, exiting pub/sub mode'); + } else { + self.pub_sub_mode = true; + } + // Subscribe commands take an optional callback and also emit an event, but only the first response is included in the callback + // TODO - document this or fix it so it works in a more obvious way + if (command_obj && typeof command_obj.callback === 'function') { + command_obj.callback(null, reply[1]); + } + self.emit(type, reply[1], reply[2]); // channcel, count + } else { + self.emit('error', new Error('subscriptions are active but got unknown reply type ' + type)); + } + } else if (!self.closing) { + self.emit('error', new Error('subscriptions are active but got an invalid reply: ' + reply)); + } +} + RedisClient.prototype.return_reply = function (reply) { - var command_obj, len, type, timestamp, argindex, args, queue_len; + var command_obj, type, queue_len; // If the 'reply' here is actually a message received asynchronously due to a // pubsub subscription, don't pop the command queue as we'll only be consuming @@ -638,84 +704,68 @@ RedisClient.prototype.return_reply = function (reply) { this.emit_idle(queue_len); if (command_obj && !command_obj.sub_command) { - if (typeof command_obj.callback === 'function') { - if ('exec' !== command_obj.command) { - reply = this.handle_reply(reply, command_obj.command, command_obj.buffer_args); - } - command_obj.callback(null, reply); - } else { - debug('No callback for reply'); - } + normal_reply(this, reply, command_obj); } else if (this.pub_sub_mode || command_obj && command_obj.sub_command) { - if (reply instanceof Array) { - if ((!command_obj || command_obj.buffer_args === false) && !this.options.return_buffers) { - reply = utils.reply_to_strings(reply); - } - type = reply[0].toString(); - - // TODO: Add buffer emiters (we have to get all pubsub messages as buffers back in that case) - if (type === 'message') { - this.emit('message', reply[1], reply[2]); // channel, message - } else if (type === 'pmessage') { - this.emit('pmessage', reply[1].toString(), reply[2], reply[3]); // pattern, channel, message - } else if (type === 'subscribe' || type === 'unsubscribe' || type === 'psubscribe' || type === 'punsubscribe') { - if (reply[2].toString() === '0') { - this.pub_sub_mode = false; - debug('All subscriptions removed, exiting pub/sub mode'); - } else { - this.pub_sub_mode = true; - } - // Subscribe commands take an optional callback and also emit an event, but only the first response is included in the callback - // TODO - document this or fix it so it works in a more obvious way - if (command_obj && typeof command_obj.callback === 'function') { - command_obj.callback(null, reply[1]); - } - this.emit(type, reply[1], reply[2]); // channel, count - } else { - this.emit('error', new Error('subscriptions are active but got unknown reply type ' + type)); - } - } else if (!this.closing) { - this.emit('error', new Error('subscriptions are active but got an invalid reply: ' + reply)); - } + return_pub_sub(this, reply, command_obj); } /* istanbul ignore else: this is a safety check that we should not be able to trigger */ else if (this.monitoring) { - if (typeof reply !== 'string') { - reply = reply.toString(); - } - // If in monitoring mode only two commands are valid ones: AUTH and MONITOR wich reply with OK - len = reply.indexOf(' '); - timestamp = reply.slice(0, len); - argindex = reply.indexOf('"'); - args = reply.slice(argindex + 1, -1).split('" "').map(function (elem) { - return elem.replace(/\\"/g, '"'); - }); - this.emit('monitor', timestamp, args); + monitor(this, reply); } else { - var err = new Error('node_redis command queue state error. If you can reproduce this, please report it.'); - err.command_obj = command_obj; - this.emit('error', err); + queue_state_error(this, command_obj); } }; +function handle_offline_command (self, command_obj) { + var command = command_obj.command; + var callback = command_obj.callback; + var err, msg; + if (self.closing || !self.enable_offline_queue) { + command = command.toUpperCase(); + if (!self.closing) { + if (self.stream.writable) { + msg = 'The connection is not yet established and the offline queue is deactivated.'; + } else { + msg = 'Stream not writeable.'; + } + } else { + msg = 'The connection has already been closed.'; + } + err = new Error(command + " can't be processed. " + msg); + err.command = command; + utils.reply_in_order(self, callback, err); + } else { + debug('Queueing ' + command + ' for next server connection.'); + self.offline_queue.push(command_obj); + } + self.should_buffer = true; +} + RedisClient.prototype.send_command = function (command, args, callback) { - var arg, command_obj, i, err, - stream = this.stream, - command_str = '', - buffer_args = false, - big_data = false, - prefix_keys, - len, args_copy; + var args_copy, arg, prefix_keys; + var i = 0; + var command_str = ''; + var len = 0; + var big_data = false; - if (typeof args === 'undefined') { - args = []; - } - if (callback && process.domain) { + if (process.domain && callback) { callback = process.domain.bind(callback); } - len = args.length; - args_copy = new Array(len); + var command_obj = new Command(command, args, callback); + + if (this.ready === false || this.stream.writable === false) { + // Handle offline commands right away + handle_offline_command(this, command_obj); + return false; // Indicate buffering + } + + if (typeof args === 'undefined') { + args_copy = []; + } else { + len = args.length; + args_copy = new Array(len); + } for (i = 0; i < len; i += 1) { if (typeof args[i] === 'string') { @@ -742,7 +792,8 @@ RedisClient.prototype.send_command = function (command, args, callback) { args_copy[i] = 'null'; // Backwards compatible :/ } else { args_copy[i] = args[i]; - buffer_args = true; + command_obj.buffer_args = true; + big_data = true; if (this.pipeline !== 0) { this.pipeline += 2; this.writeDefault = this.writeBuffers; @@ -756,34 +807,11 @@ RedisClient.prototype.send_command = function (command, args, callback) { ); args_copy[i] = 'undefined'; // Backwards compatible :/ } else { - args_copy[i] = String(args[i]); - } - } - - command_obj = new Command(command, args_copy, buffer_args, callback); - - // TODO: Replace send_anyway with `commands.hasFlag(command, 'loading') === false` as soon as pub_sub is handled in the result handler - if (this.ready === false && this.send_anyway === false || !stream.writable) { - if (this.closing || !this.enable_offline_queue) { - command = command.toUpperCase(); - if (!this.closing) { - var msg = !stream.writable ? - 'Stream not writeable.' : - 'The connection is not yet established and the offline queue is deactivated.'; - err = new Error(command + " can't be processed. " + msg); - } else { - err = new Error(command + " can't be processed. The connection has already been closed."); - } - err.command = command; - this.callback_emit_error(callback, err); - } else { - debug('Queueing ' + command + ' for next server connection.'); - this.offline_queue.push(command_obj); - this.should_buffer = true; + // Seems like numbers are converted fast using string concatenation + args_copy[i] = '' + args[i]; } - // Return false to signal buffering - return !this.should_buffer; } + args = null; if (command === 'subscribe' || command === 'psubscribe' || command === 'unsubscribe' || command === 'punsubscribe') { this.pub_sub_command(command_obj); // TODO: This has to be moved to the result handler @@ -807,7 +835,7 @@ RedisClient.prototype.send_command = function (command, args, callback) { // This means that using Buffers in commands is going to be slower, so use Strings if you don't already have a Buffer. command_str = '*' + (len + 1) + '\r\n$' + command.length + '\r\n' + command + '\r\n'; - if (buffer_args === false && big_data === false) { // Build up a string and send entire command in one write + if (big_data === false) { // Build up a string and send entire command in one write for (i = 0; i < len; i += 1) { arg = args_copy[i]; command_str += '$' + Buffer.byteLength(arg) + '\r\n' + arg + '\r\n'; @@ -820,7 +848,7 @@ RedisClient.prototype.send_command = function (command, args, callback) { for (i = 0; i < len; i += 1) { arg = args_copy[i]; - if (typeof arg !== 'object') { // string; number; boolean + if (typeof arg === 'string') { this.write('$' + Buffer.byteLength(arg) + '\r\n' + arg + '\r\n'); } else { // buffer this.write('$' + arg.length + '\r\n'); @@ -911,497 +939,26 @@ RedisClient.prototype.end = function (flush) { 'Please check the doku (https://github.com/NodeRedis/node_redis) and explictly use flush.' ); } - - this.stream._events = {}; - // Clear retry_timer if (this.retry_timer){ clearTimeout(this.retry_timer); this.retry_timer = null; } + this.stream.removeAllListeners(); this.stream.on('error', noop); - this.connected = false; this.ready = false; this.closing = true; return this.stream.destroySoon(); }; -function Multi(client, args) { - this._client = client; - this.queue = new Queue(); - var command, tmp_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); - if (Array.isArray(command)) { - this[command[0]].apply(this, command.slice(1).concat(tmp_args)); - } else { - this[command].apply(this, tmp_args); - } - } - } -} - -commands.list.forEach(function (command) { - - RedisClient.prototype[command.toUpperCase()] = RedisClient.prototype[command] = function () { - var arr, - len = 0, - callback, - i = 0; - if (arguments[0] instanceof Array) { - arr = arguments[0]; - callback = arguments[1]; // It does not matter if it exists or not - } else if (arguments[1] instanceof Array) { - len = arguments[1].length; - arr = new Array(len + 1); - arr[0] = arguments[0]; - for (; i < len; i += 1) { - arr[i + 1] = arguments[1][i]; - } - callback = arguments[2]; - } else { - len = arguments.length; - arr = new Array(len); - for (; i < len; i += 1) { - arr[i] = arguments[i]; - } - // The later should not be the average use case - if (typeof arr[i - 1] === 'function' || typeof arr[i - 1] === 'undefined') { - callback = arr.pop(); - } - } - return this.send_command(command, arr, callback); - }; - - Multi.prototype[command.toUpperCase()] = Multi.prototype[command] = function () { - var arr, - len = 0, - callback, - i = 0; - if (arguments[0] instanceof Array) { - arr = arguments[0]; - callback = arguments[1]; - } else if (arguments[1] instanceof Array) { - len = arguments[1].length; - arr = new Array(len + 1); - arr[0] = arguments[0]; - for (; i < len; i += 1) { - arr[i + 1] = arguments[1][i]; - } - callback = arguments[2]; - } else { - len = arguments.length; - arr = new Array(len); - for (; i < len; i += 1) { - arr[i] = arguments[i]; - } - // The later should not be the average use case - if (typeof arr[i - 1] === 'function' || typeof arr[i - 1] === 'undefined') { - callback = arr.pop(); - } - } - this.queue.push([command, arr, callback]); - return this; - }; -}); - -RedisClient.prototype.multi = RedisClient.prototype.MULTI = function (args) { - var multi = new Multi(this, args); - multi.exec = multi.EXEC = multi.exec_transaction; - return multi; -}; - -RedisClient.prototype.batch = RedisClient.prototype.BATCH = function (args) { - return new Multi(this, args); -}; - -// Store db in this.select_db to restore it on reconnect -RedisClient.prototype.select = RedisClient.prototype.SELECT = function (db, callback) { - var self = this; - return this.send_command('select', [db], function (err, res) { - if (err === null) { - self.selected_db = db; - } - if (typeof callback === 'function') { - callback(err, res); - } else if (err) { - self.emit('error', err); - } - }); +exports.createClient = function () { + return new RedisClient(unifyOptions.apply(null, arguments)); }; - -// Store info in this.server_info after each call -RedisClient.prototype.info = RedisClient.prototype.INFO = function (callback) { - var self = this; - this.send_anyway = true; - var tmp = this.send_command('info', [], function (err, res) { - if (res) { - var obj = {}; - var lines = res.toString().split('\r\n'); - var line, parts, sub_parts; - - for (var i = 0; i < lines.length; i++) { - parts = lines[i].split(':'); - if (parts[1]) { - if (parts[0].indexOf('db') === 0) { - sub_parts = parts[1].split(','); - obj[parts[0]] = {}; - while (line = sub_parts.pop()) { - line = line.split('='); - obj[parts[0]][line[0]] = +line[1]; - } - } else { - obj[parts[0]] = parts[1]; - } - } - } - obj.versions = []; - /* istanbul ignore else: some redis servers do not send the version */ - if (obj.redis_version) { - obj.redis_version.split('.').forEach(function (num) { - obj.versions.push(+num); - }); - } - // Expose info key/vals to users - self.server_info = obj; - } else { - self.server_info = {}; - } - if (typeof callback === 'function') { - callback(err, res); - } else if (err) { - self.emit('error', err); - } - }); - this.send_anyway = false; - return tmp; -}; - -RedisClient.prototype.callback_emit_error = function (callback, err) { - if (callback) { - setImmediate(function () { - callback(err); - }); - } else { - this.emit('error', err); - } -}; - -var noPasswordIsSet = /no password is set/; - -RedisClient.prototype.auth = function (pass, callback) { - var self = this; - debug('Sending auth to ' + self.address + ' id ' + self.connection_id); - - // Stash auth for connect and reconnect. - this.auth_pass = pass; - this.send_anyway = true; - var tmp = this.send_command('auth', [pass], function (err, res) { - if (err) { - if (noPasswordIsSet.test(err.message)) { - self.warn('Warning: Redis server does not require a password, but a password was supplied.'); - err = null; - res = 'OK'; - } else if (!callback) { - self.emit('error', err); - } - } - if (callback) { - callback(err, res); - } - }); - this.send_anyway = false; - return tmp; -}; - -RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function () { - var arr, - len = 0, - callback, - i = 0; - if (arguments[0] instanceof Array) { - arr = arguments[0]; - callback = arguments[1]; - } else if (arguments[1] instanceof Array) { - len = arguments[1].length; - arr = new Array(len + 1); - arr[0] = arguments[0]; - for (; i < len; i += 1) { - arr[i + 1] = arguments[1][i]; - } - callback = arguments[2]; - } else if (typeof arguments[1] === 'object' && (typeof arguments[2] === 'function' || typeof arguments[2] === 'undefined')) { - arr = [arguments[0]]; - for (var field in arguments[1]) { // jshint ignore: line - arr.push(field, arguments[1][field]); - } - callback = arguments[2]; - } else { - len = arguments.length; - arr = new Array(len); - for (; i < len; i += 1) { - arr[i] = arguments[i]; - } - // The later should not be the average use case - if (typeof arr[i - 1] === 'function' || typeof arr[i - 1] === 'undefined') { - callback = arr.pop(); - } - } - return this.send_command('hmset', arr, callback); -}; - -Multi.prototype.hmset = Multi.prototype.HMSET = function () { - var arr, - len = 0, - callback, - i = 0; - if (arguments[0] instanceof Array) { - arr = arguments[0]; - callback = arguments[1]; - } else if (arguments[1] instanceof Array) { - len = arguments[1].length; - arr = new Array(len + 1); - arr[0] = arguments[0]; - for (; i < len; i += 1) { - arr[i + 1] = arguments[1][i]; - } - callback = arguments[2]; - } else if (typeof arguments[1] === 'object' && (typeof arguments[2] === 'function' || typeof arguments[2] === 'undefined')) { - arr = [arguments[0]]; - for (var field in arguments[1]) { // jshint ignore: line - arr.push(field, arguments[1][field]); - } - callback = arguments[2]; - } else { - len = arguments.length; - arr = new Array(len); - for (; i < len; i += 1) { - arr[i] = arguments[i]; - } - // The later should not be the average use case - if (typeof arr[i - 1] === 'function' || typeof arr[i - 1] === 'undefined') { - callback = arr.pop(); - } - } - this.queue.push(['hmset', arr, callback]); - return this; -}; - -Multi.prototype.send_command = function (command, args, index, cb) { - var self = this; - this._client.send_command(command, args, function (err, reply) { - if (err) { - if (cb) { - cb(err); - } - err.position = index; - self.errors.push(err); - } - }); -}; - -Multi.prototype.exec_atomic = function (callback) { - if (this.queue.length < 2) { - return this.exec_batch(callback); - } - return this.exec(callback); -}; - -Multi.prototype.exec_transaction = function (callback) { - var self = this; - var len = this.queue.length; - this.errors = []; - this.callback = callback; - this._client.cork(len + 2); - this.wants_buffers = new Array(len); - this.send_command('multi', []); - // Drain queue, callback will catch 'QUEUED' or error - for (var index = 0; index < len; index++) { - var args = this.queue.get(index); - var command = args[0]; - var cb = args[2]; - // Keep track of who wants buffer responses: - if (this._client.options.detect_buffers) { - this.wants_buffers[index] = false; - for (var i = 0; i < args[1].length; i += 1) { - if (Buffer.isBuffer(args[1][i])) { - this.wants_buffers[index] = true; - break; - } - } - } - this.send_command(command, args[1], index, cb); - } - - this._client.send_command('exec', [], function(err, replies) { - self.execute_callback(err, replies); - }); - this._client.uncork(); - this._client.writeDefault = this._client.writeStrings; - return !this._client.should_buffer; -}; - -Multi.prototype.execute_callback = function (err, replies) { - var i = 0, args; - - if (err) { - // The errors would be circular - var connection_error = ['CONNECTION_BROKEN', 'UNCERTAIN_STATE'].indexOf(err.code) !== -1; - err.errors = connection_error ? [] : this.errors; - if (this.callback) { - this.callback(err); - // Exclude connection errors so that those errors won't be emitted twice - } else if (!connection_error) { - this._client.emit('error', err); - } - return; - } - - if (replies) { - while (args = this.queue.shift()) { - if (replies[i] instanceof Error) { - var match = replies[i].message.match(utils.err_code); - // LUA script could return user errors that don't behave like all other errors! - if (match) { - replies[i].code = match[1]; - } - replies[i].command = args[0].toUpperCase(); - if (typeof args[args.length - 1] === 'function') { - args[args.length - 1](replies[i]); - } - } else { - // If we asked for strings, even in detect_buffers mode, then return strings: - replies[i] = this._client.handle_reply(replies[i], args[0], this.wants_buffers[i]); - if (typeof args[args.length - 1] === 'function') { - args[args.length - 1](null, replies[i]); - } - } - i++; - } - } - - if (this.callback) { - this.callback(null, replies); - } -}; - -Multi.prototype.callback = function (cb, i) { - var self = this; - return function (err, res) { - if (err) { - self.results[i] = err; - // Add the position to the error - self.results[i].position = i; - } else { - self.results[i] = res; - } - cb(err, res); - }; -}; - -Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = function (callback) { - var len = this.queue.length; - var self = this; - var index = 0; - var args; - var args_len = 1; - var callback_without_own_cb = function (err, res) { - if (err) { - self.results.push(err); - // Add the position to the error - var i = self.results.length - 1; - self.results[i].position = i; - } else { - self.results.push(res); - } - // Do not emit an error here. Otherwise each error would result in one emit. - // The errors will be returned in the result anyway - }; - var last_callback = function (cb) { - return function (err, res) { - cb(err, res); - callback(null, self.results); - }; - }; - if (len === 0) { - if (callback) { - // The execution order won't be obtained in this case - setImmediate(function () { - callback(null, []); - }); - } - return true; - } - this.results = []; - this._client.cork(len); - while (args = this.queue.shift()) { - var command = args[0]; - var cb; - args_len = args[1].length - 1; - if (typeof args[2] === 'function') { - cb = this.callback(args[2], index); - } else { - cb = callback_without_own_cb; - } - if (callback && index === len - 1) { - cb = last_callback(cb); - } - this._client.send_command(command, args[1], cb); - index++; - } - this._client.uncork(); - this._client.writeDefault = this._client.writeStrings; - return !this._client.should_buffer; -}; - -var createClient = function (port_arg, host_arg, options) { - if (typeof port_arg === 'number' || typeof port_arg === 'string' && /^\d+$/.test(port_arg)) { - options = utils.clone(options); - options.host = host_arg; - options.port = port_arg; - } else if (typeof port_arg === 'string' || port_arg && port_arg.url) { - options = utils.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]]] - if (parsed.hostname || parsed.slashes) { // The host might be an empty string - if (parsed.auth) { - options.password = parsed.auth.split(':')[1]; - } - if (!/^([a-z]+:)?\/\//i.test(parsed.href)) { - throw new Error('Connection string must use the "redis:" protocol or begin with slashes //'); - } - 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]; - } - } - } else { - options.path = port_arg; - } - } else if (typeof port_arg === 'object' || port_arg === undefined) { - options = utils.clone(port_arg || options); - options.host = options.host || host_arg; - } - if (!options) { - throw new Error('Unknown type of connection in createClient()'); - } - return new RedisClient(options); -}; - -exports.createClient = createClient; exports.RedisClient = RedisClient; exports.print = utils.print; -exports.Multi = Multi; +exports.Multi = require('./lib/multi'); + +// Add all redis commands to the client +require('./lib/individualCommands'); +require('./lib/commands'); diff --git a/package.json b/package.json index ac010b7..7760c7c 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,7 @@ "dependencies": { "double-ended-queue": "^2.1.0-0", "redis-commands": "^1.0.1", - "redis-parser": "^1.0.0" + "redis-parser": "^1.1.0" }, "engines": { "node": ">=0.10.0" @@ -33,6 +33,7 @@ "devDependencies": { "bluebird": "^3.0.2", "coveralls": "^2.11.2", + "intercept-stdout": "~0.1.2", "jshint": "^2.8.0", "metrics": "^0.1.9", "mocha": "^2.3.2", diff --git a/test/multi.spec.js b/test/multi.spec.js index 181cdf8..334d0ae 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -120,7 +120,7 @@ describe("The 'multi' method", function () { assert(err.message.match(/The connection has already been closed/)); done(); }); - assert.strictEqual(notBuffering, true); + assert.strictEqual(notBuffering, false); }); it("reports an error if promisified", function () { diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 527633e..45e86a5 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -747,7 +747,7 @@ describe("The node_redis client", function () { enable_offline_queue: false }); client.on('ready', function () { - client.stream.writable = false; + client.stream.destroy(); client.set('foo', 'bar', function (err, res) { assert.strictEqual(err.message, "SET can't be processed. Stream not writeable."); done(); From 2684fdbb4d4f8cfa0c372911ca641cfc0477d847 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 17:14:00 +0100 Subject: [PATCH 25/28] Update changelog --- changelog.md | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/changelog.md b/changelog.md index 5c1a01b..85d59db 100644 --- a/changelog.md +++ b/changelog.md @@ -1,7 +1,13 @@ Changelog ========= -## v.2.5.0-0 - xx Jan, 2015 +## v.2.5.0-1 - 01 Mar, 2015 + +This is a big release with some substaintual underlining changes. Therefor this is released as a pre-release and I encourage anyone who's able to, to test this out. + +It took way to long to release this one and the next release cycles will be shorter again. + +This release is also going to deprecate a couple things to prepare for a future v.3 (it'll still take a while to v.3). Features @@ -14,7 +20,16 @@ Features - 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 +- Added a `warning` emitter that receives node_redis warnings like auth not required and deprecation messages +- Added a `retry_strategy` option that replaces all reconnect options +- The reconnecting event from now on also receives: + - The error message why the reconnect happend (params.error) + - The amount of times the client was connected (params.times_connected) + - The total reconnecting time since the last time connected (params.total_retry_time) +- Always respect the command execution order no matter if the reply could be returned sync or not (former exceptions: [#937](https://github.com/NodeRedis/node_redis/issues/937#issuecomment-167525939)) +- redis.createClient is now checking input values stricter and detects more faulty input +- Started refactoring internals into individual modules +- Pipelining speed improvements Bugfixes @@ -26,6 +41,9 @@ Bugfixes - Fixed redis url not accepting the protocol being omitted or protocols other than the redis protocol for convienence - Fixed parsing the db keyspace even if the first database does not begin with a zero - Fixed handling of errors occuring while receiving pub sub messages +- Fixed huge string pipelines crashing NodeJS (Pipeline size above 256mb) +- Fixed rename_commands and prefix option not working together +- Fixed ready being emitted to early in case a slave is still syncing / master down Deprecations @@ -38,9 +56,13 @@ Deprecations - 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 - From version 3.0.0 on node_redis might not return true / false as a return value anymore. Please rely on client.should_buffer instead -- The socket_nodelay option is deprecated and will be removed in v.3.0.0 +- The `socket_nodelay` option is deprecated and will be removed in v.3.0.0 - If you want to buffer commands you should use [.batch or .multi](./README.md) instead. This is necessary to reduce the amount of different options and this is very likely reducing your throughput if set to false. - If you are sure you want to activate the NAGLE algorithm you can still activate it by using client.stream.setNoDelay(false) +- The `max_attempts` option is deprecated and will be removed in v.3.0.0. Please use the `retry_strategy` instead +- The `retry_max_delay` option is deprecated and will be removed in v.3.0.0. Please use the `retry_strategy` instead +- The drain event is deprecated and will be removed in v.3.0.0. Please listen to the stream drain event instead +- The idle event is deprecated and will likely be removed in v.3.0.0. If you rely on this feature please open a new ticket in node_redis with your use case - Redis < v. 2.6.11 is not supported anymore and will not work in all cases. Please update to a newer redis version - Removed non documented command syntax (adding the callback to an arguments array instead of passing it as individual argument) From 31a2d843e8070be735a914cc231b113d90f9452a Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 17:56:04 +0100 Subject: [PATCH 26/28] Add slave ready test --- test/conect.slave.spec.js | 92 +++++++++++++++++++++++++++++++++++++++ test/conf/slave.conf | 6 +++ 2 files changed, 98 insertions(+) create mode 100644 test/conect.slave.spec.js create mode 100644 test/conf/slave.conf diff --git a/test/conect.slave.spec.js b/test/conect.slave.spec.js new file mode 100644 index 0000000..0080b23 --- /dev/null +++ b/test/conect.slave.spec.js @@ -0,0 +1,92 @@ +'use strict'; + +var assert = require("assert"); +var config = require("./lib/config"); +var helper = require('./helper'); +var RedisProcess = require("./lib/redis-process"); +var rp; +var path = require('path'); +var redis = config.redis; + +describe('master slave sync', function () { + var master = null; + var slave = null; + + before(function (done) { + helper.stopRedis(function () { + helper.startRedis('./conf/password.conf', done); + }); + }); + + before(function (done) { + master = redis.createClient({ + password: 'porkchopsandwiches' + }); + var multi = master.multi(); + var i = 0; + while (i < 1000) { + i++; + // Write some data in the redis instance, so there's something to sync + multi.set('foo' + i, 'bar' + new Array(500).join(Math.random())); + } + multi.exec(done); + }); + + it("sync process and no master should delay ready being emitted for slaves", function (done) { + if (helper.redisProcess().spawnFailed()) this.skip(); + + var port = 6381; + var firstInfo; + slave = redis.createClient({ + port: port, + retry_strategy: function (options) { + // Try to reconnect in very small intervals to catch the master_link_status down before the sync completes + return 10; + } + }); + + var tmp = slave.info.bind(slave); + var i = 0; + slave.info = function (err, res) { + i++; + tmp(err, res); + if (!firstInfo || Object.keys(firstInfo).length === 0) { + firstInfo = slave.server_info; + } + }; + + slave.on('connect', function () { + assert.strictEqual(i, 0); + }); + + var end = helper.callFuncAfter(done, 2); + + slave.on('ready', function () { + assert.strictEqual(this.server_info.master_link_status, 'up'); + assert.strictEqual(firstInfo.master_link_status, 'down'); + assert(i > 1); + this.get('foo300', function (err, res) { + assert.strictEqual(res.substr(0, 3), 'bar'); + end(err); + }); + }); + + RedisProcess.start(function (err, _rp) { + rp = _rp; + end(err); + }, path.resolve(__dirname, './conf/slave.conf'), port); + }); + + after(function (done) { + var end = helper.callFuncAfter(done, 3); + rp.stop(end); + slave.end(true); + master.flushdb(function (err) { + end(err); + master.end(true); + }); + helper.stopRedis(function () { + helper.startRedis('./conf/redis.conf', end); + }); + }); +}); diff --git a/test/conf/slave.conf b/test/conf/slave.conf new file mode 100644 index 0000000..61ea50d --- /dev/null +++ b/test/conf/slave.conf @@ -0,0 +1,6 @@ +port 6381 +bind ::1 127.0.0.1 +unixsocket /tmp/redis6381.sock +unixsocketperm 755 +slaveof localhost 6379 +masterauth porkchopsandwiches From e48e1e845f06b0e469e98544f78bd37b40968a2f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 1 Mar 2016 19:55:26 +0100 Subject: [PATCH 27/28] Windows fixes Skip redis process spawn on windows for now --- test/auth.spec.js | 6 ++++++ test/conect.slave.spec.js | 7 +++++++ test/connection.spec.js | 25 +++++++++++++------------ test/helper.js | 30 +++++++++++++----------------- test/multi.spec.js | 2 +- test/rename.spec.js | 8 ++++++++ test/tls.spec.js | 11 +++-------- 7 files changed, 51 insertions(+), 38 deletions(-) diff --git a/test/auth.spec.js b/test/auth.spec.js index 5f18f35..61e185f 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -5,6 +5,11 @@ var config = require("./lib/config"); var helper = require('./helper'); var redis = config.redis; +if (process.platform === 'win32') { + // TODO: Fix redis process spawn on windows + return; +} + describe("client authentication", function () { before(function (done) { helper.stopRedis(function () { @@ -237,6 +242,7 @@ describe("client authentication", function () { }); after(function (done) { + if (helper.redisProcess().spawnFailed()) return done(); helper.stopRedis(function () { helper.startRedis('./conf/redis.conf', done); }); diff --git a/test/conect.slave.spec.js b/test/conect.slave.spec.js index 0080b23..28f56d9 100644 --- a/test/conect.slave.spec.js +++ b/test/conect.slave.spec.js @@ -8,6 +8,11 @@ var rp; var path = require('path'); var redis = config.redis; +if (process.platform === 'win32') { + // TODO: Fix redis process spawn on windows + return; +} + describe('master slave sync', function () { var master = null; var slave = null; @@ -19,6 +24,7 @@ describe('master slave sync', function () { }); before(function (done) { + if (helper.redisProcess().spawnFailed()) return done(); master = redis.createClient({ password: 'porkchopsandwiches' }); @@ -78,6 +84,7 @@ describe('master slave sync', function () { }); after(function (done) { + if (helper.redisProcess().spawnFailed()) return done(); var end = helper.callFuncAfter(done, 3); rp.stop(end); slave.end(true); diff --git a/test/connection.spec.js b/test/connection.spec.js index 262face..8eb38f4 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -257,20 +257,21 @@ describe("connection tests", function () { client.once('ready', done); }); - if (process.platform !== 'win32') { - it("connect with path provided in the options object", function (done) { - client = redis.createClient({ - path: '/tmp/redis.sock', - parser: parser, - connect_timeout: 1000 - }); + it("connect with path provided in the options object", function (done) { + if (process.platform === 'win32') { + this.skip(); + } + client = redis.createClient({ + path: '/tmp/redis.sock', + parser: parser, + connect_timeout: 1000 + }); - var end = helper.callFuncAfter(done, 2); + var end = helper.callFuncAfter(done, 2); - client.once('ready', end); - client.set('foo', 'bar', end); - }); - } + client.once('ready', end); + client.set('foo', 'bar', end); + }); it("connects correctly with args", function (done) { client = redis.createClient.apply(redis.createClient, args); diff --git a/test/helper.js b/test/helper.js index 7408577..dd30e52 100644 --- a/test/helper.js +++ b/test/helper.js @@ -15,21 +15,6 @@ function startRedis (conf, done, port) { }, path.resolve(__dirname, conf), port); } -function startStunnel(done) { - StunnelProcess.start(function (err, _stunnel_process) { - stunnel_process = _stunnel_process; - return done(err); - }, path.resolve(__dirname, './conf')); -} - -function stopStunnel(done) { - if (stunnel_process) { - StunnelProcess.stop(stunnel_process, done); - } else { - done(); - } -} - // don't start redis every time we // include this helper file! if (!process.env.REDIS_TESTS_STARTED) { @@ -52,8 +37,19 @@ module.exports = { rp.stop(done); }, startRedis: startRedis, - stopStunnel: stopStunnel, - startStunnel: startStunnel, + stopStunnel: function (done) { + if (stunnel_process) { + StunnelProcess.stop(stunnel_process, done); + } else { + done(); + } + }, + startStunnel: function (done) { + StunnelProcess.start(function (err, _stunnel_process) { + stunnel_process = _stunnel_process; + return done(err); + }, path.resolve(__dirname, './conf')); + }, isNumber: function (expected, done) { return function (err, results) { assert.strictEqual(null, err, "expected " + expected + ", got error: " + err); diff --git a/test/multi.spec.js b/test/multi.spec.js index 334d0ae..bd6f498 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -73,7 +73,7 @@ describe("The 'multi' method", function () { describe('pipeline limit', function () { it('do not exceed maximum string size', function (done) { - this.timeout(12000); // Windows tests on 0.10 are slow + this.timeout(25000); // Windows tests are horribly slow // Triggers a RangeError: Invalid string length if not handled properly client = redis.createClient(); var multi = client.multi(); diff --git a/test/rename.spec.js b/test/rename.spec.js index e829323..0d3aa0c 100644 --- a/test/rename.spec.js +++ b/test/rename.spec.js @@ -5,6 +5,11 @@ var config = require("./lib/config"); var helper = require('./helper'); var redis = config.redis; +if (process.platform === 'win32') { + // TODO: Fix redis process spawn on windows + return; +} + describe("rename commands", function () { before(function (done) { helper.stopRedis(function () { @@ -18,6 +23,7 @@ describe("rename commands", function () { var client = null; beforeEach(function(done) { + if (helper.redisProcess().spawnFailed()) return done(); client = redis.createClient({ rename_commands: { set: '807081f5afa96845a02816a28b7258c3', @@ -32,6 +38,7 @@ describe("rename commands", function () { }); afterEach(function () { + if (helper.redisProcess().spawnFailed()) return; client.end(true); }); @@ -132,6 +139,7 @@ describe("rename commands", function () { }); after(function (done) { + if (helper.redisProcess().spawnFailed()) return done(); helper.stopRedis(function () { helper.startRedis('./conf/redis.conf', done); }); diff --git a/test/tls.spec.js b/test/tls.spec.js index 20cd246..8caddf8 100644 --- a/test/tls.spec.js +++ b/test/tls.spec.js @@ -32,26 +32,21 @@ describe("TLS connection tests", function () { skip = true; console.warn('\nTravis does not support stunnel right now. Skipping tests.\nCheck: https://github.com/travis-ci/apt-package-whitelist/issues/403\n'); } - if (skip) { - done(); - return; - } + if (skip) return done(); helper.stopStunnel(function () { helper.startStunnel(done); }); }); after(function (done) { - if (skip) { - done(); - return; - } + if (skip) return done(); helper.stopStunnel(done); }); var client; afterEach(function () { + if (skip) return; client.end(true); }); From 7c9c5e2693060faaaaec2247664940dee54c0388 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 7 Mar 2016 02:34:30 +0100 Subject: [PATCH 28/28] Update readme to include more details about retry_strategy and backpressure --- README.md | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index c48054b..15ef775 100644 --- a/README.md +++ b/README.md @@ -216,10 +216,11 @@ limits total amount of connection tries. Setting this to 1 will prevent any reco * `tls`: an object containing options to pass to [tls.connect](http://nodejs.org/api/tls.html#tls_tls_connect_port_host_options_callback), to set up a TLS connection to Redis (if, for example, it is set up to be accessible via a tunnel). * `prefix`: *null*; pass a string to prefix all used keys with that string as prefix e.g. 'namespace:test' +* `retry_strategy`: *function*; pass a function that receives a options object as parameter including the retry `attempt`, the `total_retry_time` indicating how much time passed since the last time connected, the `error` why the connection was lost and the number of `times_connected` in total. If you return a number from this function, the retry will happen exactly after that time in milliseconds. If you return a non-number no further retry is going to happen and all offline commands are flushed with errors. Return a error to return that specific error to all offline commands. Check out the example too. ```js -var redis = require("redis"), - client = redis.createClient({detect_buffers: true}); +var redis = require("redis"); +var client = redis.createClient({detect_buffers: true}); client.set("foo_rand000000000000", "OK"); @@ -235,6 +236,28 @@ client.get(new Buffer("foo_rand000000000000"), function (err, reply) { client.end(); ``` +retry_strategy example +```js +var client = redis.createClient({ + retry_strategy: function (options) { + if (options.error.code === 'ECONNREFUSED') { + // End reconnecting on a specific error and flush all commands with a individual error + return new Error('The server refused the connection'); + } + if (options.total_retry_time > 1000 * 60 * 60) { + // End reconnecting after a specific timeout and flush all commands with a individual error + return new Error('Retry time exhausted'); + } + if (options.times_connected > 10) { + // End reconnecting with built in error + return undefined; + } + // reconnect after + return Math.max(options.attempt * 100, 3000); + } +}); +``` + ## client.auth(password[, callback]) When connecting to a Redis server that requires authentication, the `AUTH` command must be sent as the @@ -246,6 +269,13 @@ NOTE: Your call to `client.auth()` should not be inside the ready handler. If you are doing this wrong, `client` will emit an error that looks something like this `Error: Ready check failed: ERR operation not permitted`. +## backpressure + +### stream + +The client exposed the used [stream](https://nodejs.org/api/stream.html) in `client.stream` and if the stream or client had to [buffer](https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback) the command in `client.should_buffer`. +In combination this can be used to implement backpressure by checking the buffer state before sending a command and listening to the stream [drain](https://nodejs.org/api/stream.html#stream_event_drain) event. + ## client.end(flush) Forcibly close the connection to the Redis server. Note that this does not wait until all replies have been parsed. @@ -272,7 +302,7 @@ client.get("foo_rand000000000000", function (err, reply) { }); ``` -`client.end()` without the flush parameter should not be used in production! +`client.end()` without the flush parameter should NOT be used in production! ## client.unref()