From 27eac2f594f58a3acf579f0bbfb2ea276312c9a9 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 29 Apr 2016 13:07:40 +0200 Subject: [PATCH 01/17] Prevent a race condition by just forcefully ending the connection in the test --- test/multi.spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/multi.spec.js b/test/multi.spec.js index 8deae7f920..9010759378 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -137,6 +137,7 @@ describe("The 'multi' method", function () { client.monitor(function (e) { client.on('error', function (err) { assert.strictEqual(err.code, 'EXECABORT'); + client.end(false); done(); }); var multi = client.multi(); @@ -149,6 +150,7 @@ describe("The 'multi' method", function () { // 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'); + client.end(false); done(); }); }); From 84abf5c4c24d3a306b09293e3a5ae98a3b09df9d Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 25 May 2016 20:35:13 +0300 Subject: [PATCH 02/17] Fix benchmark not using individual options --- benchmarks/multi_bench.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/benchmarks/multi_bench.js b/benchmarks/multi_bench.js index d93fce4439..c65876b903 100644 --- a/benchmarks/multi_bench.js +++ b/benchmarks/multi_bench.js @@ -231,8 +231,8 @@ tests.push(new Test({descr: 'SET 4B buf', command: 'set', args: ['foo_rand000000 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'], 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: 'GET 4B buf', command: 'get', args: ['foo_rand000000000000'], client_options: { return_buffers: true} })); +tests.push(new Test({descr: 'GET 4B buf', command: 'get', args: ['foo_rand000000000000'], batch: 50, client_options: { return_buffers: true} })); 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})); @@ -243,8 +243,8 @@ tests.push(new Test({descr: 'SET 4KiB buf', command: 'set', args: ['foo_rand0000 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'], 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: 'GET 4KiB buf', command: 'get', args: ['foo_rand000000000001'], client_options: { return_buffers: true} })); +tests.push(new Test({descr: 'GET 4KiB buf', command: 'get', args: ['foo_rand000000000001'], batch: 50, client_options: { return_buffers: true} })); tests.push(new Test({descr: 'INCR', command: 'incr', args: ['counter_rand000000000000']})); tests.push(new Test({descr: 'INCR', command: 'incr', args: ['counter_rand000000000000'], batch: 50})); @@ -267,8 +267,8 @@ tests.push(new Test({descr: 'SET 4MiB buf', command: 'set', args: ['foo_rand0000 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'], 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} })); +tests.push(new Test({descr: 'GET 4MiB buf', command: 'get', args: ['foo_rand000000000002'], client_options: { return_buffers: true} })); +tests.push(new Test({descr: 'GET 4MiB buf', command: 'get', args: ['foo_rand000000000002'], batch: 20, client_options: { return_buffers: true} })); function next () { var test = tests.shift(); From fe00bf271df4504a328bc592423396bf153c2f34 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 25 May 2016 22:47:09 +0300 Subject: [PATCH 03/17] Update redis-parser to v.2.0.0 Update all code accordingly --- README.md | 87 +++++++++++++++++-------------------- benchmarks/multi_bench.js | 2 +- changelog.md | 14 ++++++ index.js | 15 +++---- lib/customErrors.js | 57 +++++------------------- package.json | 5 ++- test/custom_errors.spec.js | 89 ++++++++++++++++++++++++++++++++++++++ test/node_redis.spec.js | 8 +++- 8 files changed, 171 insertions(+), 106 deletions(-) create mode 100644 test/custom_errors.spec.js diff --git a/README.md b/README.md index afe1c3ac15..f1680c6541 100644 --- a/README.md +++ b/README.md @@ -182,8 +182,8 @@ __Tip:__ If the Redis server runs on the same machine as the client consider usi | 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)). | -| 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. | +| parser | javascript | __Deprecated__ Use either the built-in JS parser [`javascript`]() or the native [`hiredis`]() parser. __Note__ `node_redis` < 2.6 uses hiredis as default if installed. This changed in v.2.6.0. | +| 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. | | detect_buffers | false | If set to `true`, then replies will be sent to callbacks as Buffers. This option lets you switch between Buffers and Strings on a per-command basis, whereas `return_buffers` applies to every command on a client. __Note__: This doesn't work properly with the pubsub mode. A subscriber has to either always return Strings or Buffers. | | socket_keepalive | true | If set to `true`, the keep-alive functionality is enabled on the underlying socket. | @@ -712,56 +712,47 @@ client.zadd(args, function (err, response) { ## Performance Much effort has been spent to make `node_redis` as fast as possible for common -operations. As pipelining happens naturally from shared connections, overall -efficiency goes up. - -Here are results of `multi_bench.js` which is similar to `redis-benchmark` from the Redis distribution. - -hiredis parser (Lenovo T450s i7-5600U): +operations. ``` -Client count: 1, node version: 4.2.2, server version: 3.0.3, parser: hiredis - PING, 1/1 min/max/avg: 0/ 2/ 0.02/ 2501ms total, 47503.80 ops/sec - PING, batch 50/1 min/max/avg: 0/ 2/ 0.09/ 2501ms total, 529668.13 ops/sec - SET 4B str, 1/1 min/max/avg: 0/ 2/ 0.02/ 2501ms total, 41900.04 ops/sec - SET 4B str, batch 50/1 min/max/avg: 0/ 2/ 0.14/ 2501ms total, 354658.14 ops/sec - SET 4B buf, 1/1 min/max/avg: 0/ 4/ 0.04/ 2501ms total, 23499.00 ops/sec - SET 4B buf, batch 50/1 min/max/avg: 0/ 2/ 0.31/ 2501ms total, 159836.07 ops/sec - GET 4B str, 1/1 min/max/avg: 0/ 4/ 0.02/ 2501ms total, 43489.80 ops/sec - GET 4B str, batch 50/1 min/max/avg: 0/ 2/ 0.11/ 2501ms total, 444202.32 ops/sec - GET 4B buf, 1/1 min/max/avg: 0/ 3/ 0.02/ 2501ms total, 38561.38 ops/sec - GET 4B buf, batch 50/1 min/max/avg: 0/ 2/ 0.11/ 2501ms total, 452139.14 ops/sec - SET 4KiB str, 1/1 min/max/avg: 0/ 2/ 0.03/ 2501ms total, 32990.80 ops/sec - SET 4KiB str, batch 50/1 min/max/avg: 0/ 2/ 0.34/ 2501ms total, 146161.54 ops/sec - SET 4KiB buf, 1/1 min/max/avg: 0/ 1/ 0.04/ 2501ms total, 23294.28 ops/sec - SET 4KiB buf, batch 50/1 min/max/avg: 0/ 2/ 0.36/ 2501ms total, 137584.97 ops/sec - GET 4KiB str, 1/1 min/max/avg: 0/ 2/ 0.03/ 2501ms total, 36350.66 ops/sec - GET 4KiB str, batch 50/1 min/max/avg: 0/ 2/ 0.32/ 2501ms total, 155157.94 ops/sec - GET 4KiB buf, 1/1 min/max/avg: 0/ 4/ 0.02/ 2501ms total, 39776.49 ops/sec - GET 4KiB buf, batch 50/1 min/max/avg: 0/ 2/ 0.32/ 2501ms total, 155457.82 ops/sec - INCR, 1/1 min/max/avg: 0/ 3/ 0.02/ 2501ms total, 43972.41 ops/sec - INCR, batch 50/1 min/max/avg: 0/ 1/ 0.12/ 2501ms total, 425809.68 ops/sec - LPUSH, 1/1 min/max/avg: 0/ 2/ 0.02/ 2501ms total, 38998.40 ops/sec - LPUSH, batch 50/1 min/max/avg: 0/ 4/ 0.14/ 2501ms total, 365013.99 ops/sec - LRANGE 10, 1/1 min/max/avg: 0/ 2/ 0.03/ 2501ms total, 31879.25 ops/sec - LRANGE 10, batch 50/1 min/max/avg: 0/ 1/ 0.32/ 2501ms total, 153698.52 ops/sec - LRANGE 100, 1/1 min/max/avg: 0/ 4/ 0.06/ 2501ms total, 16676.13 ops/sec - LRANGE 100, batch 50/1 min/max/avg: 1/ 6/ 2.03/ 2502ms total, 24520.38 ops/sec - SET 4MiB str, 1/1 min/max/avg: 1/ 6/ 2.11/ 2502ms total, 472.82 ops/sec - SET 4MiB str, batch 20/1 min/max/avg: 85/ 112/ 94.93/ 2563ms total, 210.69 ops/sec - SET 4MiB buf, 1/1 min/max/avg: 1/ 8/ 2.02/ 2502ms total, 490.01 ops/sec - SET 4MiB buf, batch 20/1 min/max/avg: 37/ 52/ 39.48/ 2528ms total, 506.33 ops/sec - GET 4MiB str, 1/1 min/max/avg: 3/ 13/ 5.26/ 2504ms total, 190.10 ops/sec - GET 4MiB str, batch 20/1 min/max/avg: 70/ 106/ 89.36/ 2503ms total, 223.73 ops/sec - GET 4MiB buf, 1/1 min/max/avg: 3/ 11/ 5.04/ 2502ms total, 198.24 ops/sec - GET 4MiB buf, batch 20/1 min/max/avg: 70/ 105/ 88.07/ 2554ms total, 227.09 ops/sec +Lenovo T450s, i7-5600U and 12gb memory +clients: 1, NodeJS: 6.2.0, Redis: 3.2.0, parser: javascript, connected by: tcp + PING, 1/1 avg/max: 0.02/ 5.26 2501ms total, 46916 ops/sec + PING, batch 50/1 avg/max: 0.06/ 4.35 2501ms total, 755178 ops/sec + SET 4B str, 1/1 avg/max: 0.02/ 4.75 2501ms total, 40856 ops/sec + SET 4B str, batch 50/1 avg/max: 0.11/ 1.51 2501ms total, 432727 ops/sec + SET 4B buf, 1/1 avg/max: 0.05/ 2.76 2501ms total, 20659 ops/sec + SET 4B buf, batch 50/1 avg/max: 0.25/ 1.76 2501ms total, 194962 ops/sec + GET 4B str, 1/1 avg/max: 0.02/ 1.55 2501ms total, 45156 ops/sec + GET 4B str, batch 50/1 avg/max: 0.09/ 3.15 2501ms total, 524110 ops/sec + GET 4B buf, 1/1 avg/max: 0.02/ 3.07 2501ms total, 44563 ops/sec + GET 4B buf, batch 50/1 avg/max: 0.10/ 3.18 2501ms total, 473171 ops/sec + SET 4KiB str, 1/1 avg/max: 0.03/ 1.54 2501ms total, 32627 ops/sec + SET 4KiB str, batch 50/1 avg/max: 0.34/ 1.89 2501ms total, 146861 ops/sec + SET 4KiB buf, 1/1 avg/max: 0.05/ 2.85 2501ms total, 20688 ops/sec + SET 4KiB buf, batch 50/1 avg/max: 0.36/ 1.83 2501ms total, 138165 ops/sec + GET 4KiB str, 1/1 avg/max: 0.02/ 1.37 2501ms total, 39389 ops/sec + GET 4KiB str, batch 50/1 avg/max: 0.24/ 1.81 2501ms total, 208157 ops/sec + GET 4KiB buf, 1/1 avg/max: 0.02/ 2.63 2501ms total, 39918 ops/sec + GET 4KiB buf, batch 50/1 avg/max: 0.31/ 8.56 2501ms total, 161575 ops/sec + INCR, 1/1 avg/max: 0.02/ 4.69 2501ms total, 45685 ops/sec + INCR, batch 50/1 avg/max: 0.09/ 3.06 2501ms total, 539964 ops/sec + LPUSH, 1/1 avg/max: 0.02/ 3.04 2501ms total, 41253 ops/sec + LPUSH, batch 50/1 avg/max: 0.12/ 1.94 2501ms total, 425090 ops/sec + LRANGE 10, 1/1 avg/max: 0.02/ 2.28 2501ms total, 39850 ops/sec + LRANGE 10, batch 50/1 avg/max: 0.25/ 1.85 2501ms total, 194302 ops/sec + LRANGE 100, 1/1 avg/max: 0.05/ 2.93 2501ms total, 21026 ops/sec + LRANGE 100, batch 50/1 avg/max: 1.52/ 2.89 2501ms total, 32767 ops/sec + SET 4MiB str, 1/1 avg/max: 5.16/ 15.55 2502ms total, 193 ops/sec + SET 4MiB str, batch 20/1 avg/max: 89.73/ 99.96 2513ms total, 223 ops/sec + SET 4MiB buf, 1/1 avg/max: 2.23/ 8.35 2501ms total, 446 ops/sec + SET 4MiB buf, batch 20/1 avg/max: 41.47/ 50.91 2530ms total, 482 ops/sec + GET 4MiB str, 1/1 avg/max: 2.79/ 10.91 2502ms total, 358 ops/sec + GET 4MiB str, batch 20/1 avg/max: 101.61/118.11 2541ms total, 197 ops/sec + GET 4MiB buf, 1/1 avg/max: 2.32/ 14.93 2502ms total, 430 ops/sec + GET 4MiB buf, batch 20/1 avg/max: 65.01/ 84.72 2536ms total, 308 ops/sec ``` -The hiredis and js parser should most of the time be on the same level. But if you use Redis for big SUNION/SINTER/LRANGE/ZRANGE hiredis is faster. -Therefor the hiredis parser is the default used in node_redis. To use `hiredis`, do: - - npm install hiredis redis - ## Debugging To get debug output run your `node_redis` application with `NODE_DEBUG=redis`. diff --git a/benchmarks/multi_bench.js b/benchmarks/multi_bench.js index c65876b903..a79d92e83c 100644 --- a/benchmarks/multi_bench.js +++ b/benchmarks/multi_bench.js @@ -26,7 +26,7 @@ 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'), + parser: returnArg('parser', 'javascript'), path: returnArg('socket') // '/tmp/redis.sock' }; var small_str, large_str, small_buf, large_buf, very_large_str, very_large_buf; diff --git a/changelog.md b/changelog.md index 168a1854c3..ce2f6037b4 100644 --- a/changelog.md +++ b/changelog.md @@ -1,6 +1,20 @@ Changelog ========= +## v.2.6.0 - 25 Mai, 2016 + +In addition to the pre-releases the following changes exist in v.2.6.0: + +Features + +- Updated [redis-parser](https://github.com/NodeRedis/redis-parser) dependency ([changelog](https://github.com/NodeRedis/redis-parser/releases/tag/v.2.0.0)) + - The JS parser is from now on the new default as it is a lot faster than the hiredis parser + - This is no BC as there is no changed behavior for the user at all but just a performance improvement. Explicitly requireing the Hiredis parser is still possible. + +Deprecations + +- The `parser` option is deprecated and should be removed. The built-in Javascript parser is a lot faster than the hiredis parser and has more features + ## v.2.6.0-2 - 29 Apr, 2016 Features diff --git a/index.js b/index.js index a4957bd219..406c8c817f 100644 --- a/index.js +++ b/index.js @@ -154,11 +154,11 @@ function RedisClient (options, stream) { this.pipeline = false; this.sub_commands_left = 0; this.times_connected = 0; - this.options = options; this.buffers = options.return_buffers || options.detect_buffers; + this.options = options; this.reply = 'ON'; // Returning replies is the default // Init parser - this.reply_parser = create_parser(this, options); + this.reply_parser = create_parser(this); 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) { @@ -184,18 +184,17 @@ util.inherits(RedisClient, EventEmitter); RedisClient.connection_id = 0; function create_parser (self) { - return Parser({ + return new Parser({ returnReply: function (data) { self.return_reply(data); }, returnError: function (err) { // Return a ReplyError to indicate Redis returned an error - self.return_error(new errorClasses.ReplyError(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 // 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({ @@ -209,8 +208,8 @@ function create_parser (self) { self.create_stream(); }, returnBuffers: self.buffers || self.message_buffers, - name: self.options.parser, - stringNumbers: self.options.string_numbers + name: self.options.parser || 'javascript', + stringNumbers: self.options.string_numbers || false }); } @@ -1093,7 +1092,7 @@ exports.RedisClient = RedisClient; exports.print = utils.print; exports.Multi = require('./lib/multi'); exports.AbortError = errorClasses.AbortError; -exports.ReplyError = errorClasses.ReplyError; +exports.ReplyError = Parser.ReplyError; exports.AggregateError = errorClasses.AggregateError; // Add all redis commands / node_redis api to the client diff --git a/lib/customErrors.js b/lib/customErrors.js index 7c6e457036..0b192f78df 100644 --- a/lib/customErrors.js +++ b/lib/customErrors.js @@ -4,75 +4,42 @@ var util = require('util'); function AbortError (obj) { Error.captureStackTrace(this, this.constructor); - var message; Object.defineProperty(this, 'name', { - get: function () { - return this.constructor.name; - } + value: 'AbortError', + configurable: true, + writable: true }); Object.defineProperty(this, 'message', { - get: function () { - return message; - }, - set: function (msg) { - message = msg; - } + value: obj.message || '', + configurable: true, + writable: true }); 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; - } + value: 'AggregateError', + configurable: true, + writable: true }); Object.defineProperty(this, 'message', { - get: function () { - return tmp; - }, - set: function (msg) { - tmp = msg; - } + value: obj.message || '', + configurable: true, + writable: true }); 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/package.json b/package.json index 15ef0ba86c..82e7f2bdde 100644 --- a/package.json +++ b/package.json @@ -21,12 +21,13 @@ "coverage": "nyc report --reporter=html", "benchmark": "node benchmarks/multi_bench.js", "test": "nyc --cache mocha ./test/*.js ./test/commands/*.js --timeout=8000", - "posttest": "eslint . --fix" + "posttest": "eslint . --fix && npm run coverage", + "compare": "node benchmarks/diff_multi_bench_output.js beforeBench.txt afterBench.txt" }, "dependencies": { "double-ended-queue": "^2.1.0-0", "redis-commands": "^1.2.0", - "redis-parser": "^1.3.0" + "redis-parser": "^2.0.0" }, "engines": { "node": ">=0.10.0" diff --git a/test/custom_errors.spec.js b/test/custom_errors.spec.js new file mode 100644 index 0000000000..d7c6ebb602 --- /dev/null +++ b/test/custom_errors.spec.js @@ -0,0 +1,89 @@ +'use strict'; + +var assert = require('assert'); +var errors = require('../lib/customErrors'); + +describe('errors', function () { + + describe('AbortError', function () { + it('should inherit from Error', function () { + var e = new errors.AbortError({}); + assert.strictEqual(e.message, ''); + assert.strictEqual(e.name, 'AbortError'); + assert.strictEqual(Object.keys(e).length, 0); + assert(e instanceof Error); + assert(e instanceof errors.AbortError); + }); + + it('should list options properties but not name and message', function () { + var e = new errors.AbortError({ + name: 'weird', + message: 'hello world', + property: true + }); + assert.strictEqual(e.message, 'hello world'); + assert.strictEqual(e.name, 'weird'); + assert.strictEqual(e.property, true); + assert.strictEqual(Object.keys(e).length, 1); + assert(e instanceof Error); + assert(e instanceof errors.AbortError); + assert(delete e.name); + assert.strictEqual(e.name, 'Error'); + }); + + it('should change name and message', function () { + var e = new errors.AbortError({ + message: 'hello world', + property: true + }); + assert.strictEqual(e.name, 'AbortError'); + assert.strictEqual(e.message, 'hello world'); + e.name = 'foo'; + e.message = 'foobar'; + assert.strictEqual(e.name, 'foo'); + assert.strictEqual(e.message, 'foobar'); + }); + }); + + describe('AggregateError', function () { + it('should inherit from Error and AbortError', function () { + var e = new errors.AggregateError({}); + assert.strictEqual(e.message, ''); + assert.strictEqual(e.name, 'AggregateError'); + assert.strictEqual(Object.keys(e).length, 0); + assert(e instanceof Error); + assert(e instanceof errors.AggregateError); + assert(e instanceof errors.AbortError); + }); + + it('should list options properties but not name and message', function () { + var e = new errors.AggregateError({ + name: 'weird', + message: 'hello world', + property: true + }); + assert.strictEqual(e.message, 'hello world'); + assert.strictEqual(e.name, 'weird'); + assert.strictEqual(e.property, true); + assert.strictEqual(Object.keys(e).length, 1); + assert(e instanceof Error); + assert(e instanceof errors.AggregateError); + assert(e instanceof errors.AbortError); + assert(delete e.name); + assert.strictEqual(e.name, 'Error'); + }); + + it('should change name and message', function () { + var e = new errors.AggregateError({ + message: 'hello world', + property: true + }); + assert.strictEqual(e.name, 'AggregateError'); + assert.strictEqual(e.message, 'hello world'); + e.name = 'foo'; + e.message = 'foobar'; + assert.strictEqual(e.name, 'foo'); + assert.strictEqual(e.message, 'foobar'); + }); + }); +}); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 8ba91fdfe4..cb36b6c4af 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -998,8 +998,12 @@ describe('The node_redis client', function () { 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')); + // Make sure we call execute out of the reply + // ready is called in a reply + process.nextTick(function () { + // 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 55528d8b1b102b2c181253ab6618d6adcd3b9973 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 25 May 2016 22:52:31 +0300 Subject: [PATCH 04/17] Revert 228573b8d78c7bb3d00c28413093422428b27325 Not inheriting from the prototype is a BC --- changelog.md | 4 ++++ lib/rawObject.js | 8 -------- lib/utils.js | 4 +--- test/commands/hgetall.spec.js | 8 +++----- test/pubsub.spec.js | 4 ++-- 5 files changed, 10 insertions(+), 18 deletions(-) delete mode 100644 lib/rawObject.js diff --git a/changelog.md b/changelog.md index ce2f6037b4..8694afa4a4 100644 --- a/changelog.md +++ b/changelog.md @@ -11,6 +11,10 @@ Features - The JS parser is from now on the new default as it is a lot faster than the hiredis parser - This is no BC as there is no changed behavior for the user at all but just a performance improvement. Explicitly requireing the Hiredis parser is still possible. +Bugfixes + +- Reverted support for `__proto__` (v.2.6.0-2) to prevent and breaking change + Deprecations - The `parser` option is deprecated and should be removed. The built-in Javascript parser is a lot faster than the hiredis parser and has more features diff --git a/lib/rawObject.js b/lib/rawObject.js deleted file mode 100644 index 26376fc360..0000000000 --- a/lib/rawObject.js +++ /dev/null @@ -1,8 +0,0 @@ -'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 d3d9c2fa59..fafffdac7f 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -1,7 +1,5 @@ '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 therefore always the same instanceof X function replyToObject (reply) { @@ -9,7 +7,7 @@ function replyToObject (reply) { if (reply.length === 0 || !(reply instanceof Array)) { return null; } - var obj = new RawObject(); + var obj = {}; 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 ba44326510..6c00e3ed09 100644 --- a/test/commands/hgetall.spec.js +++ b/test/commands/hgetall.spec.js @@ -22,12 +22,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.hmset(['hosts', 'hasOwnProperty', '1', 'another', '23', 'home', '1234'], helper.isString('OK')); client.HGETALL(['hosts'], function (err, obj) { - 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(3, Object.keys(obj).length); + assert.strictEqual('1', obj.hasOwnProperty.toString()); assert.strictEqual('23', obj.another.toString()); assert.strictEqual('1234', obj.home.toString()); done(err); diff --git a/test/pubsub.spec.js b/test/pubsub.spec.js index 1d33b12994..65fce29014 100644 --- a/test/pubsub.spec.js +++ b/test/pubsub.spec.js @@ -204,8 +204,8 @@ describe('publish/subscribe', function () { it('subscribe; close; resubscribe with prototype inherited property names', function (done) { var count = 0; - var channels = ['__proto__', 'channel 2']; - var msg = ['hi from channel __proto__', 'hi from channel 2']; + var channels = ['channel 1', 'channel 2']; + var msg = ['hi from channel 1', 'hi from channel 2']; sub.on('message', function (channel, message) { var n = Math.max(count - 1, 0); From ffaaf0f6d5311326f9a1f8f6c6a3fd7d060a0b44 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 27 May 2016 16:25:59 +0200 Subject: [PATCH 05/17] Add name property to all Redis functions --- changelog.md | 1 + lib/commands.js | 6 ++++++ lib/individualCommands.js | 4 ++-- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/changelog.md b/changelog.md index 8694afa4a4..74630e63a7 100644 --- a/changelog.md +++ b/changelog.md @@ -10,6 +10,7 @@ Features - Updated [redis-parser](https://github.com/NodeRedis/redis-parser) dependency ([changelog](https://github.com/NodeRedis/redis-parser/releases/tag/v.2.0.0)) - The JS parser is from now on the new default as it is a lot faster than the hiredis parser - This is no BC as there is no changed behavior for the user at all but just a performance improvement. Explicitly requireing the Hiredis parser is still possible. +- Added name property to all Redis functions Bugfixes diff --git a/lib/commands.js b/lib/commands.js index a99d025714..fc20ff8165 100644 --- a/lib/commands.js +++ b/lib/commands.js @@ -44,6 +44,9 @@ commands.list.forEach(function (command) { } return this.internal_send_command(command, arr, callback); }; + Object.defineProperty(RedisClient.prototype[command], 'name', { + value: command + }); } // Do not override existing functions @@ -82,5 +85,8 @@ commands.list.forEach(function (command) { this.queue.push([command, arr, callback]); return this; }; + Object.defineProperty(Multi.prototype[command], 'name', { + value: command + }); } }); diff --git a/lib/individualCommands.js b/lib/individualCommands.js index 808ad99a0b..d676b0ce7a 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -96,7 +96,7 @@ function quit_callback (self, callback) { }; } -RedisClient.prototype.QUIT = RedisClient.prototype.quit = function (callback) { +RedisClient.prototype.QUIT = RedisClient.prototype.quit = function quit (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; @@ -108,7 +108,7 @@ RedisClient.prototype.QUIT = RedisClient.prototype.quit = function (callback) { }; // Only works with batch, not in a transaction -Multi.prototype.QUIT = Multi.prototype.quit = function (callback) { +Multi.prototype.QUIT = Multi.prototype.quit = function quit (callback) { var self = this._client; var call_on_write = function () { // If called in a multi context, we expect redis is available From 8008fb5eb488b19cfd3526b6b970fea30f6626de Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 27 May 2016 16:29:43 +0200 Subject: [PATCH 06/17] Fix changelog entry --- changelog.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/changelog.md b/changelog.md index 74630e63a7..683d9d94c6 100644 --- a/changelog.md +++ b/changelog.md @@ -24,14 +24,13 @@ Deprecations Features -- Added support for the new `CLIENT REPLY ON|OFF|SKIP` command (Redis v.3.2) +- Added support for the new [CLIENT REPLY ON|OFF|SKIP](http://redis.io/commands/client-reply) 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 From a90b791295b446f7f9a05a277757b4a23c434409 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 27 May 2016 16:35:09 +0200 Subject: [PATCH 07/17] Only run detect_buffers tests once --- test/detect_buffers.spec.js | 445 ++++++++++++++++++------------------ 1 file changed, 220 insertions(+), 225 deletions(-) diff --git a/test/detect_buffers.spec.js b/test/detect_buffers.spec.js index 82f1aa99e3..2e2f7f1ed0 100644 --- a/test/detect_buffers.spec.js +++ b/test/detect_buffers.spec.js @@ -7,265 +7,260 @@ var redis = config.redis; describe('detect_buffers', function () { - helper.allTests(function (parser, ip, args) { + var client; + var args = config.configureClient('javascript', 'localhost', { + detect_buffers: true + }); + + beforeEach(function (done) { + client = redis.createClient.apply(null, args); + client.once('error', done); + client.once('connect', function () { + client.flushdb(function (err) { + client.hmset('hash key 2', 'key 1', 'val 1', 'key 2', 'val 2'); + client.set('string key 1', 'string value'); + return done(err); + }); + }); + }); - describe('using ' + parser + ' and ' + ip, function () { - var client; - var args = config.configureClient(parser, ip, { - detect_buffers: true + afterEach(function () { + client.end(true); + }); + + describe('get', function () { + describe('first argument is a string', function () { + it('returns a string', function (done) { + client.get('string key 1', helper.isString('string value', done)); }); - beforeEach(function (done) { - client = redis.createClient.apply(null, args); - client.once('error', done); - client.once('connect', function () { - client.flushdb(function (err) { - client.hmset('hash key 2', 'key 1', 'val 1', 'key 2', 'val 2'); - client.set('string key 1', 'string value'); - return done(err); - }); + it('returns a string when executed as part of transaction', function (done) { + client.multi().get('string key 1').exec(function (err, res) { + helper.isString('string value', done)(err, res[0]); }); }); + }); - afterEach(function () { - client.end(true); + describe('first argument is a buffer', function () { + it('returns a buffer', function (done) { + client.get(new Buffer('string key 1'), function (err, reply) { + assert.strictEqual(true, Buffer.isBuffer(reply)); + assert.strictEqual('', reply.inspect()); + return done(err); + }); }); - describe('get', function () { - describe('first argument is a string', function () { - it('returns a string', function (done) { - client.get('string key 1', helper.isString('string value', done)); - }); + it('returns a bufffer when executed as part of transaction', function (done) { + client.multi().get(new Buffer('string key 1')).exec(function (err, reply) { + assert.strictEqual(1, reply.length); + assert.strictEqual(true, Buffer.isBuffer(reply[0])); + assert.strictEqual('', reply[0].inspect()); + return done(err); + }); + }); + }); + }); - it('returns a string when executed as part of transaction', function (done) { - client.multi().get('string key 1').exec(function (err, res) { - helper.isString('string value', done)(err, res[0]); - }); - }); + describe('multi.hget', function () { + it('can interleave string and buffer results', function (done) { + client.multi() + .hget('hash key 2', 'key 1') + .hget(new Buffer('hash key 2'), 'key 1') + .hget('hash key 2', new Buffer('key 2')) + .hget('hash key 2', 'key 2') + .exec(function (err, reply) { + assert.strictEqual(true, Array.isArray(reply)); + assert.strictEqual(4, reply.length); + assert.strictEqual('val 1', reply[0]); + assert.strictEqual(true, Buffer.isBuffer(reply[1])); + assert.strictEqual('', reply[1].inspect()); + assert.strictEqual(true, Buffer.isBuffer(reply[2])); + assert.strictEqual('', reply[2].inspect()); + assert.strictEqual('val 2', reply[3]); + return done(err); }); + }); + }); - describe('first argument is a buffer', function () { - it('returns a buffer', function (done) { - client.get(new Buffer('string key 1'), function (err, reply) { - assert.strictEqual(true, Buffer.isBuffer(reply)); - assert.strictEqual('', reply.inspect()); - return done(err); - }); - }); + describe('batch.hget', function () { + it('can interleave string and buffer results', function (done) { + client.batch() + .hget('hash key 2', 'key 1') + .hget(new Buffer('hash key 2'), 'key 1') + .hget('hash key 2', new Buffer('key 2')) + .hget('hash key 2', 'key 2') + .exec(function (err, reply) { + assert.strictEqual(true, Array.isArray(reply)); + assert.strictEqual(4, reply.length); + assert.strictEqual('val 1', reply[0]); + assert.strictEqual(true, Buffer.isBuffer(reply[1])); + assert.strictEqual('', reply[1].inspect()); + assert.strictEqual(true, Buffer.isBuffer(reply[2])); + assert.strictEqual('', reply[2].inspect()); + assert.strictEqual('val 2', reply[3]); + return done(err); + }); + }); + }); - it('returns a bufffer when executed as part of transaction', function (done) { - client.multi().get(new Buffer('string key 1')).exec(function (err, reply) { - assert.strictEqual(1, reply.length); - assert.strictEqual(true, Buffer.isBuffer(reply[0])); - assert.strictEqual('', reply[0].inspect()); - return done(err); - }); - }); + describe('hmget', function () { + describe('first argument is a string', function () { + it('returns strings for keys requested', function (done) { + client.hmget('hash key 2', 'key 1', 'key 2', function (err, reply) { + assert.strictEqual(true, Array.isArray(reply)); + assert.strictEqual(2, reply.length); + assert.strictEqual('val 1', reply[0]); + assert.strictEqual('val 2', reply[1]); + return done(err); }); }); - describe('multi.hget', function () { - it('can interleave string and buffer results', function (done) { - client.multi() - .hget('hash key 2', 'key 1') - .hget(new Buffer('hash key 2'), 'key 1') - .hget('hash key 2', new Buffer('key 2')) - .hget('hash key 2', 'key 2') - .exec(function (err, reply) { - assert.strictEqual(true, Array.isArray(reply)); - assert.strictEqual(4, reply.length); - assert.strictEqual('val 1', reply[0]); - assert.strictEqual(true, Buffer.isBuffer(reply[1])); - assert.strictEqual('', reply[1].inspect()); - assert.strictEqual(true, Buffer.isBuffer(reply[2])); - assert.strictEqual('', reply[2].inspect()); - assert.strictEqual('val 2', reply[3]); - return done(err); - }); + it('returns strings for keys requested in transaction', function (done) { + client.multi().hmget('hash key 2', 'key 1', 'key 2').exec(function (err, reply) { + assert.strictEqual(true, Array.isArray(reply)); + assert.strictEqual(1, reply.length); + assert.strictEqual(2, reply[0].length); + assert.strictEqual('val 1', reply[0][0]); + assert.strictEqual('val 2', reply[0][1]); + return done(err); }); }); - describe('batch.hget', function () { - it('can interleave string and buffer results', function (done) { - client.batch() - .hget('hash key 2', 'key 1') - .hget(new Buffer('hash key 2'), 'key 1') - .hget('hash key 2', new Buffer('key 2')) - .hget('hash key 2', 'key 2') - .exec(function (err, reply) { - assert.strictEqual(true, Array.isArray(reply)); - assert.strictEqual(4, reply.length); - assert.strictEqual('val 1', reply[0]); - assert.strictEqual(true, Buffer.isBuffer(reply[1])); - assert.strictEqual('', reply[1].inspect()); - assert.strictEqual(true, Buffer.isBuffer(reply[2])); - assert.strictEqual('', reply[2].inspect()); - assert.strictEqual('val 2', reply[3]); - return done(err); - }); + it('handles array of strings with undefined values (repro #344)', function (done) { + client.hmget('hash key 2', 'key 3', 'key 4', function (err, reply) { + assert.strictEqual(true, Array.isArray(reply)); + assert.strictEqual(2, reply.length); + assert.equal(null, reply[0]); + assert.equal(null, reply[1]); + return done(err); }); }); - describe('hmget', function () { - describe('first argument is a string', function () { - it('returns strings for keys requested', function (done) { - client.hmget('hash key 2', 'key 1', 'key 2', function (err, reply) { - assert.strictEqual(true, Array.isArray(reply)); - assert.strictEqual(2, reply.length); - assert.strictEqual('val 1', reply[0]); - assert.strictEqual('val 2', reply[1]); - return done(err); - }); - }); - - it('returns strings for keys requested in transaction', function (done) { - client.multi().hmget('hash key 2', 'key 1', 'key 2').exec(function (err, reply) { - assert.strictEqual(true, Array.isArray(reply)); - assert.strictEqual(1, reply.length); - assert.strictEqual(2, reply[0].length); - assert.strictEqual('val 1', reply[0][0]); - assert.strictEqual('val 2', reply[0][1]); - return done(err); - }); - }); - - it('handles array of strings with undefined values (repro #344)', function (done) { - client.hmget('hash key 2', 'key 3', 'key 4', function (err, reply) { - assert.strictEqual(true, Array.isArray(reply)); - assert.strictEqual(2, reply.length); - assert.equal(null, reply[0]); - assert.equal(null, reply[1]); - return done(err); - }); - }); - - it('handles array of strings with undefined values in transaction (repro #344)', function (done) { - client.multi().hmget('hash key 2', 'key 3', 'key 4').exec(function (err, reply) { - assert.strictEqual(true, Array.isArray(reply)); - assert.strictEqual(1, reply.length); - assert.strictEqual(2, reply[0].length); - assert.equal(null, reply[0][0]); - assert.equal(null, reply[0][1]); - return done(err); - }); - }); + it('handles array of strings with undefined values in transaction (repro #344)', function (done) { + client.multi().hmget('hash key 2', 'key 3', 'key 4').exec(function (err, reply) { + assert.strictEqual(true, Array.isArray(reply)); + assert.strictEqual(1, reply.length); + assert.strictEqual(2, reply[0].length); + assert.equal(null, reply[0][0]); + assert.equal(null, reply[0][1]); + return done(err); }); + }); + }); - describe('first argument is a buffer', function () { - it('returns buffers for keys requested', function (done) { - client.hmget(new Buffer('hash key 2'), 'key 1', 'key 2', function (err, reply) { - assert.strictEqual(true, Array.isArray(reply)); - assert.strictEqual(2, reply.length); - assert.strictEqual(true, Buffer.isBuffer(reply[0])); - assert.strictEqual(true, Buffer.isBuffer(reply[1])); - assert.strictEqual('', reply[0].inspect()); - assert.strictEqual('', reply[1].inspect()); - return done(err); - }); - }); + describe('first argument is a buffer', function () { + it('returns buffers for keys requested', function (done) { + client.hmget(new Buffer('hash key 2'), 'key 1', 'key 2', function (err, reply) { + assert.strictEqual(true, Array.isArray(reply)); + assert.strictEqual(2, reply.length); + assert.strictEqual(true, Buffer.isBuffer(reply[0])); + assert.strictEqual(true, Buffer.isBuffer(reply[1])); + assert.strictEqual('', reply[0].inspect()); + assert.strictEqual('', reply[1].inspect()); + return done(err); + }); + }); - it('returns buffers for keys requested in transaction', function (done) { - client.multi().hmget(new Buffer('hash key 2'), 'key 1', 'key 2').exec(function (err, reply) { - assert.strictEqual(true, Array.isArray(reply)); - assert.strictEqual(1, reply.length); - assert.strictEqual(2, reply[0].length); - assert.strictEqual(true, Buffer.isBuffer(reply[0][0])); - assert.strictEqual(true, Buffer.isBuffer(reply[0][1])); - assert.strictEqual('', reply[0][0].inspect()); - assert.strictEqual('', reply[0][1].inspect()); - return done(err); - }); - }); + it('returns buffers for keys requested in transaction', function (done) { + client.multi().hmget(new Buffer('hash key 2'), 'key 1', 'key 2').exec(function (err, reply) { + assert.strictEqual(true, Array.isArray(reply)); + assert.strictEqual(1, reply.length); + assert.strictEqual(2, reply[0].length); + assert.strictEqual(true, Buffer.isBuffer(reply[0][0])); + assert.strictEqual(true, Buffer.isBuffer(reply[0][1])); + assert.strictEqual('', reply[0][0].inspect()); + assert.strictEqual('', reply[0][1].inspect()); + return done(err); + }); + }); - it('returns buffers for keys requested in .batch', function (done) { - client.batch().hmget(new Buffer('hash key 2'), 'key 1', 'key 2').exec(function (err, reply) { - assert.strictEqual(true, Array.isArray(reply)); - assert.strictEqual(1, reply.length); - assert.strictEqual(2, reply[0].length); - assert.strictEqual(true, Buffer.isBuffer(reply[0][0])); - assert.strictEqual(true, Buffer.isBuffer(reply[0][1])); - assert.strictEqual('', reply[0][0].inspect()); - assert.strictEqual('', reply[0][1].inspect()); - return done(err); - }); - }); + it('returns buffers for keys requested in .batch', function (done) { + client.batch().hmget(new Buffer('hash key 2'), 'key 1', 'key 2').exec(function (err, reply) { + assert.strictEqual(true, Array.isArray(reply)); + assert.strictEqual(1, reply.length); + assert.strictEqual(2, reply[0].length); + assert.strictEqual(true, Buffer.isBuffer(reply[0][0])); + assert.strictEqual(true, Buffer.isBuffer(reply[0][1])); + assert.strictEqual('', reply[0][0].inspect()); + assert.strictEqual('', reply[0][1].inspect()); + return done(err); }); }); + }); + }); - describe('hgetall', function (done) { - describe('first argument is a string', function () { - it('returns string values', function (done) { - client.hgetall('hash key 2', function (err, reply) { - assert.strictEqual('object', typeof reply); - assert.strictEqual(2, Object.keys(reply).length); - assert.strictEqual('val 1', reply['key 1']); - assert.strictEqual('val 2', reply['key 2']); - return done(err); - }); - }); + describe('hgetall', function (done) { + describe('first argument is a string', function () { + it('returns string values', function (done) { + client.hgetall('hash key 2', function (err, reply) { + assert.strictEqual('object', typeof reply); + assert.strictEqual(2, Object.keys(reply).length); + assert.strictEqual('val 1', reply['key 1']); + assert.strictEqual('val 2', reply['key 2']); + return done(err); + }); + }); - it('returns string values when executed in transaction', function (done) { - client.multi().hgetall('hash key 2').exec(function (err, reply) { - assert.strictEqual(1, reply.length); - assert.strictEqual('object', typeof reply[0]); - assert.strictEqual(2, Object.keys(reply[0]).length); - assert.strictEqual('val 1', reply[0]['key 1']); - assert.strictEqual('val 2', reply[0]['key 2']); - return done(err); - }); - }); + it('returns string values when executed in transaction', function (done) { + client.multi().hgetall('hash key 2').exec(function (err, reply) { + assert.strictEqual(1, reply.length); + assert.strictEqual('object', typeof reply[0]); + assert.strictEqual(2, Object.keys(reply[0]).length); + assert.strictEqual('val 1', reply[0]['key 1']); + assert.strictEqual('val 2', reply[0]['key 2']); + return done(err); + }); + }); - it('returns string values when executed in .batch', function (done) { - client.batch().hgetall('hash key 2').exec(function (err, reply) { - assert.strictEqual(1, reply.length); - assert.strictEqual('object', typeof reply[0]); - assert.strictEqual(2, Object.keys(reply[0]).length); - assert.strictEqual('val 1', reply[0]['key 1']); - assert.strictEqual('val 2', reply[0]['key 2']); - return done(err); - }); - }); + it('returns string values when executed in .batch', function (done) { + client.batch().hgetall('hash key 2').exec(function (err, reply) { + assert.strictEqual(1, reply.length); + assert.strictEqual('object', typeof reply[0]); + assert.strictEqual(2, Object.keys(reply[0]).length); + assert.strictEqual('val 1', reply[0]['key 1']); + assert.strictEqual('val 2', reply[0]['key 2']); + return done(err); }); + }); + }); - describe('first argument is a buffer', function () { - it('returns buffer values', function (done) { - client.hgetall(new Buffer('hash key 2'), function (err, reply) { - assert.strictEqual(null, err); - assert.strictEqual('object', typeof reply); - assert.strictEqual(2, Object.keys(reply).length); - assert.strictEqual(true, Buffer.isBuffer(reply['key 1'])); - assert.strictEqual(true, Buffer.isBuffer(reply['key 2'])); - assert.strictEqual('', reply['key 1'].inspect()); - assert.strictEqual('', reply['key 2'].inspect()); - return done(err); - }); - }); + describe('first argument is a buffer', function () { + it('returns buffer values', function (done) { + client.hgetall(new Buffer('hash key 2'), function (err, reply) { + assert.strictEqual(null, err); + assert.strictEqual('object', typeof reply); + assert.strictEqual(2, Object.keys(reply).length); + assert.strictEqual(true, Buffer.isBuffer(reply['key 1'])); + assert.strictEqual(true, Buffer.isBuffer(reply['key 2'])); + assert.strictEqual('', reply['key 1'].inspect()); + assert.strictEqual('', reply['key 2'].inspect()); + return done(err); + }); + }); - it('returns buffer values when executed in transaction', function (done) { - client.multi().hgetall(new Buffer('hash key 2')).exec(function (err, reply) { - assert.strictEqual(1, reply.length); - assert.strictEqual('object', typeof reply[0]); - assert.strictEqual(2, Object.keys(reply[0]).length); - assert.strictEqual(true, Buffer.isBuffer(reply[0]['key 1'])); - assert.strictEqual(true, Buffer.isBuffer(reply[0]['key 2'])); - assert.strictEqual('', reply[0]['key 1'].inspect()); - assert.strictEqual('', reply[0]['key 2'].inspect()); - return done(err); - }); - }); + it('returns buffer values when executed in transaction', function (done) { + client.multi().hgetall(new Buffer('hash key 2')).exec(function (err, reply) { + assert.strictEqual(1, reply.length); + assert.strictEqual('object', typeof reply[0]); + assert.strictEqual(2, Object.keys(reply[0]).length); + assert.strictEqual(true, Buffer.isBuffer(reply[0]['key 1'])); + assert.strictEqual(true, Buffer.isBuffer(reply[0]['key 2'])); + assert.strictEqual('', reply[0]['key 1'].inspect()); + assert.strictEqual('', reply[0]['key 2'].inspect()); + return done(err); + }); + }); - it('returns buffer values when executed in .batch', function (done) { - client.batch().hgetall(new Buffer('hash key 2')).exec(function (err, reply) { - assert.strictEqual(1, reply.length); - assert.strictEqual('object', typeof reply[0]); - assert.strictEqual(2, Object.keys(reply[0]).length); - assert.strictEqual(true, Buffer.isBuffer(reply[0]['key 1'])); - assert.strictEqual(true, Buffer.isBuffer(reply[0]['key 2'])); - assert.strictEqual('', reply[0]['key 1'].inspect()); - assert.strictEqual('', reply[0]['key 2'].inspect()); - return done(err); - }); - }); + it('returns buffer values when executed in .batch', function (done) { + client.batch().hgetall(new Buffer('hash key 2')).exec(function (err, reply) { + assert.strictEqual(1, reply.length); + assert.strictEqual('object', typeof reply[0]); + assert.strictEqual(2, Object.keys(reply[0]).length); + assert.strictEqual(true, Buffer.isBuffer(reply[0]['key 1'])); + assert.strictEqual(true, Buffer.isBuffer(reply[0]['key 2'])); + assert.strictEqual('', reply[0]['key 1'].inspect()); + assert.strictEqual('', reply[0]['key 2'].inspect()); + return done(err); }); }); }); From 899f9b7fe457d23376176c81f036fc4093678c8f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 27 May 2016 16:51:43 +0200 Subject: [PATCH 08/17] Fix hungry developer typo --- index.js | 2 +- lib/utils.js | 8 ++++---- test/utils.spec.js | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/index.js b/index.js index 406c8c817f..3f6fa1df6e 100644 --- a/index.js +++ b/index.js @@ -1042,7 +1042,7 @@ Object.defineProperty(RedisClient.prototype, 'offline_queue_length', { }); // Add support for camelCase by adding read only properties to the client -// All known exposed snack_case variables are added here +// All known exposed snake_case variables are added here Object.defineProperty(RedisClient.prototype, 'retryDelay', { get: function () { return this.retry_delay; diff --git a/lib/utils.js b/lib/utils.js index fafffdac7f..bc685b9a4f 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -57,14 +57,14 @@ function clone (obj) { var elems = Object.keys(obj); var elem; while (elem = elems.pop()) { - // Accept camelCase options and convert them to snack_case - var snack_case = elem.replace(/[A-Z][^A-Z]/g, '_$&').toLowerCase(); + // Accept camelCase options and convert them to snake_case + var snake_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()) { + if (snake_case !== elem.toLowerCase()) { camelCase = true; } - copy[snack_case] = clone(obj[elem]); + copy[snake_case] = clone(obj[elem]); } return copy; } diff --git a/test/utils.spec.js b/test/utils.spec.js index 094640066b..f6fa8d4e7d 100644 --- a/test/utils.spec.js +++ b/test/utils.spec.js @@ -29,7 +29,7 @@ describe('utils.js', function () { assert.strictEqual(Object.keys(b).length, 0); }); - it('transform camelCase options to snack_case and add the camel_case option', function () { + it('transform camelCase options to snake_case and add the camel_case option', function () { var a = utils.clone({ optionOneTwo: true, retryStrategy: false, From 8b6f2dd35ec232a7adaf8c8da1ea062c0dbb1c78 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 27 May 2016 17:32:42 +0200 Subject: [PATCH 09/17] Refactor command parsing --- index.js | 52 +++++++++++--------------- lib/command.js | 16 ++------ lib/commands.js | 5 ++- lib/extendedApi.js | 3 +- lib/individualCommands.js | 64 ++++++++++++++++---------------- lib/multi.js | 76 +++++++++++++++++--------------------- test/commands/info.spec.js | 6 +-- test/multi.spec.js | 5 +++ test/node_redis.spec.js | 1 + test/pubsub.spec.js | 1 + 10 files changed, 106 insertions(+), 123 deletions(-) diff --git a/index.js b/index.js index 3f6fa1df6e..84c9b64427 100644 --- a/index.js +++ b/index.js @@ -6,8 +6,6 @@ var util = require('util'); var utils = require('./lib/utils'); var Queue = require('double-ended-queue'); var errorClasses = require('./lib/customErrors'); -var Command = require('./lib/command').Command; -var OfflineCommand = require('./lib/command').OfflineCommand; var EventEmitter = require('events'); var Parser = require('redis-parser'); var commands = require('redis-commands'); @@ -156,6 +154,7 @@ function RedisClient (options, stream) { this.times_connected = 0; this.buffers = options.return_buffers || options.detect_buffers; this.options = options; + this.old_state = {}; this.reply = 'ON'; // Returning replies is the default // Init parser this.reply_parser = create_parser(this); @@ -443,14 +442,10 @@ RedisClient.prototype.on_ready = function () { // Restore modal commands from previous connection. The order of the commands is important if (this.selected_db !== undefined) { - this.internal_send_command('select', [this.selected_db]); + this.select(this.selected_db); } - if (this.old_state !== null) { - this.monitoring = this.old_state.monitoring; - 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', []); // The state is still set + if (this.old_state.monitoring) { // Monitor has to be fired before pub sub commands + this.monitor(); } var callback_count = Object.keys(this.subscription_set).length; if (!this.options.disable_resubscribing && callback_count) { @@ -466,8 +461,8 @@ RedisClient.prototype.on_ready = function () { debug('Sending pub/sub on_ready commands'); 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); + var args = this.subscription_set[key]; + this[command]([args], callback); } this.send_offline_queue(); return; @@ -530,7 +525,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, command_obj.call_on_write); + this.internal_send_command(command_obj); } this.drain(); }; @@ -575,8 +570,7 @@ RedisClient.prototype.connection_gone = function (why, error) { this.pipeline = false; var state = { - monitoring: this.monitoring, - pub_sub_mode: this.pub_sub_mode + monitoring: this.monitoring }; this.old_state = state; this.monitoring = false; @@ -834,7 +828,6 @@ RedisClient.prototype.return_reply = function (reply) { function handle_offline_command (self, command_obj) { var command = command_obj.command; - var callback = command_obj.callback; var err, msg; if (self.closing || !self.enable_offline_queue) { command = command.toUpperCase(); @@ -852,10 +845,10 @@ function handle_offline_command (self, command_obj) { code: 'NR_CLOSED', command: command }); - if (command_obj.args && command_obj.args.length) { + if (command_obj.args.length) { err.args = command_obj.args; } - utils.reply_in_order(self, callback, err); + utils.reply_in_order(self, command_obj.callback, err); } else { debug('Queueing ' + command + ' for next server connection.'); self.offline_queue.push(command_obj); @@ -865,22 +858,23 @@ function handle_offline_command (self, command_obj) { // 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; +RedisClient.prototype.internal_send_command = function (command_obj) { + var arg, prefix_keys; var i = 0; var command_str = ''; + var args = command_obj.args; + var command = command_obj.command; var len = args.length; var big_data = false; - var buffer_args = false; var args_copy = new Array(len); - if (process.domain && callback) { - callback = process.domain.bind(callback); + if (process.domain && command_obj.callback) { + command_obj.callback = process.domain.bind(command_obj.callback); } if (this.ready === false || this.stream.writable === false) { // Handle offline commands right away - handle_offline_command(this, new OfflineCommand(command, args, callback, call_on_write)); + handle_offline_command(this, command_obj); return false; // Indicate buffering } @@ -905,7 +899,7 @@ RedisClient.prototype.internal_send_command = function (command, args, callback, args_copy[i] = 'null'; // Backwards compatible :/ } else if (Buffer.isBuffer(args[i])) { args_copy[i] = args[i]; - buffer_args = true; + command_obj.buffer_args = true; big_data = true; } else { this.warn( @@ -927,8 +921,6 @@ RedisClient.prototype.internal_send_command = function (command, args, callback, args_copy[i] = '' + args[i]; } } - // 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); @@ -967,8 +959,8 @@ RedisClient.prototype.internal_send_command = function (command, args, callback, debug('send_command: buffer send ' + arg.length + ' bytes'); } } - if (call_on_write) { - call_on_write(); + if (command_obj.call_on_write) { + command_obj.call_on_write(); } // Handle `CLIENT REPLY ON|OFF|SKIP` // This has to be checked after call_on_write @@ -978,8 +970,8 @@ RedisClient.prototype.internal_send_command = function (command, args, callback, } 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 (command_obj.callback) { + utils.reply_in_order(this, command_obj.callback, null, undefined, this.command_queue); } if (this.reply === 'SKIP') { this.reply = 'SKIP_ONE_MORE'; diff --git a/lib/command.js b/lib/command.js index e63d338b5e..6414d4d6fc 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1,22 +1,12 @@ 'use strict'; -// This Command constructor is ever so slightly faster than using an object literal, but more importantly, using -// a named constructor helps it show up meaningfully in the V8 CPU profiler and in heap snapshots. -function Command (command, args, buffer_args, callback) { - this.command = command; - this.args = args; - this.buffer_args = buffer_args; - this.callback = callback; -} -function OfflineCommand (command, args, callback, call_on_write) { +function Command (command, args, callback, call_on_write) { this.command = command; this.args = args; + this.buffer_args = false; this.callback = callback; this.call_on_write = call_on_write; } -module.exports = { - Command: Command, - OfflineCommand: OfflineCommand -}; +module.exports = Command; diff --git a/lib/commands.js b/lib/commands.js index fc20ff8165..805c3b9a50 100644 --- a/lib/commands.js +++ b/lib/commands.js @@ -3,6 +3,7 @@ var commands = require('redis-commands'); var Multi = require('./multi'); var RedisClient = require('../').RedisClient; +var Command = require('./command'); // TODO: Rewrite this including the invidual commands into a Commands class // that provided a functionality to add new commands to the client @@ -42,7 +43,7 @@ commands.list.forEach(function (command) { arr[i] = arguments[i]; } } - return this.internal_send_command(command, arr, callback); + return this.internal_send_command(new Command(command, arr, callback)); }; Object.defineProperty(RedisClient.prototype[command], 'name', { value: command @@ -82,7 +83,7 @@ commands.list.forEach(function (command) { arr[i] = arguments[i]; } } - this.queue.push([command, arr, callback]); + this.queue.push(new Command(command, arr, callback)); return this; }; Object.defineProperty(Multi.prototype[command], 'name', { diff --git a/lib/extendedApi.js b/lib/extendedApi.js index ece77d22fe..0e9589775b 100644 --- a/lib/extendedApi.js +++ b/lib/extendedApi.js @@ -3,6 +3,7 @@ var utils = require('./utils'); var debug = require('./debug'); var RedisClient = require('../').RedisClient; +var Command = require('./command'); var noop = function () {}; /********************************************** @@ -36,7 +37,7 @@ RedisClient.prototype.send_command = RedisClient.prototype.sendCommand = functio // but this might change from time to time and at the moment there's no good way to distinguishe them // from each other, so let's just do it do it this way for the time being if (command === 'multi' || typeof this[command] !== 'function') { - return this.internal_send_command(command, args, callback); + return this.internal_send_command(new Command(command, args, callback)); } if (typeof callback === 'function') { args = args.concat([callback]); // Prevent manipulating the input array diff --git a/lib/individualCommands.js b/lib/individualCommands.js index d676b0ce7a..b7306c84d4 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -3,6 +3,7 @@ var utils = require('./utils'); var debug = require('./debug'); var Multi = require('./multi'); +var Command = require('./command'); var no_password_is_set = /no password is set/; var loading = /LOADING/; var RedisClient = require('../').RedisClient; @@ -42,33 +43,34 @@ function select_callback (self, db, callback) { } RedisClient.prototype.select = RedisClient.prototype.SELECT = function select (db, callback) { - return this.internal_send_command('select', [db], select_callback(this, db, callback)); + return this.internal_send_command(new Command('select', [db], select_callback(this, db, callback))); }; Multi.prototype.select = Multi.prototype.SELECT = function select (db, callback) { - this.queue.push(['select', [db], select_callback(this._client, db, callback)]); + this.queue.push(new Command('select', [db], select_callback(this._client, db, callback))); return this; }; -function monitor_callback (self, callback) { - return function (err, res) { - if (err === null) { - 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)); + var self = this; + var call_on_write = function () { + // Activating monitor mode has to happen before Redis returned the callback, + // as the client could receive monitoring commands before the callback returned through a race condition + self.monitoring = true; + }; + return this.internal_send_command(new Command('monitor', [], callback, call_on_write)); }; // 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)]); + var self = this; + var call_on_write = function () { + self._client.monitoring = true; + }; + this.queue.push(new Command('monitor', [], callback, call_on_write)); return this; } // Set multi monitoring to indicate the exec that it should abort @@ -100,7 +102,7 @@ RedisClient.prototype.QUIT = RedisClient.prototype.quit = function quit (callbac // 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)); + var backpressure_indicator = this.internal_send_command(new 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; @@ -115,7 +117,7 @@ Multi.prototype.QUIT = Multi.prototype.quit = function quit (callback) { self.closing = true; self.ready = false; }; - this.queue.push(['quit', [], quit_callback(self, callback), call_on_write]); + this.queue.push(new Command('quit', [], quit_callback(self, callback), call_on_write)); return this; }; @@ -164,7 +166,7 @@ RedisClient.prototype.info = RedisClient.prototype.INFO = function info (section } else if (section !== undefined) { args = Array.isArray(section) ? section : [section]; } - return this.internal_send_command('info', args, info_callback(this, callback)); + return this.internal_send_command(new Command('info', args, info_callback(this, callback))); }; Multi.prototype.info = Multi.prototype.INFO = function info (section, callback) { @@ -174,7 +176,7 @@ Multi.prototype.info = Multi.prototype.INFO = function info (section, callback) } else if (section !== undefined) { args = Array.isArray(section) ? section : [section]; } - this.queue.push(['info', args, info_callback(this._client, callback)]); + this.queue.push(new Command('info', args, info_callback(this._client, callback))); return this; }; @@ -205,7 +207,7 @@ RedisClient.prototype.auth = RedisClient.prototype.AUTH = function auth (pass, c 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)); + var tmp = this.internal_send_command(new Command('auth', [pass], auth_callback(this, pass, callback))); this.ready = ready; return tmp; }; @@ -216,7 +218,7 @@ Multi.prototype.auth = Multi.prototype.AUTH = function auth (pass, callback) { // Stash auth for connect and reconnect. this.auth_pass = pass; - this.queue.push(['auth', [pass], auth_callback(this._client, callback)]); + this.queue.push(new Command('auth', [pass], auth_callback(this._client, callback))); return this; }; @@ -262,7 +264,7 @@ RedisClient.prototype.client = RedisClient.prototype.CLIENT = function client () }; } } - return this.internal_send_command('client', arr, callback, call_on_write); + return this.internal_send_command(new Command('client', arr, callback, call_on_write)); }; Multi.prototype.client = Multi.prototype.CLIENT = function client () { @@ -307,7 +309,7 @@ Multi.prototype.client = Multi.prototype.CLIENT = function client () { }; } } - this.queue.push(['client', arr, callback, call_on_write]); + this.queue.push(new Command('client', arr, callback, call_on_write)); return this; }; @@ -347,7 +349,7 @@ RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function hmset () { arr[i] = arguments[i]; } } - return this.internal_send_command('hmset', arr, callback); + return this.internal_send_command(new Command('hmset', arr, callback)); }; Multi.prototype.hmset = Multi.prototype.HMSET = function hmset () { @@ -386,7 +388,7 @@ Multi.prototype.hmset = Multi.prototype.HMSET = function hmset () { arr[i] = arguments[i]; } } - this.queue.push(['hmset', arr, callback]); + this.queue.push(new Command('hmset', arr, callback)); return this; }; @@ -414,7 +416,7 @@ RedisClient.prototype.subscribe = RedisClient.prototype.SUBSCRIBE = function sub 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); + return this.internal_send_command(new Command('subscribe', arr, callback, call_on_write)); }; Multi.prototype.subscribe = Multi.prototype.SUBSCRIBE = function subscribe () { @@ -441,7 +443,7 @@ Multi.prototype.subscribe = Multi.prototype.SUBSCRIBE = function subscribe () { 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]); + this.queue.push(new Command('subscribe', arr, callback, call_on_write)); return this; }; @@ -470,7 +472,7 @@ RedisClient.prototype.unsubscribe = RedisClient.prototype.UNSUBSCRIBE = 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); + return this.internal_send_command(new Command('unsubscribe', arr, callback, call_on_write)); }; Multi.prototype.unsubscribe = Multi.prototype.UNSUBSCRIBE = function unsubscribe () { @@ -498,7 +500,7 @@ Multi.prototype.unsubscribe = Multi.prototype.UNSUBSCRIBE = function unsubscribe // 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]); + this.queue.push(new Command('unsubscribe', arr, callback, call_on_write)); return this; }; @@ -526,7 +528,7 @@ RedisClient.prototype.psubscribe = RedisClient.prototype.PSUBSCRIBE = function p 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); + return this.internal_send_command(new Command('psubscribe', arr, callback, call_on_write)); }; Multi.prototype.psubscribe = Multi.prototype.PSUBSCRIBE = function psubscribe () { @@ -553,7 +555,7 @@ Multi.prototype.psubscribe = Multi.prototype.PSUBSCRIBE = function psubscribe () 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]); + this.queue.push(new Command('psubscribe', arr, callback, call_on_write)); return this; }; @@ -582,7 +584,7 @@ RedisClient.prototype.punsubscribe = RedisClient.prototype.PUNSUBSCRIBE = functi // 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); + return this.internal_send_command(new Command('punsubscribe', arr, callback, call_on_write)); }; Multi.prototype.punsubscribe = Multi.prototype.PUNSUBSCRIBE = function punsubscribe () { @@ -610,6 +612,6 @@ Multi.prototype.punsubscribe = Multi.prototype.PUNSUBSCRIBE = function punsubscr // 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]); + this.queue.push(new Command('punsubscribe', arr, callback, call_on_write)); return this; }; diff --git a/lib/multi.js b/lib/multi.js index 433c45bd50..f80526b5c2 100644 --- a/lib/multi.js +++ b/lib/multi.js @@ -2,6 +2,7 @@ var Queue = require('double-ended-queue'); var utils = require('./utils'); +var Command = require('./command'); function Multi (client, args) { this._client = client; @@ -20,18 +21,23 @@ function Multi (client, args) { } } -function pipeline_transaction_command (self, command, args, index, cb, call_on_write) { +function pipeline_transaction_command (self, command_obj, index) { // Queueing is done first, then the commands are executed - self._client.send_command(command, args, function (err, reply) { + var tmp = command_obj.callback; + command_obj.callback = function (err, reply) { // 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); + if (tmp) { + tmp(err); } err.position = index; self.errors.push(err); } - }); + // Keep track of who wants buffer responses: + // By the time the callback is called the command_obj got the buffer_args attribute attached + self.wants_buffers[index] = command_obj.buffer_args; + }; + self._client.internal_send_command(command_obj); } Multi.prototype.exec_atomic = Multi.prototype.EXEC_ATOMIC = Multi.prototype.execAtomic = function exec_atomic (callback) { @@ -42,7 +48,7 @@ Multi.prototype.exec_atomic = Multi.prototype.EXEC_ATOMIC = Multi.prototype.exec }; function multi_callback (self, err, replies) { - var i = 0, args; + var i = 0, command_obj; if (err) { err.errors = self.errors; @@ -56,22 +62,22 @@ function multi_callback (self, err, replies) { } if (replies) { - while (args = self.queue.shift()) { + while (command_obj = self.queue.shift()) { if (replies[i] instanceof Error) { var match = replies[i].message.match(utils.err_code); // LUA script could return user errors that don't behave like all other errors! if (match) { replies[i].code = match[1]; } - replies[i].command = args[0].toUpperCase(); - if (typeof args[2] === 'function') { - args[2](replies[i]); + replies[i].command = command_obj.command.toUpperCase(); + if (typeof command_obj.callback === 'function') { + command_obj.callback(replies[i]); } } else { // If we asked for strings, even in detect_buffers mode, then return strings: - replies[i] = self._client.handle_reply(replies[i], args[0], self.wants_buffers[i]); - if (typeof args[2] === 'function') { - args[2](null, replies[i]); + replies[i] = self._client.handle_reply(replies[i], command_obj.command, self.wants_buffers[i]); + if (typeof command_obj.callback === 'function') { + command_obj.callback(null, replies[i]); } } i++; @@ -98,30 +104,16 @@ 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', [], -1); + pipeline_transaction_command(self, new Command('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 - 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 < command_obj[1].length; i += 1) { - if (command_obj[1][i] instanceof Buffer) { - self.wants_buffers[index] = true; - break; - } - } - } - pipeline_transaction_command(self, command, command_obj[1], index, cb, call_on_write); + pipeline_transaction_command(self, self.queue.get(index), index); } - self._client.internal_send_command('exec', [], function (err, replies) { + self._client.internal_send_command(new Command('exec', [], function (err, replies) { multi_callback(self, err, replies); - }); + })); self._client.uncork(); return !self._client.should_buffer; }; @@ -144,16 +136,17 @@ Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = funct var len = self.queue.length; var index = 0; var command_obj; + if (len === 0) { + utils.reply_in_order(self._client, callback, null, []); + return !self._client.should_buffer; + } 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.internal_send_command(command_obj); } 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) { @@ -175,18 +168,15 @@ Multi.prototype.exec = Multi.prototype.EXEC = Multi.prototype.exec_batch = funct }; self.results = []; 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 command_obj[2] === 'function') { - cb = batch_callback(self, command_obj[2], index); + if (typeof command_obj.callback === 'function') { + command_obj.callback = batch_callback(self, command_obj.callback, index); } else { - cb = callback_without_own_cb; + command_obj.callback = callback_without_own_cb; } if (typeof callback === 'function' && index === len - 1) { - cb = last_callback(cb); + command_obj.callback = last_callback(command_obj.callback); } - this._client.internal_send_command(command, command_obj[1], cb, call_on_write); + this._client.internal_send_command(command_obj); index++; } self._client.uncork(); diff --git a/test/commands/info.spec.js b/test/commands/info.spec.js index 3a67a1a178..39a9e9f5cc 100644 --- a/test/commands/info.spec.js +++ b/test/commands/info.spec.js @@ -56,9 +56,9 @@ describe("The 'info' method", function () { it('check redis v.2.4 support', function (done) { var end = helper.callFuncAfter(done, 2); - client.internal_send_command = function (command, args, callback) { - assert.strictEqual(args.length, 0); - assert.strictEqual(command, 'info'); + client.internal_send_command = function (command_obj) { + assert.strictEqual(command_obj.args.length, 0); + assert.strictEqual(command_obj.command, 'info'); end(); }; client.info(); diff --git a/test/multi.spec.js b/test/multi.spec.js index 9010759378..1b13642a3e 100644 --- a/test/multi.spec.js +++ b/test/multi.spec.js @@ -692,6 +692,11 @@ describe("The 'multi' method", function () { // subscribe => enters subscribe mode and this does not work in combination with exec (the same for psubscribe, unsubscribe...) // + // Make sure send_command is not called + client.send_command = function () { + throw new Error('failed'); + }; + assert.strictEqual(client.selected_db, undefined); var multi = client.multi(); multi.select(5, function (err, res) { diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index cb36b6c4af..1c897116a6 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -325,6 +325,7 @@ describe('The node_redis client', function () { bclient.blpop('blocking list 2', 5, function (err, value) { assert.strictEqual(value[0], 'blocking list 2'); assert.strictEqual(value[1], 'initial value'); + bclient.end(true); done(err); }); bclient.once('ready', function () { diff --git a/test/pubsub.spec.js b/test/pubsub.spec.js index 65fce29014..74d92c96e3 100644 --- a/test/pubsub.spec.js +++ b/test/pubsub.spec.js @@ -215,6 +215,7 @@ describe('publish/subscribe', function () { sub.stream.end(); }); + sub.select(3); sub.subscribe(channels); sub.on('ready', function (err, results) { From 25aa8f67105df65d38fdc1ac2c7d739233a07f97 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 27 May 2016 20:34:44 +0200 Subject: [PATCH 10/17] Fix monitoring mode not always activating soon enough --- lib/individualCommands.js | 4 ++-- test/node_redis.spec.js | 34 ++++++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/lib/individualCommands.js b/lib/individualCommands.js index b7306c84d4..88fca12688 100644 --- a/lib/individualCommands.js +++ b/lib/individualCommands.js @@ -55,8 +55,8 @@ RedisClient.prototype.monitor = RedisClient.prototype.MONITOR = function monitor // Use a individual command, as this is a special case that does not has to be checked for any other command var self = this; var call_on_write = function () { - // Activating monitor mode has to happen before Redis returned the callback, - // as the client could receive monitoring commands before the callback returned through a race condition + // Activating monitor mode has to happen before Redis returned the callback. The monitor result is returned first. + // Therefore we expect the command to be properly processed. If this is not the case, it's not an issue either. self.monitoring = true; }; return this.internal_send_command(new Command('monitor', [], callback, call_on_write)); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 1c897116a6..d9d0667234 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -336,6 +336,26 @@ describe('The node_redis client', function () { }); }); + it('should retry all commands even if the offline queue is disabled', function (done) { + var bclient = redis.createClient({ + parser: parser, + enableOfflineQueue: false, + retryUnfulfilledCommands: true + }); + bclient.once('ready', function () { + bclient.blpop('blocking list 2', 5, function (err, value) { + assert.strictEqual(value[0], 'blocking list 2'); + assert.strictEqual(value[1], 'initial value'); + bclient.end(true); + done(err); + }); + setTimeout(function () { + bclient.stream.destroy(); + client.rpush('blocking list 2', 'initial value', helper.isNumber(1)); + }, 100); + }); + }); + }); describe('.end', function () { @@ -650,6 +670,7 @@ describe('The node_redis client', function () { }); monitorClient.on('monitor', function (time, args, rawOutput) { + assert.strictEqual(monitorClient.monitoring, true); responses.push(args); assert(utils.monitor_regex.test(rawOutput), rawOutput); if (responses.length === 6) { @@ -670,6 +691,7 @@ describe('The node_redis client', function () { }); monitorClient.MONITOR(function (err, res) { + assert.strictEqual(monitorClient.monitoring, true); assert.strictEqual(res.inspect(), new Buffer('OK').inspect()); client.mget('hello', new Buffer('world')); }); @@ -688,6 +710,7 @@ describe('The node_redis client', function () { client.MONITOR(helper.isString('OK')); client.mget('hello', 'world'); client.on('monitor', function (time, args, rawOutput) { + assert.strictEqual(client.monitoring, true); assert(utils.monitor_regex.test(rawOutput), rawOutput); assert.deepEqual(args, ['mget', 'hello', 'world']); if (i++ === 2) { @@ -708,6 +731,7 @@ describe('The node_redis client', function () { assert.deepEqual(res, ['OK', [null, null]]); }); client.on('monitor', function (time, args, rawOutput) { + assert.strictEqual(client.monitoring, true); assert(utils.monitor_regex.test(rawOutput), rawOutput); assert.deepEqual(args, ['mget', 'hello', 'world']); if (i++ === 2) { @@ -719,20 +743,22 @@ describe('The node_redis client', function () { }); }); - it('monitor does not activate if the command could not be processed properly', function (done) { + it('monitor activates even if the command could not be processed properly after a reconnect', function (done) { client.MONITOR(function (err, res) { assert.strictEqual(err.code, 'UNCERTAIN_STATE'); }); client.on('error', function (err) {}); // Ignore error here client.stream.destroy(); + var end = helper.callFuncAfter(done, 2); client.on('monitor', function (time, args, rawOutput) { - done(new Error('failed')); // Should not be activated + assert.strictEqual(client.monitoring, true); + end(); }); client.on('reconnecting', function () { client.get('foo', function (err, res) { assert(!err); - assert.strictEqual(client.monitoring, false); - setTimeout(done, 10); // The monitor command might be returned a tiny bit later + assert.strictEqual(client.monitoring, true); + end(); }); }); }); From ce44213d656da792644c0d7a093327d6ecce544a Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 28 May 2016 14:57:31 +0200 Subject: [PATCH 11/17] A function name is only configurable from v8 >= v.4.3 --- changelog.md | 2 +- lib/commands.js | 28 ++++++++++++++++++++++------ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/changelog.md b/changelog.md index 683d9d94c6..a416a04aa9 100644 --- a/changelog.md +++ b/changelog.md @@ -10,7 +10,7 @@ Features - Updated [redis-parser](https://github.com/NodeRedis/redis-parser) dependency ([changelog](https://github.com/NodeRedis/redis-parser/releases/tag/v.2.0.0)) - The JS parser is from now on the new default as it is a lot faster than the hiredis parser - This is no BC as there is no changed behavior for the user at all but just a performance improvement. Explicitly requireing the Hiredis parser is still possible. -- Added name property to all Redis functions +- Added name property to all Redis functions (Node.js >= 4.0) Bugfixes diff --git a/lib/commands.js b/lib/commands.js index 805c3b9a50..0082330471 100644 --- a/lib/commands.js +++ b/lib/commands.js @@ -4,6 +4,18 @@ var commands = require('redis-commands'); var Multi = require('./multi'); var RedisClient = require('../').RedisClient; var Command = require('./command'); +// Feature detect if a function may change it's name +var changeFunctionName = (function () { + var fn = function abc () {}; + try { + Object.defineProperty(fn, 'name', { + value: 'foobar' + }); + return true; + } catch (e) { + return false; + } +}()); // TODO: Rewrite this including the invidual commands into a Commands class // that provided a functionality to add new commands to the client @@ -45,9 +57,11 @@ commands.list.forEach(function (command) { } return this.internal_send_command(new Command(command, arr, callback)); }; - Object.defineProperty(RedisClient.prototype[command], 'name', { - value: command - }); + if (changeFunctionName) { + Object.defineProperty(RedisClient.prototype[command], 'name', { + value: command + }); + } } // Do not override existing functions @@ -86,8 +100,10 @@ commands.list.forEach(function (command) { this.queue.push(new Command(command, arr, callback)); return this; }; - Object.defineProperty(Multi.prototype[command], 'name', { - value: command - }); + if (changeFunctionName) { + Object.defineProperty(Multi.prototype[command], 'name', { + value: command + }); + } } }); From 2c6e1e0cc08cf7a1e24639bfe7f065c37a24eafe Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 29 May 2016 01:24:26 +0200 Subject: [PATCH 12/17] Improve error stacks in development and debug mode --- README.md | 29 +++++++++++++++++++++++++++++ changelog.md | 1 + index.js | 6 ++++++ lib/command.js | 5 +++++ 4 files changed, 41 insertions(+) diff --git a/README.md b/README.md index f1680c6541..0b5a554a92 100644 --- a/README.md +++ b/README.md @@ -757,6 +757,35 @@ clients: 1, NodeJS: 6.2.0, Redis: 3.2.0, parser: javascript, connected by: tcp To get debug output run your `node_redis` application with `NODE_DEBUG=redis`. +This is also going to result in good stack traces opposed to useless ones otherwise for any async operation. +If you only want to have good stack traces but not the debug output run your application in development mode instead (`NODE_ENV=development`). + +Good stack traces are only activated in development and debug mode as this results in a significant performance penalty. + +___Comparison___: +Useless stack trace: +``` +ReplyError: ERR wrong number of arguments for 'set' command + at parseError (/home/ruben/repos/redis/node_modules/redis-parser/lib/parser.js:158:12) + at parseType (/home/ruben/repos/redis/node_modules/redis-parser/lib/parser.js:219:14) +``` +Good stack trace: +``` +ReplyError: ERR wrong number of arguments for 'set' command + at new Command (/home/ruben/repos/redis/lib/command.js:9:902) + at RedisClient.set (/home/ruben/repos/redis/lib/commands.js:9:3238) + at Context. (/home/ruben/repos/redis/test/good_stacks.spec.js:20:20) + at callFnAsync (/home/ruben/repos/redis/node_modules/mocha/lib/runnable.js:349:8) + at Test.Runnable.run (/home/ruben/repos/redis/node_modules/mocha/lib/runnable.js:301:7) + at Runner.runTest (/home/ruben/repos/redis/node_modules/mocha/lib/runner.js:422:10) + at /home/ruben/repos/redis/node_modules/mocha/lib/runner.js:528:12 + at next (/home/ruben/repos/redis/node_modules/mocha/lib/runner.js:342:14) + at /home/ruben/repos/redis/node_modules/mocha/lib/runner.js:352:7 + at next (/home/ruben/repos/redis/node_modules/mocha/lib/runner.js:284:14) + at Immediate._onImmediate (/home/ruben/repos/redis/node_modules/mocha/lib/runner.js:320:5) + at processImmediate [as _immediateCallback] (timers.js:383:17) +``` + ## How to Contribute - Open a pull request or an issue about what you want to implement / change. We're glad for any help! - Please be aware that we'll only accept fully tested code. diff --git a/changelog.md b/changelog.md index a416a04aa9..a8fdd30fda 100644 --- a/changelog.md +++ b/changelog.md @@ -11,6 +11,7 @@ Features - The JS parser is from now on the new default as it is a lot faster than the hiredis parser - This is no BC as there is no changed behavior for the user at all but just a performance improvement. Explicitly requireing the Hiredis parser is still possible. - Added name property to all Redis functions (Node.js >= 4.0) +- Improved stack traces in development and debug mode Bugfixes diff --git a/index.js b/index.js index 84c9b64427..cd4edb1a5d 100644 --- a/index.js +++ b/index.js @@ -348,6 +348,9 @@ RedisClient.prototype.flush_and_error = function (error_attributes, options) { // 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 errorClasses.AbortError(error_attributes); + if (command_obj.error) { + err.stack = err.stack + command_obj.error.stack.replace(/^Error.*?\n/, '\n'); + } err.command = command_obj.command.toUpperCase(); if (command_obj.args && command_obj.args.length) { err.args = command_obj.args; @@ -675,6 +678,9 @@ RedisClient.prototype.connection_gone = function (why, error) { RedisClient.prototype.return_error = function (err) { var command_obj = this.command_queue.shift(); + if (command_obj.error) { + err.stack = command_obj.error.stack.replace(/^Error.*?\n/, 'ReplyError: ' + err.message + '\n'); + } err.command = command_obj.command.toUpperCase(); if (command_obj.args && command_obj.args.length) { err.args = command_obj.args; diff --git a/lib/command.js b/lib/command.js index 6414d4d6fc..1c14b2d41f 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1,5 +1,7 @@ 'use strict'; +var debug_mode = require('../').debug_mode; +var betterStackTraces = process.env.NODE_ENV && process.env.NODE_ENV.toUpperCase() === 'DEVELOPMENT' || debug_mode; function Command (command, args, callback, call_on_write) { this.command = command; @@ -7,6 +9,9 @@ function Command (command, args, callback, call_on_write) { this.buffer_args = false; this.callback = callback; this.call_on_write = call_on_write; + if (betterStackTraces) { + this.error = new Error(); + } } module.exports = Command; From b3e89fee312de6df2aef1dfdfe63dec259457285 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 29 May 2016 01:39:51 +0200 Subject: [PATCH 13/17] Support Node.js 6 --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index c87df9f781..11a502e3ee 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,5 +12,5 @@ node_js: - "0.10" - "0.12" - "4" - - "5" + - "6" after_success: npm run coveralls From c1f7755142ad95e0f6b18a28f714709b66c6e657 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 31 May 2016 15:11:18 +0200 Subject: [PATCH 14/17] Keep monitoring mode if once activated and use internal function for select and monitor while connecting --- index.js | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/index.js b/index.js index cd4edb1a5d..a196394ae2 100644 --- a/index.js +++ b/index.js @@ -4,6 +4,7 @@ var net = require('net'); var tls = require('tls'); var util = require('util'); var utils = require('./lib/utils'); +var Command = require('./lib/command'); var Queue = require('double-ended-queue'); var errorClasses = require('./lib/customErrors'); var EventEmitter = require('events'); @@ -154,7 +155,6 @@ function RedisClient (options, stream) { this.times_connected = 0; this.buffers = options.return_buffers || options.detect_buffers; this.options = options; - this.old_state = {}; this.reply = 'ON'; // Returning replies is the default // Init parser this.reply_parser = create_parser(this); @@ -445,10 +445,10 @@ RedisClient.prototype.on_ready = function () { // Restore modal commands from previous connection. The order of the commands is important if (this.selected_db !== undefined) { - this.select(this.selected_db); + this.internal_send_command(new Command('select', [this.selected_db])); } - if (this.old_state.monitoring) { // Monitor has to be fired before pub sub commands - this.monitor(); + if (this.monitoring) { // Monitor has to be fired before pub sub commands + this.internal_send_command(new Command('monitor', [])); } var callback_count = Object.keys(this.subscription_set).length; if (!this.options.disable_resubscribing && callback_count) { @@ -571,12 +571,6 @@ RedisClient.prototype.connection_gone = function (why, error) { this.cork = noop; this.uncork = noop; this.pipeline = false; - - var state = { - monitoring: this.monitoring - }; - this.old_state = state; - this.monitoring = false; this.pub_sub_mode = 0; // since we are collapsing end and close, users don't expect to be called twice @@ -874,16 +868,16 @@ RedisClient.prototype.internal_send_command = function (command_obj) { var big_data = false; var args_copy = new Array(len); - if (process.domain && command_obj.callback) { - command_obj.callback = process.domain.bind(command_obj.callback); - } - if (this.ready === false || this.stream.writable === false) { // Handle offline commands right away handle_offline_command(this, command_obj); return false; // Indicate buffering } + if (process.domain && command_obj.callback) { + command_obj.callback = process.domain.bind(command_obj.callback); + } + for (i = 0; i < len; i += 1) { if (typeof args[i] === 'string') { // 30000 seemed to be a good value to switch to buffers after testing and checking the pros and cons From a0c7431787618ee8d734c4571729e2f219141e3f Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 31 May 2016 15:11:57 +0200 Subject: [PATCH 15/17] Inherit the name property in the error classes --- lib/customErrors.js | 21 +++++++++++---------- test/custom_errors.spec.js | 8 ++++---- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/lib/customErrors.js b/lib/customErrors.js index 0b192f78df..bdf2feda0c 100644 --- a/lib/customErrors.js +++ b/lib/customErrors.js @@ -4,11 +4,6 @@ var util = require('util'); function AbortError (obj) { Error.captureStackTrace(this, this.constructor); - Object.defineProperty(this, 'name', { - value: 'AbortError', - configurable: true, - writable: true - }); Object.defineProperty(this, 'message', { value: obj.message || '', configurable: true, @@ -21,11 +16,6 @@ function AbortError (obj) { function AggregateError (obj) { Error.captureStackTrace(this, this.constructor); - Object.defineProperty(this, 'name', { - value: 'AggregateError', - configurable: true, - writable: true - }); Object.defineProperty(this, 'message', { value: obj.message || '', configurable: true, @@ -39,6 +29,17 @@ function AggregateError (obj) { util.inherits(AbortError, Error); util.inherits(AggregateError, AbortError); +Object.defineProperty(AbortError.prototype, 'name', { + value: 'AbortError', + // configurable: true, + writable: true +}); +Object.defineProperty(AggregateError.prototype, 'name', { + value: 'AggregateError', + // configurable: true, + writable: true +}); + module.exports = { AbortError: AbortError, AggregateError: AggregateError diff --git a/test/custom_errors.spec.js b/test/custom_errors.spec.js index d7c6ebb602..90a5aaeb66 100644 --- a/test/custom_errors.spec.js +++ b/test/custom_errors.spec.js @@ -24,11 +24,11 @@ describe('errors', function () { assert.strictEqual(e.message, 'hello world'); assert.strictEqual(e.name, 'weird'); assert.strictEqual(e.property, true); - assert.strictEqual(Object.keys(e).length, 1); + assert.strictEqual(Object.keys(e).length, 2); assert(e instanceof Error); assert(e instanceof errors.AbortError); assert(delete e.name); - assert.strictEqual(e.name, 'Error'); + assert.strictEqual(e.name, 'AbortError'); }); it('should change name and message', function () { @@ -65,12 +65,12 @@ describe('errors', function () { assert.strictEqual(e.message, 'hello world'); assert.strictEqual(e.name, 'weird'); assert.strictEqual(e.property, true); - assert.strictEqual(Object.keys(e).length, 1); + assert.strictEqual(Object.keys(e).length, 2); assert(e instanceof Error); assert(e instanceof errors.AggregateError); assert(e instanceof errors.AbortError); assert(delete e.name); - assert.strictEqual(e.name, 'Error'); + assert.strictEqual(e.name, 'AggregateError'); }); it('should change name and message', function () { From 579584d62965987c6420086e8b2d7258907e7cff Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 31 May 2016 15:57:04 +0200 Subject: [PATCH 16/17] Test Node.js 6 on appyevor --- appveyor.yml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/appveyor.yml b/appveyor.yml index 1d5c6cdbfd..9e79508e4c 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -6,7 +6,7 @@ environment: - nodejs_version: "0.10" - nodejs_version: "0.12" - nodejs_version: "4" - - nodejs_version: "5" + - nodejs_version: "6" pull_requests: do_not_increment_build_number: true @@ -21,11 +21,10 @@ install: - redis-64\tools\redis-server.exe --service-install - redis-64\tools\redis-server.exe --service-start - '@ECHO Redis Started' - # Get the latest stable version of Node 0.STABLE.latest + # Get the required Node version - ps: Install-Product node $env:nodejs_version - # Typical npm stuff. Use msvs 2013 for the hiredis parser + # Typical npm stuff - npm install - - npm install hiredis --msvs_version=2013 # Post-install test scripts. test_script: From a41cfa9aaea788d6af878bbc44faf848cd40c3dd Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 1 Jun 2016 14:04:25 +0200 Subject: [PATCH 17/17] Add good stack traces tests and fix stack traces in debug mode --- lib/command.js | 3 +- test/good_traces.spec.js | 59 ++++++++++++++++++++++++++++++++++++++++ test/lib/good-traces.js | 20 ++++++++++++++ test/node_redis.spec.js | 2 +- 4 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 test/good_traces.spec.js create mode 100644 test/lib/good-traces.js diff --git a/lib/command.js b/lib/command.js index 1c14b2d41f..717115c82e 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1,7 +1,6 @@ 'use strict'; -var debug_mode = require('../').debug_mode; -var betterStackTraces = process.env.NODE_ENV && process.env.NODE_ENV.toUpperCase() === 'DEVELOPMENT' || debug_mode; +var betterStackTraces = /development/i.test(process.env.NODE_ENV) || /\bredis\b/i.test(process.env.NODE_DEBUG); function Command (command, args, callback, call_on_write) { this.command = command; diff --git a/test/good_traces.spec.js b/test/good_traces.spec.js new file mode 100644 index 0000000000..d8759b0f9a --- /dev/null +++ b/test/good_traces.spec.js @@ -0,0 +1,59 @@ +'use strict'; + +var assert = require('assert'); +var config = require('./lib/config'); +var fork = require('child_process').fork; +var redis = config.redis; + +describe('stack traces', function () { + + it('should return good traces with NODE_ENV=development set', function (done) { + var external = fork('./test/lib/good-traces.js', { + env: { + NODE_ENV: 'development' + } + }); + + var id = setTimeout(function () { + external.kill(); + done(new Error('Timeout')); + }, 6000); + + external.on('close', function (code) { + clearTimeout(id); + assert.strictEqual(code, 0); + done(); + }); + }); + + it('should return good traces with NODE_DEBUG=redis env set', function (done) { + var external = fork('./test/lib/good-traces.js', { + env: { + NODE_DEBUG: 'redis' + }, + silent: true + }); + + var id = setTimeout(function () { + external.kill(); + done(new Error('Timeout')); + }, 6000); + + external.on('close', function (code) { + clearTimeout(id); + assert.strictEqual(code, 0); + done(); + }); + }); + + // This is always going to return good stack traces + it('should always return good stack traces for rejected offline commands', function (done) { + var client = redis.createClient({ + enable_offline_queue: false + }); + client.set('foo', function (err, res) { + assert(/good_traces.spec.js/.test(err.stack)); + client.quit(done); + }); + }); +}); diff --git a/test/lib/good-traces.js b/test/lib/good-traces.js new file mode 100644 index 0000000000..c583da2ae2 --- /dev/null +++ b/test/lib/good-traces.js @@ -0,0 +1,20 @@ +// Spawned by the good_stacks.spec.js tests +'use strict'; + +var assert = require('assert'); +var redis = require('../../index'); +var client = redis.createClient(); + +// Both error cases would normally return bad stack traces +client.set('foo', function (err, res) { + assert(/good-traces.js:9:8/.test(err.stack)); + client.set('foo', 'bar', function (err, res) { + assert(/good-traces.js:11:12/.test(err.stack)); + client.quit(function () { + process.exit(0); + }); + }); + process.nextTick(function () { + client.stream.destroy(); + }); +}); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index d9d0667234..6c3376c7f3 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -851,7 +851,7 @@ describe('The node_redis client', function () { var id = setTimeout(function () { external.kill(); - done(Error('unref subprocess timed out')); + done(new Error('unref subprocess timed out')); }, 8000); external.on('close', function (code) {