Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

fix: restrict message sizes to 16kb #147

Merged
merged 2 commits into from
May 17, 2023
Merged

fix: restrict message sizes to 16kb #147

merged 2 commits into from
May 17, 2023

Conversation

marcus-pousette
Copy link
Contributor

@marcus-pousette marcus-pousette commented Apr 28, 2023

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.

Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marcus-pousette force-pushed the main branch 3 times, most recently from b91dbc4 to f15207e Compare May 10, 2023 05:53
@marcus-pousette
Copy link
Contributor Author

I have now rebased and hardcoded 16kb limit

@marcus-pousette marcus-pousette changed the title feat: add maxMsgSize option feat: restrict message sizes to 16kb May 10, 2023
@marcus-pousette marcus-pousette requested a review from maschad May 10, 2023 06:19
@mxinden
Copy link
Member

mxinden commented May 15, 2023

  • Restricting the size of the message passed to RTCDataChannel.send to the minimum support 16kib sounds good to me.
  • 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
Copy link
Contributor Author

  • 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

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

marcus-pousette commented May 15, 2023

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.

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/stream.ts Show resolved Hide resolved
test/stream.spec.ts Show resolved Hide resolved
test/stream.spec.ts Outdated Show resolved Hide resolved
@marcus-pousette marcus-pousette force-pushed the main branch 4 times, most recently from a93a3e6 to 3cbc4ab Compare May 16, 2023 19:23
@achingbrain achingbrain changed the title feat: restrict message sizes to 16kb fix: restrict message sizes to 16kb May 17, 2023
test/stream.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: Alex Potsides <alex@achingbrain.net>
@achingbrain achingbrain merged commit aca4422 into libp2p:main May 17, 2023
github-actions bot pushed a commit that referenced this pull request May 17, 2023
## [2.0.3](v2.0.2...v2.0.3) (2023-05-17)

### Bug Fixes

* restrict message sizes to 16kb ([#147](#147)) ([aca4422](aca4422)), closes [#144](#144) [#158](#158)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants