overlord/standby: fix a race between standby goroutine and stop

[bboozzoo]

When standby.Stop() is called before the goroutine enters the select {} block,
m.stopCh will be nil. Due to short circuit evaluation in select, the stopCh case
will never be evaluated and we will always wait for the timer to expire instead
of exiting right away.

We should probably use tomb and make Stop() wait for the goroutine to exit.

[zyga]

Thank you for the patch and for the explanation. Tricky stuff

[chipaca]

I think this is the wrong approach. As it's late, I'll work on this in a while and if i get anywhere I'll push what I think might be the right approach, and if I'm wrong we can revert in the morn.

[bboozzoo]

I think we should sync with the opinionator being called and make sure that Stop actually completes. Otherwise this feels like the issue I tried to fix in #6041. I tried the diff below locally, what do you think?

diff --git a/overlord/standby/standby_test.go b/overlord/standby/standby_test.go
index 5427814cf..a669a9dbd 100644
--- a/overlord/standby/standby_test.go
+++ b/overlord/standby/standby_test.go
@@ -153,22 +153,44 @@ func (s *standbySuite) TestStartChecks(c *C) {
 
 func (s *standbySuite) TestStopWaits(c *C) {
        defer standby.MockStandbyWait(time.Millisecond)()
-       defer standby.MockStateRequestRestart(func(*state.State, state.RestartType) {})()
+       defer standby.MockStateRequestRestart(func(*state.State, state.RestartType) {
+               c.Fatal("request restart should have not been called")
+       })()
 
        ch := make(chan struct{})
+       opineReady := make(chan struct{})
+       opineGo := make(chan struct{})
        m := standby.New(s.state)
+       synced := false
        m.AddOpinion(opine(func() bool {
+               if !synced {
+                       // synchronize with the main goroutine only at the
+                       // beginning
+                       opineReady <- struct{}{}
+                       <-opineGo
+                       synced = true
+               }
                time.Sleep(200 * time.Millisecond)
                return false
        }))
 
        m.Start()
 
-       // wait enough so the actual Opinionator is called
-       time.Sleep(10 * time.Millisecond)
+       <-opineReady
 
        go func() {
-               m.Stop()
+               stopDone := make(chan struct{})
+
+               // let the opinionator start its delay
+               opineGo <- struct{}{}
+               go func() {
+                       // this will block until standby stops
+                       m.Stop()
+                       close(stopDone)
+               }()
+
+               <-stopDone
+
                close(ch)
        }()
 
@@ -178,4 +200,12 @@ func (s *standbySuite) TestStopWaits(c *C) {
        case <-ch:
                c.Fatal("stop should have blocked and didn't")
        }
+
+       // wait for Stop to complete now
+       select {
+       case <-ch:
+               // nothing to do here
+       case <-time.After(10 * time.Second):
+               c.Fatal("stop did not complete")
+       }
 }

[bboozzoo]

@chipaca thank you for the changes! Looks much better than what I proposed.

[chipaca]

quickly +1'ing it before we do finer finer finer tuing

i kinda had my feet in this too much to +1 for real

[stolowski]

LGTM

[stolowski]

The above logic looks good.

[stolowski]

This logic looks fine.