From 4f0443cdd4fb3f203d1c654a7eb893f663522a62 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 7 Sep 2015 14:54:08 +0200 Subject: [PATCH] Emit errors instead of throwing them Thrown errors might kill the users app. By emitting the errors the user is able to catch all errors in one place without the app going down --- index.js | 31 ++++++++++++++++++++++--------- test/commands/eval.spec.js | 2 +- test/commands/mset.spec.js | 11 ++++------- test/commands/select.spec.js | 12 +++++------- test/commands/set.spec.js | 20 ++++++-------------- test/node_redis.spec.js | 12 +++++------- 6 files changed, 43 insertions(+), 45 deletions(-) diff --git a/index.js b/index.js index 6e8be469f4..d56b66e7d2 100644 --- a/index.js +++ b/index.js @@ -280,6 +280,8 @@ RedisClient.prototype.init_parser = function () { return true; } })) { + // Do not emit this error + // This should take down the app if anyone made such a huge mistake or should somehow be handled in user code throw new Error("Couldn't find named parser " + self.options.parser + " on this system"); } } else { @@ -629,10 +631,12 @@ RedisClient.prototype.return_reply = function (reply) { } this.emit(type, reply1String, reply[2]); // channel, count } else { - throw new Error("subscriptions are active but got unknown reply type " + type); + this.emit("error", new Error("subscriptions are active but got unknown reply type " + type)); + return; } - } else if (! this.closing) { - throw new Error("subscriptions are active but got an invalid reply: " + reply); + } else if (!this.closing) { + this.emit("error", new Error("subscriptions are active but got an invalid reply: " + reply)); + return; } } else if (this.monitoring) { len = reply.indexOf(" "); @@ -643,7 +647,7 @@ RedisClient.prototype.return_reply = function (reply) { }); this.emit("monitor", timestamp, args); } else { - throw new Error("node_redis command queue state error. If you can reproduce this, please report it."); + this.emit("error", new Error("node_redis command queue state error. If you can reproduce this, please report it.")); } }; @@ -702,9 +706,16 @@ RedisClient.prototype.send_command = function (command, args, callback) { // if the value is undefined or null and command is set or setx, need not to send message to redis if (command === 'set' || command === 'setex') { - if(args[args.length - 1] === undefined || args[args.length - 1] === null) { + if (args.length === 0) { + return; + } + if (args[args.length - 1] === undefined || args[args.length - 1] === null) { var err = new Error('send_command: ' + command + ' value must not be undefined or null'); - return callback && callback(err); + if (callback) { + return callback && callback(err); + } + this.emit("error", err); + return; } } @@ -731,7 +742,8 @@ RedisClient.prototype.send_command = function (command, args, callback) { if (command_obj.callback) { command_obj.callback(not_writeable_error); } else { - throw not_writeable_error; + this.emit("error", not_writeable_error); + return; } } @@ -745,7 +757,8 @@ RedisClient.prototype.send_command = function (command, args, callback) { } else if (command === "quit") { this.closing = true; } else if (this.pub_sub_mode === true) { - throw new Error("Connection in subscriber mode, only subscriber commands may be used"); + this.emit("error", new Error("Connection in subscriber mode, only subscriber commands may be used")); + return; } this.command_queue.push(command_obj); this.commands_sent += 1; @@ -1032,7 +1045,7 @@ Multi.prototype.exec = function (callback) { callback(errors); return; } else { - throw new Error(err); + self._client.emit('error', err); } } diff --git a/test/commands/eval.spec.js b/test/commands/eval.spec.js index b4a1cd0a49..df075f3d29 100644 --- a/test/commands/eval.spec.js +++ b/test/commands/eval.spec.js @@ -114,7 +114,7 @@ describe("The 'eval' method", function () { client.evalsha(sha, 0, helper.isString('eval get sha test', done)); }); - it('throws an error if SHA does not exist', function (done) { + it('returns an error if SHA does not exist', function (done) { helper.serverVersionAtLeast.call(this, client, [2, 5, 0]); client.evalsha('ffffffffffffffffffffffffffffffffffffffff', 0, helper.isError(done)); }); diff --git a/test/commands/mset.spec.js b/test/commands/mset.spec.js index 1718462b67..a79d6365b1 100644 --- a/test/commands/mset.spec.js +++ b/test/commands/mset.spec.js @@ -92,13 +92,10 @@ describe("The 'mset' method", function () { describe("with undefined 'key' and missing 'value' parameter", function () { // this behavior is different from the 'set' behavior. - it("throws an error", function (done) { - var mochaListener = helper.removeMochaListener(); - - process.once('uncaughtException', function (err) { - process.on('uncaughtException', mochaListener); - helper.isError()(err, null); - return done(); + it("emits an error", function (done) { + client.on('error', function (err) { + assert.equal(err.message, "ERR wrong number of arguments for 'mset' command"); + done(); }); client.mset(); diff --git a/test/commands/select.spec.js b/test/commands/select.spec.js index 924fc9f9c5..cb3e101c2f 100644 --- a/test/commands/select.spec.js +++ b/test/commands/select.spec.js @@ -24,7 +24,7 @@ describe("The 'select' method", function () { }); }); - it("throws an error if redis is not connected", function (done) { + it("returns an error if redis is not connected", function (done) { client.select(1, function (err, res) { assert(err.message.match(/Redis connection gone/)); done(); @@ -67,7 +67,7 @@ describe("The 'select' method", function () { }); describe("with an invalid db index", function () { - it("emits an error", function (done) { + it("returns an error", function (done) { assert.strictEqual(client.selected_db, null, "default db should be null"); client.select(9999, function (err) { assert.equal(err.message, 'ERR invalid DB index'); @@ -90,14 +90,12 @@ describe("The 'select' method", function () { }); describe("with an invalid db index", function () { - it("throws an error when callback not provided", function (done) { - var mochaListener = helper.removeMochaListener(); + it("emits an error when callback not provided", function (done) { assert.strictEqual(client.selected_db, null, "default db should be null"); - process.once('uncaughtException', function (err) { - process.on('uncaughtException', mochaListener); + client.on('error', function (err) { assert.equal(err.message, 'ERR invalid DB index'); - return done(); + done(); }); client.select(9999); diff --git a/test/commands/set.spec.js b/test/commands/set.spec.js index 927c64c6dd..15e0346ed7 100644 --- a/test/commands/set.spec.js +++ b/test/commands/set.spec.js @@ -96,8 +96,7 @@ describe("The 'set' method", function () { this.timeout(200); client.once("error", function (err) { - helper.isError()(err, null); - return done(err); + done(err); }); client.set(); @@ -107,20 +106,13 @@ describe("The 'set' method", function () { }, 100); }); - it("does not throw an error", function (done) { - this.timeout(200); - var mochaListener = helper.removeMochaListener(); - - process.once('uncaughtException', function (err) { - process.on('uncaughtException', mochaListener); - return done(err); + it("does emit an error", function (done) { + client.on('error', function (err) { + assert.equal(err.message, "ERR wrong number of arguments for 'set' command"); + done(); }); - client.set(); - - setTimeout(function () { - done(); - }, 100); + client.set('foo'); }); }); }); diff --git a/test/node_redis.spec.js b/test/node_redis.spec.js index b43483bec3..e9eb0c3359 100644 --- a/test/node_redis.spec.js +++ b/test/node_redis.spec.js @@ -647,7 +647,7 @@ describe("The node_redis client", function () { describe('enable_offline_queue', function () { describe('true', function () { - it("does not throw an error and enqueues operation", function (done) { + it("does not return an error and enqueues operation", function (done) { var client = redis.createClient(9999, null, { max_attempts: 1, parser: parser @@ -674,20 +674,18 @@ describe("The node_redis client", function () { }); describe('false', function () { - it("does not throw an error and enqueues operation", function (done) { + it("does not emit an error and enqueues operation", function (done) { var client = redis.createClient(9999, null, { parser: parser, max_attempts: 1, enable_offline_queue: false }); - client.on('error', function() { - // ignore, b/c expecting a "can't connect" error + client.on('error', function(err) { + assert(/send_command: stream not writeable|ECONNREFUSED/.test(err.message)); }); - assert.throws(function () { - client.set('foo', 'bar'); - }); + client.set('foo', 'bar'); assert.doesNotThrow(function () { client.set('foo', 'bar', function (err) {