Repository: pion/webrtc
Pull request #1834 (2021-06-15 16:25:54)

Fixed deadlock in peerconnection.go
#### Description

In some rare cases during the ice connection stage change may result in deadlock. This fix makes iceConnectionState and connectionState atomic which should prevent deadlock.

Before this fix, some of the tests would randomly timeout, after this, I'm unable to reproduce the issue.
Also, I believe this is the same problem as described in https://github.com/pion/webrtc/issues/1809.

#### Reference issue
Fixes https://github.com/pion/webrtc/issues/1809


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/1834?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#1834](https://codecov.io/gh/pion/webrtc/pull/1834?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (4dd1b73) into [master](https://codecov.io/gh/pion/webrtc/commit/f7f760f8c6e2f1aba1bf7c544fdd84d70f6e05b6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (f7f760f) will **decrease** coverage by `4.33%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/1834/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/webrtc/pull/1834?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master    #1834      +/-   ##
==========================================
- Coverage   75.39%   71.05%   -4.34%     
==========================================
  Files          83       61      -22     
  Lines        5979     2277    -3702     
==========================================
- Hits         4508     1618    -2890     
+ Misses       1083      551     -532     
+ Partials      388      108     -280     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `71.05% <ø> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/1834?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [constants.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29uc3RhbnRzLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cnRwY29kZWMuZ28=) | `15.00% <0.00%> (-75.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-67.86%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `8.33% <0.00%> (-58.34%)` | :arrow_down: |
| [icemux.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlbXV4Lmdv) | `0.00% <0.00%> (-55.56%)` | :arrow_down: |
| [gathering\_complete\_promise.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Z2F0aGVyaW5nX2NvbXBsZXRlX3Byb21pc2UuZ28=) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `22.22% <0.00%> (-38.89%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-37.50%)` | :arrow_down: |
| ... and [31 more](https://codecov.io/gh/pion/webrtc/pull/1834/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/1834?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/webrtc/pull/1834?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [f7f760f...4dd1b73](https://codecov.io/gh/pion/webrtc/pull/1834?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


Patch:
diff --git a/peerconnection.go b/peerconnection.go
index e4050ee7..fbc334e9 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -45,8 +45,8 @@ type PeerConnection struct {
 	currentRemoteDescription *SessionDescription
 	pendingRemoteDescription *SessionDescription
 	signalingState           SignalingState
-	iceConnectionState       ICEConnectionState
-	connectionState          PeerConnectionState
+	iceConnectionState       atomic.Value // ICEConnectionState
+	connectionState          atomic.Value // PeerConnectionState
 
 	idpLoginURL *string
 
@@ -66,8 +66,8 @@ type PeerConnection struct {
 	rtpTransceivers []*RTPTransceiver
 
 	onSignalingStateChangeHandler     func(SignalingState)
-	onICEConnectionStateChangeHandler func(ICEConnectionState)
-	onConnectionStateChangeHandler    func(PeerConnectionState)
+	onICEConnectionStateChangeHandler atomic.Value // func(ICEConnectionState)
+	onConnectionStateChangeHandler    atomic.Value // func(PeerConnectionState)
 	onTrackHandler                    func(*TrackRemote, *RTPReceiver)
 	onDataChannelHandler              func(*DataChannel)
 	onNegotiationNeededHandler        atomic.Value // func()
@@ -128,12 +128,12 @@ func (api *API) NewPeerConnection(configuration Configuration) (*PeerConnection,
 		lastAnswer:             "",
 		greaterMid:             -1,
 		signalingState:         SignalingStateStable,
-		iceConnectionState:     ICEConnectionStateNew,
-		connectionState:        PeerConnectionStateNew,
 
 		api: api,
 		log: api.settingEngine.LoggerFactory.NewLogger("pc"),
 	}
+	pc.iceConnectionState.Store(ICEConnectionStateNew)
+	pc.connectionState.Store(PeerConnectionStateNew)
 
 	if !api.settingEngine.disableMediaEngineCopy {
 		pc.api = &API{
@@ -458,29 +458,21 @@ func (pc *PeerConnection) onTrack(t *TrackRemote, r *RTPReceiver) {
 // OnICEConnectionStateChange sets an event handler which is called
 // when an ICE connection state is changed.
 func (pc *PeerConnection) OnICEConnectionStateChange(f func(ICEConnectionState)) {
-	pc.mu.Lock()
-	defer pc.mu.Unlock()
-	pc.onICEConnectionStateChangeHandler = f
+	pc.onICEConnectionStateChangeHandler.Store(f)
 }
 
 func (pc *PeerConnection) onICEConnectionStateChange(cs ICEConnectionState) {
-	pc.mu.Lock()
-	pc.iceConnectionState = cs
-	handler := pc.onICEConnectionStateChangeHandler
-	pc.mu.Unlock()
-
+	pc.iceConnectionState.Store(cs)
 	pc.log.Infof("ICE connection state changed: %s", cs)
-	if handler != nil {
-		go handler(cs)
+	if handler := pc.onICEConnectionStateChangeHandler.Load(); handler != nil {
+		handler.(func(ICEConnectionState))(cs)
 	}
 }
 
 // OnConnectionStateChange sets an event handler which is called
 // when the PeerConnectionState has changed
 func (pc *PeerConnection) OnConnectionStateChange(f func(PeerConnectionState)) {
-	pc.mu.Lock()
-	defer pc.mu.Unlock()
-	pc.onConnectionStateChangeHandler = f
+	pc.onConnectionStateChangeHandler.Store(f)
 }
 
 // SetConfiguration updates the configuration of this PeerConnection object.
@@ -714,9 +706,6 @@ func (pc *PeerConnection) createICEGatherer() (*ICEGatherer, error) {
 // Update the PeerConnectionState given the state of relevant transports
 // https://www.w3.org/TR/webrtc/#rtcpeerconnectionstate-enum
 func (pc *PeerConnection) updateConnectionState(iceConnectionState ICEConnectionState, dtlsTransportState DTLSTransportState) {
-	pc.mu.Lock()
-	defer pc.mu.Unlock()
-
 	connectionState := PeerConnectionStateNew
 	switch {
 	// The RTCPeerConnection object's [[IsClosed]] slot is true.
@@ -743,15 +732,14 @@ func (pc *PeerConnection) updateConnectionState(iceConnectionState ICEConnection
 		connectionState = PeerConnectionStateConnecting
 	}
 
-	if pc.connectionState == connectionState {
+	if pc.connectionState.Load() == connectionState {
 		return
 	}
 
 	pc.log.Infof("peer connection state changed: %s", connectionState)
-	pc.connectionState = connectionState
-	handler := pc.onConnectionStateChangeHandler
-	if handler != nil {
-		go handler(connectionState)
+	pc.connectionState.Store(connectionState)
+	if handler := pc.onConnectionStateChangeHandler.Load(); handler != nil {
+		go handler.(func(PeerConnectionState))(connectionState)
 	}
 }
 
@@ -1534,10 +1522,7 @@ func (pc *PeerConnection) AddICECandidate(candidate ICECandidateInit) error {
 // ICEConnectionState returns the ICE connection state of the
 // PeerConnection instance.
 func (pc *PeerConnection) ICEConnectionState() ICEConnectionState {
-	pc.mu.RLock()
-	defer pc.mu.RUnlock()
-
-	return pc.iceConnectionState
+	return pc.iceConnectionState.Load().(ICEConnectionState)
 }
 
 // GetSenders returns the RTPSender that are currently attached to this PeerConnection
@@ -1959,10 +1944,7 @@ func (pc *PeerConnection) ICEGatheringState() ICEGatheringState {
 // ConnectionState attribute returns the connection state of the
 // PeerConnection instance.
 func (pc *PeerConnection) ConnectionState() PeerConnectionState {
-	pc.mu.Lock()
-	defer pc.mu.Unlock()
-
-	return pc.connectionState
+	return pc.connectionState.Load().(PeerConnectionState)
 }
 
 // GetStats return data providing statistics about the overall connection
diff --git a/pkg/media/samplebuilder/samplebuilder.go b/pkg/media/samplebuilder/samplebuilder.go
index 69f059f7..ac330396 100644
--- a/pkg/media/samplebuilder/samplebuilder.go
+++ b/pkg/media/samplebuilder/samplebuilder.go
@@ -300,6 +300,9 @@ func (s *SampleBuilder) Pop() *media.Sample {
 // no sample is compiled).
 func (s *SampleBuilder) PopWithTimestamp() (*media.Sample, uint32) {
 	sample := s.Pop()
+	if sample == nil {
+		return nil, 0
+	}
 	return sample, sample.PacketTimestamp
 }
 
diff --git a/rtpreceiver.go b/rtpreceiver.go
index 6c44ed6c..446e8ae9 100644
--- a/rtpreceiver.go
+++ b/rtpreceiver.go
@@ -114,7 +114,7 @@ func (r *RTPReceiver) Receive(parameters RTPReceiveParameters) error {
 			track: newTrackRemote(
 				r.kind,
 				parameters.Encodings[0].SSRC,
-				"",
+				parameters.Encodings[0].RID,
 				r,
 			),
 		}
diff --git a/rtpreceiver_go.go b/rtpreceiver_go.go
new file mode 100644
index 00000000..7f347e4b
--- /dev/null
+++ b/rtpreceiver_go.go
@@ -0,0 +1,32 @@
+// +build !js
+
+package webrtc
+
+import "github.com/pion/interceptor"
+
+// SetRTPParameters applies provided RTPParameters the RTPReceiver's tracks.
+//
+// This method is part of the ORTC API. It is not
+// meant to be used together with the basic WebRTC API.
+//
+// The amount of provided codecs must match the number of tracks on the receiver.
+func (r *RTPReceiver) SetRTPParameters(params RTPParameters) {
+	headerExtensions := make([]interceptor.RTPHeaderExtension, 0, len(params.HeaderExtensions))
+	for _, h := range params.HeaderExtensions {
+		headerExtensions = append(headerExtensions, interceptor.RTPHeaderExtension{ID: h.ID, URI: h.URI})
+	}
+
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
+	for ndx, codec := range params.Codecs {
+		currentTrack := r.tracks[ndx].track
+
+		r.tracks[ndx].streamInfo.RTPHeaderExtensions = headerExtensions
+
+		currentTrack.mu.Lock()
+		currentTrack.codec = codec
+		currentTrack.params = params
+		currentTrack.mu.Unlock()
+	}
+}


Test patch:
diff --git a/peerconnection_go_test.go b/peerconnection_go_test.go
index 1dd2d770..d1cd63c0 100644
--- a/peerconnection_go_test.go
+++ b/peerconnection_go_test.go
@@ -384,17 +384,17 @@ func TestPeerConnection_PropertyGetters(t *testing.T) {
 		currentRemoteDescription: &SessionDescription{},
 		pendingRemoteDescription: &SessionDescription{},
 		signalingState:           SignalingStateHaveLocalOffer,
-		iceConnectionState:       ICEConnectionStateChecking,
-		connectionState:          PeerConnectionStateConnecting,
 	}
+	pc.iceConnectionState.Store(ICEConnectionStateChecking)
+	pc.connectionState.Store(PeerConnectionStateConnecting)
 
 	assert.Equal(t, pc.currentLocalDescription, pc.CurrentLocalDescription(), "should match")
 	assert.Equal(t, pc.pendingLocalDescription, pc.PendingLocalDescription(), "should match")
 	assert.Equal(t, pc.currentRemoteDescription, pc.CurrentRemoteDescription(), "should match")
 	assert.Equal(t, pc.pendingRemoteDescription, pc.PendingRemoteDescription(), "should match")
 	assert.Equal(t, pc.signalingState, pc.SignalingState(), "should match")
-	assert.Equal(t, pc.iceConnectionState, pc.ICEConnectionState(), "should match")
-	assert.Equal(t, pc.connectionState, pc.ConnectionState(), "should match")
+	assert.Equal(t, pc.iceConnectionState.Load(), pc.ICEConnectionState(), "should match")
+	assert.Equal(t, pc.connectionState.Load(), pc.ConnectionState(), "should match")
 }
 
 func TestPeerConnection_AnswerWithoutOffer(t *testing.T) {
diff --git a/pkg/media/samplebuilder/samplebuilder_test.go b/pkg/media/samplebuilder/samplebuilder_test.go
index ea2498d1..a5a918a5 100644
--- a/pkg/media/samplebuilder/samplebuilder_test.go
+++ b/pkg/media/samplebuilder/samplebuilder_test.go
@@ -379,3 +379,12 @@ func TestSampleBuilderWithPacketReleaseHandler(t *testing.T) {
 		t.Errorf("Unexpected packet released by samples built")
 	}
 }
+
+func TestPopWithTimestamp(t *testing.T) {
+	t.Run("Crash on nil", func(t *testing.T) {
+		s := New(0, &fakeDepacketizer{}, 1)
+		sample, timestamp := s.PopWithTimestamp()
+		assert.Nil(t, sample)
+		assert.Equal(t, uint32(0), timestamp)
+	})
+}
diff --git a/rtpreceiver_go_test.go b/rtpreceiver_go_test.go
new file mode 100644
index 00000000..106cca2f
--- /dev/null
+++ b/rtpreceiver_go_test.go
@@ -0,0 +1,67 @@
+// +build !js
+
+package webrtc
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/pion/webrtc/v3/pkg/media"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSetRTPParameters(t *testing.T) {
+	sender, receiver, wan := createVNetPair(t)
+
+	outgoingTrack, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: "video/vp8"}, "video", "pion")
+	assert.NoError(t, err)
+
+	_, err = sender.AddTrack(outgoingTrack)
+	assert.NoError(t, err)
+
+	// Those parameters wouldn't make sense in a real application,
+	// but for the sake of the test we just need different values.
+	p := RTPParameters{
+		Codecs: []RTPCodecParameters{
+			{
+				RTPCodecCapability: RTPCodecCapability{MimeTypeOpus, 48000, 2, "minptime=10;useinbandfec=1", []RTCPFeedback{{"nack", ""}}},
+				PayloadType:        111,
+			},
+		},
+		HeaderExtensions: []RTPHeaderExtensionParameter{
+			{URI: "urn:ietf:params:rtp-hdrext:sdes:mid"},
+			{URI: "urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id"},
+			{URI: "urn:ietf:params:rtp-hdrext:sdes:repaired-rtp-stream-id"},
+		},
+	}
+
+	seenPacket, seenPacketCancel := context.WithCancel(context.Background())
+	receiver.OnTrack(func(trackRemote *TrackRemote, r *RTPReceiver) {
+		r.SetRTPParameters(p)
+
+		incomingTrackCodecs := r.Track().Codec()
+
+		assert.EqualValues(t, p.HeaderExtensions, r.Track().params.HeaderExtensions)
+
+		assert.EqualValues(t, p.Codecs[0].MimeType, incomingTrackCodecs.MimeType)
+		assert.EqualValues(t, p.Codecs[0].ClockRate, incomingTrackCodecs.ClockRate)
+		assert.EqualValues(t, p.Codecs[0].Channels, incomingTrackCodecs.Channels)
+		assert.EqualValues(t, p.Codecs[0].SDPFmtpLine, incomingTrackCodecs.SDPFmtpLine)
+		assert.EqualValues(t, p.Codecs[0].RTCPFeedback, incomingTrackCodecs.RTCPFeedback)
+		assert.EqualValues(t, p.Codecs[0].PayloadType, incomingTrackCodecs.PayloadType)
+
+		seenPacketCancel()
+	})
+
+	peerConnectionsConnected := untilConnectionState(PeerConnectionStateConnected, sender, receiver)
+
+	assert.NoError(t, signalPair(sender, receiver))
+
+	peerConnectionsConnected.Wait()
+	assert.NoError(t, outgoingTrack.WriteSample(media.Sample{Data: []byte{0xAA}, Duration: time.Second}))
+
+	<-seenPacket.Done()
+	assert.NoError(t, wan.Stop())
+	closePairNow(t, sender, receiver)
+}