fix ordered map race

go.mod

@@ -20,6 +20,7 @@ require (
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.9.0
 	github.com/zeebo/xxh3 v1.0.2
+	golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8
 	golang.org/x/sync v0.7.0
 	golang.org/x/term v0.21.0
 	gopkg.in/yaml.v3 v3.0.1

go.sum

@@ -67,6 +67,8 @@ github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ=
 github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0=
 github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0=
 github.com/zeebo/xxh3 v1.0.2/go.mod h1:5NWz9Sef7zIDm2JHfFlcQvNekmcEl9ekUZQQKCYaDcA=
+golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8 h1:yixxcjnhBmY0nkL253HFVIm0JsFHwrHdT3Yh6szTnfY=
+golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8/go.mod h1:jj3sYF3dwk5D+ghuXyeI3r5MFf+NT2An6/9dOA95KSI=
 golang.org/x/sync v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
 golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=

internal/omap/orderedmap.go

@@ -4,72 +4,90 @@ import (
 	"cmp"
 	"fmt"
 	"slices"
+	"sync"
+
+	"golang.org/x/exp/maps"
 
 	"gopkg.in/yaml.v3"
 
 	"github.com/go-task/task/v3/internal/deepcopy"
-	"github.com/go-task/task/v3/internal/exp"
 )
 
 // An OrderedMap is a wrapper around a regular map that maintains an ordered
 // list of the map's keys. This allows you to run deterministic and ordered
 // operations on the map such as printing/serializing/iterating.
 type OrderedMap[K cmp.Ordered, V any] struct {
-	s []K
-	m map[K]V
+	mutex sync.RWMutex
+	s     []K
+	m     map[K]V
 }
 
 // New will create a new OrderedMap of the given type and return it.
 func New[K cmp.Ordered, V any]() OrderedMap[K, V] {
-	return OrderedMap[K, V]{
-		s: make([]K, 0),
-		m: make(map[K]V),
-	}
+	return OrderedMap[K, V]{}
 }
 
 // FromMap will create a new OrderedMap from the given map. Since Golang maps
 // are unordered, the order of the created OrderedMap will be random.
 func FromMap[K cmp.Ordered, V any](m map[K]V) OrderedMap[K, V] {
-	om := New[K, V]()
-	om.m = m
-	om.s = exp.Keys(m)
-	return om
+	mm := deepcopy.Map(m)
+	ms := maps.Keys(mm)
+	return OrderedMap[K, V]{
+		m: mm,
+		s: ms,
+	}
 }
 
 func FromMapWithOrder[K cmp.Ordered, V any](m map[K]V, order []K) OrderedMap[K, V] {
-	om := New[K, V]()
 	if len(m) != len(order) {
 		panic("length of map and order must be equal")
 	}
-	om.m = m
-	om.s = order
-	for key := range om.m {
-		if !slices.Contains(om.s, key) {
+
+	for key := range m {
+		if !slices.Contains(order, key) {
 			panic("order keys must match map keys")
 		}
 	}
-	return om
+
+	return OrderedMap[K, V]{
+		s: slices.Clone(order),
+		m: deepcopy.Map(m),
+	}
 }
 
 // Len will return the number of items in the map.
-func (om *OrderedMap[K, V]) Len() int {
-	return len(om.s)
+func (om *OrderedMap[K, V]) Len() (l int) {
+	om.mutex.RLock()
+	l = len(om.s)
+	om.mutex.RUnlock()
+
+	return
 }
 
 // Set will set the value for a given key.
 func (om *OrderedMap[K, V]) Set(key K, value V) {
+	om.mutex.Lock()
 	if om.m == nil {
 		om.m = make(map[K]V)
 	}
+
 	if _, ok := om.m[key]; !ok {
 		om.s = append(om.s, key)
 	}
 	om.m[key] = value
+	om.mutex.Unlock()
 }
 
 // Get will return the value for a given key.
 // If the key does not exist, it will return the zero value of the value type.
 func (om *OrderedMap[K, V]) Get(key K) V {
+	om.mutex.RLock()
+	defer om.mutex.RUnlock()
+
+	if om.m == nil {
+		var zero V
+		return zero
+	}
 	value, ok := om.m[key]
 	if !ok {
 		var zero V
@@ -80,61 +98,87 @@ func (om *OrderedMap[K, V]) Get(key K) V {
 
 // Exists will return whether or not the given key exists.
 func (om *OrderedMap[K, V]) Exists(key K) bool {
+	om.mutex.RLock()
 	_, ok := om.m[key]
+	om.mutex.RUnlock()
 	return ok
 }
 
 // Sort will sort the map.
 func (om *OrderedMap[K, V]) Sort() {
+	om.mutex.Lock()
 	slices.Sort(om.s)
+	om.mutex.Unlock()
 }
 
 // SortFunc will sort the map using the given function.
 func (om *OrderedMap[K, V]) SortFunc(less func(i, j K) int) {
+	om.mutex.Lock()
 	slices.SortFunc(om.s, less)
+	om.mutex.Unlock()
 }
 
 // Keys will return a slice of the map's keys in order.
 func (om *OrderedMap[K, V]) Keys() []K {
-	return om.s
+	om.mutex.RLock()
+	keys := deepcopy.Slice(om.s)
+	om.mutex.RUnlock()
+
+	return keys
 }
 
 // Values will return a slice of the map's values in order.
 func (om *OrderedMap[K, V]) Values() []V {
 	var values []V
+
+	om.mutex.RLock()
 	for _, key := range om.s {
-		values = append(values, om.m[key])
+		values = append(values, om.Get(key))
 	}
+	om.mutex.RUnlock()
+
 	return values
 }
 
 // Range will iterate over the map and call the given function for each key/value.
 func (om *OrderedMap[K, V]) Range(fn func(key K, value V) error) error {
-	for _, key := range om.s {
-		if err := fn(key, om.m[key]); err != nil {
+	keys := om.Keys()
+	for _, key := range keys {
+		if err := fn(key, om.Get(key)); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
 // Merge merges the given Vars into the caller one
-func (om *OrderedMap[K, V]) Merge(other OrderedMap[K, V]) {
-	// nolint: errcheck
-	other.Range(func(key K, value V) error {
+func (om *OrderedMap[K, V]) Merge(other *OrderedMap[K, V]) {
+	_ = other.Range(func(key K, value V) error {
 		om.Set(key, value)
 		return nil
 	})
 }
 
 func (om *OrderedMap[K, V]) DeepCopy() OrderedMap[K, V] {
+	om.mutex.RLock()
+	s := deepcopy.Slice(om.s)
+	m := deepcopy.Map(om.m)
+	om.mutex.RUnlock()
+
 	return OrderedMap[K, V]{
-		s: deepcopy.Slice(om.s),
-		m: deepcopy.Map(om.m),
+		s: s,
+		m: m,
 	}
 }
 
 func (om *OrderedMap[K, V]) UnmarshalYAML(node *yaml.Node) error {
+	if om == nil {
+		*om = OrderedMap[K, V]{
+			m: make(map[K]V),
+			s: make([]K, 0),
+		}
+	}
+
 	switch node.Kind {
 	// Even numbers contain the keys
 	// Odd numbers contain the values
@@ -158,7 +202,7 @@ func (om *OrderedMap[K, V]) UnmarshalYAML(node *yaml.Node) error {
 			om.Set(k, v)
 		}
 		return nil
+	default:
+		return fmt.Errorf("yaml: line %d: cannot unmarshal %s into variables", node.Line, node.ShortTag())
 	}
-
-	return fmt.Errorf("yaml: line %d: cannot unmarshal %s into variables", node.Line, node.ShortTag())
 }

internal/omap/orderedmap_test.go

@@ -89,7 +89,7 @@ func TestOrderedMapMerge(t *testing.T) {
 	om2.Set("b", 3)
 	om2.Set("c", 4)
 
-	om1.Merge(om2)
+	om1.Merge(&om2)
 
 	expectedKeys := []string{"a", "b", "c"}
 	expectedValues := []int{1, 3, 4}

internal/summary/summary_test.go

@@ -156,13 +156,13 @@ func TestPrintAllWithSpaces(t *testing.T) {
 	t2 := &ast.Task{Task: "t2"}
 	t3 := &ast.Task{Task: "t3"}
 
-	tasks := ast.Tasks{}
-	tasks.Set("t1", t1)
-	tasks.Set("t2", t2)
-	tasks.Set("t3", t3)
+	var tf ast.Taskfile
+	tf.Tasks.Set("t1", t1)
+	tf.Tasks.Set("t2", t2)
+	tf.Tasks.Set("t3", t3)
 
 	summary.PrintTasks(&l,
-		&ast.Taskfile{Tasks: tasks},
+		&tf,
 		[]*ast.Call{{Task: "t1"}, {Task: "t2"}, {Task: "t3"}})
 
 	assert.True(t, strings.HasPrefix(buffer.String(), "task: t1"))

taskfile/ast/taskfile.go

@@ -55,7 +55,7 @@ func (t1 *Taskfile) Merge(t2 *Taskfile, include *Include) error {
 	}
 	t1.Vars.Merge(t2.Vars, include)
 	t1.Env.Merge(t2.Env, include)
-	t1.Tasks.Merge(t2.Tasks, include, t1.Vars)
+	t1.Tasks.Merge(&t2.Tasks, include, t1.Vars)
 	return nil
 }
 
@@ -88,7 +88,7 @@ func (tf *Taskfile) UnmarshalYAML(node *yaml.Node) error {
 		tf.Shopt = taskfile.Shopt
 		tf.Vars = taskfile.Vars
 		tf.Env = taskfile.Env
-		tf.Tasks = taskfile.Tasks
+		tf.Tasks = taskfile.Tasks.DeepCopy()
 		tf.Silent = taskfile.Silent
 		tf.Dotenv = taskfile.Dotenv
 		tf.Run = taskfile.Run

taskfile/ast/tasks.go

@@ -16,6 +16,12 @@ type Tasks struct {
 	omap.OrderedMap[string, *Task]
 }
 
+func (t *Tasks) DeepCopy() Tasks {
+	return Tasks{
+		OrderedMap: t.OrderedMap.DeepCopy(),
+	}
+}
+
 type MatchingTask struct {
 	Task      *Task
 	Wildcards []string
@@ -46,7 +52,7 @@ func (t *Tasks) FindMatchingTasks(call *Call) []*MatchingTask {
 	return matchingTasks
 }
 
-func (t1 *Tasks) Merge(t2 Tasks, include *Include, includedTaskfileVars *Vars) {
+func (t1 *Tasks) Merge(t2 *Tasks, include *Include, includedTaskfileVars *Vars) {
 	_ = t2.Range(func(name string, v *Task) error {
 		// We do a deep copy of the task struct here to ensure that no data can
 		// be changed elsewhere once the taskfile is merged.
@@ -142,13 +148,10 @@ func (t *Tasks) UnmarshalYAML(node *yaml.Node) error {
 					}
 				}
 			}
-			tasks.Set(name, task)
+			t.Set(name, task)
 			return nil
 		})
 
-		*t = Tasks{
-			OrderedMap: tasks,
-		}
 		return nil
 	}