Skip to content

Commit

Permalink
Fix gathering candidates race condition
Browse files Browse the repository at this point in the history
Canceling previous gathering does not happen synchronously. If it takes
a little bit longer, it can close newly created `gatherCandidateDone`
channel. If we pass channel to the `gatherCandidates` function, it will
always close its done channel and only the latest done channel will be
available in `gatherCandidateDone`.
  • Loading branch information
m1k1o authored and Sean-Der committed Mar 28, 2023
1 parent be72bc7 commit cf6f758
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 4 deletions.
9 changes: 5 additions & 4 deletions gather.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,18 @@ func (a *Agent) GatherCandidates() error {
a.gatherCandidateCancel() // Cancel previous gathering routine
ctx, cancel := context.WithCancel(ctx)
a.gatherCandidateCancel = cancel
a.gatherCandidateDone = make(chan struct{})
done := make(chan struct{})
a.gatherCandidateDone = done

go a.gatherCandidates(ctx)
go a.gatherCandidates(ctx, done)
}); runErr != nil {
return runErr
}
return gatherErr
}

func (a *Agent) gatherCandidates(ctx context.Context) {
defer close(a.gatherCandidateDone)
func (a *Agent) gatherCandidates(ctx context.Context, done chan struct{}) {
defer close(done)
if err := a.setGatheringState(GatheringStateGathering); err != nil { //nolint:contextcheck
a.log.Warnf("failed to set gatheringState to GatheringStateGathering: %v", err)
return
Expand Down
28 changes: 28 additions & 0 deletions gather_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,34 @@ func TestListenUDP(t *testing.T) {
assert.NoError(t, a.Close())
}

func TestGatherConcurrency(t *testing.T) {
report := test.CheckRoutines(t)
defer report()

lim := test.TimeOut(time.Second * 30)
defer lim.Stop()

a, err := NewAgent(&AgentConfig{
NetworkTypes: []NetworkType{NetworkTypeUDP4, NetworkTypeUDP6},
IncludeLoopback: true,
})
assert.NoError(t, err)

candidateGathered, candidateGatheredFunc := context.WithCancel(context.Background())
assert.NoError(t, a.OnCandidate(func(c Candidate) {
candidateGatheredFunc()
}))

// tesing for panic
for i := 0; i < 10; i++ {
_ = a.GatherCandidates()
}

<-candidateGathered.Done()

assert.NoError(t, a.Close())
}

func TestLoopbackCandidate(t *testing.T) {
report := test.CheckRoutines(t)
defer report()
Expand Down

0 comments on commit cf6f758

Please sign in to comment.