Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add information about buffer.byteOffset property to Buffer.from(string) documentation #19301

Closed
bartosz-m opened this issue Mar 12, 2018 · 8 comments
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.

Comments

@bartosz-m
Copy link

I'm was trying to make this example work:

var firstBuffer = Buffer.from('hello world');
var secondBuffer = Buffer.from(firstBuffer.buffer, 0, firstBuffer.length);

console.log (firstBuffer) // <Buffer 68 65 6c 6c 6f 20 77 6f 72 6c 64>
console.log (secondBuffer) // <Buffer da 07 00 00 da 07 00 00 db 07 00>

assert(firstBuffer.buffer === secondBuffer.buffer, ".buffer property is the same");
assert (firstBuffer[0] == secondBuffer[0]) // fails

it didn't worked and I didn't know why until I found that offset property of the firstBuffer wasn't 0 as I would assume. I modify it to use correct offset and problem was solved, but I spend more than hour because there wasn't any info in official docs.

It would be good to mention in documentation that result of Buffer.from(string) can have offset different than 0

@vsemozhetbyt vsemozhetbyt added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Mar 12, 2018
@gireeshpunathil
Copy link
Member

@bartosz-m - why don't you use:

var secondBuffer = Buffer.from(firstBuffer, 0, firstBuffer.length);

rather than

var secondBuffer = Buffer.from(firstBuffer.buffer, 0, firstBuffer.length);

? the Buffer.from(buffer) API provides convenient way of creating a second buffer. The API which used receives an ArrayBuffer object that is shared by multiple objects, and the offset 0 of it may not align with the offset 0 of the buffer in question.

@bartosz-m
Copy link
Author

thanks for comment. I've refactored my code to not use Buffer at all. But in the future I may once again be in need of Buffer, so adding to documentation information that offset of freshly created Buffer may be different than 0 would be nice.

@TimothyGu
Copy link
Member

@gireeshpunathil, FYI your alternative can actually be more misleading, since the second and third arguments are ignored by Buffer.from().

I think a dedicated section on how the Buffer pooling works would be awesome.

@gireeshpunathil
Copy link
Member

@TimothyGu - thanks, I overlooked the from(buffer) API and thought that it returns deep copy of a portion of the source.

agree that the Buffer pooling can be better documented, especially which APIs use pooling and which do not. Current doc covers alloc and allocUnsafe but I guess there are more.

@ChALkeR
Copy link
Member

ChALkeR commented Mar 30, 2018

I would suggest .buffer usage to be discouraged on pooled buffers, as it could easily lead to OOB read/write errors.

@ChALkeR
Copy link
Member

ChALkeR commented Mar 30, 2018

@bartosz-m Note that this behaviour is not specific to Buffer objects.

var initial = new Uint8Array([1, 2, 3, 4, 5, 6]);
var first = initial.subarray(2, 4); // Uint8Array [ 3, 4 ]
var second = new Uint8Array(first.buffer, 0, first.length); // Uint8Array [ 1, 2 ]

@bartosz-m
Copy link
Author

I'm aware that it is not specific to Buffer. The only issue was lack of information that Buffer.from may return pooled buffer. e.g. section of Buffer.allocUnsafe could mention other method that uses pooling.

@Trott
Copy link
Member

Trott commented Apr 5, 2018

@nodejs/documentation @nodejs/buffer It would be great if someone could either open a PR or drop in some suggested text here to address #19301 (comment).

@TimothyGu TimothyGu changed the title Add information about buffer.offset property to Buffer.from(string) documentation Add information about buffer.byteOffset property to Buffer.from(string) documentation Apr 8, 2018
targos pushed a commit that referenced this issue Jul 16, 2018
Also document a common issue when casting a Buffer object to a
TypedArray object.

Fixes: #19301

PR-URL: #21718
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

No branches or pull requests

6 participants