Repository: pion/interceptor (go)

Issue:
[SendSideBWE ] `delayController` raises panic when packet arrives after interceptor is closed 
<!--
  Come join our community on Slack https://pion.ly/slack for quicker support and more resources

  We also provide a Knowledge Base https://pion.ly/knowledge-base that covers Pion and general WebRTC
  
  We maintain a FAQ of common issues and questions https://pion.ly/knowledge-base/pion-basics/faq/

 Please answer these questions before submitting your issue. Thanks!
-->

### Your environment.
  - Version: `github.com/pion/interceptor v0.1.11-0.20220401022307-09051cdde82b`
  - Other Information - 
 
Panic stack trace:
```
runtime.gopanic
        /usr/local/go/src/runtime/panic.go:965
runtime.chansend
        /usr/local/go/src/runtime/chan.go:281
runtime.chansend1
        /usr/local/go/src/runtime/chan.go:143
github.com/pion/interceptor/pkg/gcc.(*delayController).updateDelayEstimate
        /go/pkg/mod/github.com/pion/interceptor@v0.1.11-0.20220401022307-09051cdde82b/pkg/gcc/delay_based_bwe.go:96
github.com/pion/interceptor/pkg/gcc.(*SendSideBWE).WriteRTCP
        /go/pkg/mod/github.com/pion/interceptor@v0.1.11-0.20220401022307-09051cdde82b/pkg/gcc/send_side_bwe.go:141
github.com/100mslive/ion-sfu/pkg/sfu.(*TrackAllocator).OnTransportCCFeedback
        /go/pkg/mod/github.com/100mslive/ion-sfu@v1.9.9-0.20220510151633-ae293ba3929d/pkg/sfu/track_allocator.go:459
github.com/100mslive/ion-sfu/pkg/sfu.(*DownTrack).handleRTCP
        /go/pkg/mod/github.com/100mslive/ion-sfu@v1.9.9-0.20220510151633-ae293ba3929d/pkg/sfu/downtrack.go:985
github.com/100mslive/ion-sfu/pkg/sfu.(*DownTrack).Bind.func1
        /go/pkg/mod/github.com/100mslive/ion-sfu@v1.9.9-0.20220510151633-ae293ba3929d/pkg/sfu/downtrack.go:273
github.com/100mslive/ion-sfu/pkg/buffer.(*RTCPReader).Write
        /go/pkg/mod/github.com/100mslive/ion-sfu@v1.9.9-0.20220510151633-ae293ba3929d/pkg/buffer/rtcpreader.go:25
github.com/pion/srtp/v2.(*ReadStreamSRTCP).write
        /go/pkg/mod/github.com/pion/srtp/v2@v2.0.5/stream_srtcp.go:30
github.com/pion/srtp/v2.(*SessionSRTCP).decrypt
        /go/pkg/mod/github.com/pion/srtp/v2@v2.0.5/session_srtcp.go:173
github.com/pion/srtp/v2.(*session).start.func1
        /go/pkg/mod/github.com/pion/srtp/v2@v2.0.5/session.go:141

```
 
### What did you do?
<!--
If possible, provide an example for reproducing the error.
A complete runnable program is good.
-->

### What did you expect?
No panics

### What happened?
There's a race between RTCP packets written to `SendSideBWE` and interceptor being closed due to which any packets written after the interceptor is closed raised the panic `send on closed channel`. This panic happens very rarely (1 out of 200 `Peer`) which makes it pretty hard to reproduce in local env. It happens at the same time when ice state goes into `closed` state and respective peer-connection is closed 

Would ignoring any new packets after the interceptor is closed be a good fix? @Sean-Der 

Pull request:
fix(ss-bwe): `WriteRTCP` raise panic after `Close`
#### Description
Return an error when `WriteRTCP` is called after the `SendSideBWE` is closed

#### Reference issue
Fixes #131 


Patch:
diff --git a/pkg/gcc/send_side_bwe.go b/pkg/gcc/send_side_bwe.go
index ccf9ff1..eb4caae 100644
--- a/pkg/gcc/send_side_bwe.go
+++ b/pkg/gcc/send_side_bwe.go
@@ -1,6 +1,7 @@
 package gcc
 
 import (
+	"errors"
 	"sync"
 	"time"
 
@@ -17,6 +18,9 @@ const (
 	maxBitrate     = 50_000_000
 )
 
+// ErrSendSideBWEClosed is raised when SendSideBWE.WriteRTCP is called after SendSideBWE.Close
+var ErrSendSideBWEClosed = errors.New("SendSideBwe closed")
+
 // Pacer is the interface implemented by packet pacers
 type Pacer interface {
 	interceptor.RTPWriter
@@ -46,7 +50,8 @@ type SendSideBWE struct {
 	minBitrate    int
 	maxBitrate    int
 
-	close chan struct{}
+	close     chan struct{}
+	closeLock sync.RWMutex
 }
 
 // Option configures a bandwidth estimator
@@ -131,6 +136,13 @@ func (e *SendSideBWE) AddStream(info *interceptor.StreamInfo, writer interceptor
 
 // WriteRTCP adds some RTCP feedback to the bandwidth estimator
 func (e *SendSideBWE) WriteRTCP(pkts []rtcp.Packet, attributes interceptor.Attributes) error {
+	e.closeLock.RLock()
+	defer e.closeLock.RUnlock()
+
+	if e.isClosed() {
+		return ErrSendSideBWEClosed
+	}
+
 	for _, pkt := range pkts {
 		if fb, ok := pkt.(*rtcp.TransportLayerCC); ok {
 			acks, err := e.feedbackAdapter.OnTransportCCFeedback(time.Now(), fb)
@@ -176,8 +188,21 @@ func (e *SendSideBWE) OnTargetBitrateChange(f func(bitrate int)) {
 	e.onTargetBitrateChange = f
 }
 
+// isClosed returns true if SendSideBWE is closed
+func (e *SendSideBWE) isClosed() bool {
+	select {
+	case <-e.close:
+		return true
+	default:
+		return false
+	}
+}
+
 // Close stops and closes the bandwidth estimator
 func (e *SendSideBWE) Close() error {
+	e.closeLock.Lock()
+	defer e.closeLock.Unlock()
+
 	if err := e.delayController.Close(); err != nil {
 		return err
 	}


Test patch:
diff --git a/pkg/gcc/send_side_bwe_test.go b/pkg/gcc/send_side_bwe_test.go
index d8a0a04..39ce24c 100644
--- a/pkg/gcc/send_side_bwe_test.go
+++ b/pkg/gcc/send_side_bwe_test.go
@@ -90,3 +90,16 @@ func TestSendSideBWE(t *testing.T) {
 	// Sending a stream with zero loss and no RTT should increase estimate
 	require.Less(t, latestBitrate, bwe.GetTargetBitrate())
 }
+
+func TestSendSideBWE_ErrorOnWriteRTCPAtClosedState(t *testing.T) {
+	bwe, err := NewSendSideBWE()
+	require.NoError(t, err)
+	require.NotNil(t, bwe)
+
+	pkts := []rtcp.Packet{&rtcp.TransportLayerCC{}}
+	require.NoError(t, bwe.WriteRTCP(pkts, nil))
+	require.Equal(t, bwe.isClosed(), false)
+	require.NoError(t, bwe.Close())
+	require.ErrorIs(t, bwe.WriteRTCP(pkts, nil), ErrSendSideBWEClosed)
+	require.Equal(t, bwe.isClosed(), true)
+}