MEDIUM: watcher: fix race condition & plumbing stop for test

[amelhusic]

No description provided.

[amelhusic]

this fix is related to: #10

[ShimmerGlass]

This will not stop the goroutines from running once the Watcher is stopped.
One way to do that is to add a check in the infinite loops to stop them when the Watcher is stopped:

for {
  select {
    case <-w.shutdownCh:
      return
    default:
  }
  
  // work
}

Additionally, I'd prefer exposing a Stop() method rather than the chan directly.

[amelhusic]

I applied your recommendations.

[ShimmerGlass]

The return here will only return from this function, not the caller, this will not stop the for loops.
You need to put the select directly in the loop from the caller to have the effect you want.
Alternatively, if the goal here is to avoid repeating this select statement over and over, it could be rewritten as :

func (w *Watcher) isStopped() bool {
	select {
	case <-w.shutdownCh:
		return true
        default:
                return false
        }
}

And is the for loop :

for {
    if w.isStopped() {
        return
    }

    // work
}

[amelhusic]

The return here will only return from this function, not the caller, this will not stop the for loops.

You are correct about this and I updated this part.

However, placing isStopped func at the top of for loop requires using a time.Sleep in order to execute these routines, just above this line: 4a88643#diff-df428c63426402bd3667b15209ed395fR73 Otherwise, test is hanging forever.

On the other hand, placing isStopped func at the bottom of for loops works and code in these goroutines is executed at least once before exit. Also, there is no need for time.Sleep.
What do you think about this?

[ShimmerGlass]

If there are errors from consul w.isStopped() will never be called because there are continue statements on error. Having them at the top ensure the check is done.
The time.Sleep() issue can be fixed by defering watcher.Stop()

[amelhusic]

Hi @ShimmerGlass

Sorry for delay. I think w.isStopped() should be at the end of each for loop, because watcher.Run() internally spawns these goroutines and it uses sync.WaitGroup: https://github.com/haproxytech/haproxy-consul-connect/blob/master/consul/watcher.go#L108. In order for watcher.Run() to complete, we need to ensure that each of these functions have called w.ready.Done(). So, watcher.Stop() can only work if watcher.Run() is completed.

Placing w.isStopped() at the top of for loops will immediately return from there. So, w.ready.Done() will never execute and test will never pass.

If error occurs there is a timeout, errorWaitTime configured for 5s, so before test reach its timeout(30s) there would be 6 goroutines per functions, which should not be a problem.

[aiharos]

Please recheck and give a thumbs up for mergeing.