fix: restrict message sizes to 16kb

[marcus-pousette]

Solves #144

Currently, not only do one need to push messages in parts to the datachannel, but also one needs to throttle the send function so that the datachannel buffer does not exceed x bytes. If x > 256kb the datachannel will close if you are using Chrome

The unit tests added are not testing what happens when the bufferedAmount exceeds maxMsgSize even though there is functionality for that.

I tested this solution in the browser-to-browser example also by sending large messages (1mb) instead of text.

Ideally, there would be a default max msg size limit would already set so that new users are not running into this when using this transport. The default behaviour is very unpredictable as it fails on different conditions depending on what browser combination used, message sizes and messaging frequency.

[maschad]

Thanks for this PR, this is great. Following up from my comment #144 (comment) could you refactor this to exclude the optional maxMsgSize and instead include the changes that ensure the underlying buffer doesn't exceed the max size of 16kb.

This should solve #158

[marcus-pousette]

I have now rebased and hardcoded 16kb limit

[mxinden]

• Restricting the size of the message passed to RTCDataChannel.send to the minimum support 16kib sounds good to me.
  • See also corresponding limit in rust-libp2p https://github.com/libp2p/rust-libp2p/blob/1742bffcea4691f013e966393ae0d7fe95f8834c/transports/webrtc/src/tokio/substream.rs#L45-L50
• I am in favor of splitting messages according to the above.
• Do I understand correctly that this pull request effectively restricts both the message size and the RTCDataChannel bufferedAmount to never exceed 16kib?
• If I am not mistaken, given that the MessageProcessor simply buffers bytes exceeding the bufferedAmount limit in sendMessageQueue, there is no backpressure. But I guess, without being familiar with most of the APIs in js-libp2p there is no backpressure on the read or write path in the first place, correct?

[marcus-pousette]

• Do I understand correctly that this pull request effectively restricts both the message size and the RTCDataChannel bufferedAmount to never exceed 16kib?

Yes. I am not fully confident what the limits should be, and it is quite hard to tests, frankly. I made this decision reading various threads about sending large files over WebRTC and what has been done to circumvent issues.

Max buffered amount should maybe be 16 Mbytes instead
https://viblast.com/blog/2015/2/25/webrtc-bufferedamount/
https://groups.google.com/g/discuss-webrtc/c/EjevtDTsxuE/m/eowQM2wNAwAJ

Some other discussion
feross/simple-peer#393

• If I am not mistaken, given that the MessageProcessor simply buffers bytes exceeding the bufferedAmount limit in sendMessageQueue, there is no backpressure.

Yes

But I guess, without being familiar with most of the APIs in js-libp2p there is no backpressure on the read or write path in the first place, correct?

Not sure

[achingbrain]

Instead of buffering, you can slow down the ingestion of new data from the source in the _sink method using p-event:

for await (const buf of merge(closeWrite, src)) {
  if (channel.bufferedAmount > maxMsgSize) {  // this should really be maxBufferedAmountSize
    await pEvent(this.channel, 'bufferedamountlow', {
      timeout: // a sensible timeout
    })
  }
  
  // now write message as normal
  //... chunk using maxMsgSize

There's no need to buffer anything then as backpressure is implicitly applied since we stop pulling from the incoming source.

When sending the message you can then limit the max size of individual message by doing chunking in a similar way to mplex.

This should also have separate maxBufferedAmountSize and maxMsgSize config options since they control different things.

[marcus-pousette]

Instead of buffering, you can slow down the ingestion of new data from the source in the _sink method using p-event:

Thats a great idea.

I made the changes now. Not done tests for the max buffered amount, not sure how to do it cleanly without mocking everything, or doing a it with a real client which might behave unpredictable.

[achingbrain]

Nearly there - just a couple of extra bits to do, comments are inline.