Writable doesn't correctly count size of strings

[ronag]

Minor bug. Not sure if it's worth the performance impact of fixing.

When calculating the currently buffered length we are not correctly calculating the byte length of strings, we just use the string length.

function writeOrBuffer(stream, state, chunk, encoding, callback) {
  const len = (state[kState] & kObjectMode) !== 0 ? 1 : chunk.length;

  state.length += len;

[ronag]

@nodejs/streams @mcollina

[lpinca]

I think we should fix it. Is this a recent regression?

[benjamingr]

So the fix would be to call Buffer.byteLength? Might be worth checking what the perf regression is and if it's not significant to fix it.

[benjamingr]

Is this a recent regression?
Not at all, here it is in v10 https://github.com/nodejs/node/blob/v10.x/lib/_stream_readable.js#L291 and at v0.10 https://github.com/nodejs/node/blob/v0.10/lib/_stream_readable.js#L157

[lpinca]

@benjamingr it seems to be correctly calculated in v10 and v0.10. See https://github.com/nodejs/node/blob/v10.x/lib/_stream_writable.js#L374 and https://github.com/nodejs/node/blob/v0.10/lib/_stream_writable.js#L204 (note decodeChunk()).

[lpinca]

It actually seems to be correctly calculated also on main. See

node/lib/internal/streams/writable.js

Line 465 in 2c55652

chunk = Buffer.from(chunk, encoding);

.

[lpinca]

Confirmed.

const { Writable } = require('stream');

const w = new Writable({
  write() {}
});

w.write('€');
w.write('€');

console.log(w.writableLength); // 6

I think we can close this.

[benjamingr]

@lpinca that's just one case though?

[benjamingr]

To be clear:

When calculating the currently buffered length we are not correctly calculating the byte length of strings, we just use the string length.

This is across streams in several places - the fact writable works with decodeStrings set to true doesn't mean it's not a bug elsewhere?

[lpinca]

The issue description did not mention the decodeStrings option. Yes, in that case the string size is incorrectly calculated.

[lpinca]

I think using Buffer.byteLength() when the decodeStrings is set to false is acceptable. Performance should be no worse than the default case (decodeStrings set to true) when the string is converted to a Buffer.

[benjamingr]

I think whenever we have a stream of strings and not buffers we should use byteLength (or just use Buffer.byteLength for everything) and if there is no performance impact land it.

(though byteLength would use its length as utf-8 and not utf-16 "as if it was a buffer encoded as utf-8" which may be desired?)