Merge pull request #8452 from hashicorp/jbardin/locked-copy

Add locks to state structs for copying

terraform/state.go

@@ -12,6 +12,7 @@ import (
 	"sort"
 	"strconv"
 	"strings"
+	"sync"
 
 	"github.com/hashicorp/go-version"
 	"github.com/hashicorp/terraform/config"
@@ -78,8 +79,13 @@ type State struct {
 
 	// Modules contains all the modules in a breadth-first order
 	Modules []*ModuleState `json:"modules"`
+
+	mu sync.Mutex
 }
 
+func (s *State) Lock()   { s.mu.Lock() }
+func (s *State) Unlock() { s.mu.Unlock() }
+
 // NewState is used to initialize a blank state
 func NewState() *State {
 	s := &State{}
@@ -463,7 +469,7 @@ func (s *State) SameLineage(other *State) bool {
 // DeepCopy performs a deep copy of the state structure and returns
 // a new structure.
 func (s *State) DeepCopy() *State {
-	copy, err := copystructure.Copy(s)
+	copy, err := copystructure.Config{Lock: true}.Copy(s)
 	if err != nil {
 		panic(err)
 	}
@@ -573,10 +579,6 @@ func (s *State) sort() {
 	}
 }
 
-func (s *State) GoString() string {
-	return fmt.Sprintf("*%#v", *s)
-}
-
 func (s *State) String() string {
 	if s == nil {
 		return "<nil>"
@@ -617,15 +619,26 @@ type RemoteState struct {
 	// Config is used to store arbitrary configuration that
 	// is type specific
 	Config map[string]string `json:"config"`
+
+	mu sync.Mutex
 }
 
+func (s *RemoteState) Lock()   { s.mu.Lock() }
+func (s *RemoteState) Unlock() { s.mu.Unlock() }
+
 func (r *RemoteState) init() {
+	r.Lock()
+	defer r.Unlock()
+
 	if r.Config == nil {
 		r.Config = make(map[string]string)
 	}
 }
 
 func (r *RemoteState) deepcopy() *RemoteState {
+	r.Lock()
+	defer r.Unlock()
+
 	confCopy := make(map[string]string, len(r.Config))
 	for k, v := range r.Config {
 		confCopy[k] = v
@@ -655,10 +668,6 @@ func (r *RemoteState) Equals(other *RemoteState) bool {
 	return true
 }
 
-func (r *RemoteState) GoString() string {
-	return fmt.Sprintf("*%#v", *r)
-}
-
 // OutputState is used to track the state relevant to a single output.
 type OutputState struct {
 	// Sensitive describes whether the output is considered sensitive,
@@ -670,8 +679,13 @@ type OutputState struct {
 	// Value contains the value of the output, in the structure described
 	// by the Type field.
 	Value interface{} `json:"value"`
+
+	mu sync.Mutex
 }
 
+func (s *OutputState) Lock()   { s.mu.Lock() }
+func (s *OutputState) Unlock() { s.mu.Unlock() }
+
 func (s *OutputState) String() string {
 	return fmt.Sprintf("%#v", s.Value)
 }
@@ -707,18 +721,12 @@ func (s *OutputState) deepcopy() *OutputState {
 		return nil
 	}
 
-	valueCopy, err := copystructure.Copy(s.Value)
+	stateCopy, err := copystructure.Config{Lock: true}.Copy(s)
 	if err != nil {
 		panic(fmt.Errorf("Error copying output value: %s", err))
 	}
 
-	n := &OutputState{
+	return stateCopy.(*OutputState)
-		Type:      s.Type,
-		Sensitive: s.Sensitive,
-		Value:     valueCopy,
-	}
-
-	return n
 }
 
 // ModuleState is used to track all the state relevant to a single
@@ -753,8 +761,13 @@ type ModuleState struct {
 	// overall state, then it assumes it isn't managed and doesn't
 	// worry about it.
 	Dependencies []string `json:"depends_on"`
+
+	mu sync.Mutex
 }
 
+func (s *ModuleState) Lock()   { s.mu.Lock() }
+func (s *ModuleState) Unlock() { s.mu.Unlock() }
+
 // Equal tests whether one module state is equal to another.
 func (m *ModuleState) Equal(other *ModuleState) bool {
 	// Paths must be equal
@@ -862,6 +875,9 @@ func (m *ModuleState) View(id string) *ModuleState {
 }
 
 func (m *ModuleState) init() {
+	m.Lock()
+	defer m.Unlock()
+
 	if m.Path == nil {
 		m.Path = []string{}
 	}
@@ -885,21 +901,13 @@ func (m *ModuleState) deepcopy() *ModuleState {
 	if m == nil {
 		return nil
 	}
-	n := &ModuleState{
+
-		Path:         make([]string, len(m.Path)),
+	stateCopy, err := copystructure.Config{Lock: true}.Copy(m)
-		Outputs:      make(map[string]*OutputState, len(m.Outputs)),
+	if err != nil {
-		Resources:    make(map[string]*ResourceState, len(m.Resources)),
+		panic(err)
-		Dependencies: make([]string, len(m.Dependencies)),
 	}
-	copy(n.Path, m.Path)
+
-	copy(n.Dependencies, m.Dependencies)
+	return stateCopy.(*ModuleState)
-	for k, v := range m.Outputs {
-		n.Outputs[k] = v.deepcopy()
-	}
-	for k, v := range m.Resources {
-		n.Resources[k] = v.deepcopy()
-	}
-	return n
 }
 
 // prune is used to remove any resources that are no longer required
@@ -925,10 +933,6 @@ func (m *ModuleState) sort() {
 	}
 }
 
-func (m *ModuleState) GoString() string {
-	return fmt.Sprintf("*%#v", *m)
-}
-
 func (m *ModuleState) String() string {
 	var buf bytes.Buffer
 
@@ -1176,8 +1180,13 @@ type ResourceState struct {
 	// e.g. "aws_instance" goes with the "aws" provider.
 	// If the resource block contained a "provider" key, that value will be set here.
 	Provider string `json:"provider"`
+
+	mu sync.Mutex
 }
 
+func (s *ResourceState) Lock()   { s.mu.Lock() }
+func (s *ResourceState) Unlock() { s.mu.Unlock() }
+
 // Equal tests whether two ResourceStates are equal.
 func (s *ResourceState) Equal(other *ResourceState) bool {
 	if s.Type != other.Type {
@@ -1223,6 +1232,9 @@ func (r *ResourceState) Untaint() {
 }
 
 func (r *ResourceState) init() {
+	r.Lock()
+	defer r.Unlock()
+
 	if r.Primary == nil {
 		r.Primary = &InstanceState{}
 	}
@@ -1242,7 +1254,7 @@ func (r *ResourceState) init() {
 }
 
 func (r *ResourceState) deepcopy() *ResourceState {
-	copy, err := copystructure.Copy(r)
+	copy, err := copystructure.Config{Lock: true}.Copy(r)
 	if err != nil {
 		panic(err)
 	}
@@ -1270,10 +1282,6 @@ func (r *ResourceState) sort() {
 	sort.Strings(r.Dependencies)
 }
 
-func (s *ResourceState) GoString() string {
-	return fmt.Sprintf("*%#v", *s)
-}
-
 func (s *ResourceState) String() string {
 	var buf bytes.Buffer
 	buf.WriteString(fmt.Sprintf("Type = %s", s.Type))
@@ -1304,9 +1312,17 @@ type InstanceState struct {
 
 	// Tainted is used to mark a resource for recreation.
 	Tainted bool `json:"tainted"`
+
+	mu sync.Mutex
 }
 
+func (s *InstanceState) Lock()   { s.mu.Lock() }
+func (s *InstanceState) Unlock() { s.mu.Unlock() }
+
 func (i *InstanceState) init() {
+	i.Lock()
+	defer i.Unlock()
+
 	if i.Attributes == nil {
 		i.Attributes = make(map[string]string)
 	}
@@ -1316,8 +1332,23 @@ func (i *InstanceState) init() {
 	i.Ephemeral.init()
 }
 
+// Copy all the Fields from another InstanceState
+func (i *InstanceState) Set(from *InstanceState) {
+	i.Lock()
+	defer i.Unlock()
+
+	from.Lock()
+	defer from.Unlock()
+
+	i.ID = from.ID
+	i.Attributes = from.Attributes
+	i.Ephemeral = from.Ephemeral
+	i.Meta = from.Meta
+	i.Tainted = from.Tainted
+}
+
 func (i *InstanceState) DeepCopy() *InstanceState {
-	copy, err := copystructure.Copy(i)
+	copy, err := copystructure.Config{Lock: true}.Copy(i)
 	if err != nil {
 		panic(err)
 	}
@@ -1415,10 +1446,6 @@ func (s *InstanceState) MergeDiff(d *InstanceDiff) *InstanceState {
 	return result
 }
 
-func (i *InstanceState) GoString() string {
-	return fmt.Sprintf("*%#v", *i)
-}
-
 func (i *InstanceState) String() string {
 	var buf bytes.Buffer
 
@@ -1470,7 +1497,7 @@ func (e *EphemeralState) init() {
 }
 
 func (e *EphemeralState) DeepCopy() *EphemeralState {
-	copy, err := copystructure.Copy(e)
+	copy, err := copystructure.Config{Lock: true}.Copy(e)
 	if err != nil {
 		panic(err)
 	}

terraform/state_add.go

@@ -18,15 +18,15 @@ import (
 //
 // The full semantics of Add:
 //
-//                         ┌───────────────────────┬───────────────────────┬───────────────────────┐
+//                      ┌───────────────────┬───────────────────┬───────────────────┐
-//                         │    Module Address     │   Resource Address    │   Instance Address    │
+//                      │  Module Address   │ Resource Address  │ Instance Address  │
-// ┌───────────────────────┼───────────────────────┼───────────────────────┼───────────────────────┤
+//    ┌─────────────────┼───────────────────┼───────────────────┼───────────────────┤
-// │      ModuleState      │           ✓           │           x           │           x           │
+//    │   ModuleState   │         ✓         │         x         │         x         │
-// ├───────────────────────┼───────────────────────┼───────────────────────┼───────────────────────┤
+//    ├─────────────────┼───────────────────┼───────────────────┼───────────────────┤
-// │     ResourceState     │           ✓           │           ✓           │        maybe*         │
+//    │  ResourceState  │         ✓         │         ✓         │      maybe*       │
-// ├───────────────────────┼───────────────────────┼───────────────────────┼───────────────────────┤
+//    ├─────────────────┼───────────────────┼───────────────────┼───────────────────┤
-// │    Instance State     │           ✓           │           ✓           │           ✓           │
+//    │ Instance State  │         ✓         │         ✓         │         ✓         │
-// └───────────────────────┴───────────────────────┴───────────────────────┴───────────────────────┘
+//    └─────────────────┴───────────────────┴───────────────────┴───────────────────┘
 //
 // *maybe - Resources can be added at an instance address only if the resource
 //          represents a single instance (primary). Example:
@@ -219,7 +219,7 @@ func stateAddFunc_Instance_Instance(s *State, fromAddr, addr *ResourceAddress, r
 	instance := instanceRaw.(*InstanceState)
 
 	// Set it
-	*instance = *src
+	instance.Set(src)
 
 	return nil
 }

vendor/github.com/mitchellh/copystructure/copystructure.go

@@ -2,27 +2,14 @@ package copystructure
 
 import (
 	"reflect"
+	"sync"
 
 	"github.com/mitchellh/reflectwalk"
 )
 
 // Copy returns a deep copy of v.
 func Copy(v interface{}) (interface{}, error) {
-	w := new(walker)
+	return Config{}.Copy(v)
-	err := reflectwalk.Walk(v, w)
-	if err != nil {
-		return nil, err
-	}
-
-	// Get the result. If the result is nil, then we want to turn it
-	// into a typed nil if we can.
-	result := w.Result
-	if result == nil {
-		val := reflect.ValueOf(v)
-		result = reflect.Indirect(reflect.New(val.Type())).Interface()
-	}
-
-	return result, nil
 }
 
 // CopierFunc is a function that knows how to deep copy a specific type.
@@ -40,6 +27,42 @@ type CopierFunc func(interface{}) (interface{}, error)
 // this map as well as to Copy in a mutex.
 var Copiers map[reflect.Type]CopierFunc = make(map[reflect.Type]CopierFunc)
 
+type Config struct {
+	// Lock any types that are a sync.Locker and are not a mutex while copying.
+	// If there is an RLocker method, use that to get the sync.Locker.
+	Lock bool
+
+	// Copiers is a map of types associated with a CopierFunc. Use the global
+	// Copiers map if this is nil.
+	Copiers map[reflect.Type]CopierFunc
+}
+
+func (c Config) Copy(v interface{}) (interface{}, error) {
+	w := new(walker)
+	if c.Lock {
+		w.useLocks = true
+	}
+
+	if c.Copiers == nil {
+		c.Copiers = Copiers
+	}
+
+	err := reflectwalk.Walk(v, w)
+	if err != nil {
+		return nil, err
+	}
+
+	// Get the result. If the result is nil, then we want to turn it
+	// into a typed nil if we can.
+	result := w.Result
+	if result == nil {
+		val := reflect.ValueOf(v)
+		result = reflect.Indirect(reflect.New(val.Type())).Interface()
+	}
+
+	return result, nil
+}
+
 type walker struct {
 	Result interface{}
 
@@ -48,14 +71,31 @@ type walker struct {
 	vals        []reflect.Value
 	cs          []reflect.Value
 	ps          []bool
+
+	// any locks we've taken, indexed by depth
+	locks []sync.Locker
+	// take locks while walking the structure
+	useLocks bool
 }
 
 func (w *walker) Enter(l reflectwalk.Location) error {
 	w.depth++
+
+	// ensure we have enough elements to index via w.depth
+	for w.depth >= len(w.locks) {
+		w.locks = append(w.locks, nil)
+	}
+
 	return nil
 }
 
 func (w *walker) Exit(l reflectwalk.Location) error {
+	locker := w.locks[w.depth]
+	w.locks[w.depth] = nil
+	if locker != nil {
+		defer locker.Unlock()
+	}
+
 	w.depth--
 	if w.ignoreDepth > w.depth {
 		w.ignoreDepth = 0
@@ -76,13 +116,25 @@ func (w *walker) Exit(l reflectwalk.Location) error {
 		mv := w.valPop()
 		mk := w.valPop()
 		m := w.cs[len(w.cs)-1]
+
+		// If mv is the zero value, SetMapIndex deletes the key form the map,
+		// or in this case never adds it. We need to create a properly typed
+		// zero value so that this key can be set.
+		if !mv.IsValid() {
+			mv = reflect.Zero(m.Type().Elem())
+		}
 		m.SetMapIndex(mk, mv)
 	case reflectwalk.SliceElem:
 		// Pop off the value and the index and set it on the slice
 		v := w.valPop()
-		i := w.valPop().Interface().(int)
+		if v.IsValid() {
-		s := w.cs[len(w.cs)-1]
+			i := w.valPop().Interface().(int)
-		s.Index(i).Set(v)
+			s := w.cs[len(w.cs)-1]
+			se := s.Index(i)
+			if se.CanSet() {
+				se.Set(v)
+			}
+		}
 	case reflectwalk.Struct:
 		w.replacePointerMaybe()
 
@@ -112,6 +164,7 @@ func (w *walker) Map(m reflect.Value) error {
 	if w.ignoring() {
 		return nil
 	}
+	w.lock(m)
 
 	// Create the map. If the map itself is nil, then just make a nil map
 	var newMap reflect.Value
@@ -152,6 +205,7 @@ func (w *walker) Primitive(v reflect.Value) error {
 	if w.ignoring() {
 		return nil
 	}
+	w.lock(v)
 
 	// IsValid verifies the v is non-zero and CanInterface verifies
 	// that we're allowed to read this value (unexported fields).
@@ -170,6 +224,7 @@ func (w *walker) Slice(s reflect.Value) error {
 	if w.ignoring() {
 		return nil
 	}
+	w.lock(s)
 
 	var newS reflect.Value
 	if s.IsNil() {
@@ -199,6 +254,7 @@ func (w *walker) Struct(s reflect.Value) error {
 	if w.ignoring() {
 		return nil
 	}
+	w.lock(s)
 
 	var v reflect.Value
 	if c, ok := Copiers[s.Type()]; ok {
@@ -277,3 +333,58 @@ func (w *walker) replacePointerMaybe() {
 		w.valPush(reflect.Indirect(w.valPop()))
 	}
 }
+
+// if this value is a Locker, lock it and add it to the locks slice
+func (w *walker) lock(v reflect.Value) {
+	if !w.useLocks {
+		return
+	}
+
+	if !v.IsValid() || !v.CanInterface() {
+		return
+	}
+
+	type rlocker interface {
+		RLocker() sync.Locker
+	}
+
+	var locker sync.Locker
+
+	// first check if we can get a locker from the value
+	switch l := v.Interface().(type) {
+	case rlocker:
+		// don't lock a mutex directly
+		if _, ok := l.(*sync.RWMutex); !ok {
+			locker = l.RLocker()
+		}
+	case sync.Locker:
+		locker = l
+	}
+
+	// the value itself isn't a locker, so check the method on a pointer too
+	if locker == nil && v.CanAddr() {
+		switch l := v.Addr().Interface().(type) {
+		case rlocker:
+			// don't lock a mutex directly
+			if _, ok := l.(*sync.RWMutex); !ok {
+				locker = l.RLocker()
+			}
+		case sync.Locker:
+			locker = l
+		}
+	}
+
+	// still no callable locker
+	if locker == nil {
+		return
+	}
+
+	// don't lock a mutex directly
+	switch locker.(type) {
+	case *sync.Mutex, *sync.RWMutex:
+		return
+	}
+
+	locker.Lock()
+	w.locks[w.depth] = locker
+}

vendor/vendor.json

@@ -1448,10 +1448,10 @@
 			"revision": "8631ce90f28644f54aeedcb3e389a85174e067d1"
 		},
 		{
-			"checksumSHA1": "86nE93o1VIND0Doe8PuhCXnhUx0=",
+			"checksumSHA1": "y2HsVMt3eYGqywv5ljijnywJMb4=",
 			"path": "github.com/mitchellh/copystructure",
-			"revision": "cdac8253d00f2ecf0a0b19fbff173a9a72de4f82",
+			"revision": "6871c41ca9148d368715dedcda473f396f205df5",
-			"revisionTime": "2016-08-04T03:23:30Z"
+			"revisionTime": "2016-08-25T20:45:07Z"
 		},
 		{
 			"path": "github.com/mitchellh/go-homedir",