haproxy-consul-connect icon indicating copy to clipboard operation
haproxy-consul-connect copied to clipboard

Watcher data race

Open banks opened this issue 5 years ago • 0 comments

https://github.com/haproxytech/haproxy-consul-connect/blob/master/consul/watcher.go

I see a couple of issues in the watcher:

  • removeUpstream is setting a done bool under lock but the startUpstream loop reads that bool while not holding the lock which is a data race.
  • there is no way to close/stop watcher and free goroutines. This is not a big deal in normal usage but our lesson from Consul's TestAgent is that this pattern leads to really inefficient test runs with tons of orphan goroutines by the end etc. I'd recommend plumbing a Stop mechanism or Context through every goroutine spawned right from the start so all resources can be cleaned up in tests and in case you ever need to support managing multiple proxies or decoupling process lifetime and proxy lifetime etc. It's way easier if it's just there already!

banks avatar Mar 03 '20 20:03 banks