Skip to content

Commit 4f79370

Browse files
BridgeARBridgeAR
committed
Merge pull request #816 from fintura/callback
Remove try callback and emit errors if no callback is present. Fixes #456, #591, #522 and #755
2 parents 1b261fc + 4f0443c commit 4f79370

File tree

6 files changed

+58
-89
lines changed

6 files changed

+58
-89
lines changed

‎index.js

Lines changed: 29 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -152,27 +152,15 @@ RedisClient.prototype.flush_and_error = function (message) {
152152
while (this.offline_queue.length > 0) {
153153
command_obj = this.offline_queue.shift();
154154
if (typeof command_obj.callback === "function") {
155-
try {
156-
command_obj.callback(error);
157-
} catch (callback_err) {
158-
process.nextTick(function () {
159-
throw callback_err;
160-
});
161-
}
155+
command_obj.callback(error);
162156
}
163157
}
164158
this.offline_queue = new Queue();
165159

166160
while (this.command_queue.length > 0) {
167161
command_obj = this.command_queue.shift();
168162
if (typeof command_obj.callback === "function") {
169-
try {
170-
command_obj.callback(error);
171-
} catch (callback_err) {
172-
process.nextTick(function () {
173-
throw callback_err;
174-
});
175-
}
163+
command_obj.callback(error);
176164
}
177165
}
178166
this.command_queue = new Queue();
@@ -292,6 +280,8 @@ RedisClient.prototype.init_parser = function () {
292280
return true;
293281
}
294282
})) {
283+
// Do not emit this error
284+
// This should take down the app if anyone made such a huge mistake or should somehow be handled in user code
295285
throw new Error("Couldn't find named parser " + self.options.parser + " on this system");
296286
}
297287
} else {
@@ -529,38 +519,13 @@ RedisClient.prototype.return_error = function (err) {
529519
this.emit("drain");
530520
this.should_buffer = false;
531521
}
532-
533-
try {
522+
if (command_obj.callback) {
534523
command_obj.callback(err);
535-
} catch (callback_err) {
536-
// if a callback throws an exception, re-throw it on a new stack so the parser can keep going
537-
process.nextTick(function () {
538-
throw callback_err;
539-
});
524+
} else {
525+
this.emit('error', err);
540526
}
541527
};
542528

543-
// if a callback throws an exception, re-throw it on a new stack so the parser can keep going.
544-
// if a domain is active, emit the error on the domain, which will serve the same function.
545-
// put this try/catch in its own function because V8 doesn't optimize this well yet.
546-
function try_callback(callback, reply) {
547-
try {
548-
callback(null, reply);
549-
} catch (err) {
550-
if (process.domain) {
551-
var currDomain = process.domain;
552-
currDomain.emit('error', err);
553-
if (process.domain === currDomain) {
554-
currDomain.exit();
555-
}
556-
} else {
557-
process.nextTick(function () {
558-
throw err;
559-
});
560-
}
561-
}
562-
}
563-
564529
// hgetall converts its replies to an Object. If the reply is empty, null is returned.
565530
function reply_to_object(reply) {
566531
var obj = {}, j, jl, key, val;
@@ -638,7 +603,7 @@ RedisClient.prototype.return_reply = function (reply) {
638603
reply = reply_to_object(reply);
639604
}
640605

641-
try_callback(command_obj.callback, reply);
606+
command_obj.callback(null, reply);
642607
} else {
643608
debug("no callback for reply: " + (reply && reply.toString && reply.toString()));
644609
}
@@ -662,14 +627,16 @@ RedisClient.prototype.return_reply = function (reply) {
662627
// reply[1] can be null
663628
var reply1String = (reply[1] === null) ? null : reply[1].toString();
664629
if (command_obj && typeof command_obj.callback === "function") {
665-
try_callback(command_obj.callback, reply1String);
630+
command_obj.callback(null, reply1String);
666631
}
667632
this.emit(type, reply1String, reply[2]); // channel, count
668633
} else {
669-
throw new Error("subscriptions are active but got unknown reply type " + type);
634+
this.emit("error", new Error("subscriptions are active but got unknown reply type " + type));
635+
return;
670636
}
671-
} else if (! this.closing) {
672-
throw new Error("subscriptions are active but got an invalid reply: " + reply);
637+
} else if (!this.closing) {
638+
this.emit("error", new Error("subscriptions are active but got an invalid reply: " + reply));
639+
return;
673640
}
674641
} else if (this.monitoring) {
675642
len = reply.indexOf(" ");
@@ -680,7 +647,7 @@ RedisClient.prototype.return_reply = function (reply) {
680647
});
681648
this.emit("monitor", timestamp, args);
682649
} else {
683-
throw new Error("node_redis command queue state error. If you can reproduce this, please report it.");
650+
this.emit("error", new Error("node_redis command queue state error. If you can reproduce this, please report it."));
684651
}
685652
};
686653

@@ -739,9 +706,16 @@ RedisClient.prototype.send_command = function (command, args, callback) {
739706

740707
// if the value is undefined or null and command is set or setx, need not to send message to redis
741708
if (command === 'set' || command === 'setex') {
742-
if(args[args.length - 1] === undefined || args[args.length - 1] === null) {
709+
if (args.length === 0) {
710+
return;
711+
}
712+
if (args[args.length - 1] === undefined || args[args.length - 1] === null) {
743713
var err = new Error('send_command: ' + command + ' value must not be undefined or null');
744-
return callback && callback(err);
714+
if (callback) {
715+
return callback && callback(err);
716+
}
717+
this.emit("error", err);
718+
return;
745719
}
746720
}
747721

@@ -768,7 +742,8 @@ RedisClient.prototype.send_command = function (command, args, callback) {
768742
if (command_obj.callback) {
769743
command_obj.callback(not_writeable_error);
770744
} else {
771-
throw not_writeable_error;
745+
this.emit("error", not_writeable_error);
746+
return;
772747
}
773748
}
774749

@@ -782,7 +757,8 @@ RedisClient.prototype.send_command = function (command, args, callback) {
782757
} else if (command === "quit") {
783758
this.closing = true;
784759
} else if (this.pub_sub_mode === true) {
785-
throw new Error("Connection in subscriber mode, only subscriber commands may be used");
760+
this.emit("error", new Error("Connection in subscriber mode, only subscriber commands may be used"));
761+
return;
786762
}
787763
this.command_queue.push(command_obj);
788764
this.commands_sent += 1;
@@ -1069,7 +1045,7 @@ Multi.prototype.exec = function (callback) {
10691045
callback(errors);
10701046
return;
10711047
} else {
1072-
throw new Error(err);
1048+
self._client.emit('error', err);
10731049
}
10741050
}
10751051

‎test/commands/eval.spec.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,10 +114,18 @@ describe("The 'eval' method", function () {
114114
client.evalsha(sha, 0, helper.isString('eval get sha test', done));
115115
});
116116

117-
it('throws an error if SHA does not exist', function (done) {
117+
it('returns an error if SHA does not exist', function (done) {
118118
helper.serverVersionAtLeast.call(this, client, [2, 5, 0]);
119119
client.evalsha('ffffffffffffffffffffffffffffffffffffffff', 0, helper.isError(done));
120120
});
121+
122+
it('emits an error if SHA does not exist and no callback has been provided', function (done) {
123+
client.on('error', function (err) {
124+
assert.equal(err.message, 'NOSCRIPT No matching script. Please use EVAL.');
125+
done();
126+
});
127+
client.evalsha('ffffffffffffffffffffffffffffffffffffffff', 0);
128+
});
121129
});
122130

123131
it('allows a key to be incremented, and performs appropriate conversion from LUA type', function (done) {

‎test/commands/mset.spec.js

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -92,13 +92,10 @@ describe("The 'mset' method", function () {
9292

9393
describe("with undefined 'key' and missing 'value' parameter", function () {
9494
// this behavior is different from the 'set' behavior.
95-
it("throws an error", function (done) {
96-
var mochaListener = helper.removeMochaListener();
97-
98-
process.once('uncaughtException', function (err) {
99-
process.on('uncaughtException', mochaListener);
100-
helper.isError()(err, null);
101-
return done();
95+
it("emits an error", function (done) {
96+
client.on('error', function (err) {
97+
assert.equal(err.message, "ERR wrong number of arguments for 'mset' command");
98+
done();
10299
});
103100

104101
client.mset();

‎test/commands/select.spec.js

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ describe("The 'select' method", function () {
2424
});
2525
});
2626

27-
it("throws an error if redis is not connected", function (done) {
27+
it("returns an error if redis is not connected", function (done) {
2828
client.select(1, function (err, res) {
2929
assert(err.message.match(/Redis connection gone/));
3030
done();
@@ -67,7 +67,7 @@ describe("The 'select' method", function () {
6767
});
6868

6969
describe("with an invalid db index", function () {
70-
it("emits an error", function (done) {
70+
it("returns an error", function (done) {
7171
assert.strictEqual(client.selected_db, null, "default db should be null");
7272
client.select(9999, function (err) {
7373
assert.equal(err.message, 'ERR invalid DB index');
@@ -90,14 +90,12 @@ describe("The 'select' method", function () {
9090
});
9191

9292
describe("with an invalid db index", function () {
93-
it("throws an error when callback not provided", function (done) {
94-
var mochaListener = helper.removeMochaListener();
93+
it("emits an error when callback not provided", function (done) {
9594
assert.strictEqual(client.selected_db, null, "default db should be null");
9695

97-
process.once('uncaughtException', function (err) {
98-
process.on('uncaughtException', mochaListener);
96+
client.on('error', function (err) {
9997
assert.equal(err.message, 'ERR invalid DB index');
100-
return done();
98+
done();
10199
});
102100

103101
client.select(9999);

‎test/commands/set.spec.js

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,7 @@ describe("The 'set' method", function () {
9696
this.timeout(200);
9797

9898
client.once("error", function (err) {
99-
helper.isError()(err, null);
100-
return done(err);
99+
done(err);
101100
});
102101

103102
client.set();
@@ -107,20 +106,13 @@ describe("The 'set' method", function () {
107106
}, 100);
108107
});
109108

110-
it("does not throw an error", function (done) {
111-
this.timeout(200);
112-
var mochaListener = helper.removeMochaListener();
113-
114-
process.once('uncaughtException', function (err) {
115-
process.on('uncaughtException', mochaListener);
116-
return done(err);
109+
it("does emit an error", function (done) {
110+
client.on('error', function (err) {
111+
assert.equal(err.message, "ERR wrong number of arguments for 'set' command");
112+
done();
117113
});
118114

119-
client.set();
120-
121-
setTimeout(function () {
122-
done();
123-
}, 100);
115+
client.set('foo');
124116
});
125117
});
126118
});

‎test/node_redis.spec.js

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,7 @@ describe("The node_redis client", function () {
647647

648648
describe('enable_offline_queue', function () {
649649
describe('true', function () {
650-
it("does not throw an error and enqueues operation", function (done) {
650+
it("does not return an error and enqueues operation", function (done) {
651651
var client = redis.createClient(9999, null, {
652652
max_attempts: 1,
653653
parser: parser
@@ -674,20 +674,18 @@ describe("The node_redis client", function () {
674674
});
675675

676676
describe('false', function () {
677-
it("does not throw an error and enqueues operation", function (done) {
677+
it("does not emit an error and enqueues operation", function (done) {
678678
var client = redis.createClient(9999, null, {
679679
parser: parser,
680680
max_attempts: 1,
681681
enable_offline_queue: false
682682
});
683683

684-
client.on('error', function() {
685-
// ignore, b/c expecting a "can't connect" error
684+
client.on('error', function(err) {
685+
assert(/send_command: stream not writeable|ECONNREFUSED/.test(err.message));
686686
});
687687

688-
assert.throws(function () {
689-
client.set('foo', 'bar');
690-
});
688+
client.set('foo', 'bar');
691689

692690
assert.doesNotThrow(function () {
693691
client.set('foo', 'bar', function (err) {

0 commit comments

Comments
 (0)