Fix bulk string concat

Author: Ruben Bridgewater

lib/parser.js

@@ -281,27 +281,26 @@ function JavascriptRedisParser (options) {
  * @param buffer
  * @returns {String}
  */
-function concatBigString (parser, buffer) {
+function concatBulkString (parser) {
   var list = parser.bufferCache
-  var length = parser.bigStrSize
-  var stringSize = length - parser.bigOffset - 2
-  var res = list[0].toString('utf8', parser.bigOffset)
-  for (var i = 1; i < list.length - 1; i++) {
-    res += list[i].toString()
-  }
-  if (i > 1) {
-    if (res.length + list[i].length <= stringSize) {
-      res += list[i].toString()
-    } else {
-      res += list[i].toString('utf8', 0, list[i].length - 1)
+  // The first chunk might contain the whole bulk string including the \r
+  var end = parser.totalChunkSize - parser.bigStrSize === -1
+  var chunks = list.length
+  if (end) {
+    parser.offset = 1
+    if (chunks === 2) {
+      return list[0].toString('utf8', parser.bigOffset, list[0].length - 1)
     }
+  } else {
+    parser.offset = parser.bigStrSize - parser.totalChunkSize
+    chunks++
   }
-  var offset = stringSize - res.length
-  if (offset > 0) {
-    res += buffer.toString('utf8', 0, offset)
+  var res = list[0].toString('utf8', parser.bigOffset)
+  for (var i = 1; i < chunks - 2; i++) {
+    // We are only safe to fully add up elements that are neither the first nor any of the last two elements
+    res += list[i].toString()
   }
-  parser.offset = offset + 2
-  parser.bigOffset = 0
+  res += list[i].toString('utf8', 0, end ? list[i].length - 1 : parser.offset - 2)
   return res
 }
 
@@ -313,7 +312,6 @@ function decreaseBufferPool () {
   if (bufferPool.length > 96 * 1024) {
     // Decrease the bufferPool by 16kb
     bufferPool = bufferPool.slice(0, bufferPool.length - 16 * 1024)
-    // console.log(bufferPool.length)
   } else {
     clearInterval(interval)
     interval = null
@@ -326,7 +324,7 @@ function decreaseBufferPool () {
  * @param length
  * @returns {Buffer}
  */
-function concat (parser, length) {
+function concatBuffer (parser, length) {
   var list = parser.bufferCache
   var pos = 0
   if (bufferPool.length < length) {
@@ -365,13 +363,14 @@ JavascriptRedisParser.prototype.execute = function (buffer) {
     this.buffer = newBuffer.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) {
-      this.returnReply(concatBigString(this, buffer))
+      this.returnReply(concatBulkString(this))
+      this.bigOffset = 0
       this.buffer = buffer
     } else { // This applies for arrays too
-      this.bufferCache.push(buffer)
-      this.buffer = concat(this, this.totalChunkSize + buffer.length)
+      this.buffer = concatBuffer(this, this.totalChunkSize + buffer.length)
       this.offset = 0
     }
     this.bigStrSize = 0

test/parsers.spec.js

@@ -70,9 +70,11 @@ describe('parsers', function () {
           'sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. ' +
           'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ' +
           'ut aliquip ex ea commodo consequat. Duis aute irure dolor in' // 256 chars
-        var bigString = (new Array(Math.pow(2, 17) / lorem.length).join(lorem)) // Math.pow(2, 17) chars long
+        var bigString = (new Array(Math.pow(2, 17) / lorem.length + 1).join(lorem)) // Math.pow(2, 17) chars long
         var replyCount = 0
+        var sizes = [4, Math.pow(2, 17)]
         function checkReply (reply) {
+          assert.strictEqual(sizes[replyCount], reply.length)
           replyCount++
         }
         var parser = new Parser({
@@ -200,7 +202,7 @@ describe('parsers', function () {
       it('should handle \\r and \\n characters properly', function () {
         // If a string contains \r or \n characters it will always be send as a bulk string
         var replyCount = 0
-        var entries = ['foo\r', 'foo\r\nbar', '\r\nfoo', 'foo\r\n', 'foo']
+        var entries = ['foo\r', 'foo\r\nbar', '\r\nfoo', 'foo\r\n', 'foo', 'foobar', 'foo\r', 'äfooöü', 'abc']
         function checkReply (reply) {
           assert.strictEqual(reply, entries[replyCount])
           replyCount++
@@ -217,8 +219,16 @@ describe('parsers', function () {
         assert.strictEqual(replyCount, 4)
         parser.execute(new Buffer('+foo\r'))
         assert.strictEqual(replyCount, 4)
-        parser.execute(new Buffer('\n'))
+        parser.execute(new Buffer('\n$6\r\nfoobar\r'))
         assert.strictEqual(replyCount, 5)
+        parser.execute(new Buffer('\n$4\r\nfoo\r\r\n'))
+        assert.strictEqual(replyCount, 7)
+        parser.execute(new Buffer('$9\r\näfo'))
+        parser.execute(new Buffer('oö'))
+        parser.execute(new Buffer('ü\r'))
+        assert.strictEqual(replyCount, 7)
+        parser.execute(new Buffer('\n+abc\r\n'))
+        assert.strictEqual(replyCount, 9)
       })
 
       it('line breaks in the beginning of the last chunk', function () {
@@ -271,10 +281,9 @@ describe('parsers', function () {
         parser.execute(new Buffer(
           'abcdefghij\r\n' +
           '$100\r\nabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij\r\n' +
-          '$100\r\nabcdefghijabcdefghijabcdefghijabcdefghij'
+          '$100\r\nabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij'
         ))
         assert.strictEqual(replyCount, 3)
-        parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij'))
         parser.execute(new Buffer('abcdefghijabcdefghijabcdefghij\r'))
         assert.strictEqual(replyCount, 3)
         parser.execute(new Buffer('\n'))