Merge pull request #816 from fintura/callback

Remove try callback and emit errors if no callback is present. Fixes #456, #591, #522 and #755

index.js

@@ -152,13 +152,7 @@ RedisClient.prototype.flush_and_error = function (message) {
     while (this.offline_queue.length > 0) {
         command_obj = this.offline_queue.shift();
         if (typeof command_obj.callback === "function") {
-            try {
             command_obj.callback(error);
-            } catch (callback_err) {
-                process.nextTick(function () {
-                    throw callback_err;
-                });
-            }
         }
     }
     this.offline_queue = new Queue();
@@ -166,13 +160,7 @@ RedisClient.prototype.flush_and_error = function (message) {
     while (this.command_queue.length > 0) {
         command_obj = this.command_queue.shift();
         if (typeof command_obj.callback === "function") {
-            try {
             command_obj.callback(error);
-            } catch (callback_err) {
-                process.nextTick(function () {
-                    throw callback_err;
-                });
-            }
         }
     }
     this.command_queue = new Queue();
@@ -292,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 {
@@ -529,37 +519,12 @@ RedisClient.prototype.return_error = function (err) {
         this.emit("drain");
         this.should_buffer = false;
     }
-
+    if (command_obj.callback) {
-    try {
         command_obj.callback(err);
-    } catch (callback_err) {
-        // if a callback throws an exception, re-throw it on a new stack so the parser can keep going
-        process.nextTick(function () {
-            throw callback_err;
-        });
-    }
-};
-
-// if a callback throws an exception, re-throw it on a new stack so the parser can keep going.
-// if a domain is active, emit the error on the domain, which will serve the same function.
-// put this try/catch in its own function because V8 doesn't optimize this well yet.
-function try_callback(callback, reply) {
-    try {
-        callback(null, reply);
-    } catch (err) {
-        if (process.domain) {
-            var currDomain = process.domain;
-            currDomain.emit('error', err);
-            if (process.domain === currDomain) {
-                currDomain.exit();
-            }
     } else {
-            process.nextTick(function () {
+        this.emit('error', err);
-                throw err;
-            });
-        }
-    }
     }
+};
 
 // hgetall converts its replies to an Object.  If the reply is empty, null is returned.
 function reply_to_object(reply) {
@@ -638,7 +603,7 @@ RedisClient.prototype.return_reply = function (reply) {
                 reply = reply_to_object(reply);
             }
 
-            try_callback(command_obj.callback, reply);
+            command_obj.callback(null, reply);
         } else {
             debug("no callback for reply: " + (reply && reply.toString && reply.toString()));
         }
@@ -662,14 +627,16 @@ RedisClient.prototype.return_reply = function (reply) {
                 // reply[1] can be null
                 var reply1String = (reply[1] === null) ? null : reply[1].toString();
                 if (command_obj && typeof command_obj.callback === "function") {
-                    try_callback(command_obj.callback, reply1String);
+                    command_obj.callback(null, reply1String);
                 }
                 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);
+            this.emit("error", new Error("subscriptions are active but got an invalid reply: " + reply));
+            return;
         }
     } else if (this.monitoring) {
         len = reply.indexOf(" ");
@@ -680,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."));
     }
 };
 
@@ -739,10 +706,17 @@ 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.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');
+            if (callback) {
                 return callback && callback(err);
             }
+            this.emit("error", err);
+            return;
+        }
     }
 
     buffer_args = false;
@@ -768,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;
             }
         }
 
@@ -782,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;
@@ -1069,7 +1045,7 @@ Multi.prototype.exec = function (callback) {
                 callback(errors);
                 return;
             } else {
-                throw new Error(err);
+                self._client.emit('error', err);
             }
         }
 

test/commands/eval.spec.js

@@ -114,10 +114,18 @@ 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));
                 });
+
+                it('emits an error if SHA does not exist and no callback has been provided', function (done) {
+                    client.on('error', function (err) {
+                        assert.equal(err.message, 'NOSCRIPT No matching script. Please use EVAL.');
+                        done();
+                    });
+                    client.evalsha('ffffffffffffffffffffffffffffffffffffffff', 0);
+                });
             });
 
             it('allows a key to be incremented, and performs appropriate conversion from LUA type', function (done) {

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) {
+                        it("emits an error", function (done) {
-                            var mochaListener = helper.removeMochaListener();
+                            client.on('error', function (err) {
-
+                                assert.equal(err.message, "ERR wrong number of arguments for 'mset' command");
-                            process.once('uncaughtException', function (err) {
+                                done();
-                                process.on('uncaughtException', mochaListener);
-                                helper.isError()(err, null);
-                                return done();
                             });
 
                             client.mset();

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) {
+                        it("emits an error when callback not provided", function (done) {
-                            var mochaListener = helper.removeMochaListener();
                             assert.strictEqual(client.selected_db, null, "default db should be null");
 
-                            process.once('uncaughtException', function (err) {
+                            client.on('error', function (err) {
-                                process.on('uncaughtException', mochaListener);
                                 assert.equal(err.message, 'ERR invalid DB index');
-                                return done();
+                                done();
                             });
 
                             client.select(9999);

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);
+                                done(err);
-                                return done(err);
                             });
 
                             client.set();
@@ -107,20 +106,13 @@ describe("The 'set' method", function () {
                             }, 100);
                         });
 
-                        it("does not throw an error", function (done) {
+                        it("does emit an error", function (done) {
-                            this.timeout(200);
+                            client.on('error', function (err) {
-                            var mochaListener = helper.removeMochaListener();
+                                assert.equal(err.message, "ERR wrong number of arguments for 'set' command");
-
+                                done();
-                            process.once('uncaughtException', function (err) {
-                                process.on('uncaughtException', mochaListener);
-                                return done(err);
                             });
 
-                            client.set();
+                            client.set('foo');
-
-                            setTimeout(function () {
-                                done();
-                            }, 100);
                         });
                     });
                 });

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() {
+                        client.on('error', function(err) {
-                            // ignore, b/c expecting a "can't connect" error
+                            assert(/send_command: stream not writeable|ECONNREFUSED/.test(err.message));
                         });
 
-                        assert.throws(function () {
                         client.set('foo', 'bar');
-                        });
 
                         assert.doesNotThrow(function () {
                             client.set('foo', 'bar', function (err) {