fix: Close peer negotiate mutex if we haven't negotiated

kylecarbs · kylecarbs · commit c7ae085aabc1 · 2022-05-26T03:36:43.000Z
Closes #1706 and #1644.
diff --git a/peer/channel.go b/peer/channel.go
@@ -6,6 +6,7 @@ import (
 	"io"
 	"net"
 	"sync"
+	"time"
 
 	"github.com/pion/datachannel"
 	"github.com/pion/webrtc/v3"
@@ -244,6 +245,14 @@ func (c *Channel) Write(bytes []byte) (n int, err error) {
 	if c.dc.BufferedAmount()+uint64(len(bytes)) >= maxBufferedAmount {
 		<-c.sendMore
 	}
+
+	// There's an obvious race-condition here. This is an edge-case, as
+	// most-frequently data won't be pooled so synchronously, but is
+	// definitely possible.
+	//
+	// See: https://github.com/pion/sctp/issues/181
+	time.Sleep(time.Microsecond)
+
 	return c.rwc.Write(bytes)
 }
 
diff --git a/peer/conn.go b/peer/conn.go
@@ -125,7 +125,7 @@ type Conn struct {
 	remoteSessionDescriptionChannel chan webrtc.SessionDescription
 
 	negotiateMutex sync.Mutex
-	hasNegotiated  bool
+	hasNegotiated  atomic.Bool
 
 	loggerValue   atomic.Value
 	settingEngine webrtc.SettingEngine
@@ -290,10 +290,9 @@ func (c *Conn) negotiate() {
 	c.logger().Debug(context.Background(), "negotiating")
 	// ICE candidates cannot be added until SessionDescriptions have been
 	// exchanged between peers.
-	if c.hasNegotiated {
+	if c.hasNegotiated.Swap(true) {
 		c.negotiateMutex.Lock()
 	}
-	c.hasNegotiated = true
 	defer c.negotiateMutex.Unlock()
 
 	if c.offerer {
@@ -605,6 +604,13 @@ func (c *Conn) CloseWithError(err error) error {
 	// All logging, goroutines, and async functionality is cleaned up after this.
 	c.dcClosedWaitGroup.Wait()
 
+	// It's possible the connection can be closed before negotiation has
+	// began. In this case, we want to unlock the mutex to unblock any
+	// pending candidates being flushed.
+	if !c.hasNegotiated.Load() {
+		c.negotiateMutex.Unlock()
+	}
+
 	// Disable logging!
 	c.loggerValue.Store(slog.Logger{})
 	logger.Sync()
diff --git a/peer/conn_test.go b/peer/conn_test.go
@@ -59,9 +59,7 @@ func TestMain(m *testing.M) {
 }
 
 func TestConn(t *testing.T) {
-	t.Skip("known flake -- https://github.com/coder/coder/issues/1644")
 	t.Parallel()
-
 	t.Run("Ping", func(t *testing.T) {
 		t.Parallel()
 		client, server, _ := createPair(t)

Original file line number	Diff line number	Diff line change
`@@ -59,9 +59,7 @@ func TestMain(m *testing.M) {`
`59`	`59`	`}`
`60`	`60`
`61`	`61`	`func TestConn(t *testing.T) {`
`62`		`- t.Skip("known flake -- https://github.com/coder/coder/issues/1644")`
`63`	`62`	`t.Parallel()`
`64`		`-`
`65`	`63`	`t.Run("Ping", func(t *testing.T) {`
`66`	`64`	`t.Parallel()`
`67`	`65`	`client, server, _ := createPair(t)`