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

Provide SCTP Association OnClose callback #2858

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Aug 8, 2024

The only way to detect a remote connection close is to check the state of the SCTP transport.
See:
w3c/webrtc-pc#1799 (comment)

In case it wasn't clear, I'm arguing for an exception for data
channels, saying pc1.close() should fire close on both ends of a data channel, because A) that is useful, and the only way to detect remote closing of a peer connection, and B) it's a workaround for the fact that closing of peer connections isn't advertised to the other end.

Depending on datachannel close to notify us requires out of band protocol agreement as datachannels can be closed by the remote for other reasons. Having access to the SCTP layer we can provide an accurate SCTP Assocation closed event.

@sukunrt sukunrt requested a review from Sean-Der August 8, 2024 10:45
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.97%. Comparing base (c4d56d4) to head (7b15659).

Files Patch % Lines
sctptransport.go 68.75% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2858      +/-   ##
==========================================
+ Coverage   78.83%   78.97%   +0.14%     
==========================================
  Files          89       89              
  Lines        8432     8447      +15     
==========================================
+ Hits         6647     6671      +24     
+ Misses       1298     1292       -6     
+ Partials      487      484       -3     
Flag Coverage Δ
go 80.54% <68.75%> (+0.15%) ⬆️
wasm 65.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Sean-Der Sean-Der left a comment

Choose a reason for hiding this comment

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

LGTM! Merge w/e CI passes

thanks for doing this @sukunrt

@sukunrt sukunrt force-pushed the sctp-transport-onclose branch 5 times, most recently from 3d78636 to 1489ce2 Compare August 9, 2024 12:52
@sukunrt
Copy link
Member Author

sukunrt commented Aug 9, 2024

@Sean-Der Can we merge this? The ci issues are with err shadowing which in this case is more idiomatic.

@sukunrt sukunrt merged commit 6cfa00f into master Aug 12, 2024
17 of 18 checks passed
@sukunrt sukunrt deleted the sctp-transport-onclose branch August 12, 2024 16:45
@sukunrt
Copy link
Member Author

sukunrt commented Aug 12, 2024

I removed the shadowing.

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

Successfully merging this pull request may close these issues.

2 participants