Merge pull request #16961 from hashicorp/jbardin/mock-provider-race

minor race issue in mockResourceProvider

command/meta.go

@@ -160,13 +160,6 @@ type Meta struct {
 	forceInitCopy    bool
 	reconfigure      bool
 
-	// errWriter is the write side of a pipe for the FlagSet output. We need to
-	// keep track of this to close previous pipes between tests. Normal
-	// operation never needs to close this.
-	errWriter *io.PipeWriter
-	// done chan to wait for the scanner goroutine
-	errScannerDone chan struct{}
-
 	// Used with the import command to allow import of state when no matching config exists.
 	allowMissingConfig bool
 }
@@ -339,23 +332,16 @@ func (m *Meta) flagSet(n string) *flag.FlagSet {
 	// This is kind of a hack, but it does the job. Basically: create
 	// a pipe, use a scanner to break it into lines, and output each line
 	// to the UI. Do this forever.
-
-	// If a previous pipe exists, we need to close that first.
-	// This should only happen in testing.
-	if m.errWriter != nil {
-		m.errWriter.Close()
-	}
-
-	if m.errScannerDone != nil {
-		<-m.errScannerDone
-	}
-
 	errR, errW := io.Pipe()
 	errScanner := bufio.NewScanner(errR)
-	m.errWriter = errW
-	m.errScannerDone = make(chan struct{})
 	go func() {
-		defer close(m.errScannerDone)
+		// This only needs to be alive long enough to write the help info if
+		// there is a flag error. Kill the scanner after a short duriation to
+		// prevent these from accumulating during tests, and cluttering up the
+		// stack traces.
+		time.AfterFunc(2*time.Second, func() {
+			errW.Close()
+		})
 		for errScanner.Scan() {
 			m.Ui.Error(errScanner.Text())
 		}

command/plan_test.go

@@ -7,7 +7,9 @@ import (
 	"path/filepath"
 	"reflect"
 	"strings"
+	"sync"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/terraform/helper/copy"
 	"github.com/hashicorp/terraform/terraform"
@@ -832,8 +834,7 @@ func TestPlan_detailedExitcode_emptyDiff(t *testing.T) {
 }
 
 func TestPlan_shutdown(t *testing.T) {
-	cancelled := false
+	cancelled := make(chan struct{})
-	stopped := make(chan struct{})
 
 	shutdownCh := make(chan struct{})
 	p := testProvider()
@@ -847,20 +848,20 @@ func TestPlan_shutdown(t *testing.T) {
 	}
 
 	p.StopFn = func() error {
-		close(stopped)
+		close(cancelled)
-		cancelled = true
 		return nil
 	}
 
+	var once sync.Once
+
 	p.DiffFn = func(
 		*terraform.InstanceInfo,
 		*terraform.InstanceState,
 		*terraform.ResourceConfig) (*terraform.InstanceDiff, error) {
 
-		if !cancelled {
+		once.Do(func() {
 			shutdownCh <- struct{}{}
-			<-stopped
+		})
-		}
 
 		return &terraform.InstanceDiff{
 			Attributes: map[string]*terraform.ResourceAttrDiff{
@@ -875,7 +876,9 @@ func TestPlan_shutdown(t *testing.T) {
 		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
 	}
 
-	if !cancelled {
+	select {
+	case <-cancelled:
+	case <-time.After(5 * time.Second):
 		t.Fatal("command not cancelled")
 	}
 }

terraform/resource_provider_mock.go

@@ -196,13 +196,13 @@ func (p *MockResourceProvider) Diff(
 	info *InstanceInfo,
 	state *InstanceState,
 	desired *ResourceConfig) (*InstanceDiff, error) {
-
 	p.Lock()
+	defer p.Unlock()
+
 	p.DiffCalled = true
 	p.DiffInfo = info
 	p.DiffState = state
 	p.DiffDesired = desired
-	p.Unlock()
 
 	if p.DiffFn != nil {
 		return p.DiffFn(info, state, desired)