Add max data channel buffer size parameter#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new parameter to control the maximum buffer size for WebRTC data channels. When the buffer exceeds this limit, new data packets are dropped to prevent unbounded memory growth.
- Adds
MaxDataChannelBufferfield toConnectParamsstruct - Implements
WithMaxDataChannelBufferoption function for configuring the buffer limit - Adds buffer fullness check in
publishDataPacketthat drops messages when the limit is exceeded
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| signalling/interfaces.go | Adds MaxDataChannelBuffer uint64 field to ConnectParams struct |
| room.go | Adds WithMaxDataChannelBuffer ConnectOption function to configure the buffer limit |
| engine.go | Implements buffer check logic in publishDataPacket that drops messages when buffer is full |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if sctpBuffered+dc.BufferedAmount() > e.connParams.MaxDataChannelBuffer { | ||
| e.log.Warnw("DataChannel buffer full, dropping message", nil, "maxBufferSize", e.connParams.MaxDataChannelBuffer) | ||
| return errors.New("datachannel buffer full") |
There was a problem hiding this comment.
Generally speaking. It's better to drop the oldest packet in the buffer, and then add the newest packet. Rather than dropping the newest packet.
Is this something we could do? Or is that too difficult? If it's too difficult it's fine to wait for this to be a problem, maybe it never will be
There was a problem hiding this comment.
To do that, we would need to modify the SCTP pion package. I think it's possible but it won't be simple. We would need to touch several parts of the SCTP logic.
I asked to copilot and he confirmed that.
Your current approach—dropping packets at the application level when Stream.BufferedAmount() or Association.BufferedAmount() exceed a threshold—is a practical way to prevent unbounded memory growth. This is a common pattern in real-time or resource-constrained systems, especially when you can tolerate some data loss (e.g., for video/audio streaming).
SCTP Standard Compliance:
- The SCTP protocol (RFC 4960) is designed for reliable, in-order delivery by default. Dropping data at the sender before it enters the SCTP stack is not a protocol violation, but it does mean the application is responsible for handling the consequences (e.g., lost messages).
- If you modify the SCTP package itself to drop the oldest packet in the pending queue, you would be breaking the reliability guarantee of SCTP. This is not compliant with the standard unless you are using partial reliability extensions (RFC 3758), which allow for data to be abandoned under certain conditions.
Modifying the SCTP Package:
- If you want to implement buffer limits and drop the oldest packet in the pending queue, you would need to:
- Add logic to track the total buffered data in the pending queues.
- Decide which queue to drop from (ordered/unordered).
- Ensure that sequence numbers, retransmissions, and acknowledgments are handled correctly for dropped data.
- Possibly signal the application about dropped data.
- This would require changes in several places: pendingQueue, stream, and possibly association logic, especially around retransmission and acknowledgment handling.
Potential Issues:
Dropping data inside the SCTP stack (without using partial reliability) breaks the protocol’s reliability guarantees and may cause confusion for peers expecting reliable delivery.
It could also cause issues with sequence numbers, retransmissions, and SACK (Selective ACK) handling.Best Practice:
- The best approach is usually to apply backpressure at the application level (as you are doing), so the application can decide what to drop or how to handle overload.
- If you need to drop data within SCTP, consider implementing or using the partial reliability extension, which is designed for this purpose.
Summary:
- Your current approach is correct and safe for memory control.
- Modifying the SCTP package to drop data internally (without partial reliability) is not standard-compliant and would require significant changes.
- If you need in-stack dropping, use or implement SCTP’s partial reliability features.
No description provided.