Repository: vmware/govmomi (go)

Issue:
Dara race at vim25/retry.go:49
Tried to use vim25.retry in our code to retry the request but I was seeing Data Race at variable n. 
https://github.com/vmware/govmomi/blob/master/vim25/retry.go#L49

As we share retry Func object across diff requests once request 1 reaches max retry limit, all other requests also reach max limit. We use only one vimClient for all the requests which can be concurrent.
I think we should add _retryCounter_  in  **retry struct** and set it to 0 for new request. 

Pull request:
vim25: fix race in TemporaryNetworkError retry func
To maintain backward compatibility introduced new optional
variable in retry struct and new function with parameter
retryAttempts.

- Add vim25.RetryTemporaryNetworkError to replace deprecated vim25.TemporaryNetworkError

- Export IsTemporaryNetworkError function

- Add test to retry network error

Fixes #2266

Patch:
diff --git a/govc/flags/client.go b/govc/flags/client.go
index fe8b451c..97cd326e 100644
--- a/govc/flags/client.go
+++ b/govc/flags/client.go
@@ -371,7 +371,7 @@ func (flag *ClientFlag) Client() (*vim25.Client, error) {
 
 	// Retry twice when a temporary I/O error occurs.
 	// This means a maximum of 3 attempts.
-	c.RoundTripper = vim25.Retry(c.Client, vim25.TemporaryNetworkError(3))
+	c.RoundTripper = vim25.Retry(c.Client, vim25.RetryTemporaryNetworkError, 3)
 	flag.client = c
 
 	return flag.client, nil
diff --git a/vim25/retry.go b/vim25/retry.go
index f10e2cb6..bf663a10 100644
--- a/vim25/retry.go
+++ b/vim25/retry.go
@@ -25,33 +25,40 @@ import (
 
 type RetryFunc func(err error) (retry bool, delay time.Duration)
 
-// TemporaryNetworkError returns a RetryFunc that retries up to a maximum of n
-// times, only if the error returned by the RoundTrip function is a temporary
-// network error (for example: a connect timeout).
+// TemporaryNetworkError is deprecated. Use Retry() with RetryTemporaryNetworkError and retryAttempts instead.
 func TemporaryNetworkError(n int) RetryFunc {
-	return func(err error) (retry bool, delay time.Duration) {
-		var ok bool
-
-		t, ok := err.(interface {
-			// Temporary is implemented by url.Error and net.Error
-			Temporary() bool
-		})
-		if !ok {
-			// Never retry if this is not a Temporary error.
-			return false, 0
-		}
-
-		if !t.Temporary() {
-			return false, 0
+	return func(err error) (bool, time.Duration) {
+		if IsTemporaryNetworkError(err) {
+			// Don't retry if we're out of tries.
+			if n--; n <= 0 {
+				return false, 0
+			}
+			return true, 0
 		}
+		return false, 0
+	}
+}
 
-		// Don't retry if we're out of tries.
-		if n--; n <= 0 {
-			return false, 0
-		}
+// RetryTemporaryNetworkError returns a RetryFunc that returns IsTemporaryNetworkError(err)
+func RetryTemporaryNetworkError(err error) (bool, time.Duration) {
+	return IsTemporaryNetworkError(err), 0
+}
 
-		return true, 0
+// IsTemporaryNetworkError returns false unless the error implements
+// a Temporary() bool method such as url.Error and net.Error.
+// Otherwise, returns the value of the Temporary() method.
+func IsTemporaryNetworkError(err error) bool {
+	t, ok := err.(interface {
+		// Temporary is implemented by url.Error and net.Error
+		Temporary() bool
+	})
+
+	if !ok {
+		// Not a Temporary error.
+		return false
 	}
+
+	return t.Temporary()
 }
 
 type retry struct {
@@ -60,7 +67,8 @@ type retry struct {
 	// fn is a custom function that is called when an error occurs.
 	// It returns whether or not to retry, and if so, how long to
 	// delay before retrying.
-	fn RetryFunc
+	fn               RetryFunc
+	maxRetryAttempts int
 }
 
 // Retry wraps the specified soap.RoundTripper and invokes the
@@ -68,10 +76,16 @@ type retry struct {
 // retry the call, and if so, how long to wait before retrying. If
 // the result of this function is to not retry, the original error
 // is returned from the RoundTrip function.
-func Retry(roundTripper soap.RoundTripper, fn RetryFunc) soap.RoundTripper {
+// The soap.RoundTripper will return the original error if retryAttempts is specified and reached.
+func Retry(roundTripper soap.RoundTripper, fn RetryFunc, retryAttempts ...int) soap.RoundTripper {
 	r := &retry{
-		roundTripper: roundTripper,
-		fn:           fn,
+		roundTripper:     roundTripper,
+		fn:               fn,
+		maxRetryAttempts: 1,
+	}
+
+	if len(retryAttempts) == 1 {
+		r.maxRetryAttempts = retryAttempts[0]
 	}
 
 	return r
@@ -80,7 +94,7 @@ func Retry(roundTripper soap.RoundTripper, fn RetryFunc) soap.RoundTripper {
 func (r *retry) RoundTrip(ctx context.Context, req, res soap.HasFault) error {
 	var err error
 
-	for {
+	for attempt := 0; attempt < r.maxRetryAttempts; attempt++ {
 		err = r.roundTripper.RoundTrip(ctx, req, res)
 		if err == nil {
 			break
diff --git a/vim25/soap/client.go b/vim25/soap/client.go
index 531e797e..63733046 100644
--- a/vim25/soap/client.go
+++ b/vim25/soap/client.go
@@ -560,7 +560,7 @@ type statusError struct {
 }
 
 // Temporary returns true for HTTP response codes that can be retried
-// See vim25.TemporaryNetworkError
+// See vim25.IsTemporaryNetworkError
 func (e *statusError) Temporary() bool {
 	switch e.res.StatusCode {
 	case http.StatusBadGateway:


Test patch:
diff --git a/session/keep_alive_test.go b/session/keep_alive_test.go
index 26bf7bc3..ae61bcfb 100644
--- a/session/keep_alive_test.go
+++ b/session/keep_alive_test.go
@@ -40,7 +40,7 @@ func TestKeepAlive(t *testing.T) {
 
 		var mu sync.Mutex
 		n := 0
-		c.RoundTripper = vim25.Retry(c.Client, vim25.TemporaryNetworkError(3))
+		c.RoundTripper = vim25.Retry(c.Client, vim25.RetryTemporaryNetworkError, 3)
 		c.RoundTripper = session.KeepAliveHandler(c.RoundTripper, time.Millisecond, func(soap.RoundTripper) error {
 			mu.Lock()
 			n++
diff --git a/vim25/examples_test.go b/vim25/examples_test.go
index f4bd6999..e18666dc 100644
--- a/vim25/examples_test.go
+++ b/vim25/examples_test.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"time"
 
 	"github.com/vmware/govmomi/find"
 	"github.com/vmware/govmomi/simulator"
@@ -29,7 +30,11 @@ import (
 func ExampleTemporaryNetworkError() {
 	simulator.Run(func(ctx context.Context, c *vim25.Client) error {
 		// Configure retry handler
-		c.RoundTripper = vim25.Retry(c.Client, vim25.TemporaryNetworkError(3))
+		delay := time.Millisecond * 100
+		retry := func(err error) (bool, time.Duration) {
+			return vim25.IsTemporaryNetworkError(err), delay
+		}
+		c.RoundTripper = vim25.Retry(c.Client, retry, 3)
 
 		vm, err := find.NewFinder(c).VirtualMachine(ctx, "DC0_H0_VM0")
 		if err != nil {
diff --git a/vim25/retry_test.go b/vim25/retry_test.go
index 4ea5ddb7..60276a7b 100644
--- a/vim25/retry_test.go
+++ b/vim25/retry_test.go
@@ -14,13 +14,19 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package vim25
+package vim25_test
 
 import (
 	"context"
+	"net/http"
 	"testing"
+	"time"
 
+	"github.com/vmware/govmomi/find"
+	"github.com/vmware/govmomi/simulator"
+	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/soap"
+	"github.com/vmware/govmomi/vim25/types"
 )
 
 type tempError struct{}
@@ -74,7 +80,7 @@ func TestRetry(t *testing.T) {
 
 	for _, tc := range tcs {
 		var rt soap.RoundTripper = &fakeRoundTripper{errs: tc.errs}
-		rt = Retry(rt, TemporaryNetworkError(2))
+		rt = vim25.Retry(rt, vim25.RetryTemporaryNetworkError, 2)
 
 		err := rt.RoundTrip(context.TODO(), nil, nil)
 		if err != tc.expected {
@@ -82,3 +88,47 @@ func TestRetry(t *testing.T) {
 		}
 	}
 }
+
+func TestRetryNetworkError(t *testing.T) {
+	simulator.Test(func(ctx context.Context, c *vim25.Client) {
+		c.RoundTripper = vim25.Retry(c.Client, vim25.RetryTemporaryNetworkError, 2)
+
+		vm, err := find.NewFinder(c).VirtualMachine(ctx, "DC0_H0_VM0")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// Tell vcsim to respond with 502 on the 1st request
+		simulator.StatusSDK = http.StatusBadGateway
+
+		state, err := vm.PowerState(ctx)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if state != types.VirtualMachinePowerStatePoweredOn {
+			t.Errorf("state=%s", state)
+		}
+
+		retry := func(err error) (bool, time.Duration) {
+			simulator.StatusSDK = http.StatusBadGateway // Tell vcsim to respond with 502 on every request
+			return vim25.IsTemporaryNetworkError(err), 0
+		}
+		c.RoundTripper = vim25.Retry(c.Client, retry, 2)
+
+		simulator.StatusSDK = http.StatusBadGateway
+		// beyond max retry attempts, should result in an erro
+		for i := 0; i < 3; i++ {
+			_, err = vm.PowerState(ctx)
+		}
+
+		if err == nil {
+			t.Error("expected error")
+		}
+
+		if !vim25.IsTemporaryNetworkError(err) {
+			t.Errorf("unexpected error=%s", err)
+		}
+		simulator.StatusSDK = http.StatusOK
+	})
+}