search

darrachequesne / notepack

A fast Node.js implementation of the latest MessagePack spec
MIT License
75 stars 19 forks source link

Question: allow encoding on non-finite numbers #3

Closed beaucollins closed 7 years ago

beaucollins commented 7 years ago

I have a Socket.IO app using socket.io-redis that is crashing intermittently due to notepack throwing an encoding error:

Error: Number is not finite
    at _encode (./node_modules/notepack.io/lib/encode.js:93:13)
    at _encode (./node_modules/notepack.io/lib/encode.js:286:15)
    at _encode (./node_modules/notepack.io/lib/encode.js:286:15)
    at _encode (./node_modules/notepack.io/lib/encode.js:286:15)
    at _encode (./node_modules/notepack.io/lib/encode.js:286:15)
    at _encode (./node_modules/notepack.io/lib/encode.js:286:15)
    at _encode (./node_modules/notepack.io/lib/encode.js:286:15)
    at _encode (./node_modules/notepack.io/lib/encode.js:286:15)
    at _encode (./node_modules/notepack.io/lib/encode.js:286:15)
    at _encode (./node_modules/notepack.io/lib/encode.js:286:15)

I haven't been able to track down the origin of the socket.io emits that have one of these values but it seems a little extreme to crash Socket.IO when a client potentially emits NaN, Infinity, or -Infinity somewhere in the emit values.

My remedy is to run a patched version of notepack that encodes these to null values in the meantime. I'm not sure what the best approach is for a correct solution moving forward.

darrachequesne commented 7 years ago

@beaucollins what do you think of https://github.com/darrachequesne/notepack/pull/4? It seems msgpack-lite encodes those values as float64 (with writeDoubleBE)

beaucollins commented 7 years ago

@darrachequesne #4 works for our needs. (4aab3db)

darrachequesne commented 7 years ago

Closed by https://github.com/darrachequesne/notepack/pull/4.

roeycohen commented 7 years ago

this might also happen if the object has a cyclic reference. for example if you try to send a sequelizejs model. when using JSON.stringify this problem is mostly solved as these kind of object use the toJSON function. to fix that, i've created this pull request: https://github.com/darrachequesne/notepack/pull/7