max-mapper / websocket-stream

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

Duplicate writes? *unsure* #117

Closed binarykitchen closed 7 years ago

binarykitchen commented 7 years ago

This is something I am unsure about but am suspecting a bug.

On the server side I have something like

stream.write(JSON.stringify({"command":"confirmSample","args":{"sample":116}}))

where I am sending commands to the client counterpart in form of JSONs.

Figured out that, somehow in the stream and between the pipes, something gets blocked and multiple JSONs arrive on the client. And they all are duplicates.

On the client I parse in that command like this:

        stream.on('data', function (data) {
          command = JSON.parse(data.toString())
        })

and parsing sometimes fails and throws errors. Data like this

{"command":"confirmSample","args":{"sample":116}}
{"command":"confirmSample","args":{"sample":116}}

Why does this happen? On the server I have only one stream.write() call but on the client it arrives twice.

PS: as a temporary workaround I am forced to use split2(JSON.parse) to make it more robust on client side but still, there must be another bug ...

mcollina commented 7 years ago

@binarykitchen as of version 4, you should be setting objectMode if you expect the stream to respect the boundaries of the chunk when doing its own fragmenting: https://github.com/maxogden/websocket-stream#optionsobjectmode.

However, messages should not get duplicated, you might get exceptions. Are you able to reproduce, so I can have a look?

binarykitchen commented 7 years ago

@mcollina grazie ... but i do not understand this re objectMode:

"Send each chunk on its own, and do not try to pack them in a single websocket frame."

shouldn't each chunk be sent on their own anyway? like in my case, i have the problem that sometimes same chunks exist and are breaking my app. that's why i had to use split2(JSON.parse) to dedupe duplicate chunks.

do you think you can tell use cases why disabling or enabling objectMode makes sense?

i am afraid, i cannot reproduce it. it seems to happen by random for my www.videomail.io app - its npm package is the videomail-client which is undergoing lots of rewrites ...

mcollina commented 7 years ago

Since v4.0.0, websocket-stream behaves as a true binary stream, so it will chunk things based on its internal buffering. If you enable objectMode, each write() will be in its own frame.

binarykitchen commented 7 years ago

can you expand a bit more on that or at least document in the readme, especially the difference between those two modes? when to pick binary stream, when to pick object mode?

and for my videomail.io app which sends 95% of all times compressed images and 5% commands in form of json objects, all through the same websocket connection, which one mode do you recommend?

mcollina commented 7 years ago

can you add a snippet on how you use websocket-stream?

It seems to me you need some sort of protocol on top of it to distinguish between images and commands. What is that?

binarykitchen commented 7 years ago

umm, to be precise videomail.io has this: -> client sends commands in form of jsons and image frames to server <- server only sends commands in form of jsons to client

the crucial code on the server side to distinguish between a json and an image frame is this

  function isCommand (buffer) {
    // first byte of json's in a buffer is always 123 and commands are never longer than 1000 bytes
    return buffer[0] === 123 && buffer.length < 1000
  }

  function isNoop (buffer) {
    return buffer.toString().length < 1
  }

  this.process = function (buffer) {
    if (isNoop(buffer)) {
      // do nothing, it is just a ping to keep connection alive
      // todo: maybe change this into a command?
      log.debug('Got pinged to keep connection alive')
    } else if (isCommand(buffer)) {
      executeCommand(buffer)
    } else {
      const res = fileType(buffer)
      const ext = res && res.ext ? res.ext : null

      switch (ext) {
        case 'webp':
        case 'jpg':
          client.writeFrame(buffer, ext)
          break
        default: // pcm files
          client.writeSample(buffer)
          break
      }
    }
  }

and just in case if you want to know, this is how i create the websocket server:

  const wss = websocket.createServer({
    perMessageDeflate: false,
    server: server
  })

then i listen to new connections like this

wss.on('connection', function (ws) { 
   const stream  = websocket(ws)
   ...
})
mcollina commented 7 years ago

For that you will need to be using objectMode: true, because there are no boundaries between one or the other.

binarykitchen commented 7 years ago

ah, thanks. i didn't understand its meaning before. so, should i set objectMode on server only, or also on the client?

mcollina commented 7 years ago

in both, yes.

binarykitchen commented 7 years ago

hmm, ok. but didn't we say in another issue that the server should be dominating when it comes to options? i think it was the perMessageDeflate option

mcollina commented 7 years ago

that's not an option on the protocol, that's an option on the stream.

binarykitchen commented 7 years ago

hmmm, good to know. i think the readme is a bit confusing on this matter. would be good to outline which option is for the server, which is for the client or for both.

mcollina commented 7 years ago

All the option are valid everywhere. But the deflate one is subject to the server enabling it.

binarykitchen commented 7 years ago

add that to readme?

mcollina commented 7 years ago

Yeah, would you mind sending a PR?

binarykitchen commented 7 years ago

here u go https://github.com/maxogden/websocket-stream/pull/120

mcollina commented 7 years ago

Closing