From 197f6cab79cf4feb2a0e5a9fec8f38d0db29dfc6 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Tue, 7 Feb 2017 19:00:27 -0500
Subject: [PATCH] Cannot store multiple types in atomic.Value

Storing error values to atomic.Value may fail if they have different
dynamic types. Wrap error value in a consistent struct type to avoid
panics.

Make sure we return a nil error on success
---
 .../remote-exec/resource_provisioner.go       | 15 +++++++---
 .../remote-exec/resource_provisioner_test.go  | 29 +++++++++++++++++++
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/builtin/provisioners/remote-exec/resource_provisioner.go b/builtin/provisioners/remote-exec/resource_provisioner.go
index 14df1fa59..248ce5d5f 100644
--- a/builtin/provisioners/remote-exec/resource_provisioner.go
+++ b/builtin/provisioners/remote-exec/resource_provisioner.go
@@ -234,11 +234,17 @@ func copyOutput(
 }
 
 // retryFunc is used to retry a function for a given duration
+// TODO: this should probably backoff too
 func retryFunc(ctx context.Context, timeout time.Duration, f func() error) error {
 	// Build a new context with the timeout
 	ctx, done := context.WithTimeout(ctx, timeout)
 	defer done()
 
+	// container for atomic error value
+	type errWrap struct {
+		E error
+	}
+
 	// Try the function in a goroutine
 	var errVal atomic.Value
 	doneCh := make(chan struct{})
@@ -255,19 +261,20 @@ func retryFunc(ctx context.Context, timeout time.Duration, f func() error) error
 
 			// Try the function call
 			err := f()
+			errVal.Store(&errWrap{err})
+
 			if err == nil {
 				return
 			}
 
 			log.Printf("Retryable error: %v", err)
-			errVal.Store(err)
 		}
 	}()
 
 	// Wait for completion
 	select {
-	case <-doneCh:
 	case <-ctx.Done():
+	case <-doneCh:
 	}
 
 	// Check if we have a context error to check if we're interrupted or timeout
@@ -279,8 +286,8 @@ func retryFunc(ctx context.Context, timeout time.Duration, f func() error) error
 	}
 
 	// Check if we got an error executing
-	if err, ok := errVal.Load().(error); ok {
-		return err
+	if ev, ok := errVal.Load().(errWrap); ok {
+		return ev.E
 	}
 
 	return nil
diff --git a/builtin/provisioners/remote-exec/resource_provisioner_test.go b/builtin/provisioners/remote-exec/resource_provisioner_test.go
index 3bbeaca61..69e5e9cdf 100644
--- a/builtin/provisioners/remote-exec/resource_provisioner_test.go
+++ b/builtin/provisioners/remote-exec/resource_provisioner_test.go
@@ -2,8 +2,12 @@ package remoteexec
 
 import (
 	"bytes"
+	"context"
+	"errors"
 	"io"
+	"net"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/helper/schema"
@@ -158,6 +162,31 @@ func TestResourceProvider_CollectScripts_scripts(t *testing.T) {
 	}
 }
 
+func TestRetryFunc(t *testing.T) {
+	// succeed on the third try
+	errs := []error{io.EOF, &net.OpError{Err: errors.New("ERROR")}, nil}
+	count := 0
+
+	err := retryFunc(context.Background(), time.Second, func() error {
+		if count >= len(errs) {
+			return errors.New("failed to stop after nil error")
+		}
+
+		err := errs[count]
+		count++
+
+		return err
+	})
+
+	if count != 3 {
+		t.Fatal("retry func should have been called 3 times")
+	}
+
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
 func testConfig(
 	t *testing.T,
 	c map[string]interface{}) *terraform.ResourceConfig {