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.

Sign up for GitHub

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

Jump to bottom

Should Buffer.concat really return the source buffer when length == 1? #1891

Closed
ronkorving opened this issue Jun 4, 2015 · 15 comments
Closed

Should Buffer.concat really return the source buffer when length == 1? #1891

ronkorving opened this issue Jun 4, 2015 · 15 comments
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@ronkorving
Copy link
Contributor

Buffer.concat returns a new Buffer instance that contains all given buffers combined. However, if the array of buffers that is passed has length 1, the original buffer from that array is returned as-is. Not a copy. That means that manipulation of Buffer.concat's output has no effect on the source buffers if they had length > 1, yet alters the source buffer when length == 1.

This seems very unpredictable (Array.prototype.concat for example always returns a copy) and is bound to at least confuse people, and at worst cause weird bugs.

Original discussion: #1825 (comment)
@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Jun 4, 2015
@trevnorris
Copy link
Contributor

I do agree that returning the original buffer if length is 1 does seem slightly unintuitive, should mention that the way it currently works is documented:

If the list has exactly one item, then the first item of the list is returned.

@dcousens
Copy link

dcousens commented Jun 4, 2015

Most people end up doing this optimization themselves.

That is, you see code all the time handling:

if (arrayOfBuffers.length === 1) return arrayBuffers[0]
// ...

The most annoying part of the current behaviour is that if you really want a Buffer to be copied every time, you would have to handle it with new Buffer(Buffer.concat(arrayOfBuffers)) unless you directly check for it.

@thefourtheye
Copy link
Contributor

@trevnorris is there any specific reason for this behaviour?

@mscdex
Copy link
Contributor

mscdex commented Jun 4, 2015

@thefourtheye Optimization to avoid creating a new Buffer and doing a copy and making more work for the GC?

@trevnorris
Copy link
Contributor

@thefourtheye legacy? it's been that way since before I seriously started working on node.

@benjamingr
Copy link
Member

+1 on fixing it, it sounds like a good idea

@thefourtheye
Copy link
Contributor

I think that, if possible, we should fix this, so that people don't have to worry about this special case when using concat.

@monsanto
Copy link
Contributor

I agree that this should be fixed. Reminds me of the "Zalgo" problem with callbacks. Either the callback should always be called asynchronously, or always be called synchronously. Likewise, the object returned by Buffer.concat should always be safe to mutate, or never safe to mutate. (where safe means "you won't be surprised by aliasing")

@thefourtheye thefourtheye mentioned this issue Jun 10, 2015
Closed
trevnorris pushed a commit that referenced this issue Jun 10, 2015
@thefourtheye @trevnorris
buffer: fix case of one buffer passed to concat
f4f16bf 
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@trevnorris
Copy link
Contributor

Done in f4f16bf.

@benjamingr
Copy link
Member

@trevnorris you're a beast 👍 so many small but useful fixes.

@trevnorris
Copy link
Contributor

@benjamingr thanks. that patch was all @thefourtheye. I just ported it to the "next" branch. :)

@benjamingr
Copy link
Member

Well @thefourtheye already got credit for being awesome yesterday in front of a thousand people - I guess saying thanks again wouldn't hurt! thanks!

@thefourtheye
Copy link
Contributor

@trevnorris Thanks :-) @benjamingr Oh, I am the one who has to thank you, for being such an inspiration. Thanks a lot man :-)

@ronkorving
Copy link
Contributor Author

Thanks guys!

@reqshark
Copy link

woah, that's how it's done! good work!

chrisdickinson pushed a commit that referenced this issue Jun 17, 2015
@thefourtheye @chrisdickinson
buffer: fix case of one buffer passed to concat
a799038 
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
thefourtheye added a commit that referenced this issue Jul 22, 2015
@thefourtheye @rvagg
buffer: fix case of one buffer passed to concat
12b15aa 
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
thefourtheye added a commit that referenced this issue Jul 24, 2015
@thefourtheye @rvagg
buffer: fix case of one buffer passed to concat
8ec089a 
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
thefourtheye added a commit that referenced this issue Jul 30, 2015
@thefourtheye @rvagg
buffer: fix case of one buffer passed to concat
c3a0629 
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
thefourtheye added a commit that referenced this issue Aug 1, 2015
@thefourtheye @rvagg
buffer: fix case of one buffer passed to concat
aadcba2 
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
thefourtheye added a commit that referenced this issue Aug 3, 2015
@thefourtheye @rvagg
buffer: fix case of one buffer passed to concat
1c62793 
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
thefourtheye added a commit that referenced this issue Aug 4, 2015
@thefourtheye @rvagg
buffer: fix case of one buffer passed to concat
72b7f9d 
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
thefourtheye added a commit that referenced this issue Aug 4, 2015
@thefourtheye @rvagg
buffer: fix case of one buffer passed to concat
eea66e2 
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@mscdex @monsanto @trevnorris @dcousens @ronkorving @thefourtheye @reqshark @benjamingr