Merge pull request #5 from NodeRedis/interfering-parsers

v.2.0.1

Author: BridgeAR

.codeclimate.yml

@@ -0,0 +1,5 @@
+languages:
+  JavaScript: true
+exclude_paths:
+- "benchmark/index.js"
+- "benchmark/old/javascript.js"

changelog.md

@@ -1,3 +1,9 @@
+## v.2.0.1 - 0x Jun, 2016
+
+Bugfixes
+
+-  Fixed multiple parsers working concurrently resulting in faulty data in some cases
+
 ## v.2.0.0 - 29 May, 2016
 
 The javascript parser got completly rewritten by [Michael Diarmid](https://github.com/Salakar) and [Ruben Bridgewater](https://github.com/BridgeAR) and is now a lot faster than the hiredis parser.

lib/parser.js

@@ -135,9 +135,6 @@ function parseBulkString (parser) {
   var offsetEnd = parser.offset + length
   if (offsetEnd + 2 > parser.buffer.length) {
     parser.bigStrSize = offsetEnd + 2
-    parser.bigOffset = parser.offset
-    parser.totalChunkSize = parser.buffer.length
-    parser.bufferCache.push(parser.buffer)
     return
   }
 
@@ -334,7 +331,7 @@ function concatBuffer (parser, length) {
     list[i].copy(bufferPool, pos)
     pos += list[i].length
   }
-  return bufferPool.slice(parser.offset, length)
+  return bufferPool.slice(0, length)
 }
 
 /**
@@ -348,25 +345,22 @@ JavascriptRedisParser.prototype.execute = function (buffer) {
     this.offset = 0
   } else if (this.bigStrSize === 0) {
     var oldLength = this.buffer.length
-    var remainingLength = oldLength - this.offset
-    var newLength = remainingLength + buffer.length
+    var newLength = oldLength + buffer.length
     // ~ 5% speed increase over using new Buffer(length) all the time
     if (bufferPool.length < newLength) { // We can't rely on the chunk size
       bufferPool = new Buffer(newLength)
     }
-    this.buffer.copy(bufferPool, 0, this.offset, oldLength)
-    buffer.copy(bufferPool, remainingLength, 0, buffer.length)
+    this.buffer.copy(bufferPool, 0, 0, oldLength)
+    buffer.copy(bufferPool, oldLength, 0, buffer.length)
     this.buffer = bufferPool.slice(0, newLength)
-    this.offset = 0
   } else if (this.totalChunkSize + buffer.length >= this.bigStrSize) {
     this.bufferCache.push(buffer)
     // The returned type might be Array * (42) and in that case we can't improve the parsing currently
-    if (this.optionReturnBuffers === false && this.buffer[this.offset] === 36) {
+    if (this.optionReturnBuffers === false && this.buffer[0] === 36) {
       this.returnReply(concatBulkString(this))
       this.buffer = buffer
     } else { // This applies for arrays too
       this.buffer = concatBuffer(this, this.totalChunkSize + buffer.length)
-      this.offset = 0
     }
     this.bigStrSize = 0
     this.totalChunkSize = 0
@@ -382,7 +376,19 @@ JavascriptRedisParser.prototype.execute = function (buffer) {
     var type = this.buffer[this.offset++]
     var response = parseType(this, type)
     if (response === undefined) {
-      this.offset = offset
+      if (this.buffer === null) {
+        return
+      }
+      var tempBuffer = new Buffer(this.buffer.length - offset)
+      this.buffer.copy(tempBuffer, 0, offset, this.buffer.length)
+      this.buffer = tempBuffer
+      if (this.bigStrSize !== 0) {
+        this.bigStrSize -= offset
+        this.bigOffset = this.offset - offset
+        this.totalChunkSize = this.buffer.length
+        this.bufferCache.push(this.buffer)
+      }
+      this.offset = 0
       return
     }
 

lib/replyError.js

@@ -7,20 +7,17 @@ function ReplyError (message) {
   Error.stackTraceLimit = 2
   Error.captureStackTrace(this, this.constructor)
   Error.stackTraceLimit = limit
-  Object.defineProperty(this, 'name', {
-    value: 'ReplyError',
-    configurable: false,
-    enumerable: false,
-    writable: true
-  })
   Object.defineProperty(this, 'message', {
     value: message || '',
-    configurable: false,
-    enumerable: false,
     writable: true
   })
 }
 
 util.inherits(ReplyError, Error)
 
+Object.defineProperty(ReplyError.prototype, 'name', {
+  value: 'ReplyError',
+  writable: true
+})
+
 module.exports = ReplyError

test/parsers.spec.js

@@ -11,7 +11,7 @@ var parsers = [JavascriptParser, HiredisParser]
 // Mock the not needed return functions
 function returnReply () { throw new Error('failed') }
 function returnError () { throw new Error('failed') }
-function returnFatalError () { throw new Error('failed') }
+function returnFatalError (err) { throw err }
 
 describe('parsers', function () {
   describe('general parser functionality', function () {
@@ -64,6 +64,68 @@ describe('parsers', function () {
 
   parsers.forEach(function (Parser) {
     describe(Parser.name, function () {
+      it('multiple parsers do not interfere', function () {
+        var replyCount = 0
+        var results = [1234567890, 'foo bar baz', 'hello world']
+        function checkReply (reply) {
+          assert.strictEqual(results[replyCount], reply)
+          replyCount++
+        }
+        var parserOne = new Parser({
+          returnReply: checkReply,
+          returnError: returnError,
+          returnFatalError: returnFatalError
+        })
+        var parserTwo = new Parser({
+          returnReply: checkReply,
+          returnError: returnError,
+          returnFatalError: returnFatalError
+        })
+        parserOne.execute(new Buffer('+foo '))
+        parserOne.execute(new Buffer('bar '))
+        assert.strictEqual(replyCount, 0)
+        parserTwo.execute(new Buffer(':1234567890\r\n+hello '))
+        assert.strictEqual(replyCount, 1)
+        parserTwo.execute(new Buffer('wor'))
+        parserOne.execute(new Buffer('baz\r\n'))
+        assert.strictEqual(replyCount, 2)
+        parserTwo.execute(new Buffer('ld\r\n'))
+        assert.strictEqual(replyCount, 3)
+      })
+
+      it('multiple parsers do not interfere with bulk strings in arrays', function () {
+        var replyCount = 0
+        var results = [['foo', 'foo bar baz'], [1234567890, 'hello world', 'the end'], 'ttttttttttttttttttttttttttttttttttttttttttttttt']
+        function checkReply (reply) {
+          console.log(reply)
+          assert.deepEqual(results[replyCount], reply)
+          replyCount++
+        }
+        var parserOne = new Parser({
+          returnReply: checkReply,
+          returnError: returnError,
+          returnFatalError: returnFatalError
+        })
+        var parserTwo = new Parser({
+          returnReply: checkReply,
+          returnError: returnError,
+          returnFatalError: returnFatalError
+        })
+        parserOne.execute(new Buffer('*2\r\n+foo\r\n$11\r\nfoo '))
+        parserOne.execute(new Buffer('bar '))
+        assert.strictEqual(replyCount, 0)
+        parserTwo.execute(new Buffer('*3\r\n:1234567890\r\n$11\r\nhello '))
+        assert.strictEqual(replyCount, 0)
+        parserOne.execute(new Buffer('baz\r\n+ttttttttttttttttttttttttt'))
+        assert.strictEqual(replyCount, 1)
+        parserTwo.execute(new Buffer('wor'))
+        parserTwo.execute(new Buffer('ld\r\n'))
+        assert.strictEqual(replyCount, 1)
+        parserTwo.execute(new Buffer('+the end\r\n'))
+        assert.strictEqual(replyCount, 2)
+        parserOne.execute(new Buffer('tttttttttttttttttttttt\r\n'))
+      })
+
       it('chunks getting to big for the bufferPool', function () {
         // This is a edge case. Chunks should not exceed Math.pow(2, 16) bytes
         var lorem = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit, ' +