JustinTulloss / zeromq.node

Node.js bindings to the zeromq library
MIT License
1.65k stars 284 forks source link

Messages implicitly converted to string #542

Open platy opened 8 years ago

platy commented 8 years ago

Background I have just spent a lot of time debugging an issue of an array containing a single buffer being sent to ZMQ - in my test cases it worked but with production data there were extra bytes appearing all over the place.

Problem The OutBuffer silently converts things which are not buffers into strings - link The String conversion for arrays converts each element to string and concatenates them, the String conversion for Buffer treats the buffer as containing a UTF-8 encoded string. So for most cases you don't notice a problem like this - ~60% of bytes are valid and survive.

Proposal I don't think doing this conversion is worth the trouble it can cause - I think we should throw a TypeError - I will send a PR.

platy commented 8 years ago

Sent PR #543

ronkorving commented 8 years ago

Hi @platy, I'm not sure I understand the problem. Are you saying sending a [new Buffer('foo')] did not work as expected? We traverse arrays if given, and the elements are then cast to string. The same is true for single arguments that are not arrays. I think you are describing this already, so we probably agree... but what's the actual problem?

platy commented 8 years ago

No it's about the trying to handle anything given and not failing.

The problem that I hit was an array of array of buffer - it is fine the Buffer contains a utf8 string (or something which is a valid string) as the elements in the array are converted to strings and appended, the effect is just to remove the extra array.

> [[new Buffer('foo')]].map(String)
[ 'foo' ] 
> [[Buffer.of(0, 1, 2)]].map(String)
[ '\u0000\u0001\u0002' ]
> [[Buffer.of(0, 1, 2)]].map(String).map(Buffer)
[ <Buffer 00 01 02> ]

In my system this was the case for all of the tests and in production hit problems.

> [[Buffer.of(129, 130)]].map(String).map(Buffer)
[ <Buffer ef bf bd ef bf bd> ]

If we only accept strings and buffers, and don't make silent conversions, these problems are much easier to detect in tests.

ronkorving commented 8 years ago

Fair enough, I don't mind it being more strict about it. Strings and buffers (and arrays of them) only then 👍