-
Notifications
You must be signed in to change notification settings - Fork 27
introduce max message size in webrtc config #442
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
Open
dharjeezy
wants to merge
5
commits into
paritytech:master
Choose a base branch
from
dharjeezy:dami/buffer-size-webrtc-config
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
a59f6f8
introduce max message size in webrtc config
dharjeezy 79df39e
Merge branch 'master' of https://github.com/dharjeezy/litep2p into da…
dharjeezy 5baea6e
hardcode 16kb as max message size
dharjeezy de663f6
Merge branch 'master' into dami/buffer-size-webrtc-config
lexnv fccbf79
Update src/transport/webrtc/util.rs
lexnv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a constant for this in
webrtc/substream.rs. We should either use that one here or the other way round.Apart from that, this change covers the scope of #352, which seems to be only about optimizing memory usage.
However, it does not achieve spec compliance with respect to handling of max. message size. In order to get there, we would need to set the
max_message_sizeattribute in the SDP "remote description". Unfortunately, str0m doesn't offer an API for doing that and seems to hard-code the value to 256 KiB.Another issue worth considering is how oversized messages are handled in the methods called by protocols, which I believe ultimately comes down to
Substream::poll_read()andSubstream::poll_write(), but there is alsowrite_all(), which I have no idea what it does in the case of WebRTC substreams.For reading, there is this check, which is incorrect, because the maximum length of the payload is smaller than the maximum size of the whole message. Regardless of whether it is correct, I think it is also made redundant by the changes in this PR, because oversized messages would already be rejected here, in
WebRtcConnection::on_open_channel_data().For writing,
Substream::poll_write()currently silently truncates oversized messages, or rather payloads that are larger thanMAX_FRAME_SIZE(again, slightly off). This is definitely not what we want. Instead, we either want to return an error or fragment the payload into multiple messages. The latter is probably tricky to get right, especially considering the upcoming changes to theSubstreamimplementation regarding flag handling. The former is in line with the specification of thesend()function in the WebRTC JavaScript API in browsers:The question then becomes how this would work in higher-level code implementing protocols. That code is not aware of transport-specific limitations and, I assume, expects to be able to send arbitrarily large amounts of data.
@lexnv @dmitry-markin wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dharjeezy to clarify, my requested change is to use a single constant for
MAX_MESSAGE_SIZE/MAX_FRAME_SIZEin bothwebrtc/substream.rsandwebrtc/util.rs.All the other stuff is important and needs to be addressed, but is probably out of scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out rust-libp2p also silently truncates (but correctly calculates the maximum payload size).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sens to me! Thansk for the suggestions @haikoschol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the meantime I've been working on a PR to enable setting max_message_size in str0m. Will probably open it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this does not seem as straight forward as I initially thought. Turns out the rust-libp2p folks have been there and figured out how to set the attribute with SDP munging, but from my understanding of the str0m code, the value might not be passed down to the SCTP layer. I've opened an issue instead: algesten/str0m#844