Repository: pion/ice
Pull request #706 (2024-07-01 16:38:07)

Cleanly close agent goroutines
This ensures no goroutines are running when an ICE agent closes. I'd also like to backport this into v2 so that webrtc/v3 can get it.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/ice/pull/706?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
Attention: Patch coverage is `30.55556%` with `25 lines` in your changes missing coverage. Please review.
> Project coverage is 22.23%. Comparing base [(`2a9fdb5`)](https://app.codecov.io/gh/pion/ice/commit/2a9fdb5c0dde845df6a5cb4709e619dbb6164786?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) to head [(`72c7732`)](https://app.codecov.io/gh/pion/ice/commit/72c7732f775140bf6137a0db7a080ca0d0b29431?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

| [Files](https://app.codecov.io/gh/pion/ice/pull/706?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Patch % | Lines |
|---|---|---|
| [agent\_handlers.go](https://app.codecov.io/gh/pion/ice/pull/706?src=pr&el=tree&filepath=agent_handlers.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YWdlbnRfaGFuZGxlcnMuZ28=) | 39.28% | [17 Missing :warning: ](https://app.codecov.io/gh/pion/ice/pull/706?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) |
| [agent.go](https://app.codecov.io/gh/pion/ice/pull/706?src=pr&el=tree&filepath=agent.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YWdlbnQuZ28=) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/pion/ice/pull/706?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) |

> :exclamation:  There is a different number of reports uploaded between BASE (2a9fdb5) and HEAD (72c7732). Click for more details.
> 
> <details><summary>HEAD has 2 uploads less than BASE</summary>
>
>| Flag | BASE (2a9fdb5) | HEAD (72c7732) |
>|------|------|------|
>|go|2|0|
></details>

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##           master     #706       +/-   ##
===========================================
- Coverage   79.19%   22.23%   -56.96%     
===========================================
  Files          41       40        -1     
  Lines        3739     3764       +25     
===========================================
- Hits         2961      837     -2124     
- Misses        547     2885     +2338     
+ Partials      231       42      -189     
```

| [Flag](https://app.codecov.io/gh/pion/ice/pull/706/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [go](https://app.codecov.io/gh/pion/ice/pull/706/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `?` | |
| [wasm](https://app.codecov.io/gh/pion/ice/pull/706/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `22.23% <30.55%> (+0.10%)` | :arrow_up: |

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.

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pion/ice/pull/706?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

davidzhao: @paulwe tagging you on this as it closely matches with your initial comments about not leaving behind rogue goroutines

Patch:
diff --git a/agent.go b/agent.go
index 761c07c..3412e10 100644
--- a/agent.go
+++ b/agent.go
@@ -220,9 +220,9 @@ func NewAgent(config *AgentConfig) (*Agent, error) { //nolint:gocognit
 
 		userBindingRequestHandler: config.BindingRequestHandler,
 	}
-	a.connectionStateNotifier = &handlerNotifier{connectionStateFunc: a.onConnectionStateChange}
-	a.candidateNotifier = &handlerNotifier{candidateFunc: a.onCandidate}
-	a.selectedCandidatePairNotifier = &handlerNotifier{candidatePairFunc: a.onSelectedCandidatePairChange}
+	a.connectionStateNotifier = &handlerNotifier{connectionStateFunc: a.onConnectionStateChange, done: make(chan struct{})}
+	a.candidateNotifier = &handlerNotifier{candidateFunc: a.onCandidate, done: make(chan struct{})}
+	a.selectedCandidatePairNotifier = &handlerNotifier{candidatePairFunc: a.onSelectedCandidatePairChange, done: make(chan struct{})}
 
 	if a.net == nil {
 		a.net, err = stdnet.NewNet()
@@ -849,7 +849,11 @@ func (a *Agent) removeUfragFromMux() {
 
 // Close cleans up the Agent
 func (a *Agent) Close() error {
-	return a.loop.Close()
+	err := a.loop.Close()
+	a.connectionStateNotifier.Close()
+	a.candidateNotifier.Close()
+	a.selectedCandidatePairNotifier.Close()
+	return err
 }
 
 // Remove all candidates. This closes any listening sockets
diff --git a/agent_handlers.go b/agent_handlers.go
index bb0c8d3..7ebfedd 100644
--- a/agent_handlers.go
+++ b/agent_handlers.go
@@ -45,7 +45,8 @@ func (a *Agent) onConnectionStateChange(s ConnectionState) {
 
 type handlerNotifier struct {
 	sync.Mutex
-	running bool
+	running   bool
+	notifiers sync.WaitGroup
 
 	connectionStates    []ConnectionState
 	connectionStateFunc func(ConnectionState)
@@ -55,13 +56,38 @@ type handlerNotifier struct {
 
 	selectedCandidatePairs []*CandidatePair
 	candidatePairFunc      func(*CandidatePair)
+
+	// State for closing
+	done chan struct{}
+}
+
+func (h *handlerNotifier) Close() {
+	h.Lock()
+
+	select {
+	case <-h.done:
+		h.Unlock()
+		return
+	default:
+	}
+	close(h.done)
+	h.Unlock()
+
+	h.notifiers.Wait()
 }
 
 func (h *handlerNotifier) EnqueueConnectionState(s ConnectionState) {
 	h.Lock()
 	defer h.Unlock()
 
+	select {
+	case <-h.done:
+		return
+	default:
+	}
+
 	notify := func() {
+		defer h.notifiers.Done()
 		for {
 			h.Lock()
 			if len(h.connectionStates) == 0 {
@@ -79,6 +105,7 @@ func (h *handlerNotifier) EnqueueConnectionState(s ConnectionState) {
 	h.connectionStates = append(h.connectionStates, s)
 	if !h.running {
 		h.running = true
+		h.notifiers.Add(1)
 		go notify()
 	}
 }
@@ -87,7 +114,14 @@ func (h *handlerNotifier) EnqueueCandidate(c Candidate) {
 	h.Lock()
 	defer h.Unlock()
 
+	select {
+	case <-h.done:
+		return
+	default:
+	}
+
 	notify := func() {
+		defer h.notifiers.Done()
 		for {
 			h.Lock()
 			if len(h.candidates) == 0 {
@@ -105,6 +139,7 @@ func (h *handlerNotifier) EnqueueCandidate(c Candidate) {
 	h.candidates = append(h.candidates, c)
 	if !h.running {
 		h.running = true
+		h.notifiers.Add(1)
 		go notify()
 	}
 }
@@ -113,7 +148,14 @@ func (h *handlerNotifier) EnqueueSelectedCandidatePair(p *CandidatePair) {
 	h.Lock()
 	defer h.Unlock()
 
+	select {
+	case <-h.done:
+		return
+	default:
+	}
+
 	notify := func() {
+		defer h.notifiers.Done()
 		for {
 			h.Lock()
 			if len(h.selectedCandidatePairs) == 0 {
@@ -131,6 +173,7 @@ func (h *handlerNotifier) EnqueueSelectedCandidatePair(p *CandidatePair) {
 	h.selectedCandidatePairs = append(h.selectedCandidatePairs, p)
 	if !h.running {
 		h.running = true
+		h.notifiers.Add(1)
 		go notify()
 	}
 }


Test patch:
diff --git a/agent_handlers_test.go b/agent_handlers_test.go
index 35680ee..ce02733 100644
--- a/agent_handlers_test.go
+++ b/agent_handlers_test.go
@@ -19,6 +19,7 @@ func TestConnectionStateNotifier(t *testing.T) {
 			connectionStateFunc: func(_ ConnectionState) {
 				updates <- struct{}{}
 			},
+			done: make(chan struct{}),
 		}
 		// Enqueue all updates upfront to ensure that it
 		// doesn't block
@@ -38,6 +39,7 @@ func TestConnectionStateNotifier(t *testing.T) {
 			close(done)
 		}()
 		<-done
+		c.Close()
 	})
 	t.Run("TestUpdateOrdering", func(t *testing.T) {
 		defer test.CheckRoutines(t)()
@@ -46,6 +48,7 @@ func TestConnectionStateNotifier(t *testing.T) {
 			connectionStateFunc: func(cs ConnectionState) {
 				updates <- cs
 			},
+			done: make(chan struct{}),
 		}
 		done := make(chan struct{})
 		go func() {
@@ -66,5 +69,6 @@ func TestConnectionStateNotifier(t *testing.T) {
 			c.EnqueueConnectionState(ConnectionState(i))
 		}
 		<-done
+		c.Close()
 	})
 }
diff --git a/agent_test.go b/agent_test.go
index 552d421..05dc19b 100644
--- a/agent_test.go
+++ b/agent_test.go
@@ -1379,11 +1379,12 @@ func TestCloseInConnectionStateCallback(t *testing.T) {
 
 	isClosed := make(chan interface{})
 	isConnected := make(chan interface{})
+	connectionStateConnectedSeen := make(chan interface{})
 	err = aAgent.OnConnectionStateChange(func(c ConnectionState) {
 		switch c {
 		case ConnectionStateConnected:
 			<-isConnected
-			require.NoError(t, aAgent.Close())
+			close(connectionStateConnectedSeen)
 		case ConnectionStateClosed:
 			close(isClosed)
 		default:
@@ -1393,6 +1394,8 @@ func TestCloseInConnectionStateCallback(t *testing.T) {
 
 	connect(aAgent, bAgent)
 	close(isConnected)
+	<-connectionStateConnectedSeen
+	require.NoError(t, aAgent.Close())
 
 	<-isClosed
 	require.NoError(t, bAgent.Close())