New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'drain'-event #1

Closed
VanCoding opened this Issue Nov 19, 2012 · 15 comments

Comments

Projects
None yet
7 participants
@VanCoding

VanCoding commented Nov 19, 2012

Is it possible to add the 'drain'-event?

I looked at the WebSocket specification and there is no such event and I'm not sure if there's a way to do it. But it would be great!

Maybe, you have any idea on how to implement it...

@maxogden

This comment has been minimized.

Owner

maxogden commented Nov 19, 2012

i'm not doing any buffering at all so it should be drained every time write is called (i think). @dominictarr said that the send method on a websocket returns a boolean in the same way writable streams write returns booleans but I don't actually know if that is true (though binaryjs is using the return value binaryjs/streamws@7034fc1#L3R195). there is also a bufferedAmount property

@maxogden

This comment has been minimized.

Owner

maxogden commented Nov 19, 2012

Oh I just realized the above comment is wrong. sorry, coffee hasnt kicked in yet. lemme research this a bit more

@maxogden

This comment has been minimized.

Owner

maxogden commented Nov 19, 2012

@VanCoding

This comment has been minimized.

VanCoding commented Nov 20, 2012

That's a pity. Polling will not be near to performant.. Hope there will come an extension to the API anytime...

@dominictarr

This comment has been minimized.

Contributor

dominictarr commented Nov 20, 2012

It's not as bad as you think, because you only need so start polling when send(data) == false
i.e. when you should be slowing down anyway.

@NodeGuy

This comment has been minimized.

NodeGuy commented Jan 23, 2013

The return type of send has been changed from boolean to void in the WebSocket spec so we can't check it anymore.

Perhaps after each write we can poll every 50 ms until the bufferedAmount is zero, and then stop and emit a drain event.

@dominictarr

This comment has been minimized.

Contributor

dominictarr commented Jan 23, 2013

why would they do that?

polling an in memory structure isn't that bad... also, you don't need to poll it all the time, only after the buffer has filled.

@NodeGuy

This comment has been minimized.

NodeGuy commented Jan 23, 2013

Looks like this message was the inspiration for the change.

@dominictarr

This comment has been minimized.

Contributor

dominictarr commented Jan 23, 2013

hmm, that looks much more serious! it actually closes the connection when the buffer fills!
maybe just make it return false if bufferedAmount is above a certain water mark...

@Raynos

This comment has been minimized.

Contributor

Raynos commented Mar 22, 2013

@dominictarr @maxogden

We should check bufferedAmount streams2 style. If its below the hwm then return true if its above return false and set up a poller at certain rate to emit "drain" once below hwm.

@dominictarr

This comment has been minimized.

Contributor

dominictarr commented Mar 23, 2013

yes +1

@maxogden

This comment has been minimized.

Owner

maxogden commented Sep 9, 2014

I rewrote this module using streams2. Maybe it's easier to implement this now?

@jnordberg

This comment has been minimized.

Collaborator

jnordberg commented Dec 6, 2015

I'm using this patched socketWriteBrowser function to do this:

function socketWriteBrowser(chunk, enc, next) {

  if (socket.bufferedAmount > 16384) {
    setTimeout(function(){
      socketWriteBrowser(chunk, enc, next)
    }, 10)
    return
  }

  try {
    socket.send(chunk)
  } catch(err) {
    return next(err)
  }

  next()
}

Not sure if this is the best way to go about this but it's been working well so far. With this patch I can upload large (>1gb) files over websockets without chrome eating all memory and cpu.

@mcollina

This comment has been minimized.

Collaborator

mcollina commented Dec 7, 2015

@jnordberg I think we should avoid to allocate a closure, it would give us more speed :). Something like:

function socketWriteBrowser(chunk, enc, next) {

  if (socket.bufferedAmount > 16384) {
    return setTimeout(socketWriteBrowser, 10, chunk, enc, next)
  }

  try {
    socket.send(chunk)
  } catch(err) {
    return next(err)
  }

  next()
}

I'm 👍 on this. PR?

@jnordberg

This comment has been minimized.

Collaborator

jnordberg commented Dec 7, 2015

Neat, I didn't know you could pass along arguments to setTimeout.

I'll prepare a PR

jnordberg added a commit to jnordberg/websocket-stream that referenced this issue Jan 19, 2016

@mcollina mcollina closed this in #84 Jan 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment