From 5160578e186230aecb251bdcfb0219cac35c9230 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 4 Mar 2016 11:20:48 -0600 Subject: [PATCH] helper/resource: restore retval of resource.Retry on timeout In #4700 while fixing a data race I made an incorrect assumption about the return value of StateChangeConf, and ended up changing the behavior in the timeout case to always return: > timeout while waiting for state to become '[success]' When it used to capture the "most recent error" from the function itself. It's much more useful to see that error bubbling up, so here we revert to pulling it out of the function as we did before, and we protect against the data race with a good old fashioned mutex. --- helper/resource/wait.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/helper/resource/wait.go b/helper/resource/wait.go index 4167c7db7d5c..cf5f72f3f439 100644 --- a/helper/resource/wait.go +++ b/helper/resource/wait.go @@ -1,6 +1,7 @@ package resource import ( + "sync" "time" ) @@ -10,6 +11,11 @@ type RetryFunc func() error // Retry is a basic wrapper around StateChangeConf that will just retry // a function until it no longer returns an error. func Retry(timeout time.Duration, f RetryFunc) error { + // These are used to pull the error out of the function; need a mutex to + // avoid a data race. + var resultErr error + var resultErrMu sync.Mutex + c := &StateChangeConf{ Pending: []string{"error"}, Target: []string{"success"}, @@ -21,17 +27,25 @@ func Retry(timeout time.Duration, f RetryFunc) error { return 42, "success", nil } + resultErrMu.Lock() + defer resultErrMu.Unlock() + resultErr = err if rerr, ok := err.(RetryError); ok { - err = rerr.Err - return nil, "quit", err + resultErr = rerr.Err + return nil, "quit", rerr.Err } return 42, "error", nil }, } - _, err := c.WaitForState() - return err + c.WaitForState() + + // Need to acquire the lock here to be able to avoid race using resultErr as + // the return value + resultErrMu.Lock() + defer resultErrMu.Unlock() + return resultErr } // RetryError, if returned, will quit the retry immediately with the