Surprising behavior when chunk size > maxLength option

[IronSavior]

My expectation of the maxLength option is that the stream will error when buffering of any one line grows the buffer to exceed the given length. I understand this may not match the true intent of this option, but I'd like to ask for clarification. The surprising behavior occurs when processing a chunk larger than the maxLength option, but also contains no lines that individually exceed maxLength.

This is more easily demonstrated in this test:

test('chunk size > maxLength', function (t) {
  t.plan(2)

  var input = split({ maxLength: 2 })

  input.pipe(strcb(function (err, list) {
    t.error(err)
    t.deepEqual(list, ['a', 'b'])
  }))

  input.end("a\nb") // Two lines, each length 1 in single chunk of length 3
})

This is surprising because it should be very common for the typical chunk size to be much greater than the typical line length. The maxLength option could therefore only be useful when set to at least twice the typical chunk size. Since a stream can't control the size of chunks it receives, this seems like a logic error.

Could you please comment?

[IronSavior]

In the main transform function, the buffer length is checked before pushing any fully terminated lines. Would it make more sense to check the length of the buffer after pushing terminated lines?

[IronSavior]

This seems to pass all current tests as well as the one in this issue:

function transform (chunk, enc, cb) {
  this[kLast] += this[kDecoder].write(chunk)

  var list = this[kLast].split(this.matcher)

  this[kLast] = list.pop()

  for (var i = 0; i < list.length; i++) {
    push(this, this.mapper(list[i]))
  }

  if (this[kLast].length > this.maxLength) {
    return cb(new Error('maximum buffer reached'))
  }
  
  cb()
}

[mcollina]

Sure, would you like to send a PR?

[IronSavior]

I can do that. With the change described above, it delays the error until the buffer (meaning this[kLast]) is not able to be flushed/reduced to below maxLength between chunks. That should be less surprising as a mechanism to limit buffer growth, but it still doesn't operate on each line--this is acceptable in my specific use case.

I happened to stumble upon this because I was adding an alternative overflow behavior for my use case where the stream seeks to the start of the next line rather than erroring-out when maxLength is violated. The net result is that the stream would effectively filter out any lines that resulted in buffer overflows instead of throwing.

The behavior is described in this test:

test('maximum buffer limit w/skip', function (t) {
  t.plan(2)

  var input = split({ maxLength: 2, skipOverflow: true })

  input.pipe(strcb(function (err, list) {
    t.error(err)
    t.deepEqual(list, ['a', 'b', 'c'])
  }))

  input.write('a\n123')
  input.write('456')
  input.write('789\nb\nc')
  input.end()
})

Would you be interested in a PR for this alternative overflow behavior as well?

[mcollina]

go for it

[mcollina]

Fixed in 3.1.0 and #24.