Repository navigation
Writable doesn't correctly count size of strings #52818
Description
Activity
I think we should fix it. Is this a recent regression?
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.Reacted by Robert NagyIs 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@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()).It actually seems to be correctly calculated also on main. See
.node/lib/internal/streams/writable.js
Line 465 in 2c55652
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.
@lpinca that's just one case though?
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?
Reacted by Robert Nagy and Luigi PincaThe issue description did not mention the
decodeStringsoption. Yes, in that case the string size is incorrectly calculated.I think using
Buffer.byteLength()when thedecodeStringsis set tofalseis acceptable. Performance should be no worse than the default case (decodeStringsset totrue) when the string is converted to aBuffer.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?)
- added 2 commits that reference this issue
on May 5, 2024 - added a commit that references this issue
on May 10, 2024 - added a commit that references this issue
on May 11, 2024 - added a commit that references this issue
on Jun 20, 2024
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.