From fd3532812ec2eb2efa44734f0d7d8ef1c9c7cc98 Mon Sep 17 00:00:00 2001 From: Pete Davison Date: Mon, 30 Dec 2024 17:58:45 +0000 Subject: [PATCH] fix: orderedmap race condition (#1972) --- internal/templater/templater.go | 4 +- task_test.go | 2 +- taskfile/ast/include.go | 67 ++++++++++++++------- taskfile/ast/tasks.go | 61 +++++++++++++++---- taskfile/ast/var.go | 100 +++++++++++++++++++++----------- 5 files changed, 166 insertions(+), 68 deletions(-) diff --git a/internal/templater/templater.go b/internal/templater/templater.go index 41f2cc28..3f3ab20e 100644 --- a/internal/templater/templater.go +++ b/internal/templater/templater.go @@ -140,11 +140,11 @@ func ReplaceVarsWithExtra(vars *ast.Vars, cache *Cache, extra map[string]any) *a return nil } - var newVars ast.Vars + newVars := ast.NewVars() _ = vars.Range(func(k string, v ast.Var) error { newVars.Set(k, ReplaceVarWithExtra(v, cache, extra)) return nil }) - return &newVars + return newVars } diff --git a/task_test.go b/task_test.go index 5dd84ca9..093f0fa7 100644 --- a/task_test.go +++ b/task_test.go @@ -183,7 +183,7 @@ func TestRequires(t *testing.T) { buff.Reset() require.NoError(t, e.Setup()) - vars := &ast.Vars{} + vars := ast.NewVars() vars.Set("foo", ast.Var{Value: "bar"}) require.NoError(t, e.Run(context.Background(), &ast.Call{ Task: "missing-var", diff --git a/taskfile/ast/include.go b/taskfile/ast/include.go index 630f6537..55d45a2e 100644 --- a/taskfile/ast/include.go +++ b/taskfile/ast/include.go @@ -1,6 +1,8 @@ package ast import ( + "sync" + "github.com/elliotchance/orderedmap/v2" "gopkg.in/yaml.v3" @@ -8,27 +10,33 @@ import ( "github.com/go-task/task/v3/internal/deepcopy" ) -// Include represents information about included taskfiles -type Include struct { - Namespace string - Taskfile string - Dir string - Optional bool - Internal bool - Aliases []string - Excludes []string - AdvancedImport bool - Vars *Vars - Flatten bool -} - -// Includes represents information about included taskfiles -type Includes struct { - om *orderedmap.OrderedMap[string, *Include] -} - -type IncludeElement orderedmap.Element[string, *Include] +type ( + // Include represents information about included taskfiles + Include struct { + Namespace string + Taskfile string + Dir string + Optional bool + Internal bool + Aliases []string + Excludes []string + AdvancedImport bool + Vars *Vars + Flatten bool + } + // Includes is an ordered map of namespaces to includes. + Includes struct { + om *orderedmap.OrderedMap[string, *Include] + mutex sync.RWMutex + } + // An IncludeElement is a key-value pair that is used for initializing an + // Includes structure. + IncludeElement orderedmap.Element[string, *Include] +) +// NewIncludes creates a new instance of Includes and initializes it with the +// provided set of elements, if any. The elements are added in the order they +// are passed. func NewIncludes(els ...*IncludeElement) *Includes { includes := &Includes{ om: orderedmap.NewOrderedMap[string, *Include](), @@ -39,20 +47,31 @@ func NewIncludes(els ...*IncludeElement) *Includes { return includes } +// Len returns the number of includes in the Includes map. func (includes *Includes) Len() int { if includes == nil || includes.om == nil { return 0 } + defer includes.mutex.RUnlock() + includes.mutex.RLock() return includes.om.Len() } +// Get returns the value the the include with the provided key and a boolean +// that indicates if the value was found or not. If the value is not found, the +// returned include is a zero value and the bool is false. func (includes *Includes) Get(key string) (*Include, bool) { if includes == nil || includes.om == nil { return &Include{}, false } + defer includes.mutex.RUnlock() + includes.mutex.RLock() return includes.om.Get(key) } +// Set sets the value of the include with the provided key to the provided +// value. If the include already exists, its value is updated. If the include +// does not exist, it is created. func (includes *Includes) Set(key string, value *Include) bool { if includes == nil { includes = NewIncludes() @@ -60,9 +79,14 @@ func (includes *Includes) Set(key string, value *Include) bool { if includes.om == nil { includes.om = orderedmap.NewOrderedMap[string, *Include]() } + defer includes.mutex.Unlock() + includes.mutex.Lock() return includes.om.Set(key, value) } +// Range calls the provided function for each include in the map. The function +// receives the include's key and value as arguments. If the function returns +// an error, the iteration stops and the error is returned. func (includes *Includes) Range(f func(k string, v *Include) error) error { if includes == nil || includes.om == nil { return nil @@ -77,6 +101,9 @@ func (includes *Includes) Range(f func(k string, v *Include) error) error { // UnmarshalYAML implements the yaml.Unmarshaler interface. func (includes *Includes) UnmarshalYAML(node *yaml.Node) error { + if includes == nil || includes.om == nil { + *includes = *NewIncludes() + } switch node.Kind { case yaml.MappingNode: // NOTE: orderedmap does not have an unmarshaler, so we have to decode diff --git a/taskfile/ast/tasks.go b/taskfile/ast/tasks.go index d1cb3684..0acb5f7a 100644 --- a/taskfile/ast/tasks.go +++ b/taskfile/ast/tasks.go @@ -4,6 +4,7 @@ import ( "fmt" "slices" "strings" + "sync" "github.com/elliotchance/orderedmap/v2" "gopkg.in/yaml.v3" @@ -12,13 +13,25 @@ import ( "github.com/go-task/task/v3/internal/filepathext" ) -// Tasks represents a group of tasks -type Tasks struct { - om *orderedmap.OrderedMap[string, *Task] -} - -type TaskElement orderedmap.Element[string, *Task] +type ( + // Tasks is an ordered map of task names to Tasks. + Tasks struct { + om *orderedmap.OrderedMap[string, *Task] + mutex sync.RWMutex + } + // A TaskElement is a key-value pair that is used for initializing a Tasks + // structure. + TaskElement orderedmap.Element[string, *Task] + // MatchingTask represents a task that matches a given call. It includes the + // task itself and a list of wildcards that were matched. + MatchingTask struct { + Task *Task + Wildcards []string + } +) +// NewTasks creates a new instance of Tasks and initializes it with the provided +// set of elements, if any. The elements are added in the order they are passed. func NewTasks(els ...*TaskElement) *Tasks { tasks := &Tasks{ om: orderedmap.NewOrderedMap[string, *Task](), @@ -29,20 +42,31 @@ func NewTasks(els ...*TaskElement) *Tasks { return tasks } +// Len returns the number of variables in the Tasks map. func (tasks *Tasks) Len() int { if tasks == nil || tasks.om == nil { return 0 } + defer tasks.mutex.RUnlock() + tasks.mutex.RLock() return tasks.om.Len() } +// Get returns the value the the task with the provided key and a boolean that +// indicates if the value was found or not. If the value is not found, the +// returned task is a zero value and the bool is false. func (tasks *Tasks) Get(key string) (*Task, bool) { if tasks == nil || tasks.om == nil { return &Task{}, false } + defer tasks.mutex.RUnlock() + tasks.mutex.RLock() return tasks.om.Get(key) } +// Set sets the value of the task with the provided key to the provided value. +// If the task already exists, its value is updated. If the task does not exist, +// it is created. func (tasks *Tasks) Set(key string, value *Task) bool { if tasks == nil { tasks = NewTasks() @@ -50,9 +74,14 @@ func (tasks *Tasks) Set(key string, value *Task) bool { if tasks.om == nil { tasks.om = orderedmap.NewOrderedMap[string, *Task]() } + defer tasks.mutex.Unlock() + tasks.mutex.Lock() return tasks.om.Set(key, value) } +// Range calls the provided function for each task in the map. The function +// receives the task's key and value as arguments. If the function returns an +// error, the iteration stops and the error is returned. func (tasks *Tasks) Range(f func(k string, v *Task) error) error { if tasks == nil || tasks.om == nil { return nil @@ -65,10 +94,13 @@ func (tasks *Tasks) Range(f func(k string, v *Task) error) error { return nil } +// Keys returns a slice of all the keys in the Tasks map. func (tasks *Tasks) Keys() []string { if tasks == nil { return nil } + defer tasks.mutex.RUnlock() + tasks.mutex.RLock() var keys []string for pair := tasks.om.Front(); pair != nil; pair = pair.Next() { keys = append(keys, pair.Key) @@ -76,10 +108,13 @@ func (tasks *Tasks) Keys() []string { return keys } +// Values returns a slice of all the values in the Tasks map. func (tasks *Tasks) Values() []*Task { if tasks == nil { return nil } + defer tasks.mutex.RUnlock() + tasks.mutex.RLock() var values []*Task for pair := tasks.om.Front(); pair != nil; pair = pair.Next() { values = append(values, pair.Value) @@ -87,11 +122,10 @@ func (tasks *Tasks) Values() []*Task { return values } -type MatchingTask struct { - Task *Task - Wildcards []string -} - +// FindMatchingTasks returns a list of tasks that match the given call. A task +// matches a call if its name is equal to the call's task name or if it matches +// a wildcard pattern. The function returns a list of MatchingTask structs, each +// containing a task and a list of wildcards that were matched. func (t *Tasks) FindMatchingTasks(call *Call) []*MatchingTask { if call == nil { return nil @@ -117,6 +151,8 @@ func (t *Tasks) FindMatchingTasks(call *Call) []*MatchingTask { } func (t1 *Tasks) Merge(t2 *Tasks, include *Include, includedTaskfileVars *Vars) error { + defer t2.mutex.RUnlock() + t2.mutex.RLock() err := 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. @@ -207,6 +243,9 @@ func (t1 *Tasks) Merge(t2 *Tasks, include *Include, includedTaskfileVars *Vars) } func (t *Tasks) UnmarshalYAML(node *yaml.Node) error { + if t == nil || t.om == nil { + *t = *NewTasks() + } switch node.Kind { case yaml.MappingNode: // NOTE: orderedmap does not have an unmarshaler, so we have to decode diff --git a/taskfile/ast/var.go b/taskfile/ast/var.go index d582aba9..b2d24977 100644 --- a/taskfile/ast/var.go +++ b/taskfile/ast/var.go @@ -2,6 +2,7 @@ package ast import ( "strings" + "sync" "github.com/elliotchance/orderedmap/v2" "gopkg.in/yaml.v3" @@ -11,52 +12,74 @@ import ( "github.com/go-task/task/v3/internal/experiments" ) -// Vars is a string[string] variables map. -type Vars struct { - om *orderedmap.OrderedMap[string, Var] -} - -type VarElement orderedmap.Element[string, Var] +type ( + // Vars is an ordered map of variable names to values. + Vars struct { + om *orderedmap.OrderedMap[string, Var] + mutex sync.RWMutex + } + // A VarElement is a key-value pair that is used for initializing a Vars + // structure. + VarElement orderedmap.Element[string, Var] +) +// NewVars creates a new instance of Vars and initializes it with the provided +// set of elements, if any. The elements are added in the order they are passed. func NewVars(els ...*VarElement) *Vars { - vs := &Vars{ + vars := &Vars{ om: orderedmap.NewOrderedMap[string, Var](), } for _, el := range els { - vs.Set(el.Key, el.Value) + vars.Set(el.Key, el.Value) } - return vs + return vars } -func (vs *Vars) Len() int { - if vs == nil || vs.om == nil { +// Len returns the number of variables in the Vars map. +func (vars *Vars) Len() int { + if vars == nil || vars.om == nil { return 0 } - return vs.om.Len() + defer vars.mutex.RUnlock() + vars.mutex.RLock() + return vars.om.Len() } -func (vs *Vars) Get(key string) (Var, bool) { - if vs == nil || vs.om == nil { +// Get returns the value the the variable with the provided key and a boolean +// that indicates if the value was found or not. If the value is not found, the +// returned variable is a zero value and the bool is false. +func (vars *Vars) Get(key string) (Var, bool) { + if vars == nil || vars.om == nil { return Var{}, false } - return vs.om.Get(key) + defer vars.mutex.RUnlock() + vars.mutex.RLock() + return vars.om.Get(key) } -func (vs *Vars) Set(key string, value Var) bool { - if vs == nil { - vs = NewVars() +// Set sets the value of the variable with the provided key to the provided +// value. If the variable already exists, its value is updated. If the variable +// does not exist, it is created. +func (vars *Vars) Set(key string, value Var) bool { + if vars == nil { + vars = NewVars() } - if vs.om == nil { - vs.om = orderedmap.NewOrderedMap[string, Var]() + if vars.om == nil { + vars.om = orderedmap.NewOrderedMap[string, Var]() } - return vs.om.Set(key, value) + defer vars.mutex.Unlock() + vars.mutex.Lock() + return vars.om.Set(key, value) } -func (vs *Vars) Range(f func(k string, v Var) error) error { - if vs == nil || vs.om == nil { +// Range calls the provided function for each variable in the map. The function +// receives the variable's key and value as arguments. If the function returns +// an error, the iteration stops and the error is returned. +func (vars *Vars) Range(f func(k string, v Var) error) error { + if vars == nil || vars.om == nil { return nil } - for pair := vs.om.Front(); pair != nil; pair = pair.Next() { + for pair := vars.om.Front(); pair != nil; pair = pair.Next() { if err := f(pair.Key, pair.Value); err != nil { return err } @@ -64,11 +87,13 @@ func (vs *Vars) Range(f func(k string, v Var) error) error { return nil } -// ToCacheMap converts Vars to a map containing only the static +// ToCacheMap converts Vars to an unordered map containing only the static // variables -func (vs *Vars) ToCacheMap() (m map[string]any) { - m = make(map[string]any, vs.Len()) - for pair := vs.om.Front(); pair != nil; pair = pair.Next() { +func (vars *Vars) ToCacheMap() (m map[string]any) { + defer vars.mutex.RUnlock() + vars.mutex.RLock() + m = make(map[string]any, vars.Len()) + for pair := vars.om.Front(); pair != nil; pair = pair.Next() { if pair.Value.Sh != nil && *pair.Value.Sh != "" { // Dynamic variable is not yet resolved; trigger // to be used in templates. @@ -83,31 +108,38 @@ func (vs *Vars) ToCacheMap() (m map[string]any) { return } -// Wrapper around OrderedMap.Merge to ensure we don't get nil pointer errors -func (vs *Vars) Merge(other *Vars, include *Include) { - if vs == nil || vs.om == nil || other == nil { +// Merge loops over other and merges it values with the variables in vars. If +// the include parameter is not nil and its it is an advanced import, the +// directory is set set to the value of the include parameter. +func (vars *Vars) Merge(other *Vars, include *Include) { + if vars == nil || vars.om == nil || other == nil { return } + defer other.mutex.RUnlock() + other.mutex.RLock() for pair := other.om.Front(); pair != nil; pair = pair.Next() { if include != nil && include.AdvancedImport { pair.Value.Dir = include.Dir } - vs.om.Set(pair.Key, pair.Value) + vars.om.Set(pair.Key, pair.Value) } } -// DeepCopy creates a new instance of Vars and copies -// data by value from the source struct. func (vs *Vars) DeepCopy() *Vars { if vs == nil { return nil } + defer vs.mutex.RUnlock() + vs.mutex.RLock() return &Vars{ om: deepcopy.OrderedMap(vs.om), } } func (vs *Vars) UnmarshalYAML(node *yaml.Node) error { + if vs == nil || vs.om == nil { + *vs = *NewVars() + } vs.om = orderedmap.NewOrderedMap[string, Var]() switch node.Kind { case yaml.MappingNode: