max-mapper / websocket-stream

websockets with the node stream API
BSD 2-Clause "Simplified" License
667 stars 114 forks source link

Confusion about perMessageDeflate in Readme #116

Closed binarykitchen closed 7 years ago

binarykitchen commented 7 years ago

When you recommend in your Readme that it's recommended to set perMessageDeflate to false for best throughput, then why is its default set to true?

Any reasons for this? And on a side note, I wonder when it's not recommended to set it to true?

mcollina commented 7 years ago

this is the same of ws cc @lpinca

lpinca commented 7 years ago

The extension has been added in ws@0.6 (https://github.com/websockets/ws/pull/409) and since then it has always been enabled by default. I honestly don't know why, I wasn't a collaborator at the time but it wasn't a good idea imho.

A lot of modules now assume that permessage-deflate is enabled so we avoided adding an unnecessary breaking change in ws@2.

permessage-deflate basically kills ws performance and uses a huge amount of memory. There is also an outstanding issue we are not sure how to solve (https://github.com/websockets/ws/issues/804).

That said, you should use permessage-deflate if you want to minimize the amount of data transmitted over the network. Bandwidth consumption is not free.

binarykitchen commented 7 years ago

i see and need a recommendation from you if you don't mind? should i enable or disable perMessageDeflate for the following scenario?

in www.videomail.io i am sending image frames through a pipe over this websocket-stream to the server at an FPS of 15. each frame is about 7kB.

lpinca commented 7 years ago

Most services make you pay only for outgoing bandwidth and I assume that those image frames are already compressed so I think you should definitely disable permessage-deflate.

Anyway I would check what's the difference with permessage-deflate on and off, calculate the costs and decide based on this info.

binarykitchen commented 7 years ago

i'd like to clarify few things here. obviously it is documented that you can set the perMessageDeflate option here: https://github.com/maxogden/websocket-stream#optionspermessagedeflate

but when you are using the native implementation

    if (isNative && isBrowser) {
      socket = new WS(target, protocols)
    } else {
      socket = new WS(target, protocols, options)
    }

then this option is never passed on because its native implementation does not support options as a third parameter, see https://github.com/maxogden/websocket-stream/issues/82

but according to this rfc this option should be embraced: https://tools.ietf.org/html/rfc7692

all that makes me question how i really can disable perMessageDeflate at all?

lpinca commented 7 years ago

@binarykitchen the extension is negotiated so you can just disable it on the server. The client will only use it if it's also supported on the server.

It can be disabled on the client because ws / websocket-stream can be used only as a client to connect to a server you don't control.

binarykitchen commented 7 years ago

oh i see ... ok, i tried to disable that on server side and tested again. but i still see Sec-WebSocket-Extensions: permessage-deflate in the response header, see screenshot:

selection_003

does disabling it really works and is tested?

binarykitchen commented 7 years ago

ah, never mind, i figured it out. was using the wrong constructor :)