Stop old dialers when we get a new proxy config #1445
Conversation
dialer/fastconnect.go
Outdated
| outerLoop: | ||
| for { | ||
| select { | ||
| case <-fcd.stopCh: | ||
| log.Debug("Stopping parallel dialing") | ||
| return | ||
| default: | ||
| // Loop until we're connected | ||
| if len(fcd.connected.dialers) < 2 { | ||
| fcd.parallelDial(dialers) | ||
| // Add jitter to avoid thundering herd | ||
| time.Sleep(time.Duration(rand.Intn(4000)) * time.Millisecond) | ||
| } else { | ||
| break outerLoop | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This could be simplified to:
for {
// Loop until we're connected
if len(fcd.connected.dialers) < 2 {
fcd.parallelDial(dialers)
} else {
break
}
select {
case <-fcd.stopCh:
log.Debug("Stopping parallel dialing")
return
case time.After(time.Duration(rand.Intn(4000)) * time.Millisecond):
}
}
But your way works too!
There was a problem hiding this comment.
I like it! Will change.
| // New creates a new dialer that first tries to connect as quickly as possilbe while also | ||
| // optimizing for the fastest dialer. | ||
| func New(opts *Options) Dialer { | ||
| return NewTwoPhaseDialer(opts, func(opts *Options, existing Dialer) Dialer { | ||
| if currentDialer.Load() != nil { |
There was a problem hiding this comment.
Are we assuming that there will only ever be one place New is called? (ignoring tests)
There was a problem hiding this comment.
It's only called when we get new configs
| // We don't call Stop on the Dialers themselves here because they are likely | ||
| // in use by other Dialers, such as the BanditDialer. | ||
| // Stop all dialing | ||
| fcd.stopCh <- struct{}{} |
There was a problem hiding this comment.
I would make stopCh buffered or just close it so this doesn't block until outerLoop tries to read from it again.
dialer/fastconnect.go
Outdated
| time.Sleep(time.Duration(rand.Intn(4000)) * time.Millisecond) | ||
| } else { | ||
| break | ||
| } | ||
| select { | ||
| case <-fcd.stopCh: | ||
| log.Debug("Stopping parallel dialing") | ||
| return | ||
| default: | ||
| // Loop until we're connected | ||
| if len(fcd.connected.dialers) < 2 { | ||
| fcd.parallelDial(dialers) | ||
| // Add jitter to avoid thundering herd | ||
| time.Sleep(time.Duration(rand.Intn(4000)) * time.Millisecond) | ||
| } else { | ||
| break outerLoop | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
It's not big deal since the goroutine running connectAll would only hang around for a few seconds, at most, but, if you add another case and use time.After instead of time.Sleep here, the goroutine will exit as soon as it's canceled and be cleaned up. Just FYI.
There was a problem hiding this comment.
Ah good point -- fixed!
@garmr-ulfr identified a bug in our dialer whereby we keep dialing old dialers when we get a new proxy config. This stops 'em.