From 82be35a797efa3a811e84a06ef5d4694880edd8b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 30 Aug 2016 16:22:21 -0400 Subject: [PATCH 1/2] Fix races in WaitForState The WaitForState method can't read the result values in a timeout because they are still owned by the running goroutine. Keep all values scoped inside the goroutine, and save them into an atomic.Value to be returned. Fixes race introduced in #8510 --- helper/resource/state.go | 48 ++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/helper/resource/state.go b/helper/resource/state.go index e9bd26a92..5efc495b6 100644 --- a/helper/resource/state.go +++ b/helper/resource/state.go @@ -2,6 +2,7 @@ package resource import ( "log" + "sync/atomic" "time" ) @@ -61,9 +62,15 @@ func (conf *StateChangeConf) WaitForState() (interface{}, error) { conf.ContinuousTargetOccurence = 1 } - var result interface{} - var resulterr error - var currentState string + // We can't safely read the result values if we timeout, so store them in + // an atomic.Value + type Result struct { + Result interface{} + State string + Error error + } + var lastResult atomic.Value + lastResult.Store(Result{}) doneCh := make(chan struct{}) go func() { @@ -74,7 +81,6 @@ func (conf *StateChangeConf) WaitForState() (interface{}, error) { wait := 100 * time.Millisecond - var err error for first := true; ; first = false { if !first { // If a poll interval has been specified, choose that interval. @@ -99,14 +105,20 @@ func (conf *StateChangeConf) WaitForState() (interface{}, error) { } } - result, currentState, err = conf.Refresh() + res, currentState, err := conf.Refresh() + result := Result{ + Result: res, + State: currentState, + Error: err, + } + lastResult.Store(result) + if err != nil { - resulterr = err return } // If we're waiting for the absence of a thing, then return - if result == nil && len(conf.Target) == 0 { + if res == nil && len(conf.Target) == 0 { targetOccurence += 1 if conf.ContinuousTargetOccurence == targetOccurence { return @@ -115,14 +127,15 @@ func (conf *StateChangeConf) WaitForState() (interface{}, error) { } } - if result == nil { + if res == nil { // If we didn't find the resource, check if we have been // not finding it for awhile, and if so, report an error. notfoundTick += 1 if notfoundTick > conf.NotFoundChecks { - resulterr = &NotFoundError{ - LastError: resulterr, + result.Error = &NotFoundError{ + LastError: err, } + lastResult.Store(result) return } } else { @@ -151,11 +164,12 @@ func (conf *StateChangeConf) WaitForState() (interface{}, error) { } if !found { - resulterr = &UnexpectedStateError{ - LastError: resulterr, - State: currentState, + result.Error = &UnexpectedStateError{ + LastError: err, + State: result.State, ExpectedState: conf.Target, } + lastResult.Store(result) return } } @@ -164,11 +178,13 @@ func (conf *StateChangeConf) WaitForState() (interface{}, error) { select { case <-doneCh: - return result, resulterr + r := lastResult.Load().(Result) + return r.Result, r.Error case <-time.After(conf.Timeout): + r := lastResult.Load().(Result) return nil, &TimeoutError{ - LastError: resulterr, - LastState: currentState, + LastError: r.Error, + LastState: r.State, ExpectedState: conf.Target, } } From 481f12cf2cc717d4b21105678f26c8a802142979 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 30 Aug 2016 17:56:26 -0400 Subject: [PATCH 2/2] Remove `first` variable an extra if block clean up the code slighly by moving the Sleep in WaitForState to the end of the loop. --- helper/resource/state.go | 46 +++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/helper/resource/state.go b/helper/resource/state.go index 5efc495b6..300d50b78 100644 --- a/helper/resource/state.go +++ b/helper/resource/state.go @@ -81,30 +81,7 @@ func (conf *StateChangeConf) WaitForState() (interface{}, error) { wait := 100 * time.Millisecond - for first := true; ; first = false { - if !first { - // If a poll interval has been specified, choose that interval. - // Otherwise bound the default value. - if conf.PollInterval > 0 && conf.PollInterval < 180*time.Second { - wait = conf.PollInterval - } else { - if wait < conf.MinTimeout { - wait = conf.MinTimeout - } else if wait > 10*time.Second { - wait = 10 * time.Second - } - } - - log.Printf("[TRACE] Waiting %s before next try", wait) - time.Sleep(wait) - - // Wait between refreshes using exponential backoff, except when - // waiting for the target state to reoccur. - if targetOccurence == 0 { - wait *= 2 - } - } - + for { res, currentState, err := conf.Refresh() result := Result{ Result: res, @@ -173,6 +150,27 @@ func (conf *StateChangeConf) WaitForState() (interface{}, error) { return } } + + // If a poll interval has been specified, choose that interval. + // Otherwise bound the default value. + if conf.PollInterval > 0 && conf.PollInterval < 180*time.Second { + wait = conf.PollInterval + } else { + if wait < conf.MinTimeout { + wait = conf.MinTimeout + } else if wait > 10*time.Second { + wait = 10 * time.Second + } + } + + log.Printf("[TRACE] Waiting %s before next try", wait) + time.Sleep(wait) + + // Wait between refreshes using exponential backoff, except when + // waiting for the target state to reoccur. + if targetOccurence == 0 { + wait *= 2 + } } }()