introduce max message size in webrtc config #442
Conversation
…mi/buffer-size-webrtc-config # Conflicts: # src/transport/webrtc/opening.rs
src/transport/webrtc/config.rs
Outdated
| .parse() | ||
| .expect("valid multiaddress")], | ||
| datagram_buffer_size: 2048, | ||
| max_message_size: 512 * 1024, |
There was a problem hiding this comment.
Where this constant is coming from? What is libp2p way of handling this?
What will happen in case of high throughput transfer — shouldn't we be able to parse the payload split over multiple max_message_size buffers?
There was a problem hiding this comment.
Maybe we should make this optional, not entirely sure if this interferes with @timwu20 upcoming work 🤔
There was a problem hiding this comment.
I would suggest hardcoding the constant from libp2p spec, as discussed in #352.
There was a problem hiding this comment.
Oki doki makes sense 🙏 Let's hardcode it instead
There was a problem hiding this comment.
so, @lexnv should i remove the max_message_size entirely from the config?
There was a problem hiding this comment.
Hey yep, let's pick: #352 (comment) 16kIB for this
| use prost::Message; | ||
| use tokio_util::codec::{Decoder, Encoder}; | ||
|
|
||
| const MAX_MESSAGE_SIZE: usize = 16 * 1024; |
There was a problem hiding this comment.
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:
- 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?
There was a problem hiding this comment.
@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.
4 participants
closes #352