Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

process.stdout does not use Buffer length property #8913

Closed
SheetJSDev opened this issue Dec 19, 2014 · 12 comments
Closed

process.stdout does not use Buffer length property #8913

SheetJSDev opened this issue Dec 19, 2014 · 12 comments

Comments

@SheetJSDev
Copy link

As an example: start with a Buffer of length 6, fill it with 'A', shrink it to 3, then fill with 'B'.

var x = new Buffer(6);
x.fill(65); /* A */
x.length = 3;
x.fill(66); /* B */

If we then run process.stdout.write(x);, the output is BBBAAA (which means that it is not using the buffer length property).

This is inconsistent with other functions that use Buffers:

  • util.inspect (console.log(x) shows <Buffer 42 42 42>)
  • Buffer toString (console.log(x.toString()) shows BBB)
  • Buffer fill (if the fill used the full length, the first output would be BBBBBB)
@TooTallNate
Copy link

I wouldn't expect setting length on the buffer instance to work like you're expecting it to. It's not documented like that ever at least.

@SheetJSDev
Copy link
Author

From the docs, for example:

buf.toString([encoding], [start], [end])#
end Number, Optional, Default: buffer.length

http://nodejs.org/api/buffer.html#buffer_buf_tostring_encoding_start_end

The statement for fill should be updated as well:

buf.fill(value, [offset], [end])#
end Number, Optional
If the offset (defaults to 0) and end (defaults to buffer.length) are not given it will fill the entire buffer.

In fact, that language is even more confusing since it uses "entire buffer" to refer to the segment starting at index 0 and ending at index buffer.length

@TooTallNate
Copy link

That's the documentation for the toString() function, and it does not mention anything about modifying the length property of a Buffer instance.

@SheetJSDev
Copy link
Author

Both toString and fill clearly say they default to the buffer.length property. The fill documentation clearly refers to the "entire buffer" as the segment starting at 0 and ending at buffer.length. It's entirely possible that the length in buffer.length refers to a different length, but the phrasing strongly suggests that those functions read the length parameter first (and that is what we see in practice, as evidenced in my example)

@jasnell
Copy link
Member

jasnell commented Dec 19, 2014

Looking things over, unless I've missed something I think I'd have to agree with @SheetJSDev ... it appears that process.stdout is behaving incorrectly.

@TooTallNate
Copy link

It's undefined behavior because nowhere in the documentation does it say that modifying/changing the length property won't break anything. This is different than, say, Array.length, since that has well defined behavior of what happens when you change that value.

I'm guessing that on the C++ side your length change isn't being considered, which is not a bug IMO since it's undefined behavior.

To do it right, instead, use buffer.slice() to make a new window of the buffer that is only the size you are interested in. As for toString()/fill(), they both have optional end arguments for a reason. I still can't see a reason why you'd need to set length ever.

@SheetJSDev
Copy link
Author

@jasnell @TooTallNate the UB here stems from ambiguity in the streams documentation. For example:

http://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback

chunk String | Buffer The data to write

This method writes some data to the underlying system, and calls the supplied callback once the data has been fully handled.

The phrase "some data" is not well-defined here. A version of this method that only wrote the second character of a string or third byte of a Buffer would be consistent with the documentation.

@trevnorris
Copy link

Changing .length automatically results in undefined behavior. This isn't a bug.

Implementation information: .length is supposed to represent the internal data length retrieved by v8::Object::GetIndexedPropertiesExternalArrayDataLength(). Each is used on their respective ends because it's faster to access that way. Changing .length does not automatically change the internal length of the data.

@trevnorris
Copy link

As far as the documentation goes, ambiguity could be removed. I'll accept a PR that better explains those.

@SheetJSDev
Copy link
Author

@trevnorris if that's the intention, then why not just make it immutable like the string length? For example: var x = "abcdef"; x.length = 3; does not actually change the length of the string.

@trevnorris
Copy link

Making a property read-only drastically increases the instantiation time of the object. One of the pitfalls of user-land V8 API.

@vkurchatkin
Copy link

@trevnorris maybe set length on prototype as a getter property and use _length internally?

jasnell added a commit to jasnell/node-joyent that referenced this issue Dec 22, 2014
Per nodejs#8913 (comment) ...

Indicate that changing the buffer.length property results in
undefined/inconsistent behavior and should be avoided. Show an example
of using buffer.slice to achieve achieve the goal properly.
tjfontaine pushed a commit that referenced this issue Dec 22, 2014
Better wording for start and end parameters, also document .length
should be considered readonly.

RE: #8857, #8859, #8913
PR: #8910
PR-URL: #8910

Signed-off-by: Timothy J Fontaine <tjfontaine@gmail.com>
jasnell added a commit to jasnell/node-joyent that referenced this issue Jan 5, 2015
Better wording for start and end parameters, also document .length
should be considered readonly.

RE: nodejs#8857, nodejs#8859, nodejs#8913
PR: nodejs#8910
PR-URL: nodejs#8910

Signed-off-by: Timothy J Fontaine <tjfontaine@gmail.com>
jasnell added a commit to jasnell/node-joyent that referenced this issue Jan 5, 2015
Better wording for start and end parameters, also document .length
should be considered readonly.

RE: nodejs#8857, nodejs#8859, nodejs#8913
PR: nodejs#8910
PR-URL: nodejs#8910

Signed-off-by: Timothy J Fontaine <tjfontaine@gmail.com>
kaiquewdev pushed a commit to kaiquewdev/node that referenced this issue Nov 26, 2016
Name anonymous arrow function in vm module to improve readability

PR-URL: nodejs/node#9388
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <inglor@gmail.com>
Ref: nodejs#8913
isaacs pushed a commit to isaacs/node-v0.x-archive that referenced this issue Dec 8, 2016
PR-URL: nodejs/node#9389
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants