From fbc16424619c022908b283584fda29d720fecf79 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felix=20Geisendo=CC=88rfer?= <felix@debuggable.com>
Date: Mon, 17 Oct 2011 11:07:11 +0200
Subject: [PATCH] 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: https://github.com/mranney/node_redis/blob/3e95c55a0301104c9cd6451d71fa5527b7797792/index.js#L431
Vs: https://github.com/mranney/node_redis/blob/3e95c55a0301104c9cd6451d71fa5527b7797792/index.js#L447
---
 lib/parser/hiredis.js   | 23 ++++++++++++++---------
 tests/hiredis_parser.js | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 9 deletions(-)
 create mode 100644 tests/hiredis_parser.js

diff --git a/lib/parser/hiredis.js b/lib/parser/hiredis.js
index 9dba8c93e8..e7db7433f7 100644
--- a/lib/parser/hiredis.js
+++ b/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 ((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);
     }
 };
diff --git a/tests/hiredis_parser.js b/tests/hiredis_parser.js
new file mode 100644
index 0000000000..f1515b110b
--- /dev/null
+++ b/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);
+})();