diff --git a/index.js b/index.js index ab7d11e335..dcdbaf96bb 100644 --- a/index.js +++ b/index.js @@ -3,8 +3,8 @@ var net = require("net"), URL = require("url"), util = require("util"), + utils = require("./lib/utils"), Queue = require("./lib/queue"), - to_array = require("./lib/to_array"), events = require("events"), parsers = [], // This static list of commands is updated from time to time. @@ -23,14 +23,13 @@ exports.debug_mode = /\bredis\b/i.test(process.env.NODE_DEBUG); // hiredis might not be installed try { - require("./lib/parser/hiredis"); - parsers.push(require("./lib/parser/hiredis")); + parsers.push(require("./lib/parsers/hiredis")); } catch (err) { /* istanbul ignore next: won't be reached with tests */ debug("Hiredis parser not installed."); } -parsers.push(require("./lib/parser/javascript")); +parsers.push(require("./lib/parsers/javascript")); function RedisClient(stream, options) { options = options || {}; @@ -135,6 +134,7 @@ RedisClient.prototype.flush_and_error = function (error) { while (command_obj = this.offline_queue.shift()) { if (typeof command_obj.callback === "function") { + error.command = command_obj.command.toUpperCase(); command_obj.callback(error); } } @@ -142,6 +142,7 @@ RedisClient.prototype.flush_and_error = function (error) { while (command_obj = this.command_queue.shift()) { if (typeof command_obj.callback === "function") { + error.command = command_obj.command.toUpperCase(); command_obj.callback(error); } } @@ -530,41 +531,6 @@ RedisClient.prototype.return_error = function (err) { } }; -// hgetall converts its replies to an Object. If the reply is empty, null is returned. -function reply_to_object(reply) { - var obj = {}, j, jl, key, val; - - if (reply.length === 0 || !Array.isArray(reply)) { - return null; - } - - for (j = 0, jl = reply.length; j < jl; j += 2) { - key = reply[j].toString('binary'); - val = reply[j + 1]; - obj[key] = val; - } - - return obj; -} - -function reply_to_strings(reply) { - var i; - - if (Buffer.isBuffer(reply)) { - return reply.toString(); - } - - if (Array.isArray(reply)) { - for (i = 0; i < reply.length; i++) { - // Recusivly call the function as slowlog returns deep nested replies - reply[i] = reply_to_strings(reply[i]); - } - return reply; - } - - return reply; -} - RedisClient.prototype.return_reply = function (reply) { var command_obj, len, type, timestamp, argindex, args, queue_len; @@ -598,12 +564,12 @@ RedisClient.prototype.return_reply = function (reply) { if (this.options.detect_buffers && command_obj.buffer_args === false) { // If detect_buffers option was specified, then the reply from the parser will be Buffers. // If this command did not use Buffer arguments, then convert the reply to Strings here. - reply = reply_to_strings(reply); + reply = utils.reply_to_strings(reply); } // TODO - confusing and error-prone that hgetall is special cased in two places if (reply && 'hgetall' === command_obj.command) { - reply = reply_to_object(reply); + reply = utils.reply_to_object(reply); } } @@ -614,7 +580,7 @@ RedisClient.prototype.return_reply = function (reply) { } else if (this.pub_sub_mode || command_obj && command_obj.sub_command) { if (Array.isArray(reply)) { if (!this.options.return_buffers && (!command_obj || this.options.detect_buffers && command_obj.buffer_args === false)) { - reply = reply_to_strings(reply); + reply = utils.reply_to_strings(reply); } type = reply[0].toString(); @@ -850,7 +816,7 @@ RedisClient.prototype.end = function (flush) { // Flush queue if wanted if (flush) { - this.flush_and_error("Redis connection ended."); + this.flush_and_error(new Error("The command can't be processed. The connection has already been closed.")); } this.connected = false; @@ -894,7 +860,7 @@ commands.forEach(function (fullCommand) { arg = [key].concat(arg); return this.send_command(command, arg, callback); } - return this.send_command(command, to_array(arguments)); + return this.send_command(command, utils.to_array(arguments)); }; RedisClient.prototype[command.toUpperCase()] = RedisClient.prototype[command]; @@ -910,7 +876,7 @@ commands.forEach(function (fullCommand) { } this.queue.push([command, key].concat(arg)); } else { - this.queue.push([command].concat(to_array(arguments))); + this.queue.push([command].concat(utils.to_array(arguments))); } return this; }; @@ -979,7 +945,7 @@ RedisClient.prototype.hmset = RedisClient.prototype.HMSET = function (key, args, } return this.send_command("hmset", tmp_args, callback); } - return this.send_command("hmset", to_array(arguments)); + return this.send_command("hmset", utils.to_array(arguments)); }; Multi.prototype.hmset = Multi.prototype.HMSET = function (key, args, callback) { @@ -1008,7 +974,7 @@ Multi.prototype.hmset = Multi.prototype.HMSET = function (key, args, callback) { tmp_args.push(callback); } } else { - tmp_args = to_array(arguments); + tmp_args = utils.to_array(arguments); tmp_args.unshift("hmset"); } this.queue.push(tmp_args); @@ -1087,11 +1053,11 @@ Multi.prototype.execute_callback = function (err, replies) { replies[i].command = args[0].toUpperCase(); } else if (replies[i]) { if (this._client.options.detect_buffers && this.wants_buffers[i + 1] === false) { - replies[i] = reply_to_strings(replies[i]); + replies[i] = utils.reply_to_strings(replies[i]); } if (args[0] === "hgetall") { // TODO - confusing and error-prone that hgetall is special cased in two places - replies[i] = reply_to_object(replies[i]); + replies[i] = utils.reply_to_object(replies[i]); } } diff --git a/lib/parser/hiredis.js b/lib/parsers/hiredis.js similarity index 80% rename from lib/parser/hiredis.js rename to lib/parsers/hiredis.js index 765304bec5..5be6bf2ac9 100644 --- a/lib/parser/hiredis.js +++ b/lib/parsers/hiredis.js @@ -18,7 +18,13 @@ HiredisReplyParser.prototype.execute = function (data) { var reply; this.reader.feed(data); while (true) { - reply = this.reader.get(); + try { + reply = this.reader.get(); + } catch (err) { + // Protocol errors land here + this.send_error(err); + break; + } if (reply === undefined) { break; diff --git a/lib/parser/javascript.js b/lib/parsers/javascript.js similarity index 96% rename from lib/parser/javascript.js rename to lib/parsers/javascript.js index 4ce4168c43..dbbe658b6b 100644 --- a/lib/parser/javascript.js +++ b/lib/parsers/javascript.js @@ -30,14 +30,13 @@ ReplyParser.prototype._parseResult = function (type) { end = this._packetEndOffset() - 1; start = this._offset; - // include the delimiter - this._offset = end + 2; - if (end > this._buffer.length) { - this._offset = start; throw new IncompleteReadBuffer("Wait for more data."); } + // include the delimiter + this._offset = end + 2; + if (type === 45) { return new Error(this._buffer.toString(this._encoding, start, end)); } else if (this.return_buffers) { @@ -49,14 +48,13 @@ ReplyParser.prototype._parseResult = function (type) { end = this._packetEndOffset() - 1; start = this._offset; - // include the delimiter - this._offset = end + 2; - if (end > this._buffer.length) { - this._offset = start; throw new IncompleteReadBuffer("Wait for more data."); } + // include the delimiter + this._offset = end + 2; + // return the coerced numeric value return +this._buffer.toString('ascii', start, end); } else if (type === 36) { // $ @@ -74,14 +72,13 @@ ReplyParser.prototype._parseResult = function (type) { end = this._offset + packetHeader.size; start = this._offset; - // set the offset to after the delimiter - this._offset = end + 2; - if (end > this._buffer.length) { - this._offset = offset; throw new IncompleteReadBuffer("Wait for more data."); } + // set the offset to after the delimiter + this._offset = end + 2; + if (this.return_buffers) { return this._buffer.slice(start, end); } @@ -157,6 +154,11 @@ ReplyParser.prototype.execute = function (buffer) { ret = this._parseResult(type); this.send_reply(ret); + } else if (type === 10 || type === 13) { + break; + } else { + var err = new Error('Protocol error, got "' + String.fromCharCode(type) + '" as reply type byte'); + this.send_error(err); } } catch (err) { // catch the error (not enough data), rewind, and wait diff --git a/lib/to_array.js b/lib/to_array.js deleted file mode 100644 index 87804f8c3b..0000000000 --- a/lib/to_array.js +++ /dev/null @@ -1,14 +0,0 @@ -'use strict'; - -function to_array(args) { - var len = args.length, - arr = new Array(len), i; - - for (i = 0; i < len; i += 1) { - arr[i] = args[i]; - } - - return arr; -} - -module.exports = to_array; diff --git a/lib/utils.js b/lib/utils.js new file mode 100644 index 0000000000..42ae211c3a --- /dev/null +++ b/lib/utils.js @@ -0,0 +1,53 @@ +'use strict'; + +// hgetall converts its replies to an Object. If the reply is empty, null is returned. +function replyToObject(reply) { + var obj = {}, j, jl, key, val; + + if (reply.length === 0 || !Array.isArray(reply)) { + return null; + } + + for (j = 0, jl = reply.length; j < jl; j += 2) { + key = reply[j].toString('binary'); + val = reply[j + 1]; + obj[key] = val; + } + + return obj; +} + +function replyToStrings(reply) { + var i; + + if (Buffer.isBuffer(reply)) { + return reply.toString(); + } + + if (Array.isArray(reply)) { + for (i = 0; i < reply.length; i++) { + // Recusivly call the function as slowlog returns deep nested replies + reply[i] = replyToStrings(reply[i]); + } + return reply; + } + + return reply; +} + +function toArray(args) { + var len = args.length, + arr = new Array(len), i; + + for (i = 0; i < len; i += 1) { + arr[i] = args[i]; + } + + return arr; +} + +module.exports = { + reply_to_strings: replyToStrings, + reply_to_object: replyToObject, + to_array: toArray +}; diff --git a/package.json b/package.json index eb8438cc6e..208bdbbbb2 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "devDependencies": { "coveralls": "^2.11.2", "jshint": "^2.8.0", - "metrics": ">=0.1.5", + "metrics": "^0.1.9", "mocha": "^2.3.2", "nyc": "^3.2.2", "optional-dev-dependency": "^1.1.0", diff --git a/test/commands/geoadd.spec.js.future b/test/commands/geoadd.spec.js.future new file mode 100644 index 0000000000..710b528f80 --- /dev/null +++ b/test/commands/geoadd.spec.js.future @@ -0,0 +1,34 @@ +'use strict'; + +var config = require("../lib/config"); +var helper = require("../helper"); +var redis = config.redis; + +describe("The 'getoadd' method", function () { + + helper.allTests(function(parser, ip, args) { + + describe("using " + parser + " and " + ip, function () { + var client; + + beforeEach(function (done) { + client = redis.createClient.apply(redis.createClient, args); + client.once("connect", function () { + client.flushdb(done); + }); + }); + + it('returns 1 if the key exists', function (done) { + client.geoadd("mycity:21:0:location", "13.361389","38.115556","COR", function(err, res) { + console.log(err, res); + // geoadd is still in the unstable branch. As soon as it reaches the stable one, activate this test + done(); + }); + }); + + afterEach(function () { + client.end(); + }); + }); + }); +}); diff --git a/test/commands/set.spec.js b/test/commands/set.spec.js index c295af1d3d..dbdd3103e9 100644 --- a/test/commands/set.spec.js +++ b/test/commands/set.spec.js @@ -88,6 +88,16 @@ describe("The 'set' method", function () { }); }, 100); }); + + it("sets the value correctly with the array syntax", function (done) { + client.set([key, value]); + setTimeout(function () { + client.get(key, function (err, res) { + helper.isString(value)(err, res); + done(); + }); + }, 100); + }); }); describe("with undefined 'key' and missing 'value' parameter", function () { diff --git a/test/commands/sync.spec.js b/test/commands/sync.spec.js new file mode 100644 index 0000000000..7e7702c8b5 --- /dev/null +++ b/test/commands/sync.spec.js @@ -0,0 +1,43 @@ +'use strict'; + +var assert = require('assert'); +var config = require("../lib/config"); +var helper = require("../helper"); +var redis = config.redis; + +describe("The 'sync' method", function () { + + helper.allTests(function(parser, ip, args) { + + describe("using " + parser + " and " + ip, function () { + var client; + + beforeEach(function (done) { + client = redis.createClient.apply(redis.createClient, args); + client.once("connect", function () { + client.flushdb(done); + }); + }); + + // This produces a parser error + // Protocol error, got "K" as reply type byte + // I'm uncertain if this is correct behavior or not + it('try to sync with the server and fail other commands', function (done) { + client.on('error', function(err) { + assert.equal(err.message, 'Protocol error, got "K" as reply type byte'); + assert.equal(err.command, 'SET'); + done(); + }); + client.sync(function(err, res) { + assert.equal(err, null); + assert(!!res); + }); + client.set('foo', 'bar'); + }); + + afterEach(function () { + client.end(); + }); + }); + }); +}); diff --git a/test/helper.js b/test/helper.js index 776c2ca3ee..a9f66d4a40 100644 --- a/test/helper.js +++ b/test/helper.js @@ -31,12 +31,10 @@ module.exports = { redisProcess: function () { return rp; }, - stopRedis: function (done) { + stopRedis: function(done) { rp.stop(done); }, - startRedis: function (conf, done) { - startRedis(conf, done); - }, + startRedis: startRedis, isNumber: function (expected, done) { return function (err, results) { assert.strictEqual(null, err, "expected " + expected + ", got error: " + err); diff --git a/test/lib/config.js b/test/lib/config.js index d42dc21c9a..c6f1b25dae 100644 --- a/test/lib/config.js +++ b/test/lib/config.js @@ -13,7 +13,8 @@ var config = { }, configureClient: function (parser, ip, opts) { var args = []; - opts = opts || {}; + // Do not manipulate the opts => copy them each time + opts = opts ? JSON.parse(JSON.stringify(opts)) : {}; if (ip.match(/\.sock/)) { args.push(ip); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index 2dabc68c8d..34ba79e3dd 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -156,8 +156,8 @@ describe("The node_redis client", function () { process.once('uncaughtException', function (err) { process.on('uncaughtException', mochaListener); - // assert(/ERR Protocol error/.test(err.message)); - // assert.equal(err.command, true); + assert(/ERR Protocol error/.test(err.message)); + assert.equal(err.command, true); assert.equal(err.code, 'ERR'); done(); }); @@ -177,25 +177,36 @@ describe("The node_redis client", function () { describe(".end", function () { it('used without flush', function(done) { - var err = null; - client.set('foo', 'bar'); - client.end(); - client.get('foo', function(err, res) { - err = new Error('failed'); - }); - setTimeout(function() { - done(err); - }, 200); + var end = helper.callFuncAfter(function() { + done(new Error('failed')); + }, 20); + var cb = function(err, res) { + assert.equal(err.message, "SET can't be processed. The connection has already been closed."); + end(); + }; + for (var i = 0; i < 20; i++) { + if (i === 10) { + client.end(); + } + client.set('foo', 'bar', cb); + } + setTimeout(done, 250); }); it('used with flush set to true', function(done) { - client.set('foo', 'bar'); - client.end(); - client.get('foo', function(err, res) { - assert.strictEqual(err.command, 'GET'); - assert.strictEqual(err.message, "GET can't be processed. The connection has already been closed."); + var end = helper.callFuncAfter(function() { done(); - }); + }, 20); + var cb = function(err, res) { + assert(/The connection has already been closed./.test(err.message)); + end(); + }; + for (var i = 0; i < 20; i++) { + if (i === 10) { + client.end(true); + } + client.set('foo', 'bar', cb); + } }); }); @@ -730,7 +741,7 @@ describe("The node_redis client", function () { describe('retry_max_delay', function () { var client; var args = config.configureClient(parser, ip, { - retry_max_delay: 1 + retry_max_delay: 1 // ms }); it("sets upper bound on how long client waits before reconnecting", function (done) { @@ -747,7 +758,7 @@ describe("The node_redis client", function () { } else { client.end(); var lasted = new Date().getTime() - time; - assert.ok(lasted < 1000); + assert.ok(lasted < 50); return done(); } }); diff --git a/test/parser/javascript.spec.js b/test/parser/javascript.spec.js index 19df991675..491a292ab1 100644 --- a/test/parser/javascript.spec.js +++ b/test/parser/javascript.spec.js @@ -1,7 +1,7 @@ 'use strict'; var assert = require('assert'); -var Parser = require("../../lib/parser/javascript").Parser; +var Parser = require("../../lib/parsers/javascript").Parser; var config = require("../lib/config"); var redis = config.redis;