Skip to content

Conversation

@boblauer
Copy link

The buffer length was being calculated using buffer.length but should be calculated using Buffer.byteLength to account for non-ASCII characters whose length differs from the number of bytes they take up. I was able to get the test that I modified to fail before applying my changes, 9 failures in total but here's the last one:

  9) #buffer
       stream child client
         should never allow buffer to exceed maxBufferSize:

      AssertionError [ERR_ASSERTION]: Buffer size 113 exceeded maxBufferSize 100 after message 9
      + expected - actual

      -false
      +true

      at test/buffer.js:122:20
      at onListening (test/helpers/helpers.js:118:5)
      at createServer (test/helpers/helpers.js:218:5)
      at Context.<anonymous> (test/buffer.js:110:18)
      at process.processImmediate (node:internal/timers:491:21)

I attempted to replace all usages of .length with Buffer.byteLength where buffer size is being calculated, hopefully I didn't miss any.

currentBufferBytes, messageBytes, this.maxBufferSize);
this.flushQueue(callback);
this.bufferHolder.buffer += (this.bufferHolder.buffer === '' ? '' : '\n') + message;
this.bufferHolder.buffer = message;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unrelated change, but because we know flushQueue was just called, we know that bufferHolder.buffer is the empty string, so the this.bufferHolder.buffer === '' ? '' : '\n' check is unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant