feat(network): NET-1356 browser webrtc send buffering #2756
Conversation
| this.running = true | ||
| let sendAttempts = 0; | ||
| (async () => { | ||
| while (this.queue.length > 0 && this.running) { |
There was a problem hiding this comment.
Does the bufferedAmout really get smaller in the same tick on browser? I know in node-datachannel it does because there is a worker thread emptying the queue, but is the situation the same in the browser?
There was a problem hiding this comment.
Likely not smaller but it seems that it does get larger. We do have a listener for the onBufferedAmountLow event
| this.queue.unshift(messageToSend) | ||
| } | ||
| sendAttempts++ | ||
| if (sendAttempts % 200 === 0) { |
There was a problem hiding this comment.
Isn't 200 attempts quite a lot?
There was a problem hiding this comment.
This isn't really a reattempt or anything, it is simply a counter used for CPU breaking
| } | ||
| sendAttempts++ | ||
| if (sendAttempts % 200 === 0) { | ||
| await wait(0) // give up CPU cycle |
There was a problem hiding this comment.
I think a shorter pause should be used here see https://stackoverflow.com/questions/18826570/settimeout0-vs-window-postmessage-vs-messageport-postmessage on how to implement one. (I have tried this in the past, and the send buffer does get drained also during the shorter pauses)
| if (this.lastState === 'connected') { | ||
| this.dataChannel?.send(data as Buffer) | ||
| if (this.dataChannel!.bufferedAmount > 1 * 1000 * 1000) { | ||
| this.messageQueue.pause() |
There was a problem hiding this comment.
This looks a bit dangerous, doSend() is not an arrow function, so it is not guarantedd that "this" points to the right object. Please make doSend() an arrow function.
There was a problem hiding this comment.
In line 96 the doSend function is bound to the message queue using an arrow function. No need to make the class method itself in to an arrow function
Summary
Added additional buffering logic to the browser webrtc connections. If the connection buffer fills up messages are dropped. This will alleviate the issue by allowing more messages to be buffered.