Repository: pion/webrtc (go)

Issue:
DataChannel negotiated peer connection's server.AddTrack doesn't always trigger client.OnTrack 
### NOTE:  This is my first issue/contribution to Pion. I've done my best to follow the contribution guidelines but if I've missed the mark please let me know.  

### Your environment.
  - 2 webrtc peers using pion/webrtc/[v3.2.40](https://github.com/pion/webrtc/releases/tag/v3.2.40)

### What did you do?
1. Established a PeerConnection negotiated over a data channel between a client (recvonly) and a server (sendonly)
2. Once `pc.OnOpen` is called on both the client & server, the server creates a `NewTrackLocalStaticSample` sending h264, passes it to `pc.AddTrack`, and starts calling `trackLocal.WriteSample`
3. Client waits for `pc.OnTrack` to be invoked so it can start receiving the `RTP` packets written by server

Repro test case: https://github.com/pion/webrtc/pull/2775

### What did you expect?
The Client's `pc.OnTrack` callback function is always invoked

### What happened?
Sometimes the client's `pc.OnTrack` callback function is not invoked

### Additional Notes:
I think the problem might be in peerconnection.go 's `onNegotiationNeeded` state machine which changes states even if the `onNegotiationNeededHandler` is not installed.  If I uncomment [these lines](https://github.com/pion/webrtc/pull/2775/files#diff-c94e5058864f96b58c5e039ac0485b3a9640613f32a9e31bba905dc3f8438e7aR298-R300) the repro test case doesn't fail.

Pull request:
Reset state machine after negotiationNeededOp
- Fixes #2774

Prior to this change, when `OnNegotiationNeeded` was not set before negotiation needed causing events were added to the op queue, there's a chance (based on ordering of goroutines) that the `negotiationNeededState` will get stuck in `negotiationNeededStateQueue`. This is kind of a consequence of breaking the canon, but this fix and test appear to be fine. 


Patch:
diff --git a/peerconnection.go b/peerconnection.go
index 69489058..ef370664 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -291,6 +291,16 @@ func (pc *PeerConnection) onNegotiationNeeded() {
 }
 
 func (pc *PeerConnection) negotiationNeededOp() {
+	// non-canon, reset needed state machine and run again if there was a request
+	defer func() {
+		pc.mu.Lock()
+		defer pc.mu.Unlock()
+		if pc.negotiationNeededState == negotiationNeededStateQueue {
+			defer pc.onNegotiationNeeded()
+		}
+		pc.negotiationNeededState = negotiationNeededStateEmpty
+	}()
+
 	// Don't run NegotiatedNeeded checks if OnNegotiationNeeded is not set
 	if handler, ok := pc.onNegotiationNeededHandler.Load().(func()); !ok || handler == nil {
 		return
@@ -307,16 +317,6 @@ func (pc *PeerConnection) negotiationNeededOp() {
 		return
 	}
 
-	// non-canon, run again if there was a request
-	defer func() {
-		pc.mu.Lock()
-		defer pc.mu.Unlock()
-		if pc.negotiationNeededState == negotiationNeededStateQueue {
-			defer pc.onNegotiationNeeded()
-		}
-		pc.negotiationNeededState = negotiationNeededStateEmpty
-	}()
-
 	// Step 2.3
 	if pc.SignalingState() != SignalingStateStable {
 		return


Test patch:
diff --git a/peerconnection_go_test.go b/peerconnection_go_test.go
index e36c62f7..2dde3069 100644
--- a/peerconnection_go_test.go
+++ b/peerconnection_go_test.go
@@ -1606,3 +1606,41 @@ func TestPeerConnectionState(t *testing.T) {
 	assert.NoError(t, pc.Close())
 	assert.Equal(t, PeerConnectionStateClosed, pc.ConnectionState())
 }
+
+// See https://github.com/pion/webrtc/issues/2774
+func TestNegotiationNeededAddedAfterOpQueueDone(t *testing.T) {
+	lim := test.TimeOut(time.Second * 30)
+	defer lim.Stop()
+
+	report := test.CheckRoutines(t)
+	defer report()
+
+	pc, err := NewPeerConnection(Configuration{})
+	if err != nil {
+		t.Error(err.Error())
+	}
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+
+	_, err = pc.CreateDataChannel("initial_data_channel", nil)
+	assert.NoError(t, err)
+
+	// after there are no ops left in the queue, a previously faulty version
+	// of negotiationNeededOp would keep the negotiation needed state in
+	// negotiationNeededStateQueue which will cause all subsequent
+	// onNegotiationNeeded calls to never queue again, only if
+	// OnNegotiationNeeded has not been set yet.
+	for !pc.ops.IsEmpty() {
+		time.Sleep(time.Millisecond)
+	}
+
+	pc.OnNegotiationNeeded(wg.Done)
+
+	_, err = pc.CreateDataChannel("another_data_channel", nil)
+	assert.NoError(t, err)
+
+	wg.Wait()
+
+	assert.NoError(t, pc.Close())
+}