Should Buffer.concat really return the source buffer when length == 1? #1891
Labels
buffer
Issues and PRs related to the buffer subsystem.
Comments
|
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:
|
|
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 |
|
@trevnorris is there any specific reason for this behaviour? |
|
@thefourtheye Optimization to avoid creating a new Buffer and doing a copy and making more work for the GC? |
|
@thefourtheye legacy? it's been that way since before I seriously started working on node. |
|
+1 on fixing it, it sounds like a good idea |
|
I think that, if possible, we should fix this, so that people don't have to worry about this special case when using |
|
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 |
trevnorris
pushed a commit
that referenced
this issue
Jun 10, 2015
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
|
Done in f4f16bf. |
|
@trevnorris you're a beast 👍 so many small but useful fixes. |
|
@benjamingr thanks. that patch was all @thefourtheye. I just ported it to the "next" branch. :) |
|
Well @thefourtheye already got credit for being awesome yesterday in front of a thousand people - I guess saying thanks again wouldn't hurt! thanks! |
|
@trevnorris Thanks :-) @benjamingr Oh, I am the one who has to thank you, for being such an inspiration. Thanks a lot man :-) |
|
Thanks guys! |
|
woah, that's how it's done! good work! |
chrisdickinson
pushed a commit
that referenced
this issue
Jun 17, 2015
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
thefourtheye
added a commit
that referenced
this issue
Jul 22, 2015
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
thefourtheye
added a commit
that referenced
this issue
Jul 24, 2015
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
thefourtheye
added a commit
that referenced
this issue
Jul 30, 2015
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
thefourtheye
added a commit
that referenced
this issue
Aug 1, 2015
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
thefourtheye
added a commit
that referenced
this issue
Aug 3, 2015
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
thefourtheye
added a commit
that referenced
this issue
Aug 4, 2015
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
thefourtheye
added a commit
that referenced
this issue
Aug 4, 2015
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Buffer.concatreturns 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 ofBuffer.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)
The text was updated successfully, but these errors were encountered: