Skip to content
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

webrtc: close data channels cleanly #2724

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

webrtc: close data channels cleanly #2724

wants to merge 2 commits into from

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Mar 6, 2024

WebRTC data channel close is a synchronous close procedure. We close our outgoing stream, in response the peer is expected to close its outgoing stream. If the peer doesn't close its side of the stream we will end up with a memory leak where the SCTP transport keeps reference to the stream. So we check the number of invalid data channel closures and when this goes over a threshold we close the connection.

For our custom purposes we can fork SCTP and implement a unilateral stream Reset which is feasible because we anyway have a state machine on top of the data channels. But for a RFC compliant SCTP implementation, this is how the spec is supposed to work. SCTP stream numbers are limited (uint16) so we do need to reuse the stream ids forcing us to use a synchronous close mechanism.

WebRTC data channel close is a synchronous close procedure. We close our
outgoing stream, in response the peer is expected to close its outgoing
stream. If the peer doesn't close its side of the stream we will end up
with a memory leak where the SCTP transport keeps reference to the
stream. So we check the number of invalid data channel closures and when
this goes over a threshold we close the connection.

For our custom purposes we can fork SCTP and implement a unilateral
stream Reset which is feasible because we anyway have a state machine
on top of the data channels. But for a RFC compliant SCTP
implementation, this is how the spec is supposed to work. SCTP stream
numbers are limited (uint16) so we do need to reuse the stream ids
forcing us to use a synchronous close mechanism.
@sukunrt sukunrt requested a review from MarcoPolo March 6, 2024 10:23
@sukunrt sukunrt marked this pull request as draft March 6, 2024 10:23
@MarcoPolo
Copy link
Contributor

Was there some scenario that we've run into that hits this case? Is it expected to see a remote peer not ack the close? Or is this preventative?

@sukunrt
Copy link
Member Author

sukunrt commented Mar 7, 2024

It is preventative
peer can create 32k streams. Not configurable in pion :(
So the total memory for the streams = 32k * 200B/stream = 6.4MB
Add 1MB for the receive window in the connection. So we are roughly at 10MB.

This would have been fine but pion/sctp has a receive window calculation
formula that only counts the size of the userData in chunks and not the whole chunk size.
This leads to a 100x amplification of memory used vs that allocated in receive window
so we can use 100MB memory per connection in the peer by creating 10k datachannels and sending 100, 1byte chunks on every channel.
10k * 100 * 100 = 100MB

profile.pb.gz
This was created by the benchmark in branch: https://github.com/libp2p/go-libp2p/tree/webrtc-bm

An alternative is to limit our receive window to 100kB. And let reserve 10MB memory for every webrtc connection(thanks @marten-seemann for the idea). This would slow down things but is much simpler.

@MarcoPolo
Copy link
Contributor

This would have been fine but pion/sctp has a receive window calculation
formula that only counts the size of the userData in chunks and not the whole chunk size.
This leads to a 100x amplification of memory used vs that allocated in receive window
so we can use 100MB memory per connection in the peer by creating 10k datachannels and sending 100, 1byte chunks on every channel.
10k * 100 * 100 = 100MB

I'm missing where the second 100 comes from:
10k (data channels) * 100 (1 byte chunks) * 100 (?) = 100 MB

@MarcoPolo
Copy link
Contributor

profile.pb.gz This was created by the benchmark in branch: https://github.com/libp2p/go-libp2p/tree/webrtc-bm

This 404's for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants