add Abort function to sctptransport

[craymond12]

Description

the association should send the abort message before before changing the state to closed and closing the association as defined by the webrtc spec in step 6
https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close

When the peerConnection is closed without sending an abort message, it cause the other peer to hang on it's connection until it face a timeout or other kind of event showing that the connection is closed
This can be an issue with device who can only do a few concurrent stream

This was tested with real cameras and instead of the connection hanging for 30 seconds, it closed immediately

[sukunrt]

This change is correct, but we probably also need to ensure that we don't send the DTLS Close Alert on connection close. We do that currently.

This is an annoying aspect of the spec. The way to detect peer connection close is by opening a datachannel that will be closed on close of the peer connection. See here:
w3c/webrtc-pc#1799 (comment)

If you're not using it from the browser, you can use the OnClose callback on the SCTPTransport

webrtc/sctptransport.go

Line 303 in 179397b

func (r *SCTPTransport) onClose(err error) {

event to detect connection close.

[craymond12]

This change is correct, but we probably also need to ensure that we don't send the DTLS Close Alert on connection close. We do that currently.

This is an annoying aspect of the spec. The way to detect peer connection close is by opening a datachannel that will be closed on close of the peer connection. See here: w3c/webrtc-pc#1799 (comment)

If you're not using it from the browser, you can use the OnClose callback on the SCTPTransport

webrtc/sctptransport.go

Line 303 in 179397b

func (r *SCTPTransport) onClose(err error) {

event to detect connection close.

If I understand correctly, we should replace the close Notify by the abort message
On my tests with real devices, I had pion on the consumer side and an unknown implementation of webrtc on the producer side

It is kind of a big behavior change, on how the peerConnection notify the other party of the close but abort should be the way to go as it is the standard and we can expect this behavior everywhere else

Currently the Close of the conn.go in Dtls always send the bool byUser to true to the private close
Should I simply remove the byUser param and the following code:

if c.isHandshakeCompletedSuccessfully() && byUser {
		// Discard error from notify() to return non-error on the first user call of Close()
		// even if the underlying connection is already closed.
		_ = c.notify(context.Background(), alert.Warning, alert.CloseNotify)
	}

or give access to the bool in the public Close and rename it for notifyClose instead of byUser
And lastly, should I merge this PR, then modify the Dtls lib, then do a second pr in webrtc to update the dtls version?
Or just update the Dtls in this PR?

I did not have any issue by having both the abort and notifyClose in my tests

[adriancable]

I like this (also working with cameras that have pretty low concurrent stream limits). I don't think it's an issue sending both the Abort message and also the DTLS Close Notify. Does anyone else have any experience with this?

[Sean-Der]

I am in favor of merging this also!

I use the DTLS Close in a few places (when I don't have SCTP available)

But having SCTP Abort + DTLS Close is even better. The easier/better we make for clients I am in support of :)

[Sean-Der]

@craymond12 What do you think of always sending an ABORT?

In what case would a user want to Stop the SCTPTransport, but not want an Abort?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.99%. Comparing base (a857d57) to head (74b3d7a).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2900   +/-   ##
=======================================
  Coverage   78.98%   78.99%           
=======================================
  Files          89       89           
  Lines        8489     8487    -2     
=======================================
- Hits         6705     6704    -1     
+ Misses       1297     1294    -3     
- Partials      487      489    +2     

Flag	Coverage Δ
go	80.55% <100.00%> (+<0.01%)	⬆️
wasm	65.12% <ø> (ø)

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.

[craymond12]

@craymond12 What do you think of always sending an ABORT?

In what case would a user want to Stop the SCTPTransport, but not want an Abort?

In the context of the PeerConnection.Close, you always want to abort to follow the webrtc standards
If someone Really want to play with things, he can still call the old Stop function

Honestly I do not know enough about the SCTP to say if someone want to close without the abort however
I only know that we need the abort in the peerConnection close

[Sean-Der]

👍

I think we should make ABORT the default behavior. If someone wants the old behavior they can file a ticket!

I think that is better then adding more API surface until someone asks!

[craymond12]

👍

I think we should make ABORT the default behavior. If someone wants the old behavior they can file a ticket!

I think that is better then adding more API surface until someone asks!

I agree, do you want me to just make the Sctp Stop private or remove it?

[Sean-Der]

My vote is remove it. I would move your code into Stop and then I say we merged it!

[craymond12]

@Sean-Der Done

[Sean-Der]

LGTM @craymond12!

Mind squashing them all into one commit. When CI passes I will merge it in :)

Also adding you to the Pion org. You can start branches etc... makes dev easier

[adriancable]

@craymond12 / @Sean-Der - once you've squashed the commits so it passes the metadata lint CI check, I'll draft a new release. I have a beta test group with about 600 WebRTC cameras including a good spread of different models that I will get this change out to, so I will very quickly know if it introduces any regressions.

[adriancable]

@craymond12 - you'll need to fix this for the lint CI check to pass:

Commit message check failed
Separate subject from body with a blank line

[craymond12]

@craymond12 - you'll need to fix this for the lint CI check to pass:

Commit message check failed Separate subject from body with a blank line

Yeah, the squash did not seems to have worked, trying to find out why i cant squash em

[Sean-Der]

I got it @craymond12 thanks for contributing this :) I can fix locally

[craymond12]

@Sean-Der thank you
working from a branch instead of a fork will be far better next time

[Sean-Der]

@adriancable v4.0.0-beta.30 was tagged