You've already forked node-redis
mirror of
https://github.com/redis/node-redis.git
synced 2025-08-07 13:22:56 +03:00
Fix: Hiredis parser traps pubsub event exceptions
While using this module with the hiredis parser, I noticed that exceptions thrown inside of subscribe/message/etc. callbacks would be reported as parser error exceptions, which was confusing. Example: ``` var client = require('./index').createClient(); client.on('subscribe', function() { throw new Error('My Error'); }); client.subscribe('channel'); ``` I would expect the above to throw 'My Error' as a simple exception, however, when using the hiredis parser I get: ``` events.js:45 throw arguments[1]; // Unhandled 'error' event ^ Error: Redis reply parser error: Error: My Error at RedisClient.<anonymous> (/Users/felix/code/node_redis/hiredis.js:4:9) at RedisClient.emit (events.js:67:17) at RedisClient.return_reply (/Users/felix/code/node_redis/index.js:457:22) at HiredisReplyParser.<anonymous> (/Users/felix/code/node_redis/index.js:220:14) at HiredisReplyParser.emit (events.js:64:17) at HiredisReplyParser.execute (/Users/felix/code/node_redis/lib/parser/hiredis.js:35:22) at RedisClient.on_data (/Users/felix/code/node_redis/index.js:365:27) at Socket.<anonymous> (/Users/felix/code/node_redis/index.js:58:14) ... ``` The attached patch fixes this problem by changing the way the HiredisReplyParser traps exceptions, and also provides a unit test for it. Please let me know if you need any tweaks on the patch for landing it, I was not quite sure where to put my test / how to structure it, since most test cases seem to be integration tests at this point. Also, if you would prefer an integration test for this, here you go: ``` javascript var client = require('./index').createClient(); var assert = require('assert'); process.on('uncaughtException', function listener(err) { process.removeListener('uncaughtException', listener); // Drop the stack, otherwise the assert gets trapped by node process.nextTick(function() { assert.ok(!/parser error/.test(err.message), err.message); client.quit(); }); }); client.on('subscribe', function() { throw new Error('My Error'); }); client.subscribe('channel'); ``` PS: If you wonder why this only affects pubsub events: This is because RedisClient#return_reply does exception trapping differently for events than it does for callbacks. See:3e95c55a03/index.js (L431)
Vs:3e95c55a03/index.js (L447)
This commit is contained in:
@@ -27,15 +27,20 @@ HiredisReplyParser.prototype.reset = function () {
|
||||
HiredisReplyParser.prototype.execute = function (data) {
|
||||
var reply;
|
||||
this.reader.feed(data);
|
||||
try {
|
||||
while ((reply = this.reader.get()) !== undefined) {
|
||||
if (reply && reply.constructor === Error) {
|
||||
this.emit("reply error", reply);
|
||||
} else {
|
||||
this.emit("reply", reply);
|
||||
}
|
||||
while (true) {
|
||||
try {
|
||||
reply = this.reader.get();
|
||||
} catch (err) {
|
||||
this.emit("error", err);
|
||||
break;
|
||||
}
|
||||
|
||||
if (reply === undefined) break;
|
||||
|
||||
if (reply && reply.constructor === Error) {
|
||||
this.emit("reply error", reply);
|
||||
} else {
|
||||
this.emit("reply", reply);
|
||||
}
|
||||
} catch (err) {
|
||||
this.emit("error", err);
|
||||
}
|
||||
};
|
||||
|
38
tests/hiredis_parser.js
Normal file
38
tests/hiredis_parser.js
Normal file
@@ -0,0 +1,38 @@
|
||||
var Parser = require('../lib/parser/hiredis').Parser;
|
||||
var assert = require('assert');
|
||||
|
||||
/*
|
||||
This test makes sure that exceptions thrown inside of "reply" event handlers
|
||||
are not trapped and mistakenly emitted as parse errors.
|
||||
*/
|
||||
(function testExecuteDoesNotCatchReplyCallbackExceptions() {
|
||||
var parser = new Parser();
|
||||
var replies = [{}];
|
||||
|
||||
parser.reader = {
|
||||
feed: function() {},
|
||||
get: function() {
|
||||
return replies.shift();
|
||||
}
|
||||
};
|
||||
|
||||
var emittedError = false;
|
||||
var caughtException = false;
|
||||
|
||||
parser
|
||||
.on('error', function() {
|
||||
emittedError = true;
|
||||
})
|
||||
.on('reply', function() {
|
||||
throw new Error('bad');
|
||||
});
|
||||
|
||||
try {
|
||||
parser.execute();
|
||||
} catch (err) {
|
||||
caughtException = true;
|
||||
}
|
||||
|
||||
assert.equal(caughtException, true);
|
||||
assert.equal(emittedError, false);
|
||||
})();
|
Reference in New Issue
Block a user