set close state before unregisterStream

[edaniels]

Although OpenStream checks state before creating a stream, readLoop's unregisterStream can be called outside of a direct Close. When this happens, setState(closed) happens after unregisterStream which is an incorrect ordering causing an OpenStream to proceed. readLoop should set the closed state prior to unregistering streams.

Repro with carefully placed sleeps to mimic an internal close of connections being closed:

diff --git a/association.go b/association.go
index 29e9978..2cf2539 100644
--- a/association.go
+++ b/association.go
@@ -578,6 +578,7 @@ func (a *Association) readLoop() {
 			a.unregisterStream(s, closeErr)
 		}
 		a.lock.Unlock()
+		println("unregged")
 		close(a.acceptCh)
 		close(a.readLoopCloseCh)
 
@@ -650,6 +651,8 @@ loop:
 		}
 	}
 
+	time.Sleep(3 * time.Second)
+	println("now closed")
 	a.setState(closed)
 	a.closeAllTimers()
 }
@@ -1504,13 +1507,18 @@ func (a *Association) getMyReceiverWindowCredit() uint32 {
 
 // OpenStream opens a stream
 func (a *Association) OpenStream(streamIdentifier uint16, defaultPayloadType PayloadProtocolIdentifier) (*Stream, error) {
+	time.Sleep(time.Second)
 	a.lock.Lock()
 	defer a.lock.Unlock()
 
+	println("adding")
+
 	switch a.getState() {
 	case shutdownAckSent, shutdownPending, shutdownReceived, shutdownSent, closed:
+		println("no")
 		return nil, ErrAssociationClosed
 	}
+	println("yes")
 
 	return a.getOrCreateStream(streamIdentifier, false, defaultPayloadType), nil
 }

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.18%. Comparing base (fae4852) to head (de3c406).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #350   +/-   ##
=======================================
  Coverage   81.17%   81.18%           
=======================================
  Files          51       51           
  Lines        3347     3348    +1     
=======================================
+ Hits         2717     2718    +1     
  Misses        484      484           
  Partials      146      146           

Flag	Coverage Δ
go	81.18% <100.00%> (+<0.01%)	⬆️
wasm	67.32% <100.00%> (+<0.01%)	⬆️

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.

[sukunrt]

readLoop's unregisterStream can be called outside of a direct Close

When does this happen?

[edaniels]

readLoop's unregisterStream can be called outside of a direct Close

When does this happen?

In the particular real case I noticed, a DTLS CloseNotify was received which closes the DTLS Conn decryption channel which sets all Reads to be EOFs. That is then received by the SCTP Assocation's readLoop and it unrolls from there.

[sukunrt]

I think this change is fine. I believe the case you're mentioning happens when we close a webrtc connection and pion/webrtc closes the peerconnection by closing the DTLS connection and doesn't send a SCTP Abort chunk as the specs recommend:

6. If connection.[[SctpTransport]] is not null, tear down the underlying SCTP association by sending an SCTP ABORT chunk and set the [[SctpTransportState]] to "closed"

https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close

[edaniels]

I think this change is fine. I believe the case you're mentioning happens when we close a webrtc connection and pion/webrtc closes the peerconnection by closing the DTLS connection and doesn't send a SCTP Abort chunk as the specs recommend:

6. If connection.[[SctpTransport]] is not null, tear down the underlying SCTP association by sending an SCTP ABORT chunk and set the [[SctpTransportState]] to "closed"

https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close

Wrote my reply too soon :). Yes, you're correct. I think due to DTLS being unordered, there's a chance we can still get that notification even if the SCTP abort is sent. There's also the case of bad implementations cause this race to happen as well.