Merge pull request #139 from felixge/master

Fix: Hiredis parser traps pubsub event exceptions

lib/parser/hiredis.js

@@ -27,15 +27,20 @@ HiredisReplyParser.prototype.reset = function () {
 HiredisReplyParser.prototype.execute = function (data) {
     var reply;
     this.reader.feed(data);
-    try {
+    while (true) {
-        while ((reply = this.reader.get()) !== undefined) {
+        try {
-            if (reply && reply.constructor === Error) {
+          reply = this.reader.get();
-                this.emit("reply error", reply);
+        } catch (err) {
-            } else {
+          this.emit("error", err);
-                this.emit("reply", reply);
+          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);
     }
 };

tests/hiredis_parser.js

@@ -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);
+})();