Skip to content
Open
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/transport/webrtc/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ use crate::{codec::unsigned_varint::UnsignedVarint, error::ParseError, transport
use prost::Message;
use tokio_util::codec::{Decoder, Encoder};

const MAX_MESSAGE_SIZE: usize = 16 * 1024;
Copy link
Contributor

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_size attribute 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() and Substream::poll_write(), but there is also write_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 than MAX_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 the Substream implementation regarding flag handling. The former is in line with the specification of the send() function in the WebRTC JavaScript API in browsers:

  1. If the byte size of data exceeds the value of maxMessageSize on channel's associated RTCSctpTransport, throw a TypeError.

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?

Copy link
Contributor

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_SIZE in both webrtc/substream.rs and webrtc/util.rs.

All the other stuff is important and needs to be addressed, but is probably out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

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_SIZE in both webrtc/substream.rs and webrtc/util.rs.

Makes sens to me! Thansk for the suggestions @haikoschol

Copy link
Contributor

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.

Copy link
Contributor

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.

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


/// WebRTC mesage.
#[derive(Debug)]
pub struct WebRtcMessage {
Expand Down Expand Up @@ -74,7 +76,7 @@ impl WebRtcMessage {
/// Decode payload into [`WebRtcMessage`].
pub fn decode(payload: &[u8]) -> Result<Self, ParseError> {
// TODO: https://github.com/paritytech/litep2p/issues/352 set correct size
let mut codec = UnsignedVarint::new(None);
let mut codec = UnsignedVarint::new(Some(MAX_MESSAGE_SIZE));
let mut data = bytes::BytesMut::from(payload);
let result = codec
.decode(&mut data)
Expand Down