-
Notifications
You must be signed in to change notification settings - Fork 88
Implement SCTP Negotiation Acceleration Protocol (SNAP) #449
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #449 +/- ##
==========================================
- Coverage 83.79% 83.69% -0.11%
==========================================
Files 52 52
Lines 4054 4108 +54
==========================================
+ Hits 3397 3438 +41
- Misses 481 488 +7
- Partials 176 182 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This commit adds a createAssociationWithOutOfBandTokens method that creates the SCTP association with a local and remote sctp-init token as described in draft-hancke-tsvwg-snap-00. Tokens can be generated using the newly added GenerateOutOfBandToken method and set in the sctp.Config as LocalSctpInit and RemoteSctpInit. This allows the peer connection to generate and negotiate the sctp-init attribute in the SDP which skips two network round trips.
| LocalSctpInit []byte | ||
| RemoteSctpInit []byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break the compatibility checks, but we plan to deprecate the config struct this week for the favor of options pattern so this could be WithSCTPInit when we update or something!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall this feels like we should separate options (configured by the API) from negotiated parameters (maxMessageSize, remote sctpPort, sctpInit). Still a breaking change but if we need one anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
association.go
Outdated
| assoc.setRWND(min32(localInit.advertisedReceiverWindowCredit, remoteInit.advertisedReceiverWindowCredit)) | ||
| assoc.peerVerificationTag = remoteInit.initiateTag | ||
| assoc.sourcePort = defaultSCTPSrcDstPort | ||
| assoc.destinationPort = defaultSCTPSrcDstPort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a=sctp-port is currently ignored even from the remote SDP. Surprisingly with SNAP I don't think it matters anymore since it only shows up in one of the acks (so SCTP can detect NATs) but that is not relevant to WebRTC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(added to SctpOptions which makes this fix-able in the future)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're currently updating this library to use Pion's options patterns, SctpOptions will not be needed after this is merged #450 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to SctpParameters which is more in line with terminology like RTCIceParameters and RTCDtlsParameters
association.go
Outdated
| if len(config.RemoteSctpInit) != 0 && len(config.LocalSctpInit) != 0 { | ||
| // SNAP, aka sctp-init in the SDP. | ||
| remote := &chunkInit{} | ||
| err := remote.unmarshal(config.RemoteSctpInit) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| local := &chunkInit{} | ||
| err = local.unmarshal(config.LocalSctpInit) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| assoc = createAssociationWithOutOfBandTokens(config, local, remote) | ||
| assoc.lock.Lock() | ||
| assoc.setState(established) | ||
| defer assoc.lock.Unlock() | ||
|
|
||
| go assoc.readLoop() | ||
| go assoc.writeLoop() | ||
|
|
||
| return assoc, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic should be done in createAssociation and we prob should have one path to createAssociationWithTSN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't like that part either. Problem is that createAssociationWithOutOfBandTokens does too many tasks:
- Validate the sctp-init's are ok
- creating the association
- setting some parameters which is usually in
handleInit - updating the state (could probably move into createAssociationWithOutOfBandTokens and no lock required)
- run the loop (normally done in init)
The errors handled below this are basically the handshake being interrupted which can no longer happen.
🤔 but that means createAssociationWithOutOfBandTokens does things I don't want "createAssociation" to do... brb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly better. WDYT about renaming init() to sendInit() and only do as client, pulling the loop goroutines out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot better yeah.
association.go
Outdated
| init.initialTSN = globalMathRandomGenerator.Uint32() | ||
| init.numOutboundStreams = math.MaxUint16 | ||
| init.numInboundStreams = math.MaxUint16 | ||
| init.initiateTag = globalMathRandomGenerator.Uint32() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can return 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, also happens in existing code? Yay bugs that only happens once every 2^32 - 1 times... will update in both places.
| return assoc, nil | ||
| } | ||
| assoc = createAssociation(config) | ||
| assoc.init(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the previous comment but we skip assoc.init. So it's better overall if we try to make a single path for initialization. all of these APIs are private so feel free to refactor them.
b234227 to
a41c9fd
Compare
This commit adds a createAssociationWithOutOfBandTokens method that creates the SCTP association with a local and remote sctp-init token as described in draft-hancke-tsvwg-snap-00.
Tokens can be generated using the newly added GenerateOutOfBandToken method and set in the sctp.Config as LocalSctpInit and RemoteSctpInit.
This allows the peer connection to generate and negotiate the sctp-init in the SDP which skips two network round trips.
pion/WebRTC changes are in pion/webrtc#3327