streams: Readable highWaterMark is measured in bytes *or* characters

[mscdex]

• Version: all
• Platform: all
• Subsystem: streams

Currently the Readable streams documentation states that highWaterMark is measured in bytes for non-object mode streams. However, when .setEncoding() is called on the Readable stream, then highWaterMark is measured in characters and not bytes since state.length += chunk.length happens after chunk is overwritten with a decoded string and state.length is what is compared with highWaterMark.

This seems to be an issue since at least v0.10, so I wasn't sure if we should just update the documentation to note this behavior or if we should change the existing behavior to match the documentation.

/cc @nodejs/streams

[jasnell]

My initial take on this is that measuring characters is a bug.

[mcollina]

@jasnell I agree! this is a nice issue if anybody wants to start contributing on streams and on core cc @nodejs/streams.

[MylesBorins]

I'd be interested in digging into this to learn more about streams.... will take a look in the morning

[mscdex]

One other issue to think about with this is when someone calls setEncoding() when state.length > 0, what to do then? Convert all the buffered data to strings and update state.length or just loop through the buffer using Buffer.byteLength(chunk, encoding) to update state.length ?

[mcollina]

I think we should just use byte length for highWaterMark. Converting values... sounds tricky.

[jalafel]

Hi! It seems like it's been awhile since anyone's mentioned anything on this issue. Does anyone mind if I take a go at this?

[mcollina]

@jessicaquynh please do! Ping @nodejs/streams if you need any help.

[jalafel]

@mcollina Thanks! Should I also include a test for this change?

[cjihrig]

@jessicaquynh yes please!

[jalafel]

Great, will do! Thanks @cjihrig!