From dfb1d93a4fd59b21843fae9ab8af4b85efcbf8aa Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 03:42:25 +0200 Subject: [PATCH 01/29] Explicitly ask for the platform in the issue template --- .github/ISSUE_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE.md index fb90edf57ad..42f2d3eeb61 100644 --- a/.github/ISSUE_TEMPLATE.md +++ b/.github/ISSUE_TEMPLATE.md @@ -9,7 +9,7 @@ to a small test case, but it's highly appreciated to have as much data as possib Thank you!_ * **Version**: What node_redis and what redis version is the issue happening on? -* **Platform**: What platform / version? (For example Node.js 0.10 or Node.js 5.7.0) +* **Platform**: What platform / version? (For example Node.js 0.10 or Node.js 5.7.0 on Windows 7 / Ubuntu 15.10 / Azure) * **Description**: Description of your issue, stack traces from errors and code that reproduces the issue [gitter]: https://gitter.im/NodeRedis/node_redis?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge \ No newline at end of file From 4e529814d40416250e3f19d9c702d373787e00ee Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 03:46:38 +0200 Subject: [PATCH 02/29] Small Readme fixes --- README.md | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index e91da2973bf..8bbbd501df3 100644 --- a/README.md +++ b/README.md @@ -12,9 +12,7 @@ Install with: npm install redis -## Usage - -Simple example, included as `examples/simple.js`: +## Usage Example ```js var redis = require("redis"), @@ -83,7 +81,7 @@ return client.multi().get('foo').execAsync().then(function(res) { Each Redis command is exposed as a function on the `client` object. All functions take either an `args` Array plus optional `callback` Function or a variable number of individual arguments followed by an optional callback. -Here are examples how to use the api: +Examples: ```js client.hmset(["key", "test keys 1", "test val 1", "test keys 2", "test val 2"], function (err, res) {}); @@ -182,7 +180,7 @@ __Tip:__ If the Redis server runs on the same machine as the client consider usi | host | 127.0.0.1 | IP address of the Redis server | | port | 6379 | Port of the Redis server | | path | null | The UNIX socket string of the Redis server | -| url | null | The URL of the Redis server. Format: `[redis:]//[user][:password@][host][:port][/db-number][?db=db-number[&password=bar[&option=value]]]` (More info avaliable at [IANA](http://www.iana.org/assignments/uri-schemes/prov/redis)). | +| url | null | The URL of the Redis server. Format: `[redis:]//[[user][:password@]][host][:port][/db-number][?db=db-number[&password=bar[&option=value]]]` (More info avaliable at [IANA](http://www.iana.org/assignments/uri-schemes/prov/redis)). | | parser | hiredis | If hiredis is not installed, automatic fallback to the built-in javascript parser | | string_numbers | null | Set to `true`, `node_redis` will return Redis number values as Strings instead of javascript Numbers. Useful if you need to handle big numbers (above `Number.MAX_SAFE_INTEGER === 2^53`). Hiredis is incapable of this behavior, so setting this option to `true` will result in the built-in javascript parser being used no matter the value of the `parser` option. | | return_buffers | false | If set to `true`, then all replies will be sent to callbacks as Buffers instead of Strings. | @@ -363,7 +361,7 @@ client.HMSET(key1, "0123456789", "abcdefghij", "some manner of key", "a type of ## Publish / Subscribe -Here is a simple example of the API for publish / subscribe. This program opens two +Example of the publish / subscribe API. This program opens two client connections, subscribes to a channel on one of them, and publishes to that channel on the other: @@ -539,7 +537,7 @@ across all client connections, including from other client libraries and other c A `monitor` event is going to be emitted for every command fired from any client connected to the server including the monitoring client itself. The callback for the `monitor` event takes a timestamp from the Redis server, an array of command arguments and the raw monitoring string. -Here is a simple example: +Example: ```js var client = require("redis").createClient(); From a11e0c5ff9f47da89e763cfe46a9bcd6f02b5098 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 03:46:51 +0200 Subject: [PATCH 03/29] Don't expose internal variables --- README.md | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 8bbbd501df3..d2b82a0ee2a 100644 --- a/README.md +++ b/README.md @@ -613,27 +613,16 @@ All commands are sent as multi-bulk commands. `args` can either be an Array of a Boolean tracking the state of the connection to the Redis server. -## client.command_queue.length +## client.command_queue_length The number of commands that have been sent to the Redis server but not yet replied to. You can use this to enforce some kind of maximum queue depth for commands while connected. -Don't mess with `client.command_queue` though unless you really know what you are doing. - -## client.offline_queue.length +## client.offline_queue_length The number of commands that have been queued up for a future connection. You can use this to enforce some kind of maximum queue depth for pre-connection commands. -## client.retry_delay - -Current delay in milliseconds before a connection retry will be attempted. This starts at `200`. - -## client.retry_backoff - -Multiplier for future retry timeouts. This should be larger than 1 to add more time between retries. -Defaults to 1.7. The default initial connection retry is 200, so the second retry will be 340, followed by 578, etc. - ### Commands with Optional and Keyword arguments This applies to anything that uses an optional `[WITHSCORES]` or `[LIMIT offset count]` in the [redis.io/commands](http://redis.io/commands) documentation. From 228573b8d78c7bb3d00c28413093422428b27325 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 03:48:33 +0200 Subject: [PATCH 04/29] Support __proto__ and similar as object attribute in hgetall --- lib/rawObject.js | 8 ++++++ lib/utils.js | 4 ++- test/commands/hgetall.spec.js | 4 +-- test/pubsub.spec.js | 46 +++++++++-------------------------- 4 files changed, 25 insertions(+), 37 deletions(-) create mode 100644 lib/rawObject.js diff --git a/lib/rawObject.js b/lib/rawObject.js new file mode 100644 index 00000000000..26376fc3608 --- /dev/null +++ b/lib/rawObject.js @@ -0,0 +1,8 @@ +'use strict'; + +// Using a predefined object with this prototype is faster than calling `Object.create(null)` directly +// This is needed to make sure `__proto__` and similar reserved words can be used +function RawObject () {} +RawObject.prototype = Object.create(null); + +module.exports = RawObject; diff --git a/lib/utils.js b/lib/utils.js index 61e9a64f755..4093b61dcbd 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -1,5 +1,7 @@ 'use strict'; +var RawObject = require('./rawObject'); + // 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) { @@ -7,7 +9,7 @@ function replyToObject (reply) { if (reply.length === 0 || !(reply instanceof Array)) { return null; } - var obj = {}; + var obj = new RawObject(); for (var i = 0; i < reply.length; i += 2) { obj[reply[i].toString('binary')] = reply[i + 1]; } diff --git a/test/commands/hgetall.spec.js b/test/commands/hgetall.spec.js index d345b13c922..55a0e247803 100644 --- a/test/commands/hgetall.spec.js +++ b/test/commands/hgetall.spec.js @@ -22,10 +22,10 @@ describe("The 'hgetall' method", function () { }); it('handles simple keys and values', function (done) { - client.hmset(['hosts', 'mjr', '1', 'another', '23', 'home', '1234'], helper.isString('OK')); + client.hmset(['hosts', '__proto__', '1', 'another', '23', 'home', '1234'], helper.isString('OK')); client.HGETALL(['hosts'], function (err, obj) { assert.strictEqual(3, Object.keys(obj).length); - assert.strictEqual('1', obj.mjr.toString()); + assert.strictEqual('1', obj.__proto__.toString()); // eslint-disable-line no-proto assert.strictEqual('23', obj.another.toString()); assert.strictEqual('1234', obj.home.toString()); return done(err); diff --git a/test/pubsub.spec.js b/test/pubsub.spec.js index b24c1899dc9..9cb388b03e1 100644 --- a/test/pubsub.spec.js +++ b/test/pubsub.spec.js @@ -206,49 +206,27 @@ describe('publish/subscribe', function () { sub.subscribe(channel); }); - it('handles SUBSCRIBE_CLOSE_RESUBSCRIBE', function (done) { + it('subscribe; close; resubscribe with prototype inherited property names', function (done) { var count = 0; - /* Create two clients. c1 subscribes to two channels, c2 will publish to them. - c2 publishes the first message. - c1 gets the message and drops its connection. It must resubscribe itself. - When it resubscribes, c2 publishes the second message, on the same channel - c1 gets the message and drops its connection. It must resubscribe itself, again. - When it resubscribes, c2 publishes the third message, on the second channel - c1 gets the message and drops its connection. When it reconnects, the test ends. - */ + var channels = ['__proto__', 'channel 2']; + var msg = ['hi from channel __proto__', 'hi from channel 2']; + sub.on('message', function (channel, message) { - if (channel === 'chan1') { - assert.strictEqual(message, 'hi on channel 1'); - sub.stream.end(); - } else if (channel === 'chan2') { - assert.strictEqual(message, 'hi on channel 2'); - sub.stream.end(); - } else { - sub.quit(); - pub.quit(); - assert.fail('test failed'); - } + var n = Math.max(count - 1, 0); + assert.strictEqual(channel, channels[n]); + assert.strictEqual(message, msg[n]); + if (count === 2) return done(); + sub.stream.end(); }); - sub.subscribe('chan1', 'chan2'); + sub.subscribe(channels); sub.on('ready', function (err, results) { + pub.publish(channels[count], msg[count]); count++; - if (count === 1) { - pub.publish('chan1', 'hi on channel 1'); - return; - } else if (count === 2) { - pub.publish('chan2', 'hi on channel 2'); - } else { - sub.quit(function () { - pub.quit(function () { - return done(); - }); - }); - } }); - pub.publish('chan1', 'hi on channel 1'); + pub.publish(channels[count], msg[count]); }); }); From 5e42302636432d0d997b72cda3a3f45609c5ecab Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 03:50:01 +0200 Subject: [PATCH 05/29] Accept arbitrary arguments in the debug function --- lib/debug.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/debug.js b/lib/debug.js index 3f9d482bbca..0e6333f2ec3 100644 --- a/lib/debug.js +++ b/lib/debug.js @@ -2,9 +2,9 @@ var index = require('../'); -function debug (msg) { +function debug () { if (index.debug_mode) { - console.error(msg); + console.error.apply(null, arguments); } } From dfd493f6ee3ed49f47ad02be95d88b7e298dde17 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 03:51:06 +0200 Subject: [PATCH 06/29] Update benchmark file --- benchmarks/multi_bench.js | 42 ++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/benchmarks/multi_bench.js b/benchmarks/multi_bench.js index f13b9bd845b..d93fce4439e 100644 --- a/benchmarks/multi_bench.js +++ b/benchmarks/multi_bench.js @@ -23,6 +23,7 @@ function returnArg (name, def) { } var num_clients = returnArg('clients', 1); var run_time = returnArg('time', 2500); // ms +var pipeline = returnArg('pipeline', 1); // number of concurrent commands var versions_logged = false; var client_options = { parser: returnArg('parser', 'hiredis'), @@ -41,17 +42,18 @@ function lpad (input, len, chr) { metrics.Histogram.prototype.print_line = function () { var obj = this.printObj(); - return lpad((obj.min / 1e6).toFixed(2), 6) + '/' + lpad((obj.max / 1e6).toFixed(2), 6) + '/' + lpad((obj.mean / 1e6).toFixed(2), 6); + return lpad((obj.mean / 1e6).toFixed(2), 6) + '/' + lpad((obj.max / 1e6).toFixed(2), 6); }; function Test (args) { this.args = args; + this.args.pipeline = +pipeline; this.callback = null; this.clients = []; this.clients_ready = 0; this.commands_sent = 0; this.commands_completed = 0; - this.max_pipeline = this.args.pipeline || 50; + this.max_pipeline = +pipeline; this.batch_pipeline = this.args.batch || 0; this.client_options = args.client_options || {}; this.client_options.parser = client_options.parser; @@ -206,7 +208,7 @@ Test.prototype.print_stats = function () { var duration = Date.now() - this.test_start; totalTime += duration; - console.log('min/max/avg: ' + this.command_latency.print_line() + ' ' + lpad(duration, 6) + 'ms total, ' + + console.log('avg/max: ' + this.command_latency.print_line() + lpad(duration, 5) + 'ms total, ' + lpad(Math.round(this.commands_completed / (duration / 1000)), 7) + ' ops/sec'); }; @@ -217,55 +219,55 @@ large_buf = new Buffer(large_str); 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: []})); 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]})); 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]})); 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']})); 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'], 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]})); 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]})); 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']})); 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'], 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']})); 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]})); 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']})); 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']})); 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]})); 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]})); 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']})); 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'], 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 d2b8f2f39171d008630d4567f339c27428cb0c23 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 03:54:19 +0200 Subject: [PATCH 07/29] Add support for camelCase Fixes missing `EXEC_BATCH` on multi --- README.md | 4 +- index.js | 98 ++++++++++++++++++++++++++++++++++---- lib/extendedApi.js | 2 +- lib/multi.js | 2 +- lib/utils.js | 18 ++++++- test/auth.spec.js | 14 +++++- test/commands/info.spec.js | 6 +-- test/connection.spec.js | 18 +++---- test/multi.spec.js | 2 +- test/node_redis.spec.js | 5 +- test/utils.spec.js | 19 +++++++- 11 files changed, 154 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index d2b82a0ee2a..e9c92bdac55 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,8 @@ This will display: mjr:~/work/node_redis (master)$ Note that the API is entirely asynchronous. To get data back from the server, you'll need to use a callback. +From v.2.6 on the API supports camelCase and snack_case and all options / variables / events etc. can be used either way. +It is recommended to use camelCase as this is the default for the Node.js landscape. ### Promises @@ -109,8 +111,6 @@ client.get("missingkey", function(err, reply) { For a list of Redis commands, see [Redis Command Reference](http://redis.io/commands) -The commands can be specified in uppercase or lowercase for convenience. `client.get()` is the same as `client.GET()`. - Minimal parsing is done on the replies. Commands that return a integer return JavaScript Numbers, arrays return JavaScript Array. `HGETALL` returns an Object keyed by the hash keys. All strings will either be returned as string or as buffer depending on your setting. Please be aware that sending null, undefined and Boolean values will result in the value coerced to a string! diff --git a/index.js b/index.js index 9cdc46dc37f..78e07e65799 100644 --- a/index.js +++ b/index.js @@ -479,13 +479,19 @@ RedisClient.prototype.send_offline_queue = function () { var retry_connection = function (self, error) { debug('Retrying connection...'); - self.emit('reconnecting', { + var reconnect_params = { delay: self.retry_delay, attempt: self.attempts, - error: error, - times_connected: self.times_connected, - total_retry_time: self.retry_totaltime - }); + error: error + }; + if (self.options.camel_case) { + reconnect_params.totalRetryTime = self.retry_totaltime; + reconnect_params.timesConnected = self.times_connected; + } else { + reconnect_params.total_retry_time = self.retry_totaltime; + reconnect_params.times_connected = self.times_connected; + } + self.emit('reconnecting', reconnect_params); self.retry_totaltime += self.retry_delay; self.attempts += 1; @@ -529,12 +535,18 @@ RedisClient.prototype.connection_gone = function (why, error) { } if (typeof this.options.retry_strategy === 'function') { - this.retry_delay = this.options.retry_strategy({ + var retry_params = { attempt: this.attempts, - error: error, - total_retry_time: this.retry_totaltime, - times_connected: this.times_connected - }); + error: error + }; + if (this.options.camel_case) { + retry_params.totalRetryTime = this.retry_totaltime; + retry_params.timesConnected = this.times_connected; + } else { + retry_params.total_retry_time = this.retry_totaltime; + retry_params.times_connected = this.times_connected; + } + this.retry_delay = this.options.retry_strategy(retry_params); if (typeof this.retry_delay !== 'number') { // Pass individual error through if (this.retry_delay instanceof Error) { @@ -902,6 +914,72 @@ RedisClient.prototype.write = function (data) { return; }; +Object.defineProperty(exports, 'debugMode', { + get: function () { + return this.debug_mode; + }, + set: function (val) { + this.debug_mode = val; + } +}); + +// Don't officially expose the command_queue directly but only the length as read only variable +Object.defineProperty(RedisClient.prototype, 'command_queue_length', { + get: function () { + return this.command_queue.length; + } +}); + +Object.defineProperty(RedisClient.prototype, 'offline_queue_length', { + get: function () { + return this.offline_queue.length; + } +}); + +// Add support for camelCase by adding read only properties to the client +// All known exposed snack_case variables are added here +Object.defineProperty(RedisClient.prototype, 'retryDelay', { + get: function () { + return this.retry_delay; + } +}); + +Object.defineProperty(RedisClient.prototype, 'retryBackoff', { + get: function () { + return this.retry_backoff; + } +}); + +Object.defineProperty(RedisClient.prototype, 'commandQueueLength', { + get: function () { + return this.command_queue.length; + } +}); + +Object.defineProperty(RedisClient.prototype, 'offlineQueueLength', { + get: function () { + return this.offline_queue.length; + } +}); + +Object.defineProperty(RedisClient.prototype, 'shouldBuffer', { + get: function () { + return this.should_buffer; + } +}); + +Object.defineProperty(RedisClient.prototype, 'connectionId', { + get: function () { + return this.connection_id; + } +}); + +Object.defineProperty(RedisClient.prototype, 'serverInfo', { + get: function () { + return this.server_info; + } +}); + exports.createClient = function () { return new RedisClient(unifyOptions.apply(null, arguments)); }; diff --git a/lib/extendedApi.js b/lib/extendedApi.js index e9182028e71..0d70bf14189 100644 --- a/lib/extendedApi.js +++ b/lib/extendedApi.js @@ -10,7 +10,7 @@ All documented and exposed API belongs in here **********************************************/ // Redirect calls to the appropriate function and use to send arbitrary / not supported commands -RedisClient.prototype.send_command = function (command, args, callback) { +RedisClient.prototype.send_command = RedisClient.prototype.sendCommand = function (command, args, callback) { // Throw to fail early instead of relying in order in this case if (typeof command !== 'string') { throw new Error('Wrong input type "' + (command !== null && command !== undefined ? command.constructor.name : command) + '" for command name'); diff --git a/lib/multi.js b/lib/multi.js index ca7afda4813..eaa6dd618e4 100644 --- a/lib/multi.js +++ b/lib/multi.js @@ -70,7 +70,7 @@ function pipeline_transaction_command (self, command, args, index, cb) { }); } -Multi.prototype.exec_atomic = function exec_atomic (callback) { +Multi.prototype.exec_atomic = Multi.prototype.EXEC_ATOMIC = Multi.prototype.execAtomic = function exec_atomic (callback) { if (this.queue.length < 2) { return this.exec_batch(callback); } diff --git a/lib/utils.js b/lib/utils.js index 4093b61dcbd..b46b08b1998 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -41,8 +41,10 @@ function print (err, reply) { } } +var camelCase; // 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) +// All capital letters are going to be replaced with a lower case letter and a underscore infront of it function clone (obj) { var copy; if (Array.isArray(obj)) { @@ -57,7 +59,14 @@ function clone (obj) { var elems = Object.keys(obj); var elem; while (elem = elems.pop()) { - copy[elem] = clone(obj[elem]); + // Accept camelCase options and convert them to snack_case + var snack_case = elem.replace(/[A-Z][^A-Z]/g, '_$&').toLowerCase(); + // If camelCase is detected, pass it to the client, so all variables are going to be camelCased + // There are no deep nested options objects yet, but let's handle this future proof + if (snack_case !== elem.toLowerCase()) { + camelCase = true; + } + copy[snack_case] = clone(obj[elem]); } return copy; } @@ -65,7 +74,12 @@ function clone (obj) { } function convenienceClone (obj) { - return clone(obj) || {}; + camelCase = false; + obj = clone(obj) || {}; + if (camelCase) { + obj.camel_case = true; + } + return obj; } function callbackOrEmit (self, callback, err, res) { diff --git a/test/auth.spec.js b/test/auth.spec.js index 47fa0964a2f..b9bb3c033c1 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -166,8 +166,18 @@ describe('client authentication', function () { client = redis.createClient.apply(null, args); client.auth(auth); client.on('ready', function () { - if (this.times_connected === 1) { - client.stream.destroy(); + if (this.times_connected < 3) { + var interval = setInterval(function () { + if (client.commandQueueLength !== 0) { + return; + } + clearInterval(interval); + interval = null; + client.stream.destroy(); + client.set('foo', 'bar'); + client.get('foo'); // Errors would bubble + assert.strictEqual(client.offlineQueueLength, 2); + }, 1); } else { done(); } diff --git a/test/commands/info.spec.js b/test/commands/info.spec.js index 838dac21b8e..3a67a1a178f 100644 --- a/test/commands/info.spec.js +++ b/test/commands/info.spec.js @@ -23,16 +23,16 @@ describe("The 'info' method", function () { client.end(true); }); - it('update server_info after a info command', function (done) { + it('update serverInfo after a info command', function (done) { client.set('foo', 'bar'); client.info(); client.select(2, function () { - assert.strictEqual(client.server_info.db2, undefined); + assert.strictEqual(client.serverInfo.db2, undefined); }); client.set('foo', 'bar'); client.info(); setTimeout(function () { - assert.strictEqual(typeof client.server_info.db2, 'object'); + assert.strictEqual(typeof client.serverInfo.db2, 'object'); done(); }, 30); }); diff --git a/test/connection.spec.js b/test/connection.spec.js index 1279ec4e37b..41de60b3b4c 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -132,12 +132,14 @@ describe('connection tests', function () { describe('on lost connection', function () { it('emit an error after max retry attempts and do not try to reconnect afterwards', function (done) { - var max_attempts = 3; + var maxAttempts = 3; var options = { parser: parser, - max_attempts: max_attempts + maxAttempts: maxAttempts }; client = redis.createClient(options); + assert.strictEqual(client.retryBackoff, 1.7); + assert.strictEqual(client.retryDelay, 200); assert.strictEqual(Object.keys(options).length, 2); var calls = 0; @@ -152,7 +154,7 @@ describe('connection tests', function () { client.on('error', function (err) { if (/Redis connection in broken state: maximum connection attempts.*?exceeded./.test(err.message)) { process.nextTick(function () { // End is called after the error got emitted - assert.strictEqual(calls, max_attempts - 1); + assert.strictEqual(calls, maxAttempts - 1); assert.strictEqual(client.emitted_end, true); assert.strictEqual(client.connected, false); assert.strictEqual(client.ready, false); @@ -248,7 +250,7 @@ describe('connection tests', function () { }); }); - it('retry_strategy used to reconnect with individual error', function (done) { + it('retryStrategy used to reconnect with individual error', function (done) { var text = ''; var unhookIntercept = intercept(function (data) { text += data; @@ -256,8 +258,8 @@ describe('connection tests', function () { }); var end = helper.callFuncAfter(done, 2); client = redis.createClient({ - retry_strategy: function (options) { - if (options.total_retry_time > 150) { + retryStrategy: function (options) { + if (options.totalRetryTime > 150) { client.set('foo', 'bar', function (err, res) { assert.strictEqual(err.message, 'Connection timeout'); end(); @@ -267,8 +269,8 @@ describe('connection tests', function () { } return Math.min(options.attempt * 25, 200); }, - max_attempts: 5, - retry_max_delay: 123, + maxAttempts: 5, + retryMaxDelay: 123, port: 9999 }); diff --git a/test/multi.spec.js b/test/multi.spec.js index 817433db348..a969bfbbd53 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -571,7 +571,7 @@ describe("The 'multi' method", function () { test = true; }; multi.set('baz', 'binary'); - multi.exec_atomic(); + multi.EXEC_ATOMIC(); assert(test); }); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 060c62926c1..fd9fc2c2d0c 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -30,6 +30,7 @@ describe('The node_redis client', function () { it('check if all options got copied properly', function (done) { client.selected_db = 2; var client2 = client.duplicate(); + assert.strictEqual(client.connectionId + 1, client2.connection_id); assert.strictEqual(client2.selected_db, 2); assert(client.connected); assert(!client2.connected); @@ -360,7 +361,7 @@ describe('The node_redis client', function () { client.on('error', function (err) { assert.strictEqual(err.message, 'SET can\'t be processed. The connection has already been closed.'); assert.strictEqual(err.command, 'SET'); - assert.strictEqual(client.offline_queue.length, 0); + assert.strictEqual(client.offline_queue_length, 0); done(); }); setTimeout(function () { @@ -966,7 +967,7 @@ describe('The node_redis client', function () { multi.set('foo' + (i + 2), 'bar' + (i + 2)); } multi.exec(); - assert.equal(client.command_queue.length, 15); + assert.equal(client.command_queue_length, 15); helper.killConnection(client); }); diff --git a/test/utils.spec.js b/test/utils.spec.js index d77c7eb607c..17aaf89a84d 100644 --- a/test/utils.spec.js +++ b/test/utils.spec.js @@ -11,7 +11,7 @@ describe('utils.js', function () { it('ignore the object prototype and clone a nested array / object', function () { var obj = { a: [null, 'foo', ['bar'], { - "I'm special": true + "i'm special": true }], number: 5, fn: function noop () {} @@ -22,13 +22,28 @@ describe('utils.js', function () { assert(typeof clone.fn === 'function'); }); - it('replace faulty values with an empty object as return value', function () { + it('replace falsy 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('transform camelCase options to snack_case and add the camel_case option', function () { + var a = utils.clone({ + optionOneTwo: true, + retryStrategy: false, + nested: { + onlyContainCamelCaseOnce: true + } + }); + assert.strictEqual(Object.keys(a).length, 4); + assert.strictEqual(a.option_one_two, true); + assert.strictEqual(a.retry_strategy, false); + assert.strictEqual(a.camel_case, true); + assert.strictEqual(Object.keys(a.nested).length, 1); + }); + it('throws on circular data', function () { try { var a = {}; From 8e24380d533485298523b2a03c07ab8f13f897cb Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 04:00:23 +0200 Subject: [PATCH 08/29] Add optional callback option to duplicate function --- README.md | 3 ++- lib/extendedApi.js | 19 ++++++++++++++++++- test/node_redis.spec.js | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e9c92bdac55..5aa7804d1c1 100644 --- a/README.md +++ b/README.md @@ -597,9 +597,10 @@ the second word as first parameter: client.multi().script('load', 'return 1').exec(...); client.multi([['script', 'load', 'return 1']]).exec(...); -## client.duplicate([options]) +## client.duplicate([options][, callback]) Duplicate all current options and return a new redisClient instance. All options passed to the duplicate function are going to replace the original option. +If you pass a callback, duplicate is going to wait until the client is ready and returns it in the callback. If an error occurs in the meanwhile, that is going to return an error instead in the callback. ## client.send_command(command_name[, [args][, callback]]) diff --git a/lib/extendedApi.js b/lib/extendedApi.js index 0d70bf14189..21013146b50 100644 --- a/lib/extendedApi.js +++ b/lib/extendedApi.js @@ -79,7 +79,11 @@ RedisClient.prototype.unref = function () { } }; -RedisClient.prototype.duplicate = function (options) { +RedisClient.prototype.duplicate = function (options, callback) { + if (typeof options === 'function') { + callback = options; + options = null; + } var existing_options = utils.clone(this.options); options = utils.clone(options); for (var elem in options) { // jshint ignore: line @@ -87,5 +91,18 @@ RedisClient.prototype.duplicate = function (options) { } var client = new RedisClient(existing_options); client.selected_db = this.selected_db; + if (typeof callback === 'function') { + var ready_listener = function () { + callback(null, client); + client.removeAllListeners(error_listener); + }; + var error_listener = function (err) { + callback(err); + client.end(true); + }; + client.once('ready', ready_listener); + client.once('error', error_listener); + return; + } return client; }; diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index fd9fc2c2d0c..e556dc75054 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -66,6 +66,38 @@ describe('The node_redis client', function () { done(); }); }); + + it('works with a callback', function (done) { + client.duplicate(function (err, client) { + assert(!err); + assert.strictEqual(client.ready, true); + client.quit(done); + }); + }); + + it('works with a callback and errors out', function (done) { + client.duplicate({ + port: '9999' + }, function (err, client) { + assert.strictEqual(err.code, 'ECONNREFUSED'); + done(client); + }); + }); + + it('works with a promises', function () { + return client.duplicateAsync().then(function (client) { + assert.strictEqual(client.ready, true); + return client.quitAsync(); + }); + }); + + it('works with a promises and errors', function () { + return client.duplicateAsync({ + port: 9999 + }).catch(function (err) { + assert.strictEqual(err.code, 'ECONNREFUSED'); + }); + }); }); describe('big data', function () { From aff765adf0455a7df8318ecbcb236b3d5bff3c6f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 04:04:03 +0200 Subject: [PATCH 09/29] Fix execution order If the command_queue and the offline_queue holds commands, the offline_queue should be choosen instead of the command_queue. --- lib/utils.js | 3 ++- test/utils.spec.js | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index b46b08b1998..f5a5c902e15 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -91,7 +91,8 @@ function callbackOrEmit (self, callback, err, res) { } function replyInOrder (self, callback, err, res) { - var command_obj = self.command_queue.peekBack() || self.offline_queue.peekBack(); + // The offline queue has to be checked first, as there might be commands in both queues at the same time + var command_obj = self.offline_queue.peekBack() || self.command_queue.peekBack(); if (!command_obj) { process.nextTick(function () { callbackOrEmit(self, callback, err, res); diff --git a/test/utils.spec.js b/test/utils.spec.js index 17aaf89a84d..094640066b5 100644 --- a/test/utils.spec.js +++ b/test/utils.spec.js @@ -107,7 +107,7 @@ describe('utils.js', function () { emitted = false; }); - it('no elements in either queue. Reply in the next tick', function (done) { + it('no elements in either queue. Reply in the next tick with callback', function (done) { var called = false; utils.reply_in_order(clientMock, function () { called = true; @@ -116,7 +116,7 @@ describe('utils.js', function () { assert(!called); }); - it('no elements in either queue. Reply in the next tick', function (done) { + it('no elements in either queue. Reply in the next tick without callback', function (done) { assert(!emitted); utils.reply_in_order(clientMock, null, new Error('tada')); assert(!emitted); @@ -153,16 +153,21 @@ describe('utils.js', function () { } }); - 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 () { + it('elements in the offline queue and the command_queue. Reply all other commands got handled respect the command_obj', function (done) { + clientMock.command_queue.push(create_command_obj(), create_command_obj()); + clientMock.offline_queue.push(create_command_obj(), {}); + utils.reply_in_order(clientMock, function (err, res) { assert.strictEqual(clientMock.command_queue.length, 0); + assert.strictEqual(clientMock.offline_queue.length, 0); assert(!emitted); - assert.strictEqual(res_count, 1); + assert.strictEqual(res_count, 3); done(); }, null, null); + while (clientMock.offline_queue.length) { + clientMock.command_queue.push(clientMock.offline_queue.shift()); + } while (clientMock.command_queue.length) { - clientMock.command_queue.shift().callback(null, 'bar'); + clientMock.command_queue.shift().callback(null, 'hello world'); } }); }); From a9d565b8f457bd5de4570613700f40c7f187e94f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 04:07:25 +0200 Subject: [PATCH 10/29] Fix auth regression Fixes #1028 --- index.js | 2 ++ lib/individualCommands.js | 2 +- test/auth.spec.js | 6 +++--- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 78e07e65799..b5c62f3c65c 100644 --- a/index.js +++ b/index.js @@ -269,7 +269,9 @@ RedisClient.prototype.create_stream = function () { // Fire the command before redis is connected to be sure it's the first fired command if (this.auth_pass !== undefined) { + this.ready = true; this.auth(this.auth_pass); + this.ready = false; } }; diff --git a/lib/individualCommands.js b/lib/individualCommands.js index 4f4c8ab7a57..953a9337f24 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -149,7 +149,7 @@ RedisClient.prototype.auth = RedisClient.prototype.AUTH = function auth (pass, c // Stash auth for connect and reconnect. this.auth_pass = pass; - this.ready = this.offline_queue.length === 0; // keep the execution order intakt + this.ready = ready || this.offline_queue.length === 0; // keep the execution order intakt var tmp = this.internal_send_command('auth', [pass], function (err, res) { if (err) { if (no_password_is_set.test(err.message)) { diff --git a/test/auth.spec.js b/test/auth.spec.js index b9bb3c033c1..2e28f895dfb 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -160,7 +160,7 @@ describe('client authentication', function () { client.on('ready', done); }); - it('reconnects with appropriate authentication', function (done) { + it('reconnects with appropriate authentication while offline commands are present', function (done) { if (helper.redisProcess().spawnFailed()) this.skip(); client = redis.createClient.apply(null, args); @@ -187,7 +187,7 @@ describe('client authentication', function () { }); }); - it('should return an error if the password is not of type string and a callback has been provided', function (done) { + it('should return an error if the password is not correct and a callback has been provided', function (done) { if (helper.redisProcess().spawnFailed()) this.skip(); client = redis.createClient.apply(null, args); @@ -202,7 +202,7 @@ describe('client authentication', function () { assert(async); }); - it('should emit an error if the password is not of type string and no callback has been provided', function (done) { + it('should emit an error if the password is not correct and no callback has been provided', function (done) { if (helper.redisProcess().spawnFailed()) this.skip(); client = redis.createClient.apply(null, args); From cd58e1fd8961ffabafe2bfea960229df5a0bcc0c Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 04:18:39 +0200 Subject: [PATCH 11/29] Implement message_buffer and pmessage_buffer events --- README.md | 10 ++++++++ index.js | 60 +++++++++++++++++++++++++-------------------- test/pubsub.spec.js | 24 ++++++++++++++++++ 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 5aa7804d1c1..3a4abf9eb96 100644 --- a/README.md +++ b/README.md @@ -410,6 +410,16 @@ Client will emit `pmessage` for every message received that matches an active su Listeners are passed the original pattern used with `PSUBSCRIBE` as `pattern`, the sending channel name as `channel`, and the message as `message`. +### "message_buffer" (channel, message) + +This is the same as the `message` event with the exception, that it is always going to emit a buffer. +If you listen to the `message` event at the same time as the `message_buffer`, it is always going to emit a string. + +### "pmessage_buffer" (pattern, channel, message) + +This is the same as the `pmessage` event with the exception, that it is always going to emit a buffer. +If you listen to the `pmessage` event at the same time as the `pmessage_buffer`, it is always going to emit a string. + ### "subscribe" (channel, count) Client will emit `subscribe` in response to a `SUBSCRIBE` command. Listeners are passed the diff --git a/index.js b/index.js index b5c62f3c65c..5848279e005 100644 --- a/index.js +++ b/index.js @@ -27,7 +27,7 @@ if (typeof EventEmitter !== 'function') { function noop () {} function handle_detect_buffers_reply (reply, command, buffer_args) { - if (buffer_args === false) { + if (buffer_args === false || this.message_buffers) { // If detect_buffers option was specified, then the reply from the parser will be a buffer. // If this command did not use Buffer arguments, then convert the reply to Strings here. reply = utils.reply_to_strings(reply); @@ -138,6 +138,7 @@ function RedisClient (options, stream) { this.pub_sub_mode = 0; this.subscription_set = {}; this.monitoring = false; + this.message_buffers = false; this.closing = false; this.server_info = {}; this.auth_pass = options.auth_pass || options.password; @@ -149,23 +150,7 @@ function RedisClient (options, stream) { this.options = options; this.buffers = options.return_buffers || options.detect_buffers; // Init parser - this.reply_parser = Parser({ - returnReply: function (data) { - self.return_reply(data); - }, - returnError: function (err) { - self.return_error(err); - }, - 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: this.buffers, - name: options.parser, - stringNumbers: options.string_numbers - }); + this.reply_parser = create_parser(this, options); 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) { @@ -179,6 +164,10 @@ function RedisClient (options, stream) { '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.' ); + } else if (event === 'message_buffer' || event === 'pmessage_buffer' || event === 'messageBuffer' || event === 'pmessageBuffer' && !this.buffers) { + this.message_buffers = true; + this.handle_reply = handle_detect_buffers_reply; + this.reply_parser = create_parser(this); } }); } @@ -186,6 +175,26 @@ util.inherits(RedisClient, EventEmitter); RedisClient.connection_id = 0; +function create_parser (self) { + return Parser({ + returnReply: function (data) { + self.return_reply(data); + }, + returnError: function (err) { + self.return_error(err); + }, + 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: self.buffers || self.message_buffers, + name: self.options.parser, + stringNumbers: self.options.string_numbers + }); +} + /****************************************************************************** All functions in here are internal besides the RedisClient constructor @@ -696,21 +705,18 @@ function subscribe_unsubscribe (self, reply, type, subscribe) { function return_pub_sub (self, reply) { var type = reply[0].toString(); if (type === 'message') { // channel, message - // TODO: Implement message_buffer - // if (self.buffers) { - // self.emit('message_buffer', reply[1], reply[2]); - // } - if (!self.options.return_buffers) { // backwards compatible. Refactor this in v.3 to always return a string on the normal emitter + if (!self.options.return_buffers || self.message_buffers) { // backwards compatible. Refactor this in v.3 to always return a string on the normal emitter self.emit('message', reply[1].toString(), reply[2].toString()); + self.emit('message_buffer', reply[1], reply[2]); + self.emit('messageBuffer', reply[1], reply[2]); } else { self.emit('message', reply[1], reply[2]); } } else if (type === 'pmessage') { // pattern, channel, message - // if (self.buffers) { - // self.emit('pmessage_buffer', reply[1], reply[2], reply[3]); - // } - if (!self.options.return_buffers) { // backwards compatible. Refactor this in v.3 to always return a string on the normal emitter + if (!self.options.return_buffers || self.message_buffers) { // backwards compatible. Refactor this in v.3 to always return a string on the normal emitter self.emit('pmessage', reply[1].toString(), reply[2].toString(), reply[3].toString()); + self.emit('pmessage_buffer', reply[1], reply[2], reply[3]); + self.emit('pmessageBuffer', reply[1], reply[2], reply[3]); } else { self.emit('pmessage', reply[1], reply[2], reply[3]); } diff --git a/test/pubsub.spec.js b/test/pubsub.spec.js index 9cb388b03e1..f9401f63ad5 100644 --- a/test/pubsub.spec.js +++ b/test/pubsub.spec.js @@ -504,6 +504,30 @@ describe('publish/subscribe', function () { pub.publish('/foo', 'hello world', helper.isNumber(3)); }); }); + + it('allows to listen to pmessageBuffer and pmessage', function (done) { + var batch = sub.batch(); + batch.psubscribe('*'); + batch.subscribe('/foo'); + batch.unsubscribe('/foo'); + batch.unsubscribe(); + batch.subscribe(['/foo']); + batch.exec(); + assert.strictEqual(sub.shouldBuffer, false); + sub.on('pmessageBuffer', function (pattern, channel, message) { + assert.strictEqual(pattern.inspect(), new Buffer('*').inspect()); + assert.strictEqual(channel.inspect(), new Buffer('/foo').inspect()); + sub.quit(done); + }); + sub.on('pmessage', function (pattern, channel, message) { + assert.strictEqual(pattern, '*'); + assert.strictEqual(channel, '/foo'); + }); + pub.pubsub('numsub', '/foo', function (err, res) { + assert.deepEqual(res, ['/foo', 1]); + }); + pub.publish('/foo', 'hello world', helper.isNumber(2)); + }); }); describe('punsubscribe', function () { From 5d1265958338681a5f2662bd695efd3a07312310 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 13 Apr 2016 04:29:02 +0200 Subject: [PATCH 12/29] Fix typos / comments --- index.js | 15 ++++++++------- lib/extendedApi.js | 2 +- lib/utils.js | 2 +- test/auth.spec.js | 2 +- test/connection.spec.js | 2 +- test/node_redis.spec.js | 6 +++--- 6 files changed, 15 insertions(+), 14 deletions(-) diff --git a/index.js b/index.js index 5848279e005..e6e565c59b3 100644 --- a/index.js +++ b/index.js @@ -63,7 +63,7 @@ function RedisClient (options, stream) { cnx_options.family = (!options.family && net.isIP(cnx_options.host)) || (options.family === 'IPv6' ? 6 : 4); this.address = cnx_options.host + ':' + cnx_options.port; } - /* istanbul ignore next: travis does not work with stunnel atm. Therefor the tls tests are skipped on travis */ + /* istanbul ignore next: travis does not work with stunnel atm. Therefore the tls tests are skipped on travis */ for (var tls_option in options.tls) { // jshint ignore: line cnx_options[tls_option] = options.tls[tls_option]; } @@ -220,7 +220,7 @@ RedisClient.prototype.create_stream = function () { this.stream.destroy(); } - /* istanbul ignore if: travis does not work with stunnel atm. Therefor the tls tests are skipped on travis */ + /* istanbul ignore if: travis does not work with stunnel atm. Therefore the tls tests are skipped on travis */ if (this.options.tls) { this.stream = tls.connect(this.connection_options); } else { @@ -230,12 +230,13 @@ RedisClient.prototype.create_stream = function () { if (this.options.connect_timeout) { this.stream.setTimeout(this.connect_timeout, function () { + // Note: This is only tested if a internet connection is established self.retry_totaltime = self.connect_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 */ + /* istanbul ignore next: travis does not work with stunnel atm. Therefore the tls tests are skipped on travis */ var connect_event = this.options.tls ? 'secureConnect' : 'connect'; this.stream.once(connect_event, function () { this.removeAllListeners('timeout'); @@ -244,7 +245,7 @@ RedisClient.prototype.create_stream = function () { }); this.stream.on('data', function (buffer_from_socket) { - // The buffer_from_socket.toString() has a significant impact on big chunks and therefor this should only be used if necessary + // The buffer_from_socket.toString() has a significant impact on big chunks and therefore this should only be used if necessary debug('Net read ' + self.address + ' id ' + self.connection_id); // + ': ' + buffer_from_socket.toString()); self.reply_parser.execute(buffer_from_socket); self.emit_idle(); @@ -400,12 +401,12 @@ RedisClient.prototype.on_ready = function () { this.pub_sub_mode = this.old_state.pub_sub_mode; } if (this.monitoring) { // Monitor has to be fired before pub sub commands - this.internal_send_command('monitor', []); + this.internal_send_command('monitor', []); // The state is still set } var callback_count = Object.keys(this.subscription_set).length; if (!this.options.disable_resubscribing && callback_count) { // only emit 'ready' when all subscriptions were made again - // TODO: Remove the countdown for ready here. This is not coherent with all other modes and should therefor not be handled special + // TODO: Remove the countdown for ready here. This is not coherent with all other modes and should therefore not be handled special // We know we are ready as soon as all commands were fired var callback = function () { callback_count--; @@ -680,7 +681,7 @@ function subscribe_unsubscribe (self, reply, type, subscribe) { } else { var running_command; var i = 1; - // This should be a rare case and therefor handling it this way should be good performance wise for the general case + // This should be a rare case and therefore handling it this way should be good performance wise for the general case while (running_command = self.command_queue.get(i)) { if (SUBSCRIBE_COMMANDS[running_command.command]) { self.command_queue.shift(); diff --git a/lib/extendedApi.js b/lib/extendedApi.js index 21013146b50..5cd78038f06 100644 --- a/lib/extendedApi.js +++ b/lib/extendedApi.js @@ -39,7 +39,7 @@ RedisClient.prototype.send_command = RedisClient.prototype.sendCommand = functio return this.internal_send_command(command, args, callback); } if (typeof callback === 'function') { - args = args.concat([callback]); + args = args.concat([callback]); // Prevent manipulating the input array } return this[command].apply(this, args); }; diff --git a/lib/utils.js b/lib/utils.js index f5a5c902e15..1b6ee62124f 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -3,7 +3,7 @@ var RawObject = require('./rawObject'); // 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 +// These function are only called with internal data and have therefore always the same instanceof X function replyToObject (reply) { // The reply might be a string or a buffer if this is called in a transaction (multi) if (reply.length === 0 || !(reply instanceof Array)) { diff --git a/test/auth.spec.js b/test/auth.spec.js index 2e28f895dfb..4c262dccd23 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -53,7 +53,7 @@ describe('client authentication', function () { client.auth(auth, function (err, res) { assert.strictEqual('retry worked', res); var now = Date.now(); - // Hint: setTimeout sometimes triggers early and therefor the value can be like one or two ms to early + // Hint: setTimeout sometimes triggers early and therefore the value can be like one or two ms to early assert(now - time >= 98, 'Time should be above 100 ms (the reconnect time) and is ' + (now - time)); assert(now - time < 225, 'Time should be below 255 ms (the reconnect should only take a bit above 100 ms) and is ' + (now - time)); done(); diff --git a/test/connection.spec.js b/test/connection.spec.js index 41de60b3b4c..ad91730c350 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -20,7 +20,7 @@ describe('connection tests', function () { it('unofficially support for a private stream', function () { // While using a private stream, reconnection and other features are not going to work properly. // Besides that some functions also have to be monkey patched to be safe from errors in this case. - // Therefor this is not officially supported! + // Therefore this is not officially supported! var socket = new net.Socket(); client = new redis.RedisClient({ prefix: 'test' diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index e556dc75054..02a10f7c951 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -597,7 +597,7 @@ describe('The node_redis client', function () { assert.strictEqual(typeof rawOutput, 'string'); assert(utils.monitor_regex.test(rawOutput), rawOutput); assert.deepEqual(args, ['mget', 'hello', 'world']); - // Quit immediatly ends monitoring mode and therefor does not stream back the quit command + // Quit immediatly ends monitoring mode and therefore does not stream back the quit command monitorClient.quit(done); }); }); @@ -668,7 +668,7 @@ describe('The node_redis client', function () { assert.deepEqual(responses[5], ['unsubscribe', 'baz']); assert.deepEqual(responses[6], ['publish', '/foo', 'hello world']); // The publish is called right after the reconnect and the monitor is called before the message is emitted. - // Therefor we have to wait till the next tick + // Therefore we have to wait till the next tick process.nextTick(function () { assert(called); client.quit(done); @@ -891,7 +891,7 @@ describe('The node_redis client', 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 + // Fail the set answer. Has no corresponding command obj and will therefore land in the error handler and set client.reply_parser.execute(new Buffer('a*1\r*1\r$1`zasd\r\na')); }); }); From 683815de9d715dd413aa79c72779c94b61914a58 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 Apr 2016 00:57:34 +0200 Subject: [PATCH 13/29] Refactor pipelining --- index.js | 72 ++++++++++++++++++----------------------- lib/multi.js | 7 ++-- test/multi.spec.js | 12 +++++-- test/node_redis.spec.js | 10 +++--- 4 files changed, 47 insertions(+), 54 deletions(-) diff --git a/index.js b/index.js index e6e565c59b3..ac2c0fc517a 100644 --- a/index.js +++ b/index.js @@ -122,6 +122,7 @@ function RedisClient (options, stream) { } 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 + this.pipeline_queue = new Queue(); // Holds all pipelined commands // 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 @@ -144,8 +145,8 @@ function RedisClient (options, stream) { this.auth_pass = options.auth_pass || options.password; this.selected_db = options.db; // Save the selected db here, used when reconnecting this.old_state = null; - this.send_anyway = false; - this.pipeline = 0; + this.fire_strings = true; // Determine if strings or buffers should be written to the stream + this.pipeline = false; this.times_connected = 0; this.options = options; this.buffers = options.return_buffers || options.detect_buffers; @@ -374,23 +375,25 @@ RedisClient.prototype.on_ready = function () { debug('on_ready called ' + this.address + ' id ' + this.connection_id); this.ready = true; - var cork; - if (!this.stream.cork) { - cork = function (len) { - self.pipeline = len; - self.pipeline_queue = new Queue(len); - }; - } else { - cork = function (len) { - self.pipeline = len; - self.pipeline_queue = new Queue(len); + this.cork = function () { + self.pipeline = true; + if (self.stream.cork) { self.stream.cork(); - }; - this.uncork = function () { + } + }; + this.uncork = function () { + if (self.fire_strings) { + self.write_strings(); + } else { + self.write_buffers(); + } + self.pipeline = false; + self.fire_strings = true; + if (self.stream.uncork) { + // TODO: Consider using next tick here. See https://github.com/NodeRedis/node_redis/issues/1033 self.stream.uncork(); - }; - } - this.cork = cork; + } + }; // Restore modal commands from previous connection. The order of the commands is important if (this.selected_db !== undefined) { @@ -523,7 +526,8 @@ RedisClient.prototype.connection_gone = function (why, error) { this.ready = false; // Deactivate cork to work with the offline queue this.cork = noop; - this.pipeline = 0; + this.uncork = noop; + this.pipeline = false; var state = { monitoring: this.monitoring, @@ -792,10 +796,6 @@ RedisClient.prototype.internal_send_command = function (command, args, callback) if (args[i].length > 30000) { big_data = true; 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]; } @@ -813,10 +813,6 @@ RedisClient.prototype.internal_send_command = function (command, args, callback) args_copy[i] = args[i]; buffer_args = true; big_data = true; - if (this.pipeline !== 0) { - this.pipeline += 2; - this.writeDefault = this.writeBuffers; - } } else { this.warn( 'Deprecated: The ' + command.toUpperCase() + ' command contains a argument of type ' + args[i].constructor.name + '.\n' + @@ -870,6 +866,7 @@ RedisClient.prototype.internal_send_command = function (command, args, callback) this.write(command_str); } else { debug('Send command (' + command_str + ') has Buffer arguments'); + this.fire_strings = false; this.write(command_str); for (i = 0; i < len; i += 1) { @@ -887,40 +884,33 @@ RedisClient.prototype.internal_send_command = function (command, args, callback) return !this.should_buffer; }; -RedisClient.prototype.writeDefault = RedisClient.prototype.writeStrings = function (data) { +RedisClient.prototype.write_strings = function () { 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); + this.should_buffer = !this.stream.write(str); str = ''; } str += command; } - this.should_buffer = !this.stream.write(str + data); + if (str !== '') { + this.should_buffer = !this.stream.write(str); + } }; -RedisClient.prototype.writeBuffers = function (data) { +RedisClient.prototype.write_buffers = function () { for (var command = this.pipeline_queue.shift(); command; command = this.pipeline_queue.shift()) { - this.stream.write(command); + this.should_buffer = !this.stream.write(command); } - this.should_buffer = !this.stream.write(data); }; RedisClient.prototype.write = function (data) { - if (this.pipeline === 0) { + if (this.pipeline === false) { this.should_buffer = !this.stream.write(data); return; } - - this.pipeline--; - if (this.pipeline === 0) { - this.writeDefault(data); - return; - } - this.pipeline_queue.push(data); - return; }; Object.defineProperty(exports, 'debugMode', { diff --git a/lib/multi.js b/lib/multi.js index eaa6dd618e4..bb2173cd2e6 100644 --- a/lib/multi.js +++ b/lib/multi.js @@ -126,7 +126,7 @@ Multi.prototype.exec_transaction = function exec_transaction (callback) { var len = self.queue.length; self.errors = []; self.callback = callback; - self._client.cork(len + 2); + self._client.cork(); self.wants_buffers = new Array(len); pipeline_transaction_command(self, 'multi', []); // Drain queue, callback will catch 'QUEUED' or error @@ -151,7 +151,6 @@ Multi.prototype.exec_transaction = function exec_transaction (callback) { multi_callback(self, err, replies); }); self._client.uncork(); - self._client.writeDefault = self._client.writeStrings; return !self._client.should_buffer; }; @@ -198,7 +197,7 @@ Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = funct return true; } self.results = []; - self._client.cork(len); + self._client.cork(); while (args = self.queue.shift()) { var command = args[0]; var cb; @@ -213,9 +212,7 @@ Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = funct self._client.internal_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; }; diff --git a/test/multi.spec.js b/test/multi.spec.js index a969bfbbd53..2ad9a230512 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -125,9 +125,8 @@ describe("The 'multi' method", function () { describe('when connected', function () { - beforeEach(function (done) { + beforeEach(function () { client = redis.createClient.apply(null, args); - client.once('connect', done); }); it('executes a pipelined multi properly in combination with the offline queue', function (done) { @@ -135,6 +134,7 @@ describe("The 'multi' method", function () { multi1.set('m1', '123'); multi1.get('m1'); multi1.exec(done); + assert.strictEqual(client.offline_queue.length, 4); }); it('executes a pipelined multi properly after a reconnect in combination with the offline queue', function (done) { @@ -612,11 +612,17 @@ describe("The 'multi' method", function () { }); it('emits error once if reconnecting after multi has been executed but not yet returned without callback', function (done) { + // NOTE: If uncork is called async by postponing it to the next tick, this behavior is going to change. + // The command won't be processed anymore two errors are returned instead of one client.on('error', function (err) { assert.strictEqual(err.code, 'UNCERTAIN_STATE'); - done(); + client.get('foo', function (err, res) { + assert.strictEqual(res, 'bar'); + done(); + }); }); + // The commands should still be fired, no matter that the socket is destroyed on the same tick client.multi().set('foo', 'bar').get('foo').exec(); // Abort connection before the value returned client.stream.destroy(); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 02a10f7c951..fdfc503d534 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -121,12 +121,12 @@ describe('The node_redis client', function () { str += str; } var called = false; - var temp = client.writeBuffers.bind(client); - assert(String(client.writeBuffers) !== String(client.writeDefault)); - client.writeBuffers = function (data) { + var temp = client.write_buffers.bind(client); + assert(client.fire_strings); + client.write_buffers = function (data) { called = true; // To increase write performance for strings the value is converted to a buffer - assert(String(client.writeBuffers) === String(client.writeDefault)); + assert(!client.fire_strings); temp(data); }; client.multi().set('foo', str).get('foo', function (err, res) { @@ -136,7 +136,7 @@ describe('The node_redis client', function () { assert.strictEqual(res[1], str); done(); }); - assert(String(client.writeBuffers) !== String(client.writeDefault)); + assert(client.fire_strings); }); }); From 0424cb0bf39f747e2c9d66077d7bb8bd654d4701 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 Apr 2016 01:11:20 +0200 Subject: [PATCH 14/29] Move pub sub command into individual commands and use call_on_write --- index.js | 23 ++-- lib/command.js | 7 +- lib/individualCommands.js | 239 +++++++++++++++++++++++++++++++++++++- lib/multi.js | 86 +++++--------- test/multi.spec.js | 4 +- 5 files changed, 280 insertions(+), 79 deletions(-) diff --git a/index.js b/index.js index ac2c0fc517a..8b5d917be97 100644 --- a/index.js +++ b/index.js @@ -484,7 +484,7 @@ RedisClient.prototype.ready_check = function () { RedisClient.prototype.send_offline_queue = function () { for (var command_obj = this.offline_queue.shift(); command_obj; command_obj = this.offline_queue.shift()) { debug('Sending offline command: ' + command_obj.command); - this.internal_send_command(command_obj.command, command_obj.args, command_obj.callback); + this.internal_send_command(command_obj.command, command_obj.args, command_obj.callback, command_obj.call_on_write); } this.drain(); // Even though items were shifted off, Queue backing store still uses memory until next add, so just get a new Queue @@ -771,8 +771,10 @@ function handle_offline_command (self, command_obj) { self.should_buffer = true; } -RedisClient.prototype.internal_send_command = function (command, args, callback) { - var arg, prefix_keys; +// Do not call internal_send_command directly, if you are not absolutly certain it handles everything properly +// e.g. monitor / info does not work with internal_send_command only +RedisClient.prototype.internal_send_command = function (command, args, callback, call_on_write) { + var arg, prefix_keys, command_obj; var i = 0; var command_str = ''; var len = args.length; @@ -786,7 +788,7 @@ RedisClient.prototype.internal_send_command = function (command, args, callback) if (this.ready === false || this.stream.writable === false) { // Handle offline commands right away - handle_offline_command(this, new OfflineCommand(command, args, callback)); + handle_offline_command(this, new OfflineCommand(command, args, callback, call_on_write)); return false; // Indicate buffering } @@ -834,15 +836,7 @@ RedisClient.prototype.internal_send_command = function (command, args, callback) } } args = null; - var command_obj = new Command(command, args_copy, callback); - command_obj.buffer_args = buffer_args; - - if (SUBSCRIBE_COMMANDS[command] && this.pub_sub_mode === 0) { - // If pub sub is already activated, keep it that way, otherwise set the number of commands to resolve until pub sub mode activates - // Deactivation of the pub sub mode happens in the result handler - this.pub_sub_mode = this.command_queue.length + 1; - } - this.command_queue.push(command_obj); + command_obj = new Command(command, args_copy, buffer_args, callback); if (this.options.prefix) { prefix_keys = commands.getKeyIndexes(command, args_copy); @@ -881,6 +875,9 @@ RedisClient.prototype.internal_send_command = function (command, args, callback) debug('send_command: buffer send ' + arg.length + ' bytes'); } } + if (call_on_write) { + call_on_write(); + } return !this.should_buffer; }; diff --git a/lib/command.js b/lib/command.js index e4467fb55da..ee1181ea7e3 100644 --- a/lib/command.js +++ b/lib/command.js @@ -2,18 +2,19 @@ // 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, callback) { +function Command (command, args, buffer_args, callback) { this.command = command; this.args = args; // We only need the args for the offline commands => move them into another class. We need the number of args though for pub sub - this.buffer_args = false; + this.buffer_args = buffer_args; this.callback = callback; this.sub_commands_left = args.length; } -function OfflineCommand (command, args, callback) { +function OfflineCommand (command, args, callback, call_on_write) { this.command = command; this.args = args; this.callback = callback; + this.call_on_write = call_on_write; } module.exports = { diff --git a/lib/individualCommands.js b/lib/individualCommands.js index 953a9337f24..4ebd8ae1705 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -7,9 +7,18 @@ var no_password_is_set = /no password is set/; var loading = /LOADING/; var RedisClient = require('../').RedisClient; -/******************************** -Replace built-in redis functions -********************************/ +/******************************************************************************************** + Replace built-in redis functions + + The callback may be hooked as needed. The same does not apply to the rest of the function. + State should not be set outside of the callback if not absolutly necessary. + This is important to make sure it works the same as single command or in a multi context. + To make sure everything works with the offline queue use the "call_on_write" function. + This is going to be executed while writing to the stream. + + TODO: Implement individal command generation as soon as possible to prevent divergent code + on single and multi calls! +********************************************************************************************/ RedisClient.prototype.multi = RedisClient.prototype.MULTI = function multi (args) { var multi = new Multi(this, args); @@ -209,3 +218,227 @@ RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function hmset () { } return this.internal_send_command('hmset', arr, callback); }; + +RedisClient.prototype.subscribe = RedisClient.prototype.SUBSCRIBE = function subscribe () { + var arr, + len = arguments.length, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } 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]; + } + } + var self = this; + var call_on_write = function () { + self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; + }; + return this.internal_send_command('subscribe', arr, callback, call_on_write); +}; + +Multi.prototype.subscribe = Multi.prototype.SUBSCRIBE = function subscribe () { + var arr, + len = arguments.length, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } 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]; + } + } + var self = this._client; + var call_on_write = function () { + self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; + }; + this.queue.push(['subscribe', arr, callback, call_on_write]); + return this; +}; + +RedisClient.prototype.unsubscribe = RedisClient.prototype.UNSUBSCRIBE = function unsubscribe () { + var arr, + len = arguments.length, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } 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]; + } + } + var self = this; + var call_on_write = function () { + // Pub sub has to be activated even if not in pub sub mode, as the return value is manipulated in the callback + self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; + }; + return this.internal_send_command('unsubscribe', arr, callback, call_on_write); +}; + +Multi.prototype.unsubscribe = Multi.prototype.UNSUBSCRIBE = function unsubscribe () { + var arr, + len = arguments.length, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } 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]; + } + } + var self = this._client; + var call_on_write = function () { + // Pub sub has to be activated even if not in pub sub mode, as the return value is manipulated in the callback + self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; + }; + this.queue.push(['unsubscribe', arr, callback, call_on_write]); + return this; +}; + +RedisClient.prototype.psubscribe = RedisClient.prototype.PSUBSCRIBE = function psubscribe () { + var arr, + len = arguments.length, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } 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]; + } + } + var self = this; + var call_on_write = function () { + self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; + }; + return this.internal_send_command('psubscribe', arr, callback, call_on_write); +}; + +Multi.prototype.psubscribe = Multi.prototype.PSUBSCRIBE = function psubscribe () { + var arr, + len = arguments.length, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } 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]; + } + } + var self = this; + var call_on_write = function () { + self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; + }; + this.queue.push(['psubscribe', arr, callback, call_on_write]); + return this; +}; + +RedisClient.prototype.punsubscribe = RedisClient.prototype.PUNSUBSCRIBE = function punsubscribe () { + var arr, + len = arguments.length, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } 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]; + } + } + var self = this; + var call_on_write = function () { + // Pub sub has to be activated even if not in pub sub mode, as the return value is manipulated in the callback + self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; + }; + return this.internal_send_command('punsubscribe', arr, callback, call_on_write); +}; + +Multi.prototype.punsubscribe = Multi.prototype.PUNSUBSCRIBE = function punsubscribe () { + var arr, + len = arguments.length, + callback, + i = 0; + if (Array.isArray(arguments[0])) { + arr = arguments[0]; + callback = arguments[1]; + } 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]; + } + } + var self = this; + var call_on_write = function () { + // Pub sub has to be activated even if not in pub sub mode, as the return value is manipulated in the callback + self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; + }; + this.queue.push(['punsubscribe', arr, callback, call_on_write]); + return this; +}; diff --git a/lib/multi.js b/lib/multi.js index bb2173cd2e6..bdf37fe6ade 100644 --- a/lib/multi.js +++ b/lib/multi.js @@ -20,45 +20,8 @@ function Multi (client, 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) { +function pipeline_transaction_command (self, command, args, index, cb, call_on_write) { + // Queueing is done first, then the commands are executed self._client.send_command(command, args, function (err, reply) { if (err) { if (cb) { @@ -131,20 +94,22 @@ Multi.prototype.exec_transaction = function exec_transaction (callback) { 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]; + // The commands may not be shifted off, since they are needed in the result handler + var command_obj = self.queue.get(index); + var command = command_obj[0]; + var cb = command_obj[2]; + var call_on_write = command_obj.length === 4 ? command_obj[3] : undefined; // 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) { + for (var i = 0; i < command_obj[1].length; i += 1) { + if (command_obj[1][i] instanceof Buffer) { self.wants_buffers[index] = true; break; } } } - pipeline_transaction_command(self, command, args[1], index, cb); + pipeline_transaction_command(self, command, command_obj[1], index, cb, call_on_write); } self._client.internal_send_command('exec', [], function (err, replies) { @@ -171,7 +136,18 @@ Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = funct var self = this; var len = self.queue.length; var index = 0; - var args; + var command_obj; + self._client.cork(); + if (!callback) { + while (command_obj = self.queue.shift()) { + self._client.internal_send_command(command_obj[0], command_obj[1], command_obj[2], (command_obj.length === 4 ? command_obj[3] : undefined)); + } + self._client.uncork(); + return !self._client.should_buffer; + } else if (len === 0) { + utils.reply_in_order(self._client, callback, null, []); + return !self._client.should_buffer; + } var callback_without_own_cb = function (err, res) { if (err) { self.results.push(err); @@ -190,26 +166,20 @@ Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = funct callback(null, self.results); }; }; - if (len === 0) { - if (callback) { - utils.reply_in_order(self._client, callback, null, []); - } - return true; - } self.results = []; - self._client.cork(); - while (args = self.queue.shift()) { - var command = args[0]; + while (command_obj = self.queue.shift()) { + var command = command_obj[0]; + var call_on_write = command_obj.length === 4 ? command_obj[3] : undefined; var cb; - if (typeof args[2] === 'function') { - cb = batch_callback(self, args[2], index); + if (typeof command_obj[2] === 'function') { + cb = batch_callback(self, command_obj[2], index); } else { cb = callback_without_own_cb; } if (typeof callback === 'function' && index === len - 1) { cb = last_callback(cb); } - self._client.internal_send_command(command, args[1], cb); + this._client.internal_send_command(command, command_obj[1], cb, call_on_write); index++; } self._client.uncork(); diff --git a/test/multi.spec.js b/test/multi.spec.js index 2ad9a230512..3227ecda2af 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -255,12 +255,12 @@ describe("The 'multi' method", function () { multi2.set('m2', '456'); multi1.set('m1', '123'); multi1.get('m1'); - multi2.get('m2'); + multi2.get('m1'); multi2.ping(); multi1.exec(end); multi2.exec(function (err, res) { - assert.strictEqual(res[1], '456'); + assert.strictEqual(res[1], '123'); end(); }); }); From 3038c9043de88a1ca85ccb028d7f798782ef0ad5 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 Apr 2016 01:14:41 +0200 Subject: [PATCH 15/29] Make sure all individual handled command work in multi context the same Fix quit possibly resulting in reconnections --- index.js | 31 +++++- lib/individualCommands.js | 228 ++++++++++++++++++++++++++------------ test/auth.spec.js | 42 +++++++ test/batch.spec.js | 16 ++- test/connection.spec.js | 21 ++++ test/multi.spec.js | 34 ++++++ test/node_redis.spec.js | 61 ++++++---- 7 files changed, 334 insertions(+), 99 deletions(-) diff --git a/index.js b/index.js index 8b5d917be97..f9b93396370 100644 --- a/index.js +++ b/index.js @@ -735,12 +735,35 @@ function return_pub_sub (self, reply) { } RedisClient.prototype.return_reply = function (reply) { - if (this.pub_sub_mode === 1 && reply instanceof Array && reply.length !== 0 && reply[0]) { + // If in monitor mode, all normal commands are still working and we only want to emit the streamlined commands + // As this is not the average use case and monitor is expensive anyway, let's change the code here, to improve + // the average performance of all other commands in case of no monitor mode + if (this.monitoring) { + var replyStr; + if (this.buffers && Buffer.isBuffer(reply)) { + replyStr = reply.toString(); + } else { + replyStr = reply; + } + // While reconnecting the redis server does not recognize the client as in monitor mode anymore + // Therefore the monitor command has to finish before it catches further commands + if (typeof replyStr === 'string' && utils.monitor_regex.test(replyStr)) { + var timestamp = replyStr.slice(0, replyStr.indexOf(' ')); + var args = replyStr.slice(replyStr.indexOf('"') + 1, -1).split('" "').map(function (elem) { + return elem.replace(/\\"/g, '"'); + }); + this.emit('monitor', timestamp, args, replyStr); + return; + } + } + if (this.pub_sub_mode === 0) { + normal_reply(this, reply); + } else if (this.pub_sub_mode !== 1) { + this.pub_sub_mode--; + normal_reply(this, reply); + } else if (reply instanceof Array && reply.length > 2 && reply[0]) { return_pub_sub(this, reply); } else { - if (this.pub_sub_mode !== 0 && this.pub_sub_mode !== 1) { - this.pub_sub_mode--; - } normal_reply(this, reply); } }; diff --git a/lib/individualCommands.js b/lib/individualCommands.js index 4ebd8ae1705..bcbbecd2fa6 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -31,88 +31,101 @@ 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.internal_send_command('select', [db], function (err, res) { +function select_callback (self, db, callback) { + return function (err, res) { if (err === null) { + // Store db in this.select_db to restore it on reconnect self.selected_db = db; } utils.callback_or_emit(self, callback, err, res); - }); + }; +} + +RedisClient.prototype.select = RedisClient.prototype.SELECT = function select (db, callback) { + return this.internal_send_command('select', [db], select_callback(this, db, callback)); }; -RedisClient.prototype.monitor = RedisClient.prototype.MONITOR = function (callback) { - // Use a individual command, as this is a special case that does not has to be checked for any other command - var self = this; - return this.internal_send_command('monitor', [], function (err, res) { +Multi.prototype.select = Multi.prototype.SELECT = function select (db, callback) { + this.queue.push(['select', [db], select_callback(this._client, db, callback)]); + return this; +}; + +function monitor_callback (self, callback) { + return function (err, res) { if (err === null) { - self.reply_parser.returnReply = function (reply) { - // If in monitor mode, all normal commands are still working and we only want to emit the streamlined commands - // As this is not the average use case and monitor is expensive anyway, let's change the code here, to improve - // the average performance of all other commands in case of no monitor mode - if (self.monitoring) { - var replyStr; - if (self.buffers && Buffer.isBuffer(reply)) { - replyStr = reply.toString(); - } else { - replyStr = reply; - } - // While reconnecting the redis server does not recognize the client as in monitor mode anymore - // Therefor the monitor command has to finish before it catches further commands - if (typeof replyStr === 'string' && utils.monitor_regex.test(replyStr)) { - var timestamp = replyStr.slice(0, replyStr.indexOf(' ')); - var args = replyStr.slice(replyStr.indexOf('"') + 1, -1).split('" "').map(function (elem) { - return elem.replace(/\\"/g, '"'); - }); - self.emit('monitor', timestamp, args, replyStr); - return; - } - } - self.return_reply(reply); - }; self.monitoring = true; } utils.callback_or_emit(self, callback, err, res); - }); + }; +} + +RedisClient.prototype.monitor = RedisClient.prototype.MONITOR = function monitor (callback) { + // Use a individual command, as this is a special case that does not has to be checked for any other command + return this.internal_send_command('monitor', [], monitor_callback(this, callback)); }; -RedisClient.prototype.quit = RedisClient.prototype.QUIT = function (callback) { - var self = this; - var callback_hook = function (err, res) { - // TODO: Improve this by handling everything with coherend error codes and find out if there's anything missing - if (err && (err.code === 'NR_OFFLINE' || - err.message === 'Redis connection gone from close event.' || - err.message === 'The command can\'t be processed. The connection has already been closed.' - )) { +// Only works with batch, not in a transaction +Multi.prototype.monitor = Multi.prototype.MONITOR = function monitor (callback) { + // Use a individual command, as this is a special case that does not has to be checked for any other command + if (this.exec !== this.exec_transaction) { + this.queue.push(['monitor', [], monitor_callback(this._client, callback)]); + return this; + } + var err = new Error( + 'You used the monitor command in combination with a transaction. Due to faulty return values of ' + + 'Redis in this context, the monitor command is now executed without transaction instead and ignored ' + + 'in the multi statement.' + ); + err.command = 'MONITOR'; + utils.reply_in_order(this._client, callback, err); + this._client.monitor('monitor', callback); + return this; +}; + +function quit_callback (self, callback) { + return function (err, res) { + if (err && err.code === 'NR_OFFLINE') { // Pretent the quit command worked properly in this case. // Either the quit landed in the offline queue and was flushed at the reconnect // or the offline queue is deactivated and the command was rejected right away // or the stream is not writable - // or while sending the quit, the connection dropped + // or while sending the quit, the connection ended / closed err = null; res = 'OK'; } utils.callback_or_emit(self, callback, err, res); + if (self.stream.writable) { + // If the socket is still alive, kill it. This could happen if quit got a NR_OFFLINE error code + self.stream.destroy(); + } }; - var backpressure_indicator = this.internal_send_command('quit', [], callback_hook); +} + +RedisClient.prototype.QUIT = RedisClient.prototype.quit = function (callback) { + // TODO: Consider this for v.3 + // Allow the quit command to be fired as soon as possible to prevent it landing in the offline queue. + // this.ready = this.offline_queue.length === 0; + var backpressure_indicator = this.internal_send_command('quit', [], quit_callback(this, callback)); // Calling quit should always end the connection, no matter if there's a connection or not this.closing = true; + this.ready = false; return backpressure_indicator; }; -// Store info in this.server_info after each call -RedisClient.prototype.info = RedisClient.prototype.INFO = function info (section, callback) { - var self = this; - var ready = this.ready; - var args = []; - if (typeof section === 'function') { - callback = section; - } else if (section !== undefined) { - args = Array.isArray(section) ? section : [section]; - } - this.ready = ready || this.offline_queue.length === 0; // keep the execution order intakt - var tmp = this.internal_send_command('info', args, function (err, res) { +// Only works with batch, not in a transaction +Multi.prototype.QUIT = Multi.prototype.quit = function (callback) { + var self = this._client; + var call_on_write = function () { + // If called in a multi context, we expect redis is available + self.closing = true; + self.ready = false; + }; + this.queue.push(['quit', [], quit_callback(self, callback), call_on_write]); + return this; +}; + +function info_callback (self, callback) { + return function (err, res) { if (res) { var obj = {}; var lines = res.toString().split('\r\n'); @@ -146,20 +159,33 @@ RedisClient.prototype.info = RedisClient.prototype.INFO = function info (section self.server_info = {}; } utils.callback_or_emit(self, callback, err, res); - }); - this.ready = ready; - return tmp; + }; +} + +// Store info in this.server_info after each call +RedisClient.prototype.info = RedisClient.prototype.INFO = function info (section, callback) { + var args = []; + if (typeof section === 'function') { + callback = section; + } else if (section !== undefined) { + args = Array.isArray(section) ? section : [section]; + } + return this.internal_send_command('info', args, info_callback(this, callback)); }; -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); +Multi.prototype.info = Multi.prototype.INFO = function info (section, callback) { + var args = []; + if (typeof section === 'function') { + callback = section; + } else if (section !== undefined) { + args = Array.isArray(section) ? section : [section]; + } + this.queue.push(['info', args, info_callback(this._client, callback)]); + return this; +}; - // Stash auth for connect and reconnect. - this.auth_pass = pass; - this.ready = ready || this.offline_queue.length === 0; // keep the execution order intakt - var tmp = this.internal_send_command('auth', [pass], function (err, res) { +function auth_callback (self, pass, callback) { + return function (err, res) { if (err) { if (no_password_is_set.test(err.message)) { self.warn('Warning: Redis server does not require a password, but a password was supplied.'); @@ -175,11 +201,31 @@ RedisClient.prototype.auth = RedisClient.prototype.AUTH = function auth (pass, c } } utils.callback_or_emit(self, callback, err, res); - }); + }; +} + +RedisClient.prototype.auth = RedisClient.prototype.AUTH = function auth (pass, callback) { + debug('Sending auth to ' + this.address + ' id ' + this.connection_id); + + // Stash auth for connect and reconnect. + this.auth_pass = pass; + var ready = this.ready; + this.ready = ready || this.offline_queue.length === 0; + var tmp = this.internal_send_command('auth', [pass], auth_callback(this, pass, callback)); this.ready = ready; return tmp; }; +// Only works with batch, not in a transaction +Multi.prototype.auth = Multi.prototype.AUTH = function auth (pass, callback) { + debug('Sending auth to ' + this.address + ' id ' + this.connection_id); + + // Stash auth for connect and reconnect. + this.auth_pass = pass; + this.queue.push(['auth', [pass], auth_callback(this._client, callback)]); + return this; +}; + RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function hmset () { var arr, len = arguments.length, @@ -198,7 +244,7 @@ RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function hmset () { 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')) { + } 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]); @@ -219,6 +265,46 @@ RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function hmset () { return this.internal_send_command('hmset', arr, callback); }; +Multi.prototype.hmset = Multi.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]; + } + } + this.queue.push(['hmset', arr, callback]); + return this; +}; + RedisClient.prototype.subscribe = RedisClient.prototype.SUBSCRIBE = function subscribe () { var arr, len = arguments.length, @@ -378,7 +464,7 @@ Multi.prototype.psubscribe = Multi.prototype.PSUBSCRIBE = function psubscribe () arr[i] = arguments[i]; } } - var self = this; + var self = this._client; var call_on_write = function () { self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; }; @@ -434,7 +520,7 @@ Multi.prototype.punsubscribe = Multi.prototype.PUNSUBSCRIBE = function punsubscr arr[i] = arguments[i]; } } - var self = this; + var self = this._client; var call_on_write = function () { // Pub sub has to be activated even if not in pub sub mode, as the return value is manipulated in the callback self.pub_sub_mode = self.pub_sub_mode || self.command_queue.length + 1; diff --git a/test/auth.spec.js b/test/auth.spec.js index 4c262dccd23..ac6548e0488 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -291,6 +291,48 @@ describe('client authentication', function () { }); }); }); + + it('indivdual commands work properly with batch', function (done) { + // quit => might return an error instead of "OK" in the exec callback... (if not connected) + // auth => might return an error instead of "OK" in the exec callback... (if no password is required / still loading on Redis <= 2.4) + // This could be fixed by checking the return value of the callback in the exec callback and + // returning the manipulated [error, result] from the callback. + // There should be a better solution though + + var args = config.configureClient(parser, 'localhost', { + noReadyCheck: true + }); + client = redis.createClient.apply(redis.createClient, args); + assert.strictEqual(client.selected_db, undefined); + var end = helper.callFuncAfter(done, 8); + client.on('monitor', function () { + end(); // Should be called for each command after monitor + }); + client.batch() + .auth(auth) + .SELECT(5, function (err, res) { + assert.strictEqual(client.selected_db, 5); + assert.strictEqual(res, 'OK'); + assert.notDeepEqual(client.serverInfo.db5, { avg_ttl: 0, expires: 0, keys: 1 }); + }) + .monitor() + .set('foo', 'bar', helper.isString('OK')) + .INFO(function (err, res) { + assert.strictEqual(res.indexOf('# Server\r\nredis_version:'), 0); + assert.deepEqual(client.serverInfo.db5, { avg_ttl: 0, expires: 0, keys: 1 }); + }) + .get('foo', helper.isString('bar')) + .subscribe(['foo', 'bar']) + .unsubscribe('foo') + .SUBSCRIBE('/foo', helper.isString('/foo')) + .psubscribe('*') + .quit(helper.isString('OK')) // this might be interesting + .exec(function (err, res) { + res[4] = res[4].substr(0, 10); + assert.deepEqual(res, ['OK', 'OK', 'OK', 'OK', '# Server\r\n', 'bar', 'bar', 'foo', '/foo', '*', 'OK']); + end(); + }); + }); }); }); diff --git a/test/batch.spec.js b/test/batch.spec.js index 5f25be65cba..7b382eee421 100644 --- a/test/batch.spec.js +++ b/test/batch.spec.js @@ -12,8 +12,6 @@ describe("The 'batch' method", function () { describe('using ' + parser + ' and ' + ip, function () { describe('when not connected', function () { - // TODO: This is somewhat broken and should be fixed in v.3 - // The commands should return an error instead of returning an empty result var client; beforeEach(function (done) { @@ -24,7 +22,7 @@ describe("The 'batch' method", function () { client.on('end', done); }); - it('returns an empty array', function (done) { + it('returns an empty array for missing commands', function (done) { var batch = client.batch(); batch.exec(function (err, res) { assert.strictEqual(err, null); @@ -33,7 +31,17 @@ describe("The 'batch' method", function () { }); }); - it('returns an empty array if promisified', function () { + it('returns an error for batch with commands', function (done) { + var batch = client.batch(); + batch.set('foo', 'bar'); + batch.exec(function (err, res) { + assert.strictEqual(err, null); + assert.strictEqual(res[0].code, 'NR_OFFLINE'); + done(); + }); + }); + + it('returns an empty array for missing commands if promisified', function () { return client.batch().execAsync().then(function (res) { assert.strictEqual(res.length, 0); }); diff --git a/test/connection.spec.js b/test/connection.spec.js index ad91730c350..531dce59bf5 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -93,6 +93,27 @@ describe('connection tests', function () { assert.strictEqual(bool, false); }); + it('calling quit while connected without offline queue should end the connection when all commands have finished', function (done) { + var called = false; + client = redis.createClient({ + enable_offline_queue: false + }); + client.on('ready', function () { + client.set('foo', 'bar', function (err, res) { + assert.strictEqual(res, 'OK'); + called = true; + }); + var bool = client.quit(function (err, res) { + assert.strictEqual(res, 'OK'); + assert.strictEqual(err, null); + assert(called); + done(); + }); + // TODO: In v.3 the quit command would be fired right away, so bool should be true + assert.strictEqual(bool, true); + }); + }); + it('do not quit before connected or a connection issue is detected', function (done) { client = redis.createClient(); client.set('foo', 'bar', helper.isString('OK')); diff --git a/test/multi.spec.js b/test/multi.spec.js index 3227ecda2af..ec2a81aafc1 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -628,6 +628,40 @@ describe("The 'multi' method", function () { client.stream.destroy(); }); + it('indivdual commands work properly with multi', function (done) { + // Neither of the following work properly in a transactions: + // (This is due to Redis not returning the reply as expected / resulting in undefined behavior) + // (Likely there are more commands that do not work with a transaction) + // + // auth => can't be called after a multi command + // monitor => results in faulty return values e.g. multi().monitor().set('foo', 'bar').get('foo') + // returns ['OK, 'OK', 'monitor reply'] instead of ['OK', 'OK', 'bar'] + // quit => ends the connection before the exec + // client reply skip|off => results in weird return values. Not sure what exactly happens + // subscribe => enters subscribe mode and this does not work in combination with exec (the same for psubscribe, unsubscribe...) + // + + assert.strictEqual(client.selected_db, undefined); + var multi = client.multi(); + multi.select(5, function (err, res) { + assert.strictEqual(client.selected_db, 5); + assert.strictEqual(res, 'OK'); + assert.notDeepEqual(client.server_info.db5, { avg_ttl: 0, expires: 0, keys: 1 }); + }); + // multi.client('reply', 'on', helper.isString('OK')); // Redis v.3.2 + multi.set('foo', 'bar', helper.isString('OK')); + multi.info(function (err, res) { + assert.strictEqual(res.indexOf('# Server\r\nredis_version:'), 0); + assert.deepEqual(client.server_info.db5, { avg_ttl: 0, expires: 0, keys: 1 }); + }); + multi.get('foo', helper.isString('bar')); + multi.exec(function (err, res) { + res[3] = res[3].substr(0, 10); + assert.deepEqual(res, ['OK', 'OK', '# Server\r\n', 'bar']); + done(); + }); + }); + }); }); }); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index fdfc503d534..774e57a46d1 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -618,6 +618,26 @@ describe('The node_redis client', function () { }); }); + it('monitors reconnects properly and works with the offline queue in a batch statement', function (done) { + var i = 0; + var multi = client.batch(); + multi.MONITOR(helper.isString('OK')); + multi.mget('hello', 'world'); + multi.exec(function (err, res) { + assert.deepEqual(res, ['OK', [null, null]]); + }); + client.on('monitor', function (time, args, rawOutput) { + assert(utils.monitor_regex.test(rawOutput), rawOutput); + assert.deepEqual(args, ['mget', 'hello', 'world']); + if (i++ === 2) { + // End after two reconnects + return done(); + } + client.stream.destroy(); + client.mget('hello', 'world'); + }); + }); + it('monitor does not activate if the command could not be processed properly', function (done) { client.MONITOR(function (err, res) { assert.strictEqual(err.code, 'UNCERTAIN_STATE'); @@ -748,26 +768,27 @@ describe('The node_redis client', function () { }); }); - 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); - }); - }); + // TODO: consider allowing loading commands in v.3 + // 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 () { From 97ae78877b3da8b5a99ba49486b852ae501cef8b Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 Apr 2016 01:17:43 +0200 Subject: [PATCH 16/29] Implement CLIENT REPLY ON|OFF|SKIP --- index.js | 17 +++++++ lib/individualCommands.js | 91 ++++++++++++++++++++++++++++++++++++ lib/utils.js | 12 +++-- test/commands/client.spec.js | 86 +++++++++++++++++++++++++++++----- test/helper.js | 51 ++++++++++++++++---- 5 files changed, 233 insertions(+), 24 deletions(-) diff --git a/index.js b/index.js index f9b93396370..ad21aae0c2b 100644 --- a/index.js +++ b/index.js @@ -150,6 +150,7 @@ function RedisClient (options, stream) { this.times_connected = 0; this.options = options; this.buffers = options.return_buffers || options.detect_buffers; + this.reply = 'ON'; // Returning replies is the default // Init parser this.reply_parser = create_parser(this, options); this.create_stream(); @@ -901,6 +902,22 @@ RedisClient.prototype.internal_send_command = function (command, args, callback, if (call_on_write) { call_on_write(); } + // Handle `CLIENT REPLY ON|OFF|SKIP` + // This has to be checked after call_on_write + if (this.reply === 'ON') { + this.command_queue.push(command_obj); + } else { + // Do not expect a reply + // Does this work in combination with the pub sub mode? + if (callback) { + utils.reply_in_order(this, callback, null, undefined, this.command_queue); + } + if (this.reply === 'SKIP') { + this.reply = 'SKIP_ONE_MORE'; + } else if (this.reply === 'SKIP_ONE_MORE') { + this.reply = 'ON'; + } + } return !this.should_buffer; }; diff --git a/lib/individualCommands.js b/lib/individualCommands.js index bcbbecd2fa6..de03e210f37 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -226,6 +226,97 @@ Multi.prototype.auth = Multi.prototype.AUTH = function auth (pass, callback) { return this; }; +RedisClient.prototype.client = RedisClient.prototype.CLIENT = function client () { + 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 { + 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]; + } + } + var self = this; + var call_on_write = undefined; + // CLIENT REPLY ON|OFF|SKIP + /* istanbul ignore next: TODO: Remove this as soon as Travis runs Redis 3.2 */ + if (arr.length === 2 && arr[0].toString().toUpperCase() === 'REPLY') { + var reply_on_off = arr[1].toString().toUpperCase(); + if (reply_on_off === 'ON' || reply_on_off === 'OFF' || reply_on_off === 'SKIP') { + call_on_write = function () { + self.reply = reply_on_off; + }; + } + } + return this.internal_send_command('client', arr, callback, call_on_write); +}; + +Multi.prototype.client = Multi.prototype.CLIENT = function client () { + 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 { + 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]; + } + } + var self = this._client; + var call_on_write = undefined; + // CLIENT REPLY ON|OFF|SKIP + /* istanbul ignore next: TODO: Remove this as soon as Travis runs Redis 3.2 */ + if (arr.length === 2 && arr[0].toString().toUpperCase() === 'REPLY') { + var reply_on_off = arr[1].toString().toUpperCase(); + if (reply_on_off === 'ON' || reply_on_off === 'OFF' || reply_on_off === 'SKIP') { + call_on_write = function () { + self.reply = reply_on_off; + }; + } + } + this.queue.push(['client', arr, callback, call_on_write]); + return this; +}; + RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function hmset () { var arr, len = arguments.length, diff --git a/lib/utils.js b/lib/utils.js index 1b6ee62124f..cd04429b0c7 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -90,9 +90,15 @@ function callbackOrEmit (self, callback, err, res) { } } -function replyInOrder (self, callback, err, res) { - // The offline queue has to be checked first, as there might be commands in both queues at the same time - var command_obj = self.offline_queue.peekBack() || self.command_queue.peekBack(); +function replyInOrder (self, callback, err, res, queue) { + // If the queue is explicitly passed, use that, otherwise fall back to the offline queue first, + // as there might be commands in both queues at the same time + var command_obj; + if (queue) { + command_obj = queue.peekBack(); + } else { + command_obj = self.offline_queue.peekBack() || self.command_queue.peekBack(); + } if (!command_obj) { process.nextTick(function () { callbackOrEmit(self, callback, err, res); diff --git a/test/commands/client.spec.js b/test/commands/client.spec.js index d73c5036aa5..935db8b6926 100644 --- a/test/commands/client.spec.js +++ b/test/commands/client.spec.js @@ -30,25 +30,89 @@ describe("The 'client' method", function () { }); it("lists connected clients when invoked with multi's chaining syntax", function (done) { - client.multi().client('list').exec(function (err, results) { - assert(pattern.test(results[0]), "expected string '" + results + "' to match " + pattern.toString()); - return done(); - }); + client.multi().client('list', helper.isType.string()).exec(helper.match(pattern, done)); }); it('lists connected clients when invoked with array syntax on client', function (done) { - client.multi().client(['list']).exec(function (err, results) { - assert(pattern.test(results[0]), "expected string '" + results + "' to match " + pattern.toString()); - return done(); - }); + client.multi().client(['list']).exec(helper.match(pattern, done)); }); it("lists connected clients when invoked with multi's array syntax", function (done) { client.multi([ ['client', 'list'] - ]).exec(function (err, results) { - assert(pattern.test(results[0]), "expected string '" + results + "' to match " + pattern.toString()); - return done(); + ]).exec(helper.match(pattern, done)); + }); + }); + + describe('reply', function () { + describe('as normal command', function () { + it('on', function (done) { + helper.serverVersionAtLeast.call(this, client, [3, 2, 0]); + assert.strictEqual(client.reply, 'ON'); + client.client('reply', 'on', helper.isString('OK')); + assert.strictEqual(client.reply, 'ON'); + client.set('foo', 'bar', done); + }); + + it('off', function (done) { + helper.serverVersionAtLeast.call(this, client, [3, 2, 0]); + assert.strictEqual(client.reply, 'ON'); + client.client(new Buffer('REPLY'), 'OFF', helper.isUndefined()); + assert.strictEqual(client.reply, 'OFF'); + client.set('foo', 'bar', helper.isUndefined(done)); + }); + + it('skip', function (done) { + helper.serverVersionAtLeast.call(this, client, [3, 2, 0]); + assert.strictEqual(client.reply, 'ON'); + client.client('REPLY', new Buffer('SKIP'), helper.isUndefined()); + assert.strictEqual(client.reply, 'SKIP_ONE_MORE'); + client.set('foo', 'bar', helper.isUndefined()); + client.get('foo', helper.isString('bar', done)); + }); + }); + + describe('in a batch context', function () { + it('on', function (done) { + helper.serverVersionAtLeast.call(this, client, [3, 2, 0]); + var batch = client.batch(); + assert.strictEqual(client.reply, 'ON'); + batch.client('reply', 'on', helper.isString('OK')); + assert.strictEqual(client.reply, 'ON'); + batch.set('foo', 'bar'); + batch.exec(function (err, res) { + assert.deepEqual(res, ['OK', 'OK']); + done(err); + }); + }); + + it('off', function (done) { + helper.serverVersionAtLeast.call(this, client, [3, 2, 0]); + var batch = client.batch(); + assert.strictEqual(client.reply, 'ON'); + batch.set('hello', 'world'); + batch.client(new Buffer('REPLY'), new Buffer('OFF'), helper.isUndefined()); + batch.set('foo', 'bar', helper.isUndefined()); + batch.exec(function (err, res) { + assert.strictEqual(client.reply, 'OFF'); + assert.deepEqual(res, ['OK', undefined, undefined]); + done(err); + }); + }); + + it('skip', function (done) { + helper.serverVersionAtLeast.call(this, client, [3, 2, 0]); + assert.strictEqual(client.reply, 'ON'); + client.batch() + .set('hello', 'world') + .client('REPLY', 'SKIP', helper.isUndefined()) + .set('foo', 'bar', helper.isUndefined()) + .get('foo') + .exec(function (err, res) { + assert.strictEqual(client.reply, 'ON'); + assert.deepEqual(res, ['OK', undefined, undefined, 'bar']); + done(err); + }); }); }); }); diff --git a/test/helper.js b/test/helper.js index f28568b5003..bb408a19fd1 100644 --- a/test/helper.js +++ b/test/helper.js @@ -29,6 +29,14 @@ if (!process.env.REDIS_TESTS_STARTED) { }); } +function arrayHelper (results) { + if (results instanceof Array) { + assert.strictEqual(results.length, 1, 'The array length may only be one element'); + return results[0]; + } + return results; +} + module.exports = { redisProcess: function () { return rp; @@ -52,8 +60,9 @@ module.exports = { }, isNumber: function (expected, done) { return function (err, results) { - assert.strictEqual(null, err, 'expected ' + expected + ', got error: ' + err); - assert.strictEqual(expected, results, expected + ' !== ' + results); + assert.strictEqual(err, null, 'expected ' + expected + ', got error: ' + err); + results = arrayHelper(results); + assert.strictEqual(results, expected, expected + ' !== ' + results); assert.strictEqual(typeof results, 'number', 'expected a number, got ' + typeof results); if (done) done(); }; @@ -61,18 +70,28 @@ module.exports = { isString: function (str, done) { str = '' + str; // Make sure it's a string return function (err, results) { - assert.strictEqual(null, err, "expected string '" + str + "', got error: " + err); + assert.strictEqual(err, null, "expected string '" + str + "', got error: " + err); + results = arrayHelper(results); if (Buffer.isBuffer(results)) { // If options are passed to return either strings or buffers... results = results.toString(); } - assert.strictEqual(str, results, str + ' does not match ' + results); + assert.strictEqual(results, str, str + ' does not match ' + results); if (done) done(); }; }, isNull: function (done) { return function (err, results) { - assert.strictEqual(null, err, 'expected null, got error: ' + err); - assert.strictEqual(null, results, results + ' is not null'); + assert.strictEqual(err, null, 'expected null, got error: ' + err); + results = arrayHelper(results); + assert.strictEqual(results, null, results + ' is not null'); + if (done) done(); + }; + }, + isUndefined: function (done) { + return function (err, results) { + assert.strictEqual(err, null, 'expected null, got error: ' + err); + results = arrayHelper(results); + assert.strictEqual(results, undefined, results + ' is not undefined'); if (done) done(); }; }, @@ -91,27 +110,39 @@ module.exports = { isType: { number: function (done) { return function (err, results) { - assert.strictEqual(null, err, 'expected any number, got error: ' + err); + assert.strictEqual(err, null, 'expected any number, got error: ' + err); assert.strictEqual(typeof results, 'number', results + ' is not a number'); if (done) done(); }; }, + string: function (done) { + return function (err, results) { + assert.strictEqual(err, null, 'expected any string, got error: ' + err); + assert.strictEqual(typeof results, 'string', results + ' is not a string'); + if (done) done(); + }; + }, positiveNumber: function (done) { return function (err, results) { - assert.strictEqual(null, err, 'expected positive number, got error: ' + err); - assert.strictEqual(true, (results > 0), results + ' is not a positive number'); + assert.strictEqual(err, null, 'expected positive number, got error: ' + err); + assert(results > 0, results + ' is not a positive number'); if (done) done(); }; } }, match: function (pattern, done) { return function (err, results) { - assert.strictEqual(null, err, 'expected ' + pattern.toString() + ', got error: ' + err); + assert.strictEqual(err, null, 'expected ' + pattern.toString() + ', got error: ' + err); + results = arrayHelper(results); assert(pattern.test(results), "expected string '" + results + "' to match " + pattern.toString()); if (done) done(); }; }, serverVersionAtLeast: function (connection, desired_version) { + // Wait until a connection has established (otherwise a timeout is going to be triggered at some point) + if (Object.keys(connection.server_info).length === 0) { + throw new Error('Version check not possible as the client is not yet ready or did not expose the version'); + } // Return true if the server version >= desired_version var version = connection.server_info.versions; for (var i = 0; i < 3; i++) { From a857829a36ba88ded2cfba9d82721964879169a4 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 Apr 2016 02:08:12 +0200 Subject: [PATCH 17/29] Improve error handling Arguments are now passed to an command error in case they exist An error is only emitted if that very same error is not already handled in a callback --- index.js | 58 +++++++++++++++++++++++++++++++---------- lib/customError.js | 16 ++++++++++++ lib/extendedApi.js | 5 +++- lib/multi.js | 12 ++++----- test/auth.spec.js | 2 +- test/connection.spec.js | 23 +++++----------- test/multi.spec.js | 3 ++- test/node_redis.spec.js | 30 ++++++++++++++++----- 8 files changed, 103 insertions(+), 46 deletions(-) create mode 100644 lib/customError.js diff --git a/index.js b/index.js index ad21aae0c2b..9161e358a47 100644 --- a/index.js +++ b/index.js @@ -5,6 +5,7 @@ var tls = require('tls'); var util = require('util'); var utils = require('./lib/utils'); var Queue = require('double-ended-queue'); +var CommandError = require('./lib/customError'); var Command = require('./lib/command').Command; var OfflineCommand = require('./lib/command').OfflineCommand; var EventEmitter = require('events'); @@ -264,11 +265,11 @@ RedisClient.prototype.create_stream = function () { }); this.stream.once('close', function (hadError) { - self.connection_gone('close', new Error('Stream connection closed' + (hadError ? ' because of a transmission error' : ''))); + self.connection_gone('close', hadError ? new Error('Stream connection closed with a transmission error') : null); }); this.stream.once('end', function () { - self.connection_gone('end', new Error('Stream connection ended')); + self.connection_gone('end', null); }); this.stream.on('drain', function () { @@ -320,16 +321,29 @@ RedisClient.prototype.warn = function (msg) { // Flush provided queues, erroring any items with a callback first RedisClient.prototype.flush_and_error = function (error, queue_names) { + var callbacks_not_called = []; queue_names = queue_names || ['offline_queue', 'command_queue']; for (var i = 0; i < queue_names.length; i++) { for (var command_obj = this[queue_names[i]].shift(); command_obj; command_obj = this[queue_names[i]].shift()) { + var err = new CommandError(error); + err.command = command_obj.command.toUpperCase(); + if (command_obj.args.length) { + err.args = command_obj.args; + } if (typeof command_obj.callback === 'function') { - error.command = command_obj.command.toUpperCase(); - command_obj.callback(error); + command_obj.callback(err); + } else { + callbacks_not_called.push(err); } } this[queue_names[i]] = new Queue(); } + // Mutate the original error that will be emitted + // This is fine, as we don't manipulate any user errors + if (callbacks_not_called.length !== 0) { + error.errors = callbacks_not_called; + } + return callbacks_not_called.length === 0; }; RedisClient.prototype.on_error = function (err) { @@ -546,8 +560,10 @@ RedisClient.prototype.connection_gone = function (why, error) { // If this is a requested shutdown, then don't retry if (this.closing) { - debug('Connection ended from quit command, not retrying.'); - this.flush_and_error(new Error('Redis connection gone from ' + why + ' event.')); + debug('Connection ended by quit / end command, not retrying.'); + error = new Error('Stream connection ended and running command aborted. It might have been processed.'); + error.code = 'NR_OFFLINE'; + this.flush_and_error(error); return; } @@ -567,10 +583,18 @@ RedisClient.prototype.connection_gone = function (why, error) { if (typeof this.retry_delay !== 'number') { // Pass individual error through if (this.retry_delay instanceof Error) { - error = this.retry_delay; + error = new CommandError(this.retry_delay); + } + // Attention: there might be the case where there's no error! + if (!error) { + error = new Error('Stream connection ended and running command aborted. It might have been processed.'); + error.code = 'NR_OFFLINE'; + } + // Only emit an error in case that a running command had no callback + if (!this.flush_and_error(error)) { + error.message = 'Stream connection ended and all running commands aborted. They might have been processed.'; + this.emit('error', error); } - this.flush_and_error(error); - this.emit('error', error); this.end(false); return; } @@ -595,11 +619,11 @@ RedisClient.prototype.connection_gone = function (why, error) { } else if (this.command_queue.length !== 0) { error = new Error('Redis connection lost and command aborted in uncertain state. It might have been processed.'); error.code = 'UNCERTAIN_STATE'; - this.flush_and_error(error, ['command_queue']); - error.message = 'Redis connection lost and commands aborted in uncertain state. They might have been processed.'; - // TODO: Reconsider emitting this always, as each running command is handled anyway - // This should likely be removed in v.3. This is different to the broken connection as we'll reconnect here - this.emit('error', error); + if (!this.flush_and_error(error, ['command_queue'])) { + // Only emit if not all commands had a callback that already handled the error + error.message = 'Redis connection lost and commands aborted in uncertain state. They might have been processed.'; + this.emit('error', error); + } } if (this.retry_max_delay !== null && this.retry_delay > this.retry_max_delay) { @@ -618,6 +642,9 @@ RedisClient.prototype.return_error = function (err) { var command_obj = this.command_queue.shift(); if (command_obj && command_obj.command && command_obj.command.toUpperCase) { err.command = command_obj.command.toUpperCase(); + if (command_obj.args.length) { + err.args = command_obj.args; + } } var match = err.message.match(utils.err_code); @@ -786,6 +813,9 @@ function handle_offline_command (self, command_obj) { } err = new Error(command + " can't be processed. " + msg); err.command = command; + if (command_obj.args.length) { + err.args = command_obj.args; + } err.code = 'NR_OFFLINE'; utils.reply_in_order(self, callback, err); } else { diff --git a/lib/customError.js b/lib/customError.js new file mode 100644 index 00000000000..07101b58063 --- /dev/null +++ b/lib/customError.js @@ -0,0 +1,16 @@ +'use strict'; + +var util = require('util'); + +function CommandError (error) { + Error.captureStackTrace(this, this.constructor); + this.name = this.constructor.name; + this.message = error.message; + for (var keys = Object.keys(error), key = keys.pop(); key; key = keys.pop()) { + this[key] = error[key]; + } +} + +util.inherits(CommandError, Error); + +module.exports = CommandError; diff --git a/lib/extendedApi.js b/lib/extendedApi.js index 5cd78038f06..e512ae97e36 100644 --- a/lib/extendedApi.js +++ b/lib/extendedApi.js @@ -47,7 +47,10 @@ RedisClient.prototype.send_command = RedisClient.prototype.sendCommand = functio RedisClient.prototype.end = function (flush) { // Flush queue if wanted if (flush) { - this.flush_and_error(new Error("The command can't be processed. The connection has already been closed.")); + var err = new Error("The command can't be processed. The connection has already been closed."); + err.code = 'NR_OFFLINE'; + this.flush_and_error(err); + // TODO: Emit an error in case a command did not have a callback } else if (arguments.length === 0) { this.warn( 'Using .end() without the flush parameter is deprecated and throws from v.3.0.0 on.\n' + diff --git a/lib/multi.js b/lib/multi.js index bdf37fe6ade..a4399473c98 100644 --- a/lib/multi.js +++ b/lib/multi.js @@ -23,7 +23,8 @@ function Multi (client, args) { function pipeline_transaction_command (self, command, args, index, cb, call_on_write) { // Queueing is done first, then the commands are executed self._client.send_command(command, args, function (err, reply) { - if (err) { + // Ignore the multi command. This is applied by node_redis and the user does not benefit by it + if (err && index !== -1) { if (cb) { cb(err); } @@ -44,13 +45,12 @@ 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; + err.errors = self.errors; + err.command = 'EXEC'; if (self.callback) { self.callback(err); // Exclude connection errors so that those errors won't be emitted twice - } else if (!connection_error) { + } else if (err.code !== 'CONNECTION_BROKEN') { self._client.emit('error', err); } return; @@ -91,7 +91,7 @@ Multi.prototype.exec_transaction = function exec_transaction (callback) { self.callback = callback; self._client.cork(); self.wants_buffers = new Array(len); - pipeline_transaction_command(self, 'multi', []); + pipeline_transaction_command(self, 'multi', [], -1); // Drain queue, callback will catch 'QUEUED' or error for (var index = 0; index < len; index++) { // The commands may not be shifted off, since they are needed in the result handler diff --git a/test/auth.spec.js b/test/auth.spec.js index ac6548e0488..38f46855609 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -183,7 +183,7 @@ describe('client authentication', function () { } }); client.on('reconnecting', function (params) { - assert.strictEqual(params.error.message, 'Stream connection closed'); + assert.strictEqual(params.error, null); }); }); diff --git a/test/connection.spec.js b/test/connection.spec.js index 531dce59bf5..9b5634ab67b 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -53,7 +53,7 @@ describe('connection tests', function () { } }); client.set('foo', 'bar', function (err, res) { - assert.strictEqual(err.message, 'Redis connection gone from close event.'); + assert.strictEqual(err.message, 'Stream connection ended and running command aborted. It might have been processed.'); called = -1; }); }); @@ -62,7 +62,7 @@ describe('connection tests', function () { var called = false; client = redis.createClient(9999); client.set('foo', 'bar', function (err, res) { - assert.strictEqual(err.message, 'Redis connection gone from close event.'); + assert.strictEqual(err.message, 'Stream connection ended and running command aborted. It might have been processed.'); called = true; }); var bool = client.quit(function (err, res) { @@ -277,13 +277,12 @@ describe('connection tests', function () { text += data; return ''; }); - var end = helper.callFuncAfter(done, 2); client = redis.createClient({ retryStrategy: function (options) { if (options.totalRetryTime > 150) { client.set('foo', 'bar', function (err, res) { assert.strictEqual(err.message, 'Connection timeout'); - end(); + done(); }); // Pass a individual error message to the error handler return new Error('Connection timeout'); @@ -294,28 +293,23 @@ describe('connection tests', function () { retryMaxDelay: 123, port: 9999 }); - - client.on('error', function (err) { - unhookIntercept(); + process.nextTick(function () { 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(); + unhookIntercept(); }); }); 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(); + done(); }); return false; } @@ -323,11 +317,6 @@ describe('connection tests', function () { }, port: 9999 }); - - client.on('error', function (err) { - assert.strictEqual(err.code, 'ECONNREFUSED'); - end(); - }); }); }); diff --git a/test/multi.spec.js b/test/multi.spec.js index ec2a81aafc1..48acf3f63af 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -180,7 +180,8 @@ describe("The 'multi' method", function () { client.multi([['set', 'foo', 'bar'], ['get', 'foo']]).exec(function (err, res) { assert(/Redis connection in broken state/.test(err.message)); - assert.strictEqual(err.errors.length, 0); + assert.strictEqual(err.errors.length, 2); + assert.strictEqual(err.errors[0].args.length, 2); }); }); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 774e57a46d1..548e70151e0 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -366,7 +366,7 @@ describe('The node_redis client', function () { client = redis.createClient(); client.quit(function () { client.get('foo', function (err, res) { - assert(err.message.indexOf('Redis connection gone') !== -1); + assert.strictEqual(err.message, 'Stream connection ended and running command aborted. It might have been processed.'); assert.strictEqual(client.offline_queue.length, 0); done(); }); @@ -1024,14 +1024,25 @@ describe('The node_redis client', function () { helper.killConnection(client); }); + var end = helper.callFuncAfter(done, 3); client.on('error', function (err) { - if (/uncertain state/.test(err.message)) { - assert.equal(client.command_queue.length, 0); - done(); + if (err.command === 'EXEC') { + assert.strictEqual(client.command_queue.length, 0); + assert.strictEqual(err.errors.length, 9); + assert.strictEqual(err.errors[1].command, 'SET'); + assert.deepEqual(err.errors[1].args, ['foo1', 'bar1']); + end(); + } else if (err.code === 'UNCERTAIN_STATE') { + assert.strictEqual(client.command_queue.length, 0); + assert.strictEqual(err.errors.length, 4); + assert.strictEqual(err.errors[0].command, 'SET'); + assert.deepEqual(err.errors[0].args, ['foo0', 'bar0']); + end(); } else { assert.equal(err.code, 'ECONNREFUSED'); assert.equal(err.errno, 'ECONNREFUSED'); assert.equal(err.syscall, 'connect'); + end(); } }); }); @@ -1101,14 +1112,21 @@ describe('The node_redis client', function () { helper.killConnection(client); }); + var end = helper.callFuncAfter(done, 3); client.on('error', function (err) { - if (err.code === 'UNCERTAIN_STATE') { + if (err.command === 'EXEC') { assert.equal(client.command_queue.length, 0); - done(); + assert.equal(err.errors.length, 9); + end(); + } else if (err.code === 'UNCERTAIN_STATE') { + assert.equal(client.command_queue.length, 0); + assert.equal(err.errors.length, 4); + end(); } else { assert.equal(err.code, 'ECONNREFUSED'); assert.equal(err.errno, 'ECONNREFUSED'); assert.equal(err.syscall, 'connect'); + end(); } }); }); From e58e31022568d38c1b5829e353be9a88d7386c68 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 Apr 2016 02:09:25 +0200 Subject: [PATCH 18/29] Remove unnecessary unallocation. This is done by the queue itself The total size is kept in the queue but this does not have to be reset each time --- index.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/index.js b/index.js index 9161e358a47..99438d4b9b6 100644 --- a/index.js +++ b/index.js @@ -502,8 +502,6 @@ RedisClient.prototype.send_offline_queue = function () { this.internal_send_command(command_obj.command, command_obj.args, command_obj.callback, command_obj.call_on_write); } this.drain(); - // Even though items were shifted off, Queue backing store still uses memory until next add, so just get a new Queue - this.offline_queue = new Queue(); }; var retry_connection = function (self, error) { From 8308a3e6ae8bb3ac1c4f72ae37ab9cb5b8b11ab8 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 Apr 2016 02:10:58 +0200 Subject: [PATCH 19/29] Update dependencies --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 48af96aac2a..2c4ea07519e 100644 --- a/package.json +++ b/package.json @@ -26,8 +26,8 @@ }, "dependencies": { "double-ended-queue": "^2.1.0-0", - "redis-commands": "^1.1.0", - "redis-parser": "^1.2.0" + "redis-commands": "^1.2.0", + "redis-parser": "^1.3.0" }, "engines": { "node": ">=0.10.0" From 5fac5958c3d41e0c9e18fb8811ffe5fd9df871af Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 14 Apr 2016 02:11:13 +0200 Subject: [PATCH 20/29] Fix async test executed sync --- test/connection.spec.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/connection.spec.js b/test/connection.spec.js index 9b5634ab67b..aa86a7e9db2 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -356,7 +356,7 @@ describe('connection tests', function () { }); }); - it('use the system socket timeout if the connect_timeout has not been provided', function () { + it('use the system socket timeout if the connect_timeout has not been provided', function (done) { client = redis.createClient({ parser: parser, host: '2001:db8::ff00:42:8329' // auto detect ip v6 @@ -365,6 +365,7 @@ describe('connection tests', function () { assert.strictEqual(client.connection_options.family, 6); process.nextTick(function () { assert.strictEqual(client.stream.listeners('timeout').length, 0); + done(); }); }); From 0dc45bd0a3937551c013899b202250c6cd9edeff Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 19 Apr 2016 00:13:09 +0200 Subject: [PATCH 21/29] Improve pub sub mode further --- index.js | 66 ++++++++++++++++++++--------------------- lib/command.js | 3 +- test/pubsub.spec.js | 71 ++++++++++++++++++++++++++++++--------------- 3 files changed, 80 insertions(+), 60 deletions(-) diff --git a/index.js b/index.js index 99438d4b9b6..9ee001e6100 100644 --- a/index.js +++ b/index.js @@ -148,6 +148,7 @@ function RedisClient (options, stream) { this.old_state = null; this.fire_strings = true; // Determine if strings or buffers should be written to the stream this.pipeline = false; + this.sub_commands_left = 0; this.times_connected = 0; this.options = options; this.buffers = options.return_buffers || options.detect_buffers; @@ -645,6 +646,11 @@ RedisClient.prototype.return_error = function (err) { } } + // Count down pub sub mode if in entering modus + if (this.pub_sub_mode > 1) { + this.pub_sub_mode--; + } + var match = err.message.match(utils.err_code); // LUA script could return user errors that don't behave like all other errors! if (match) { @@ -677,50 +683,39 @@ function normal_reply (self, reply) { } } -function set_subscribe (self, type, subscribe, channel) { - // Every channel has to be saved / removed one after the other and the type has to be the same too, - // to make sure partly subscribe / unsubscribe works well together - if (subscribe) { - self.subscription_set[type + '_' + channel] = channel; - } else { - type = type === 'unsubscribe' ? 'subscribe' : 'psubscribe'; // Make types consistent - delete self.subscription_set[type + '_' + channel]; - } -} - -function subscribe_unsubscribe (self, reply, type, subscribe) { +function subscribe_unsubscribe (self, reply, type) { // Subscribe commands take an optional callback and also emit an event, but only the _last_ response is included in the callback // The pub sub commands return each argument in a separate return value and have to be handled that way var command_obj = self.command_queue.get(0); var buffer = self.options.return_buffers || self.options.detect_buffers && command_obj.buffer_args; var channel = (buffer || reply[1] === null) ? reply[1] : reply[1].toString(); var count = +reply[2]; // Return the channel counter as number no matter if `string_numbers` is activated or not - debug('Subscribe / unsubscribe command'); + debug(type, channel); // Emit first, then return the callback if (channel !== null) { // Do not emit or "unsubscribe" something if there was no channel to unsubscribe from self.emit(type, channel, count); - set_subscribe(self, type, subscribe, channel); - } - if (command_obj.sub_commands_left <= 1) { - if (count !== 0) { - if (!subscribe && command_obj.args.length === 0) { // Unsubscribe from all channels - command_obj.sub_commands_left = count; - return; - } + if (type === 'subscribe' || type === 'psubscribe') { + self.subscription_set[type + '_' + channel] = channel; } else { + type = type === 'unsubscribe' ? 'subscribe' : 'psubscribe'; // Make types consistent + delete self.subscription_set[type + '_' + channel]; + } + } + + if (command_obj.args.length === 1 || self.sub_commands_left === 1 || command_obj.args.length === 0 && (count === 0 || channel === null)) { + if (count === 0) { // unsubscribed from all channels var running_command; var i = 1; + self.pub_sub_mode = 0; // Deactivating pub sub mode // This should be a rare case and therefore handling it this way should be good performance wise for the general case while (running_command = self.command_queue.get(i)) { if (SUBSCRIBE_COMMANDS[running_command.command]) { - self.command_queue.shift(); - self.pub_sub_mode = i; - return; + self.pub_sub_mode = i; // Entering pub sub mode again + break; } i++; } - self.pub_sub_mode = 0; } self.command_queue.shift(); if (typeof command_obj.callback === 'function') { @@ -728,8 +723,13 @@ function subscribe_unsubscribe (self, reply, type, subscribe) { // Evaluate to change this in v.3 to return all subscribed / unsubscribed channels in an array including the number of channels subscribed too command_obj.callback(null, channel); } + self.sub_commands_left = 0; } else { - command_obj.sub_commands_left--; + if (self.sub_commands_left !== 0) { + self.sub_commands_left--; + } else { + self.sub_commands_left = command_obj.args.length ? command_obj.args.length - 1 : count; + } } } @@ -751,12 +751,8 @@ function return_pub_sub (self, reply) { } else { self.emit('pmessage', reply[1], reply[2], reply[3]); } - } else if (type === 'subscribe' || type === 'psubscribe') { - subscribe_unsubscribe(self, reply, type, true); - } else if (type === 'unsubscribe' || type === 'punsubscribe') { - subscribe_unsubscribe(self, reply, type, false); } else { - normal_reply(self, reply); + subscribe_unsubscribe(self, reply, type); } } @@ -787,10 +783,12 @@ RedisClient.prototype.return_reply = function (reply) { } else if (this.pub_sub_mode !== 1) { this.pub_sub_mode--; normal_reply(this, reply); - } else if (reply instanceof Array && reply.length > 2 && reply[0]) { - return_pub_sub(this, reply); - } else { + } else if (!(reply instanceof Array) || reply.length <= 2) { + // Only PING and QUIT are allowed in this context besides the pub sub commands + // Ping replies with ['pong', null|value] and quit with 'OK' normal_reply(this, reply); + } else { + return_pub_sub(this, reply); } }; diff --git a/lib/command.js b/lib/command.js index ee1181ea7e3..e63d338b5e7 100644 --- a/lib/command.js +++ b/lib/command.js @@ -4,10 +4,9 @@ // a named constructor helps it show up meaningfully in the V8 CPU profiler and in heap snapshots. function Command (command, args, buffer_args, callback) { this.command = command; - this.args = args; // We only need the args for the offline commands => move them into another class. We need the number of args though for pub sub + this.args = args; this.buffer_args = buffer_args; this.callback = callback; - this.sub_commands_left = args.length; } function OfflineCommand (command, args, callback, call_on_write) { diff --git a/test/pubsub.spec.js b/test/pubsub.spec.js index f9401f63ad5..ffce23f442d 100644 --- a/test/pubsub.spec.js +++ b/test/pubsub.spec.js @@ -79,6 +79,7 @@ describe('publish/subscribe', function () { it('does not fire subscribe events after reconnecting', function (done) { var i = 0; + var end = helper.callFuncAfter(done, 2); sub.on('subscribe', function (chnl, count) { assert.strictEqual(typeof count, 'number'); assert.strictEqual(++i, count); @@ -91,9 +92,10 @@ describe('publish/subscribe', function () { sub.unsubscribe(function (err, res) { // Do not pass a channel here! assert.strictEqual(sub.pub_sub_mode, 2); assert.deepEqual(sub.subscription_set, {}); + end(); }); sub.set('foo', 'bar', helper.isString('OK')); - sub.subscribe(channel2, done); + sub.subscribe(channel2, end); }); }); @@ -181,25 +183,19 @@ describe('publish/subscribe', function () { sub.subscribe('chan9'); sub.unsubscribe('chan9'); pub.publish('chan8', 'something'); - sub.subscribe('chan9', function () { - return done(); - }); + sub.subscribe('chan9', done); }); it('handles SUB_UNSUB_MSG_SUB 2', function (done) { - sub.psubscribe('abc*'); + sub.psubscribe('abc*', helper.isString('abc*')); sub.subscribe('xyz'); sub.unsubscribe('xyz'); pub.publish('abcd', 'something'); - sub.subscribe('xyz', function () { - return done(); - }); + sub.subscribe('xyz', done); }); it('emits end event if quit is called from within subscribe', function (done) { - sub.on('end', function () { - return done(); - }); + sub.on('end', done); sub.on('subscribe', function (chnl, count) { sub.quit(); }); @@ -236,6 +232,10 @@ describe('publish/subscribe', function () { var end = helper.callFuncAfter(done, 2); sub.select(3); sub.set('foo', 'bar'); + sub.set('failure', helper.isError()); // Triggering a warning while subscribing should work + sub.mget('foo', 'bar', 'baz', 'hello', 'world', function (err, res) { + assert.deepEqual(res, ['bar', null, null, null, null]); + }); sub.subscribe('somechannel', 'another channel', function (err, res) { end(); sub.stream.destroy(); @@ -280,7 +280,7 @@ describe('publish/subscribe', function () { it('should only resubscribe to channels not unsubscribed earlier on a reconnect', function (done) { sub.subscribe('/foo', '/bar'); - sub.unsubscribe('/bar', function () { + sub.batch().unsubscribe(['/bar'], function () { pub.pubsub('channels', function (err, res) { assert.deepEqual(res, ['/foo']); sub.stream.destroy(); @@ -291,7 +291,7 @@ describe('publish/subscribe', function () { }); }); }); - }); + }).exec(); }); it('unsubscribes, subscribes, unsubscribes... single and multiple entries mixed. Withouth callbacks', function (done) { @@ -490,7 +490,7 @@ describe('publish/subscribe', function () { return_buffers: true }); sub2.on('ready', function () { - sub2.psubscribe('*'); + sub2.batch().psubscribe('*', helper.isString('*')).exec(); sub2.subscribe('/foo'); sub2.on('pmessage', function (pattern, channel, message) { assert.strictEqual(pattern.inspect(), new Buffer('*').inspect()); @@ -501,32 +501,58 @@ describe('publish/subscribe', function () { pub.pubsub('numsub', '/foo', function (err, res) { assert.deepEqual(res, ['/foo', 2]); }); + // sub2 is counted twice as it subscribed with psubscribe and subscribe pub.publish('/foo', 'hello world', helper.isNumber(3)); }); }); it('allows to listen to pmessageBuffer and pmessage', function (done) { var batch = sub.batch(); + var end = helper.callFuncAfter(done, 6); + assert.strictEqual(sub.message_buffers, false); batch.psubscribe('*'); batch.subscribe('/foo'); batch.unsubscribe('/foo'); - batch.unsubscribe(); - batch.subscribe(['/foo']); + batch.unsubscribe(helper.isNull()); + batch.subscribe(['/foo'], helper.isString('/foo')); batch.exec(); assert.strictEqual(sub.shouldBuffer, false); sub.on('pmessageBuffer', function (pattern, channel, message) { assert.strictEqual(pattern.inspect(), new Buffer('*').inspect()); assert.strictEqual(channel.inspect(), new Buffer('/foo').inspect()); - sub.quit(done); + sub.quit(end); }); + // Either message_buffers or buffers has to be true, but not both at the same time + assert.notStrictEqual(sub.message_buffers, sub.buffers); sub.on('pmessage', function (pattern, channel, message) { assert.strictEqual(pattern, '*'); assert.strictEqual(channel, '/foo'); + assert.strictEqual(message, 'hello world'); + end(); }); - pub.pubsub('numsub', '/foo', function (err, res) { - assert.deepEqual(res, ['/foo', 1]); + sub.on('message', function (channel, message) { + assert.strictEqual(channel, '/foo'); + assert.strictEqual(message, 'hello world'); + end(); }); - pub.publish('/foo', 'hello world', helper.isNumber(2)); + setTimeout(function () { + pub.pubsub('numsub', '/foo', function (err, res) { + // There's one subscriber to this channel + assert.deepEqual(res, ['/foo', 1]); + end(); + }); + pub.pubsub('channels', function (err, res) { + // There's exactly one channel that is listened too + assert.deepEqual(res, ['/foo']); + end(); + }); + pub.pubsub('numpat', function (err, res) { + // One pattern is active + assert.strictEqual(res, 1); + end(); + }); + pub.publish('/foo', 'hello world', helper.isNumber(2)); + }, 50); }); }); @@ -536,10 +562,7 @@ describe('publish/subscribe', function () { }); it('executes callback when punsubscribe is called and there are no subscriptions', function (done) { - pub.punsubscribe(function (err, results) { - assert.strictEqual(null, results); - done(err); - }); + pub.batch().punsubscribe(helper.isNull()).exec(done); }); }); From f500398cabe52213e8075a86ed297856e903c7dc Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 19 Apr 2016 00:17:58 +0200 Subject: [PATCH 22/29] Run tests only with the js parser instead of hiredis and js parser from now on This removes the optional-dev-dependency as this is not needed from now on anymore --- package.json | 2 -- test/helper.js | 9 +++++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 2c4ea07519e..d92511f6217 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,6 @@ "coverage": "nyc report --reporter=html", "benchmark": "node benchmarks/multi_bench.js", "test": "nyc --cache mocha ./test/*.js ./test/commands/*.js --timeout=8000", - "pretest": "optional-dev-dependency hiredis", "posttest": "eslint . --fix" }, "dependencies": { @@ -40,7 +39,6 @@ "metrics": "^0.1.9", "mocha": "^2.3.2", "nyc": "^6.0.0", - "optional-dev-dependency": "^1.1.0", "tcp-port-used": "^0.1.2", "uuid": "^2.0.1", "win-spawn": "^2.0.0" diff --git a/test/helper.js b/test/helper.js index bb408a19fd1..e851b7f7b35 100644 --- a/test/helper.js +++ b/test/helper.js @@ -163,10 +163,11 @@ module.exports = { } var parsers = ['javascript']; var protocols = ['IPv4']; - try { - require('hiredis'); - parsers.push('hiredis'); - } catch (e) {/* ignore eslint */} + // The js parser works the same as the hiredis parser, just activate this if you want to be on the safe side + // try { + // require('hiredis'); + // parsers.push('hiredis'); + // } catch (e) {/* ignore eslint */} if (process.platform !== 'win32') { protocols.push('IPv6', '/tmp/redis.sock'); } From 625f14e6baf9dc6333496132b18ceb68e0f41d2c Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 19 Apr 2016 18:46:56 +0200 Subject: [PATCH 23/29] Fix address always set to 127.0.0.1:6379 in case the host/port is set in the tls options --- index.js | 12 ++++++++---- test/tls.spec.js | 12 ++++++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/index.js b/index.js index 9ee001e6100..777383173ef 100644 --- a/index.js +++ b/index.js @@ -50,6 +50,14 @@ function RedisClient (options, stream) { EventEmitter.call(this); var cnx_options = {}; var self = this; + /* istanbul ignore next: travis does not work with stunnel atm. Therefore the tls tests are skipped on travis */ + for (var tls_option in options.tls) { // jshint ignore: line + cnx_options[tls_option] = options.tls[tls_option]; + // Copy the tls options into the general options to make sure the address is set right + if (tls_option === 'port' || tls_option === 'host' || tls_option === 'path' || tls_option === 'family') { + options[tls_option] = options.tls[tls_option]; + } + } if (stream) { // The stream from the outside is used so no connection from this side is triggered but from the server this client should talk to // Reconnect etc won't work with this. This requires monkey patching to work, so it is not officially supported @@ -64,10 +72,6 @@ function RedisClient (options, stream) { cnx_options.family = (!options.family && net.isIP(cnx_options.host)) || (options.family === 'IPv6' ? 6 : 4); this.address = cnx_options.host + ':' + cnx_options.port; } - /* istanbul ignore next: travis does not work with stunnel atm. Therefore the tls tests are skipped on travis */ - 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) { diff --git a/test/tls.spec.js b/test/tls.spec.js index 40a424c27c2..d977ee7d9ac 100644 --- a/test/tls.spec.js +++ b/test/tls.spec.js @@ -60,6 +60,7 @@ describe('TLS connection tests', function () { tls: tls_options }); var time = 0; + assert.strictEqual(client.address, '127.0.0.1:' + tls_port); client.once('ready', function () { helper.killConnection(client); @@ -87,18 +88,20 @@ describe('TLS connection tests', function () { describe('when not connected', function () { - it('connect with host and port provided in the options object', function (done) { + it('connect with host and port provided in the tls object', function (done) { if (skip) this.skip(); + var tls = utils.clone(tls_options); + tls.port = tls_port; + tls.host = 'localhost'; client = redis.createClient({ - host: 'localhost', connect_timeout: 1000, - port: tls_port, - tls: tls_options + tls: tls }); // verify connection is using TCP, not UNIX socket assert.strictEqual(client.connection_options.host, 'localhost'); assert.strictEqual(client.connection_options.port, tls_port); + assert.strictEqual(client.address, 'localhost:' + tls_port); assert(client.stream.encrypted); client.set('foo', 'bar'); @@ -115,6 +118,7 @@ describe('TLS connection tests', function () { port: tls_port, tls: faulty_cert }); + assert.strictEqual(client.address, 'localhost:' + tls_port); client.on('error', function (err) { assert(/DEPTH_ZERO_SELF_SIGNED_CERT/.test(err.code || err.message), err); client.end(true); From f7c4d131be82c4e0b42c1bfebd187b67f53b9981 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Apr 2016 02:49:06 +0200 Subject: [PATCH 24/29] Remove jshint comments and update istanbul comments --- index.js | 7 ++++--- lib/createClient.js | 2 +- lib/extendedApi.js | 2 +- lib/individualCommands.js | 5 ++--- lib/utils.js | 1 + 5 files changed, 9 insertions(+), 8 deletions(-) diff --git a/index.js b/index.js index 777383173ef..a94710a67aa 100644 --- a/index.js +++ b/index.js @@ -51,7 +51,7 @@ function RedisClient (options, stream) { var cnx_options = {}; var self = this; /* istanbul ignore next: travis does not work with stunnel atm. Therefore the tls tests are skipped on travis */ - for (var tls_option in options.tls) { // jshint ignore: line + for (var tls_option in options.tls) { cnx_options[tls_option] = options.tls[tls_option]; // Copy the tls options into the general options to make sure the address is set right if (tls_option === 'port' || tls_option === 'host' || tls_option === 'path' || tls_option === 'family') { @@ -102,7 +102,7 @@ function RedisClient (options, stream) { if (options.socket_keepalive === undefined) { options.socket_keepalive = true; } - for (var command in options.rename_commands) { // jshint ignore: line + for (var command in options.rename_commands) { options.rename_commands[command.toLowerCase()] = options.rename_commands[command]; } options.return_buffers = !!options.return_buffers; @@ -438,7 +438,7 @@ RedisClient.prototype.on_ready = function () { } }; debug('Sending pub/sub on_ready commands'); - for (var key in this.subscription_set) { // jshint ignore: line + for (var key in this.subscription_set) { var command = key.slice(0, key.indexOf('_')); var args = self.subscription_set[key]; self.internal_send_command(command, [args], callback); @@ -934,6 +934,7 @@ RedisClient.prototype.internal_send_command = function (command, args, callback, } // Handle `CLIENT REPLY ON|OFF|SKIP` // This has to be checked after call_on_write + /* istanbul ignore else: TODO: Remove this as soon as we test Redis 3.2 on travis */ if (this.reply === 'ON') { this.command_queue.push(command_obj); } else { diff --git a/lib/createClient.js b/lib/createClient.js index f97823e01d2..72d5e2745cb 100644 --- a/lib/createClient.js +++ b/lib/createClient.js @@ -44,7 +44,7 @@ module.exports = function createClient (port_arg, host_arg, options) { } if (parsed.search !== '') { var elem; - for (elem in parsed.query) { // jshint ignore: line + for (elem in parsed.query) { // If options are passed twice, only the parsed options will be used if (elem in options) { if (options[elem] === parsed.query[elem]) { diff --git a/lib/extendedApi.js b/lib/extendedApi.js index e512ae97e36..44dc9e28232 100644 --- a/lib/extendedApi.js +++ b/lib/extendedApi.js @@ -89,7 +89,7 @@ RedisClient.prototype.duplicate = function (options, callback) { } var existing_options = utils.clone(this.options); options = utils.clone(options); - for (var elem in options) { // jshint ignore: line + for (var elem in options) { existing_options[elem] = options[elem]; } var client = new RedisClient(existing_options); diff --git a/lib/individualCommands.js b/lib/individualCommands.js index de03e210f37..f1b1984f07b 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -147,7 +147,6 @@ function info_callback (self, callback) { } } 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); @@ -337,7 +336,7 @@ RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function hmset () { } } 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 + for (var field in arguments[1]) { arr.push(field, arguments[1][field]); } callback = arguments[2]; @@ -376,7 +375,7 @@ Multi.prototype.hmset = Multi.prototype.HMSET = function hmset () { } } 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 + for (var field in arguments[1]) { arr.push(field, arguments[1][field]); } callback = arguments[2]; diff --git a/lib/utils.js b/lib/utils.js index cd04429b0c7..d3d9c2fa594 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -94,6 +94,7 @@ function replyInOrder (self, callback, err, res, queue) { // If the queue is explicitly passed, use that, otherwise fall back to the offline queue first, // as there might be commands in both queues at the same time var command_obj; + /* istanbul ignore if: TODO: Remove this as soon as we test Redis 3.2 on travis */ if (queue) { command_obj = queue.peekBack(); } else { From eae16938cdfb498fb6201eb0539cfa7cb7827a65 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Apr 2016 02:54:42 +0200 Subject: [PATCH 25/29] Add monitor transaction warning / error --- lib/individualCommands.js | 11 +++------ lib/multi.js | 8 +++++++ test/multi.spec.js | 48 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/lib/individualCommands.js b/lib/individualCommands.js index f1b1984f07b..c9cef49b1b6 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -71,14 +71,9 @@ Multi.prototype.monitor = Multi.prototype.MONITOR = function monitor (callback) this.queue.push(['monitor', [], monitor_callback(this._client, callback)]); return this; } - var err = new Error( - 'You used the monitor command in combination with a transaction. Due to faulty return values of ' + - 'Redis in this context, the monitor command is now executed without transaction instead and ignored ' + - 'in the multi statement.' - ); - err.command = 'MONITOR'; - utils.reply_in_order(this._client, callback, err); - this._client.monitor('monitor', callback); + // Set multi monitoring to indicate the exec that it should abort + // Remove this "hack" as soon as Redis might fix this + this.monitoring = true; return this; }; diff --git a/lib/multi.js b/lib/multi.js index a4399473c98..2a7431e9b66 100644 --- a/lib/multi.js +++ b/lib/multi.js @@ -85,6 +85,14 @@ function multi_callback (self, err, replies) { } Multi.prototype.exec_transaction = function exec_transaction (callback) { + if (this.monitoring || this._client.monitoring) { + var err = new Error( + 'Using transaction with a client that is in monitor mode does not work due to faulty return values of Redis.' + ); + err.command = 'EXEC'; + err.code = 'EXECABORT'; + return utils.reply_in_order(this._client, callback, err); + } var self = this; var len = self.queue.length; self.errors = []; diff --git a/test/multi.spec.js b/test/multi.spec.js index 48acf3f63af..e49e3390daa 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -3,6 +3,7 @@ var assert = require('assert'); var config = require('./lib/config'); var helper = require('./helper'); +var utils = require('../lib/utils'); var redis = config.redis; var zlib = require('zlib'); var client; @@ -129,6 +130,53 @@ describe("The 'multi' method", function () { client = redis.createClient.apply(null, args); }); + describe('monitor and transactions do not work together', function () { + + it('results in a execabort', function (done) { + // Check that transactions in combination with monitor result in an error + client.monitor(function (e) { + client.on('error', function (err) { + assert.strictEqual(err.code, 'EXECABORT'); + done(); + }); + var multi = client.multi(); + multi.set('hello', 'world'); + multi.exec(); + }); + }); + + it('results in a execabort #2', function (done) { + // Check that using monitor with a transactions results in an error + client.multi().set('foo', 'bar').monitor().exec(function (err, res) { + assert.strictEqual(err.code, 'EXECABORT'); + done(); + }); + }); + + it('sanity check', function (done) { + // Remove the listener and add it back again after the error + var mochaListener = helper.removeMochaListener(); + process.on('uncaughtException', function (err) { + helper.removeMochaListener(); + process.on('uncaughtException', mochaListener); + done(); + }); + // Check if Redis still has the error + client.monitor(); + client.send_command('multi'); + client.send_command('set', ['foo', 'bar']); + client.send_command('get', ['foo']); + client.send_command('exec', function (err, res) { + // res[0] is going to be the monitor result of set + // res[1] is going to be the result of the set command + assert(utils.monitor_regex.test(res[0])); + assert.strictEqual(res[1], 'OK'); + assert.strictEqual(res.length, 2); + client.end(false); + }); + }); + }); + it('executes a pipelined multi properly in combination with the offline queue', function (done) { var multi1 = client.multi(); multi1.set('m1', '123'); From ce1678c77809c62d115c1907a00da15583ec5141 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 21 Apr 2016 02:56:06 +0200 Subject: [PATCH 26/29] Improve coverage; make tests ready for Redis 3.2 Add command sanity check --- test/auth.spec.js | 18 +++++++++--------- test/commands/client.spec.js | 8 ++++---- test/commands/hgetall.spec.js | 8 ++++---- test/commands/hmset.spec.js | 2 +- test/connection.spec.js | 18 ++++++++++++++++++ test/multi.spec.js | 2 +- test/node_redis.spec.js | 19 +++++++++++++++++++ test/pubsub.spec.js | 26 ++++++++++++++++++++++---- test/return_buffers.spec.js | 2 +- 9 files changed, 79 insertions(+), 24 deletions(-) diff --git a/test/auth.spec.js b/test/auth.spec.js index 38f46855609..8411a4b618d 100644 --- a/test/auth.spec.js +++ b/test/auth.spec.js @@ -272,13 +272,13 @@ describe('client authentication', function () { var args = config.configureClient(parser, ip, { password: auth }); - client = redis.createClient.apply(redis.createClient, args); + client = redis.createClient.apply(null, args); client.set('foo', 'bar'); client.subscribe('somechannel', 'another channel', function (err, res) { client.once('ready', function () { assert.strictEqual(client.pub_sub_mode, 1); client.get('foo', function (err, res) { - assert.strictEqual(err.message, 'ERR only (P)SUBSCRIBE / (P)UNSUBSCRIBE / QUIT allowed in this context'); + assert(/ERR only \(P\)SUBSCRIBE \/ \(P\)UNSUBSCRIBE/.test(err.message)); done(); }); }); @@ -292,7 +292,7 @@ describe('client authentication', function () { }); }); - it('indivdual commands work properly with batch', function (done) { + it('individual commands work properly with batch', function (done) { // quit => might return an error instead of "OK" in the exec callback... (if not connected) // auth => might return an error instead of "OK" in the exec callback... (if no password is required / still loading on Redis <= 2.4) // This could be fixed by checking the return value of the callback in the exec callback and @@ -302,7 +302,7 @@ describe('client authentication', function () { var args = config.configureClient(parser, 'localhost', { noReadyCheck: true }); - client = redis.createClient.apply(redis.createClient, args); + client = redis.createClient.apply(null, args); assert.strictEqual(client.selected_db, undefined); var end = helper.callFuncAfter(done, 8); client.on('monitor', function () { @@ -317,9 +317,9 @@ describe('client authentication', function () { }) .monitor() .set('foo', 'bar', helper.isString('OK')) - .INFO(function (err, res) { - assert.strictEqual(res.indexOf('# Server\r\nredis_version:'), 0); - assert.deepEqual(client.serverInfo.db5, { avg_ttl: 0, expires: 0, keys: 1 }); + .INFO('stats', function (err, res) { + assert.strictEqual(res.indexOf('# Stats\r\n'), 0); + assert.strictEqual(client.serverInfo.sync_full, '0'); }) .get('foo', helper.isString('bar')) .subscribe(['foo', 'bar']) @@ -328,8 +328,8 @@ describe('client authentication', function () { .psubscribe('*') .quit(helper.isString('OK')) // this might be interesting .exec(function (err, res) { - res[4] = res[4].substr(0, 10); - assert.deepEqual(res, ['OK', 'OK', 'OK', 'OK', '# Server\r\n', 'bar', 'bar', 'foo', '/foo', '*', 'OK']); + res[4] = res[4].substr(0, 9); + assert.deepEqual(res, ['OK', 'OK', 'OK', 'OK', '# Stats\r\n', 'bar', 'bar', 'foo', '/foo', '*', 'OK']); end(); }); }); diff --git a/test/commands/client.spec.js b/test/commands/client.spec.js index 935db8b6926..7ac32ae41ac 100644 --- a/test/commands/client.spec.js +++ b/test/commands/client.spec.js @@ -121,7 +121,7 @@ describe("The 'client' method", function () { var client2; beforeEach(function (done) { - client2 = redis.createClient.apply(redis.createClient, args); + client2 = redis.createClient.apply(null, args); client2.once('ready', function () { done(); }); @@ -136,9 +136,9 @@ describe("The 'client' method", function () { // per chunk. So the execution order is only garanteed on each client var end = helper.callFuncAfter(done, 2); - client.client('setname', 'RUTH', helper.isString('OK')); - client2.client('setname', 'RENEE', helper.isString('OK')); - client2.client('setname', 'MARTIN', helper.isString('OK')); + client.client('setname', 'RUTH'); + client2.client('setname', ['RENEE'], helper.isString('OK')); + client2.client(['setname', 'MARTIN'], helper.isString('OK')); client2.client('getname', function (err, res) { assert.equal(res, 'MARTIN'); end(); diff --git a/test/commands/hgetall.spec.js b/test/commands/hgetall.spec.js index 55a0e247803..8e8b88a309c 100644 --- a/test/commands/hgetall.spec.js +++ b/test/commands/hgetall.spec.js @@ -28,23 +28,23 @@ describe("The 'hgetall' method", function () { assert.strictEqual('1', obj.__proto__.toString()); // eslint-disable-line no-proto assert.strictEqual('23', obj.another.toString()); assert.strictEqual('1234', obj.home.toString()); - return done(err); + done(err); }); }); it('handles fetching keys set using an object', function (done) { - client.HMSET('msg_test', { message: 'hello' }, helper.isString('OK')); + client.batch().HMSET('msg_test', { message: 'hello' }, undefined).exec(); client.hgetall('msg_test', function (err, obj) { assert.strictEqual(1, Object.keys(obj).length); assert.strictEqual(obj.message, 'hello'); - return done(err); + done(err); }); }); it('handles fetching a messing key', function (done) { client.hgetall('missing', function (err, obj) { assert.strictEqual(null, obj); - return done(err); + done(err); }); }); }); diff --git a/test/commands/hmset.spec.js b/test/commands/hmset.spec.js index c96b7c5b670..93514cc2295 100644 --- a/test/commands/hmset.spec.js +++ b/test/commands/hmset.spec.js @@ -39,7 +39,7 @@ describe("The 'hmset' method", function () { }); it('handles object-style syntax and the key being a number', function (done) { - client.HMSET(231232, {'0123456789': 'abcdefghij', 'some manner of key': 'a type of value', 'otherTypes': 555}, helper.isString('OK')); + client.HMSET(231232, {'0123456789': 'abcdefghij', 'some manner of key': 'a type of value', 'otherTypes': 555}, undefined); client.HGETALL(231232, function (err, obj) { assert.equal(obj['0123456789'], 'abcdefghij'); assert.equal(obj['some manner of key'], 'a type of value'); diff --git a/test/connection.spec.js b/test/connection.spec.js index aa86a7e9db2..eb807295ed8 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -318,6 +318,24 @@ describe('connection tests', function () { port: 9999 }); }); + + it('retry_strategy used to reconnect with defaults', function (done) { + client = redis.createClient({ + retry_strategy: function (options) { + client.set('foo', 'bar'); + return null; + } + }); + setTimeout(function () { + client.stream.destroy(); + }, 50); + client.on('error', function (err) { + assert.strictEqual(err.code, 'NR_OFFLINE'); + assert.strictEqual(err.errors.length, 1); + assert.notStrictEqual(err.message, err.errors[0].message); + done(); + }); + }); }); describe('when not connected', function () { diff --git a/test/multi.spec.js b/test/multi.spec.js index e49e3390daa..8f500958fab 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -705,7 +705,7 @@ describe("The 'multi' method", function () { }); multi.get('foo', helper.isString('bar')); multi.exec(function (err, res) { - res[3] = res[3].substr(0, 10); + res[2] = res[2].substr(0, 10); assert.deepEqual(res, ['OK', 'OK', '# Server\r\n', 'bar']); done(); }); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 548e70151e0..c87968dea8a 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -1,6 +1,8 @@ 'use strict'; var assert = require('assert'); +var fs = require('fs'); +var path = require('path'); var config = require('./lib/config'); var helper = require('./helper'); var utils = require('../lib/utils'); @@ -9,6 +11,23 @@ var redis = config.redis; describe('The node_redis client', function () { + it('individual commands sanity check', function (done) { + // All commands should work the same in multi context or without + // Therefor individual commands always have to be handled in both cases + fs.readFile(path.resolve(__dirname, '../lib/individualCommands.js'), 'utf8', function (err, data) { + var client_prototype = data.match(/(\n| = )RedisClient\.prototype.[a-zA-Z_]+/g); + var multi_prototype = data.match(/(\n| = )Multi\.prototype\.[a-zA-Z_]+/g); + // Check that every entry RedisClient entry has a correspondend Multi entry + assert.strictEqual(client_prototype.filter(function (entry) { + return multi_prototype.indexOf(entry.replace('RedisClient', 'Multi')) === -1; + }).length, 4); // multi and batch are included too + assert.strictEqual(client_prototype.length, multi_prototype.length + 4); + // Check that all entries exist in uppercase and in lowercase variants + assert.strictEqual(data.match(/(\n| = )RedisClient\.prototype.[a-z_]+/g).length * 2, client_prototype.length); + done(); + }); + }); + helper.allTests(function (parser, ip, args) { describe('using ' + parser + ' and ' + ip, function () { diff --git a/test/pubsub.spec.js b/test/pubsub.spec.js index ffce23f442d..1d33b129941 100644 --- a/test/pubsub.spec.js +++ b/test/pubsub.spec.js @@ -19,8 +19,8 @@ describe('publish/subscribe', function () { beforeEach(function (done) { var end = helper.callFuncAfter(done, 2); - pub = redis.createClient.apply(redis.createClient, args); - sub = redis.createClient.apply(redis.createClient, args); + pub = redis.createClient.apply(null, args); + sub = redis.createClient.apply(null, args); pub.once('connect', function () { pub.flushdb(function () { end(); @@ -576,7 +576,7 @@ describe('publish/subscribe', function () { }); // Get is forbidden sub.get('foo', function (err, res) { - assert.strictEqual(err.message, 'ERR only (P)SUBSCRIBE / (P)UNSUBSCRIBE / QUIT allowed in this context'); + assert(/^ERR only \(P\)SUBSCRIBE \/ \(P\)UNSUBSCRIBE/.test(err.message)); assert.strictEqual(err.command, 'GET'); }); // Quit is allowed @@ -586,7 +586,7 @@ describe('publish/subscribe', function () { it('emit error if only pub sub commands are allowed without callback', function (done) { sub.subscribe('channel'); sub.on('error', function (err) { - assert.strictEqual(err.message, 'ERR only (P)SUBSCRIBE / (P)UNSUBSCRIBE / QUIT allowed in this context'); + assert(/^ERR only \(P\)SUBSCRIBE \/ \(P\)UNSUBSCRIBE/.test(err.message)); assert.strictEqual(err.command, 'GET'); done(); }); @@ -639,6 +639,24 @@ describe('publish/subscribe', function () { }); }); + it('arguments variants', function (done) { + sub.batch() + .info(['stats']) + .info() + .client('KILL', ['type', 'pubsub']) + .client('KILL', ['type', 'pubsub'], function () {}) + .unsubscribe() + .psubscribe(['pattern:*']) + .punsubscribe('unkown*') + .punsubscribe(['pattern:*']) + .exec(function (err, res) { + sub.client('kill', ['type', 'pubsub']); + sub.psubscribe('*'); + sub.punsubscribe('pa*'); + sub.punsubscribe(['a', '*'], done); + }); + }); + afterEach(function () { // Explicitly ignore still running commands pub.end(false); diff --git a/test/return_buffers.spec.js b/test/return_buffers.spec.js index 1274c8be9e8..eb18d79393f 100644 --- a/test/return_buffers.spec.js +++ b/test/return_buffers.spec.js @@ -252,7 +252,7 @@ describe('return_buffers', function () { var subConnected; pub = redis.createClient.apply(redis.createClient, basicArgs); - sub = redis.createClient.apply(redis.createClient, args); + sub = redis.createClient.apply(null, args); pub.once('connect', function () { pub.flushdb(function () { pubConnected = true; From 5368e7477efba42d6d1d95b815c2608200911d60 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 22 Apr 2016 02:29:06 +0200 Subject: [PATCH 27/29] Add changelog --- changelog.md | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/changelog.md b/changelog.md index ae2ed508dd4..274986b393c 100644 --- a/changelog.md +++ b/changelog.md @@ -1,6 +1,43 @@ Changelog ========= +## v.2.6.0-2 - xx Apr, 2016 + +Features + +- Added support for the new `CLIENT REPLY ON|OFF|SKIP` command (Redis v.3.2) +- Added support for camelCase + - The Node.js landscape default is to use camelCase. node_redis is a bit out of the box here + but from now on it is possible to use both, just as you prefer! + - If there's any documented variable missing as camelCased, please open a issue for it +- Improve error handling significantly + - Only emit an error if the error has not already been handled in a callback + - Emit an error if a command would otherwise silently fail (no callback present) + - Improved unspecific error messages e.g. "Connection gone from end / close event" + - Added `args` to command errors to improve identification of the error + - Added origin to errors if there's e.g. a connection error + - Added ReplyError class. All Redis errors are from now on going to be of that class + - Added AbortError class. A subclass of AbortError. All unresolved and by node_redis rejected commands are from now on of that class + - Added AggregateError class. If a unresolved and by node_redis rejected command has no callback and + this applies to more than a single command, the errors for the commands without callback are aggregated + to a single error that is emitted in debug_mode in that case. +- Added `message_buffer` / `pmessage_buffer` events. That event is always going to emit a buffer + - Listening to the `message` event at the same time is always going to return the same message as string +- Added callback option to the duplicate function +- Added support for `__proto__` and other reserved keywords as hgetall field +- Updated [redis-commands](https://github.com/NodeRedis/redis-commands) dependency ([changelog](https://github.com/NodeRedis/redis-commands/releases/tag/v.1.2.0)) + +Bugfixes + +- Fixed v.2.5.0 auth command regression (under special circumstances a reconnect would not authenticate properly) +- Fixed v.2.6.0-0 pub sub mode and quit command regressions: + - Entering pub sub mode not working if a earlier called and still running command returned an error + - Unsubscribe callback not called if unsubscribing from all channels and resubscribing right away + - Quit command resulting in an error in some cases +- Fixed special handled functions in batch and multi context not working the same as without (e.g. select and info) + - Be aware that not all commands work in combination with transactions but they all work with batch +- Fixed address always set to 127.0.0.1:6379 in case host / port is set in the `tls` options instead of the general options + ## v.2.6.0-1 - 01 Apr, 2016 A second pre-release with further fixes. This is likely going to be released as 2.6.0 stable without further changes. From bf394923fd548419928ee9108aa87bc16a6e8d33 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 25 Apr 2016 01:35:56 +0200 Subject: [PATCH 28/29] Use built-in error classes to make errors more specific --- lib/createClient.js | 10 +++++----- lib/extendedApi.js | 6 +++--- lib/multi.js | 3 +-- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/createClient.js b/lib/createClient.js index 72d5e2745cb..a019fc7a383 100644 --- a/lib/createClient.js +++ b/lib/createClient.js @@ -12,7 +12,7 @@ module.exports = function createClient (port_arg, host_arg, options) { host = host_arg; } else { if (options && host_arg) { - throw new Error('Unknown type of connection in createClient()'); + throw new TypeError('Unknown type of connection in createClient()'); } options = options || host_arg; } @@ -50,14 +50,14 @@ module.exports = function createClient (port_arg, host_arg, 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'); + throw new RangeError('The ' + elem + ' option is added twice and does not match'); } } 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'); + throw new RangeError('The redis url must begin with slashes "//" or contain slashes after the redis protocol'); } else { options.path = port_arg; } @@ -67,12 +67,12 @@ module.exports = function createClient (port_arg, host_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'); + throw new TypeError('To many arguments passed to createClient. Please only pass the options object'); } } if (!options) { - throw new Error('Unknown type of connection in createClient()'); + throw new TypeError('Unknown type of connection in createClient()'); } return options; diff --git a/lib/extendedApi.js b/lib/extendedApi.js index 44dc9e28232..ef6a2faa6ea 100644 --- a/lib/extendedApi.js +++ b/lib/extendedApi.js @@ -13,7 +13,7 @@ All documented and exposed API belongs in here RedisClient.prototype.send_command = RedisClient.prototype.sendCommand = function (command, args, callback) { // Throw to fail early instead of relying in order in this case if (typeof command !== 'string') { - throw new Error('Wrong input type "' + (command !== null && command !== undefined ? command.constructor.name : command) + '" for command name'); + throw new TypeError('Wrong input type "' + (command !== null && command !== undefined ? command.constructor.name : command) + '" for command name'); } if (!Array.isArray(args)) { if (args === undefined || args === null) { @@ -22,11 +22,11 @@ RedisClient.prototype.send_command = RedisClient.prototype.sendCommand = functio callback = args; args = []; } else { - throw new Error('Wrong input type "' + args.constructor.name + '" for args'); + throw new TypeError('Wrong input type "' + args.constructor.name + '" for args'); } } if (typeof callback !== 'function' && callback !== undefined) { - throw new Error('Wrong input type "' + (callback !== null ? callback.constructor.name : 'null') + '" for callback function'); + throw new TypeError('Wrong input type "' + (callback !== null ? callback.constructor.name : 'null') + '" for callback function'); } // Using the raw multi command is only possible with this function diff --git a/lib/multi.js b/lib/multi.js index 2a7431e9b66..433c45bd501 100644 --- a/lib/multi.js +++ b/lib/multi.js @@ -46,7 +46,6 @@ function multi_callback (self, err, replies) { if (err) { err.errors = self.errors; - err.command = 'EXEC'; if (self.callback) { self.callback(err); // Exclude connection errors so that those errors won't be emitted twice @@ -86,7 +85,7 @@ function multi_callback (self, err, replies) { Multi.prototype.exec_transaction = function exec_transaction (callback) { if (this.monitoring || this._client.monitoring) { - var err = new Error( + var err = new RangeError( 'Using transaction with a client that is in monitor mode does not work due to faulty return values of Redis.' ); err.command = 'EXEC'; From 03f1a606f7337dd6b4ce9b16e2f96c039266c0f4 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 25 Apr 2016 01:36:49 +0200 Subject: [PATCH 29/29] Improve error handling Added individual error classes Don't silently fail for commands without callback from now on General polishing (e.g. better error messages) Fix typos --- README.md | 48 +++++++++- index.js | 164 +++++++++++++++++++++------------- lib/customError.js | 16 ---- lib/customErrors.js | 78 ++++++++++++++++ lib/extendedApi.js | 8 +- lib/individualCommands.js | 4 +- test/batch.spec.js | 2 +- test/commands/dbsize.spec.js | 2 +- test/commands/flushdb.spec.js | 2 +- test/commands/get.spec.js | 4 +- test/commands/getset.spec.js | 2 +- test/commands/hgetall.spec.js | 6 +- test/commands/mset.spec.js | 5 +- test/commands/select.spec.js | 2 +- test/commands/set.spec.js | 2 +- test/connection.spec.js | 36 +++++--- test/multi.spec.js | 4 +- test/node_redis.spec.js | 111 +++++++++++++++++++---- 18 files changed, 367 insertions(+), 129 deletions(-) delete mode 100644 lib/customError.js create mode 100644 lib/customErrors.js diff --git a/README.md b/README.md index 3a4abf9eb96..afe1c3ac15c 100644 --- a/README.md +++ b/README.md @@ -137,6 +137,7 @@ are passed an object containing `delay` (in ms) and `attempt` (the attempt #) at ### "error" `client` will emit `error` when encountering an error connecting to the Redis server or when any other in node_redis occurs. +If you use a command without callback and encounter a ReplyError it is going to be emitted to the error listener. So please attach the error listener to node_redis. @@ -293,6 +294,51 @@ client.get("foo_rand000000000000", function (err, reply) { `client.end()` without the flush parameter set to true should NOT be used in production! +## Error handling (>= v.2.6) + +All redis errors are returned as `ReplyError`. +All unresolved commands that get rejected due to what ever reason return a `AbortError`. +As subclass of the `AbortError` a `AggregateError` exists. This is emitted in case multiple unresolved commands without callback got rejected in debug_mode. +They are all aggregated and a single error is emitted in that case. + +Example: +```js +var redis = require('./'); +var assert = require('assert'); +var client = redis.createClient(); + +client.on('error', function (err) { + assert(err instanceof Error); + assert(err instanceof redis.AbortError); + assert(err instanceof redis.AggregateError); + assert.strictEqual(err.errors.length, 2); // The set and get got aggregated in here + assert.strictEqual(err.code, 'NR_CLOSED'); +}); +client.set('foo', 123, 'bar', function (err, res) { // To many arguments + assert(err instanceof redis.ReplyError); // => true + assert.strictEqual(err.command, 'SET'); + assert.deepStrictEqual(err.args, ['foo', 123, 'bar']); + + redis.debug_mode = true; + client.set('foo', 'bar'); + client.get('foo'); + process.nextTick(function () { + client.end(true); // Force closing the connection while the command did not yet return + redis.debug_mode = false; + }); +}); + +``` + +Every `ReplyError` contains the `command` name in all-caps and the arguments (`args`). + +If node_redis emits a library error because of another error, the triggering error is added to the returned error as `origin` attribute. + +___Error codes___ + +node_redis returns a `NR_CLOSED` error code if the clients connection dropped. If a command unresolved command got rejected a `UNERCTAIN_STATE` code is returned. +A `CONNECTION_BROKEN` error code is used in case node_redis gives up to reconnect. + ## client.unref() Call `unref()` on the underlying socket connection to the Redis server, allowing the program to exit once no more commands are pending. @@ -537,7 +583,7 @@ Redis. The interface in `node_redis` is to return an individual `Batch` object b The only difference between .batch and .multi is that no transaction is going to be used. Be aware that the errors are - just like in multi statements - in the result. Otherwise both, errors and results could be returned at the same time. -If you fire many commands at once this is going to **boost the execution speed by up to 400%** [sic!] compared to fireing the same commands in a loop without waiting for the result! See the benchmarks for further comparison. Please remember that all commands are kept in memory until they are fired. +If you fire many commands at once this is going to boost the execution speed significantly compared to fireing the same commands in a loop without waiting for the result! See the benchmarks for further comparison. Please remember that all commands are kept in memory until they are fired. ## Monitor mode diff --git a/index.js b/index.js index a94710a67aa..a4957bd2198 100644 --- a/index.js +++ b/index.js @@ -5,7 +5,7 @@ var tls = require('tls'); var util = require('util'); var utils = require('./lib/utils'); var Queue = require('double-ended-queue'); -var CommandError = require('./lib/customError'); +var errorClasses = require('./lib/customErrors'); var Command = require('./lib/command').Command; var OfflineCommand = require('./lib/command').OfflineCommand; var EventEmitter = require('events'); @@ -189,13 +189,24 @@ function create_parser (self) { self.return_reply(data); }, returnError: function (err) { - self.return_error(err); + // Return a ReplyError to indicate Redis returned an error + self.return_error(new errorClasses.ReplyError(err)); }, 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); + // Note: the execution order is important. First flush and emit, then create the stream + err = new errorClasses.ReplyError(err); + err.message += '. Please report this.'; + self.ready = false; + self.flush_and_error({ + message: 'Fatal error encountert. Command aborted.', + code: 'NR_FATAL' + }, { + error: err, + queues: ['command_queue'] + }); + self.emit('error', err); + self.create_stream(); }, returnBuffers: self.buffers || self.message_buffers, name: self.options.parser, @@ -240,7 +251,7 @@ RedisClient.prototype.create_stream = function () { this.stream.setTimeout(this.connect_timeout, function () { // Note: This is only tested if a internet connection is established self.retry_totaltime = self.connect_timeout; - self.connection_gone('timeout', new Error('Redis connection gone from timeout event')); + self.connection_gone('timeout'); }); } @@ -270,11 +281,11 @@ RedisClient.prototype.create_stream = function () { }); this.stream.once('close', function (hadError) { - self.connection_gone('close', hadError ? new Error('Stream connection closed with a transmission error') : null); + self.connection_gone('close'); }); this.stream.once('end', function () { - self.connection_gone('end', null); + self.connection_gone('end'); }); this.stream.on('drain', function () { @@ -325,30 +336,46 @@ RedisClient.prototype.warn = function (msg) { }; // Flush provided queues, erroring any items with a callback first -RedisClient.prototype.flush_and_error = function (error, queue_names) { - var callbacks_not_called = []; - queue_names = queue_names || ['offline_queue', 'command_queue']; +RedisClient.prototype.flush_and_error = function (error_attributes, options) { + options = options || {}; + var aggregated_errors = []; + var queue_names = options.queues || ['command_queue', 'offline_queue']; // Flush the command_queue first to keep the order intakt for (var i = 0; i < queue_names.length; i++) { + // If the command was fired it might have been processed so far + if (queue_names[i] === 'command_queue') { + error_attributes.message += ' It might have been processed.'; + } else { // As the command_queue is flushed first, remove this for the offline queue + error_attributes.message = error_attributes.message.replace(' It might have been processed.', ''); + } + // Don't flush everything from the queue for (var command_obj = this[queue_names[i]].shift(); command_obj; command_obj = this[queue_names[i]].shift()) { - var err = new CommandError(error); + var err = new errorClasses.AbortError(error_attributes); err.command = command_obj.command.toUpperCase(); - if (command_obj.args.length) { + if (command_obj.args && command_obj.args.length) { err.args = command_obj.args; } + if (options.error) { + err.origin = options.error; + } if (typeof command_obj.callback === 'function') { command_obj.callback(err); } else { - callbacks_not_called.push(err); + aggregated_errors.push(err); } } - this[queue_names[i]] = new Queue(); } - // Mutate the original error that will be emitted - // This is fine, as we don't manipulate any user errors - if (callbacks_not_called.length !== 0) { - error.errors = callbacks_not_called; + // Currently this would be a breaking change, therefore it's only emitted in debug_mode + if (exports.debug_mode && aggregated_errors.length) { + var error; + if (aggregated_errors.length === 1) { + error = aggregated_errors[0]; + } else { + error_attributes.message = error_attributes.message.replace('It', 'They').replace(/command/i, '$&s'); + error = new errorClasses.AggregateError(error_attributes); + error.errors = aggregated_errors; + } + this.emit('error', error); } - return callbacks_not_called.length === 0; }; RedisClient.prototype.on_error = function (err) { @@ -538,6 +565,7 @@ RedisClient.prototype.connection_gone = function (why, error) { if (this.retry_timer) { return; } + error = error || null; debug('Redis connection is gone from ' + why + ' event.'); this.connected = false; @@ -564,9 +592,12 @@ RedisClient.prototype.connection_gone = function (why, error) { // If this is a requested shutdown, then don't retry if (this.closing) { debug('Connection ended by quit / end command, not retrying.'); - error = new Error('Stream connection ended and running command aborted. It might have been processed.'); - error.code = 'NR_OFFLINE'; - this.flush_and_error(error); + this.flush_and_error({ + message: 'Stream connection ended and command aborted.', + code: 'NR_CLOSED' + }, { + error: error + }); return; } @@ -586,31 +617,39 @@ RedisClient.prototype.connection_gone = function (why, error) { if (typeof this.retry_delay !== 'number') { // Pass individual error through if (this.retry_delay instanceof Error) { - error = new CommandError(this.retry_delay); - } - // Attention: there might be the case where there's no error! - if (!error) { - error = new Error('Stream connection ended and running command aborted. It might have been processed.'); - error.code = 'NR_OFFLINE'; - } - // Only emit an error in case that a running command had no callback - if (!this.flush_and_error(error)) { - error.message = 'Stream connection ended and all running commands aborted. They might have been processed.'; - this.emit('error', error); + error = this.retry_delay; } + this.flush_and_error({ + message: 'Stream connection ended and command aborted.', + code: 'NR_CLOSED' + }, { + 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.' : - 'maximum connection attempts exceeded.'; - error = new Error('Redis connection in broken state: ' + message); - error.code = 'CONNECTION_BROKEN'; - this.flush_and_error(error); - this.emit('error', error); + var message = 'Redis connection in broken state: '; + if (this.retry_totaltime >= this.connect_timeout) { + message += 'connection timeout exceeded.'; + } else { + message += 'maximum connection attempts exceeded.'; + } + + this.flush_and_error({ + message: message, + code: 'CONNECTION_BROKEN', + }, { + error: error + }); + var err = new Error(message); + err.code = 'CONNECTION_BROKEN'; + if (error) { + err.origin = error; + } + this.emit('error', err); this.end(false); return; } @@ -620,13 +659,13 @@ RedisClient.prototype.connection_gone = function (why, error) { this.offline_queue.unshift.apply(this.offline_queue, this.command_queue.toArray()); this.command_queue.clear(); } else if (this.command_queue.length !== 0) { - error = new Error('Redis connection lost and command aborted in uncertain state. It might have been processed.'); - error.code = 'UNCERTAIN_STATE'; - if (!this.flush_and_error(error, ['command_queue'])) { - // Only emit if not all commands had a callback that already handled the error - error.message = 'Redis connection lost and commands aborted in uncertain state. They might have been processed.'; - this.emit('error', error); - } + this.flush_and_error({ + message: 'Redis connection lost and command aborted.', + code: 'UNCERTAIN_STATE' + }, { + error: error, + queues: ['command_queue'] + }); } if (this.retry_max_delay !== null && this.retry_delay > this.retry_max_delay) { @@ -643,11 +682,9 @@ RedisClient.prototype.connection_gone = function (why, error) { RedisClient.prototype.return_error = function (err) { var command_obj = this.command_queue.shift(); - if (command_obj && command_obj.command && command_obj.command.toUpperCase) { - err.command = command_obj.command.toUpperCase(); - if (command_obj.args.length) { - err.args = command_obj.args; - } + err.command = command_obj.command.toUpperCase(); + if (command_obj.args && command_obj.args.length) { + err.args = command_obj.args; } // Count down pub sub mode if in entering modus @@ -661,7 +698,7 @@ RedisClient.prototype.return_error = function (err) { err.code = match[1]; } - utils.callback_or_emit(this, command_obj && command_obj.callback, err); + utils.callback_or_emit(this, command_obj.callback, err); }; RedisClient.prototype.drain = function () { @@ -809,14 +846,16 @@ function handle_offline_command (self, command_obj) { msg = 'Stream not writeable.'; } } else { - msg = 'The connection has already been closed.'; + msg = 'The connection is already closed.'; } - err = new Error(command + " can't be processed. " + msg); - err.command = command; - if (command_obj.args.length) { + err = new errorClasses.AbortError({ + message: command + " can't be processed. " + msg, + code: 'NR_CLOSED', + command: command + }); + if (command_obj.args && command_obj.args.length) { err.args = command_obj.args; } - err.code = 'NR_OFFLINE'; utils.reply_in_order(self, callback, err); } else { debug('Queueing ' + command + ' for next server connection.'); @@ -889,8 +928,8 @@ RedisClient.prototype.internal_send_command = function (command, args, callback, args_copy[i] = '' + args[i]; } } - args = null; - command_obj = new Command(command, args_copy, buffer_args, callback); + // Pass the original args to make sure in error cases the original arguments are returned + command_obj = new Command(command, args, buffer_args, callback); if (this.options.prefix) { prefix_keys = commands.getKeyIndexes(command, args_copy); @@ -1053,6 +1092,9 @@ exports.createClient = function () { exports.RedisClient = RedisClient; exports.print = utils.print; exports.Multi = require('./lib/multi'); +exports.AbortError = errorClasses.AbortError; +exports.ReplyError = errorClasses.ReplyError; +exports.AggregateError = errorClasses.AggregateError; // Add all redis commands / node_redis api to the client require('./lib/individualCommands'); diff --git a/lib/customError.js b/lib/customError.js deleted file mode 100644 index 07101b58063..00000000000 --- a/lib/customError.js +++ /dev/null @@ -1,16 +0,0 @@ -'use strict'; - -var util = require('util'); - -function CommandError (error) { - Error.captureStackTrace(this, this.constructor); - this.name = this.constructor.name; - this.message = error.message; - for (var keys = Object.keys(error), key = keys.pop(); key; key = keys.pop()) { - this[key] = error[key]; - } -} - -util.inherits(CommandError, Error); - -module.exports = CommandError; diff --git a/lib/customErrors.js b/lib/customErrors.js new file mode 100644 index 00000000000..7c6e457036a --- /dev/null +++ b/lib/customErrors.js @@ -0,0 +1,78 @@ +'use strict'; + +var util = require('util'); + +function AbortError (obj) { + Error.captureStackTrace(this, this.constructor); + var message; + Object.defineProperty(this, 'name', { + get: function () { + return this.constructor.name; + } + }); + Object.defineProperty(this, 'message', { + get: function () { + return message; + }, + set: function (msg) { + message = msg; + } + }); + for (var keys = Object.keys(obj), key = keys.pop(); key; key = keys.pop()) { + this[key] = obj[key]; + } + // Explicitly add the message + // If the obj is a error itself, the message is not enumerable + this.message = obj.message; +} + +function ReplyError (obj) { + Error.captureStackTrace(this, this.constructor); + var tmp; + Object.defineProperty(this, 'name', { + get: function () { + return this.constructor.name; + } + }); + Object.defineProperty(this, 'message', { + get: function () { + return tmp; + }, + set: function (msg) { + tmp = msg; + } + }); + this.message = obj.message; +} + +function AggregateError (obj) { + Error.captureStackTrace(this, this.constructor); + var tmp; + Object.defineProperty(this, 'name', { + get: function () { + return this.constructor.name; + } + }); + Object.defineProperty(this, 'message', { + get: function () { + return tmp; + }, + set: function (msg) { + tmp = msg; + } + }); + for (var keys = Object.keys(obj), key = keys.pop(); key; key = keys.pop()) { + this[key] = obj[key]; + } + this.message = obj.message; +} + +util.inherits(ReplyError, Error); +util.inherits(AbortError, Error); +util.inherits(AggregateError, AbortError); + +module.exports = { + ReplyError: ReplyError, + AbortError: AbortError, + AggregateError: AggregateError +}; diff --git a/lib/extendedApi.js b/lib/extendedApi.js index ef6a2faa6ea..ece77d22fe2 100644 --- a/lib/extendedApi.js +++ b/lib/extendedApi.js @@ -47,10 +47,10 @@ RedisClient.prototype.send_command = RedisClient.prototype.sendCommand = functio RedisClient.prototype.end = function (flush) { // Flush queue if wanted if (flush) { - var err = new Error("The command can't be processed. The connection has already been closed."); - err.code = 'NR_OFFLINE'; - this.flush_and_error(err); - // TODO: Emit an error in case a command did not have a callback + this.flush_and_error({ + message: 'Connection forcefully ended and command aborted.', + code: 'NR_CLOSED' + }); } else if (arguments.length === 0) { this.warn( 'Using .end() without the flush parameter is deprecated and throws from v.3.0.0 on.\n' + diff --git a/lib/individualCommands.js b/lib/individualCommands.js index c9cef49b1b6..808ad99a0b0 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -79,7 +79,7 @@ Multi.prototype.monitor = Multi.prototype.MONITOR = function monitor (callback) function quit_callback (self, callback) { return function (err, res) { - if (err && err.code === 'NR_OFFLINE') { + if (err && err.code === 'NR_CLOSED') { // Pretent the quit command worked properly in this case. // Either the quit landed in the offline queue and was flushed at the reconnect // or the offline queue is deactivated and the command was rejected right away @@ -90,7 +90,7 @@ function quit_callback (self, callback) { } utils.callback_or_emit(self, callback, err, res); if (self.stream.writable) { - // If the socket is still alive, kill it. This could happen if quit got a NR_OFFLINE error code + // If the socket is still alive, kill it. This could happen if quit got a NR_CLOSED error code self.stream.destroy(); } }; diff --git a/test/batch.spec.js b/test/batch.spec.js index 7b382eee421..762b2f70491 100644 --- a/test/batch.spec.js +++ b/test/batch.spec.js @@ -36,7 +36,7 @@ describe("The 'batch' method", function () { batch.set('foo', 'bar'); batch.exec(function (err, res) { assert.strictEqual(err, null); - assert.strictEqual(res[0].code, 'NR_OFFLINE'); + assert.strictEqual(res[0].code, 'NR_CLOSED'); done(); }); }); diff --git a/test/commands/dbsize.spec.js b/test/commands/dbsize.spec.js index b1a3d3c32e9..4fdf80010d1 100644 --- a/test/commands/dbsize.spec.js +++ b/test/commands/dbsize.spec.js @@ -31,7 +31,7 @@ describe("The 'dbsize' method", function () { it('reports an error', function (done) { client.dbsize([], function (err, res) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); done(); }); }); diff --git a/test/commands/flushdb.spec.js b/test/commands/flushdb.spec.js index 90811053499..61535e2e714 100644 --- a/test/commands/flushdb.spec.js +++ b/test/commands/flushdb.spec.js @@ -31,7 +31,7 @@ describe("The 'flushdb' method", function () { it('reports an error', function (done) { client.flushdb(function (err, res) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); done(); }); }); diff --git a/test/commands/get.spec.js b/test/commands/get.spec.js index 83a330ad12d..e2b9a7db071 100644 --- a/test/commands/get.spec.js +++ b/test/commands/get.spec.js @@ -31,14 +31,14 @@ describe("The 'get' method", function () { it('reports an error', function (done) { client.get(key, function (err, res) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); done(); }); }); it('reports an error promisified', function () { return client.getAsync(key).then(assert, function (err) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); }); }); }); diff --git a/test/commands/getset.spec.js b/test/commands/getset.spec.js index ea6b5d5ef76..e5da8573119 100644 --- a/test/commands/getset.spec.js +++ b/test/commands/getset.spec.js @@ -32,7 +32,7 @@ describe("The 'getset' method", function () { it('reports an error', function (done) { client.get(key, function (err, res) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); done(); }); }); diff --git a/test/commands/hgetall.spec.js b/test/commands/hgetall.spec.js index 8e8b88a309c..ba443265100 100644 --- a/test/commands/hgetall.spec.js +++ b/test/commands/hgetall.spec.js @@ -24,8 +24,10 @@ describe("The 'hgetall' method", function () { it('handles simple keys and values', function (done) { client.hmset(['hosts', '__proto__', '1', 'another', '23', 'home', '1234'], helper.isString('OK')); client.HGETALL(['hosts'], function (err, obj) { - assert.strictEqual(3, Object.keys(obj).length); - assert.strictEqual('1', obj.__proto__.toString()); // eslint-disable-line no-proto + if (!/^v0\.10/.test(process.version)) { + assert.strictEqual(3, Object.keys(obj).length); + assert.strictEqual('1', obj.__proto__.toString()); // eslint-disable-line no-proto + } assert.strictEqual('23', obj.another.toString()); assert.strictEqual('1234', obj.home.toString()); done(err); diff --git a/test/commands/mset.spec.js b/test/commands/mset.spec.js index c6963641a2d..9fac90728cf 100644 --- a/test/commands/mset.spec.js +++ b/test/commands/mset.spec.js @@ -33,7 +33,7 @@ describe("The 'mset' method", function () { it('reports an error', function (done) { client.mset(key, value, key2, value2, function (err, res) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); done(); }); }); @@ -96,7 +96,8 @@ describe("The 'mset' method", function () { // this behavior is different from the 'set' behavior. it('emits an error', function (done) { client.on('error', function (err) { - assert.equal(err.message, "ERR wrong number of arguments for 'mset' command"); + assert.strictEqual(err.message, "ERR wrong number of arguments for 'mset' command"); + assert.strictEqual(err.name, 'ReplyError'); done(); }); diff --git a/test/commands/select.spec.js b/test/commands/select.spec.js index d8878fe48c3..4297dca7f34 100644 --- a/test/commands/select.spec.js +++ b/test/commands/select.spec.js @@ -23,7 +23,7 @@ describe("The 'select' method", function () { it('returns an error if redis is not connected', function (done) { var buffering = client.select(1, function (err, res) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); done(); }); assert(typeof buffering === 'boolean'); diff --git a/test/commands/set.spec.js b/test/commands/set.spec.js index 7cb4b8314a9..01b62443818 100644 --- a/test/commands/set.spec.js +++ b/test/commands/set.spec.js @@ -31,7 +31,7 @@ describe("The 'set' method", function () { it('reports an error', function (done) { client.set(key, value, function (err, res) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); done(); }); }); diff --git a/test/connection.spec.js b/test/connection.spec.js index eb807295ed8..9661f42d3bd 100644 --- a/test/connection.spec.js +++ b/test/connection.spec.js @@ -53,7 +53,7 @@ describe('connection tests', function () { } }); client.set('foo', 'bar', function (err, res) { - assert.strictEqual(err.message, 'Stream connection ended and running command aborted. It might have been processed.'); + assert.strictEqual(err.message, 'Stream connection ended and command aborted.'); called = -1; }); }); @@ -62,7 +62,7 @@ describe('connection tests', function () { var called = false; client = redis.createClient(9999); client.set('foo', 'bar', function (err, res) { - assert.strictEqual(err.message, 'Stream connection ended and running command aborted. It might have been processed.'); + assert.strictEqual(err.message, 'Stream connection ended and command aborted.'); called = true; }); var bool = client.quit(function (err, res) { @@ -259,13 +259,12 @@ describe('connection tests', function () { it('emits error once if reconnecting after command has been executed but not yet returned without callback', function (done) { client = redis.createClient.apply(null, args); - client.on('error', function (err) { - assert.strictEqual(err.code, 'UNCERTAIN_STATE'); - done(); - }); client.on('ready', function () { - client.set('foo', 'bar'); + client.set('foo', 'bar', function (err) { + assert.strictEqual(err.code, 'UNCERTAIN_STATE'); + done(); + }); // Abort connection before the value returned client.stream.destroy(); }); @@ -281,7 +280,8 @@ describe('connection tests', function () { retryStrategy: function (options) { if (options.totalRetryTime > 150) { client.set('foo', 'bar', function (err, res) { - assert.strictEqual(err.message, 'Connection timeout'); + assert.strictEqual(err.message, 'Stream connection ended and command aborted.'); + assert.strictEqual(err.origin.message, 'Connection timeout'); done(); }); // Pass a individual error message to the error handler @@ -308,7 +308,9 @@ describe('connection tests', function () { retry_strategy: function (options) { if (options.total_retry_time > 150) { client.set('foo', 'bar', function (err, res) { - assert.strictEqual(err.code, 'ECONNREFUSED'); + assert.strictEqual(err.message, 'Stream connection ended and command aborted.'); + assert.strictEqual(err.code, 'NR_CLOSED'); + assert.strictEqual(err.origin.code, 'ECONNREFUSED'); done(); }); return false; @@ -319,10 +321,15 @@ describe('connection tests', function () { }); }); - it('retry_strategy used to reconnect with defaults', function (done) { + it('retryStrategy used to reconnect with defaults', function (done) { + var unhookIntercept = intercept(function () { + return ''; + }); + redis.debugMode = true; client = redis.createClient({ - retry_strategy: function (options) { + retryStrategy: function (options) { client.set('foo', 'bar'); + assert(redis.debugMode); return null; } }); @@ -330,9 +337,10 @@ describe('connection tests', function () { client.stream.destroy(); }, 50); client.on('error', function (err) { - assert.strictEqual(err.code, 'NR_OFFLINE'); - assert.strictEqual(err.errors.length, 1); - assert.notStrictEqual(err.message, err.errors[0].message); + assert.strictEqual(err.code, 'NR_CLOSED'); + assert.strictEqual(err.message, 'Stream connection ended and command aborted.'); + unhookIntercept(); + redis.debugMode = false; done(); }); }); diff --git a/test/multi.spec.js b/test/multi.spec.js index 8f500958fab..8deae7f920b 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -111,7 +111,7 @@ describe("The 'multi' method", function () { it('reports an error', function (done) { var multi = client.multi(); var notBuffering = multi.exec(function (err, res) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); done(); }); assert.strictEqual(notBuffering, false); @@ -119,7 +119,7 @@ describe("The 'multi' method", function () { it('reports an error if promisified', function () { return client.multi().execAsync().catch(function (err) { - assert(err.message.match(/The connection has already been closed/)); + assert(err.message.match(/The connection is already closed/)); }); }); }); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index c87968dea8a..8ba91fdfe47 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -3,6 +3,7 @@ var assert = require('assert'); var fs = require('fs'); var path = require('path'); +var intercept = require('intercept-stdout'); var config = require('./lib/config'); var helper = require('./helper'); var utils = require('../lib/utils'); @@ -58,6 +59,12 @@ describe('The node_redis client', function () { assert.strictEqual(client2.options[elem], client.options[elem]); } } + client2.on('error', function (err) { + assert.strictEqual(err.message, 'Connection forcefully ended and command aborted. It might have been processed.'); + assert.strictEqual(err.command, 'SELECT'); + assert(err instanceof Error); + assert.strictEqual(err.name, 'AbortError'); + }); client2.on('ready', function () { client2.end(true); done(); @@ -193,10 +200,10 @@ describe('The node_redis client', function () { }); }); - it('using multi with send_command should work as individual command instead of using the internal multi', function (done) { + it('using multi with sendCommand should work as individual command instead of using the internal multi', function (done) { // This is necessary to keep backwards compatibility and it is the only way to handle multis as you want in node_redis - client.send_command('multi'); - client.send_command('set', ['foo', 'bar'], helper.isString('QUEUED')); + client.sendCommand('multi'); + client.sendCommand('set', ['foo', 'bar'], helper.isString('QUEUED')); client.get('foo'); client.exec(function (err, res) { // exec is not manipulated if not fired by the individual multi command // As the multi command is handled individually by the user he also has to handle the return value @@ -340,7 +347,8 @@ describe('The node_redis client', function () { } }, 20); var cb = function (err, res) { - assert(/The connection has already been closed/.test(err.message)); + assert(/Connection forcefully ended|The connection is already closed./.test(err.message)); + assert.strictEqual(err.code, 'NR_CLOSED'); end(); }; for (var i = 0; i < 20; i++) { @@ -361,7 +369,7 @@ describe('The node_redis client', function () { done(); }, 20); var cb = function (err, res) { - assert(/The connection has already been closed./.test(err.message)); + assert(/Connection forcefully ended|The connection is already closed./.test(err.message)); end(); }; for (var i = 0; i < 20; i++) { @@ -373,6 +381,59 @@ describe('The node_redis client', function () { } }); + it('emits an aggregate error if no callback was present for multiple commands in debug_mode', function (done) { + redis.debug_mode = true; + var unhookIntercept = intercept(function (data) { + return ''; // Don't print the debug messages + }); + client.set('foo', 'bar'); + client.set('baz', 'hello world'); + client.on('error', function (err) { + assert(err instanceof Error); + assert(err instanceof redis.AbortError); + assert(err instanceof redis.AggregateError); + assert.strictEqual(err.name, 'AggregateError'); + assert.strictEqual(err.errors.length, 2); + assert.strictEqual(err.message, 'Connection forcefully ended and commands aborted.'); + assert.strictEqual(err.code, 'NR_CLOSED'); + assert.strictEqual(err.errors[0].message, 'Connection forcefully ended and command aborted. It might have been processed.'); + assert.strictEqual(err.errors[0].command, 'SET'); + assert.strictEqual(err.errors[0].code, 'NR_CLOSED'); + assert.deepEqual(err.errors[0].args, ['foo', 'bar']); + done(); + }); + client.end(true); + unhookIntercept(); + redis.debug_mode = false; + }); + + it('emits an abort error if no callback was present for a single commands', function (done) { + redis.debug_mode = true; + var unhookIntercept = intercept(function (data) { + return ''; // Don't print the debug messages + }); + client.set('foo', 'bar'); + client.on('error', function (err) { + assert(err instanceof Error); + assert(err instanceof redis.AbortError); + assert(!(err instanceof redis.AggregateError)); + assert.strictEqual(err.message, 'Connection forcefully ended and command aborted. It might have been processed.'); + assert.strictEqual(err.command, 'SET'); + assert.strictEqual(err.code, 'NR_CLOSED'); + assert.deepEqual(err.args, ['foo', 'bar']); + done(); + }); + client.end(true); + unhookIntercept(); + redis.debug_mode = false; + }); + + it('does not emit abort errors if no callback was present while not being in debug_mode ', function (done) { + client.set('foo', 'bar'); + client.end(true); + setTimeout(done, 100); + }); + }); describe('commands after using .quit should fail', function () { @@ -385,7 +446,7 @@ describe('The node_redis client', function () { client = redis.createClient(); client.quit(function () { client.get('foo', function (err, res) { - assert.strictEqual(err.message, 'Stream connection ended and running command aborted. It might have been processed.'); + assert.strictEqual(err.message, 'Stream connection ended and command aborted. It might have been processed.'); assert.strictEqual(client.offline_queue.length, 0); done(); }); @@ -398,7 +459,7 @@ describe('The node_redis client', function () { client.quit(); setTimeout(function () { client.get('foo', function (err, res) { - assert.strictEqual(err.message, 'GET can\'t be processed. The connection has already been closed.'); + assert.strictEqual(err.message, 'GET can\'t be processed. The connection is already closed.'); assert.strictEqual(err.command, 'GET'); assert.strictEqual(client.offline_queue.length, 0); done(); @@ -410,7 +471,7 @@ describe('The node_redis client', function () { if (helper.redisProcess().spawnFailed()) this.skip(); client.quit(); client.on('error', function (err) { - assert.strictEqual(err.message, 'SET can\'t be processed. The connection has already been closed.'); + assert.strictEqual(err.message, 'SET can\'t be processed. The connection is already closed.'); assert.strictEqual(err.command, 'SET'); assert.strictEqual(client.offline_queue_length, 0); done(); @@ -542,7 +603,7 @@ describe('The node_redis client', function () { }); domain.on('error', function (err) { - assert.strictEqual(err.message, 'SET can\'t be processed. The connection has already been closed.'); + assert.strictEqual(err.message, 'SET can\'t be processed. The connection is already closed.'); domain.exit(); done(); }); @@ -919,8 +980,11 @@ describe('The node_redis client', function () { it('should gracefully recover and only fail on the already send commands', function (done) { client = redis.createClient.apply(null, args); + var error; client.on('error', function (err) { - assert.strictEqual(err.message, 'Protocol error, got "a" as reply type byte'); + assert.strictEqual(err.message, 'Protocol error, got "a" as reply type byte. Please report this.'); + assert.strictEqual(err, error); + assert(err instanceof redis.ReplyError); // 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'); @@ -929,7 +993,10 @@ describe('The node_redis client', function () { }); client.once('ready', function () { client.set('foo', 'bar', function (err, res) { - assert.strictEqual(err.message, 'Protocol error, got "a" as reply type byte'); + assert.strictEqual(err.message, 'Fatal error encountert. Command aborted. It might have been processed.'); + assert.strictEqual(err.code, 'NR_FATAL'); + assert(err instanceof redis.AbortError); + error = err.origin; }); // Fail the set answer. Has no corresponding command obj and will therefore land in the error handler and set client.reply_parser.execute(new Buffer('a*1\r*1\r$1`zasd\r\na')); @@ -974,7 +1041,7 @@ describe('The node_redis client', function () { setTimeout(function () { client.set('foo', 'bar', function (err, result) { if (!finished) done(err); - assert.strictEqual(err.message, "The command can't be processed. The connection has already been closed."); + assert.strictEqual(err.message, 'Connection forcefully ended and command aborted.'); }); setTimeout(function () { @@ -993,10 +1060,15 @@ describe('The node_redis client', function () { var i = 0; client.on('error', function (err) { - if (err.message === 'Redis connection in broken state: maximum connection attempts exceeded.') { + if (err.code === 'CONNECTION_BROKEN') { assert(i, 3); assert.strictEqual(client.offline_queue.length, 0); - done(); + assert.strictEqual(err.origin.code, 'ECONNREFUSED'); + if (!(err instanceof redis.AbortError)) { + done(); + } else { + assert.strictEqual(err.command, 'SET'); + } } else { assert.equal(err.code, 'ECONNREFUSED'); assert.equal(err.errno, 'ECONNREFUSED'); @@ -1111,10 +1183,13 @@ describe('The node_redis client', function () { it('flushes the command queue if connection is lost', function (done) { client = redis.createClient({ parser: parser, - max_attempts: 2, enable_offline_queue: false }); + redis.debug_mode = true; + var unhookIntercept = intercept(function () { + return ''; + }); client.once('ready', function () { var multi = client.multi(); multi.config('bar'); @@ -1133,18 +1208,20 @@ describe('The node_redis client', function () { var end = helper.callFuncAfter(done, 3); client.on('error', function (err) { + assert.equal(client.command_queue.length, 0); if (err.command === 'EXEC') { - assert.equal(client.command_queue.length, 0); assert.equal(err.errors.length, 9); end(); } else if (err.code === 'UNCERTAIN_STATE') { - assert.equal(client.command_queue.length, 0); assert.equal(err.errors.length, 4); end(); } else { assert.equal(err.code, 'ECONNREFUSED'); assert.equal(err.errno, 'ECONNREFUSED'); assert.equal(err.syscall, 'connect'); + redis.debug_mode = false; + client.end(true); + unhookIntercept(); end(); } });