Skip to content
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

Support backpressure #18

Closed
feross opened this issue Apr 20, 2015 · 3 comments
Closed

Support backpressure #18

feross opened this issue Apr 20, 2015 · 3 comments

Comments

@feross
Copy link
Owner

feross commented Apr 20, 2015

Right now, we just send as fast as possible. We should support backpressure and detect when bufferedAmount gets too high and pause for a while.

Relevant:

The solution implemented for simple-peer can also be used for websocket libraries like websocket-stream, since data channels and websockets have the same API, and thus, the same difficulties detecting when backpressure is needed.

cc @mafintosh, @maxogden

@mafintosh
Copy link
Collaborator

I think an easy impl would be to poll bufferedAmount every 250 ms and only call the write callback if it is 0. We should use a higher than normal watermark (like 1mb) since our feedback loop has higher latency because of the poll.

Something like this:

var SimplePeer = function () {
  stream.Duplex.call({highWaterMark: 1024 * 1024})
  ...
  var self = this
  setInterval(function () {
    if (self._datachannel.bufferedAmount || !self._cb) return
    var cb = self._cb
    self._cb = null
    cb()
  }, 250)
  ...
}
SimplePeer.prototype._write = function (data, enc, cb) {
  this._datachannel.send(data)
  if (!this._datachannel.bufferedAmount) return cb()
  this._cb = cb
}

It would also be interesting to investigate if there was a hack to detect when bufferedAmount changes (make it observable / use setters/getters)

@mafintosh
Copy link
Collaborator

Using the 1mb watermark and polling every 250ms would support 4mb/s which seems ok for now (we could dynamically adjust the watermark based how quickly the stream drains etc)

@feross feross closed this as completed in 43583fb Apr 24, 2015
@feross
Copy link
Owner Author

feross commented Apr 24, 2015

@mafintosh Nice strategy. Just released it as 5.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants