From 58aa298581a8439e45d6f36bc15a6b0cca3b6539 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Wed, 20 Jan 2021 12:16:56 -0800 Subject: [PATCH] Fix locking around Fill and Lookup Signed-off-by: Andrea Luzzardi --- dagger/compiler.go | 2 +- dagger/env.go | 12 ----------- dagger/spec.go | 23 ++++----------------- dagger/value.go | 51 ++++++++++++++++------------------------------ 4 files changed, 22 insertions(+), 66 deletions(-) diff --git a/dagger/compiler.go b/dagger/compiler.go index 9ca9d271..1f44e2e0 100644 --- a/dagger/compiler.go +++ b/dagger/compiler.go @@ -18,7 +18,7 @@ import ( // (we call it compiler to avoid confusion with dagger runtime) // Use this instead of cue.Runtime type Compiler struct { - sync.Mutex + sync.RWMutex cue.Runtime spec *Spec } diff --git a/dagger/env.go b/dagger/env.go index 0ab8e9f6..cdeea27b 100644 --- a/dagger/env.go +++ b/dagger/env.go @@ -3,7 +3,6 @@ package dagger import ( "context" "os" - "sync" "cuelang.org/go/cue" cueflow "cuelang.org/go/tools/flow" @@ -144,14 +143,9 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) { return nil, err } - l := sync.Mutex{} - // Cueflow config flowCfg := &cueflow.Config{ UpdateFunc: func(c *cueflow.Controller, t *cueflow.Task) error { - l.Lock() - defer l.Unlock() - if t == nil { return nil } @@ -180,9 +174,6 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) { } // Cueflow match func flowMatchFn := func(v cue.Value) (cueflow.Runner, error) { - l.Lock() - defer l.Unlock() - lg := lg. With(). Str("path", v.Path().String()). @@ -200,9 +191,6 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) { return nil, err } return cueflow.RunnerFunc(func(t *cueflow.Task) error { - l.Lock() - defer l.Unlock() - return fn(ctx, c, t) }), nil } diff --git a/dagger/spec.go b/dagger/spec.go index 295d89b4..1b555860 100644 --- a/dagger/spec.go +++ b/dagger/spec.go @@ -1,7 +1,6 @@ package dagger import ( - "cuelang.org/go/cue" cueerrors "cuelang.org/go/cue/errors" "github.com/pkg/errors" ) @@ -12,28 +11,14 @@ type Spec struct { } // eg. Validate(op, "#Op") -func (s Spec) Validate(v *Value, defpath string) (err error) { - // Expand cue errors to get full details - // FIXME: there is probably a cleaner way to do this. - defer func() { - if err != nil { - err = errors.New(cueerrors.Details(err, nil)) - } - }() - +func (s Spec) Validate(v *Value, defpath string) error { // Lookup def by name, eg. "#Script" or "#Copy" // See dagger/spec.cue def := s.root.Get(defpath) - if err := def.Validate(); err != nil { - return err - } - merged := def.Unwrap().Fill(v.Value) - if err := merged.Err(); err != nil { - return err - } - if err := merged.Validate(cue.Final()); err != nil { - return err + if err := def.Fill(v.Value); err != nil { + return errors.New(cueerrors.Details(err, nil)) } + return nil } diff --git a/dagger/value.go b/dagger/value.go index d4ee3d62..bb0eea04 100644 --- a/dagger/value.go +++ b/dagger/value.go @@ -18,31 +18,18 @@ type Value struct { inst *cue.Instance } -func (v *Value) Lock() { - if v.cc == nil { - return - } - v.cc.Lock() -} - -func (v *Value) Unlock() { - if v.cc == nil { - return - } - v.cc.Unlock() -} - func (v *Value) Lookup(path ...string) *Value { - v.Lock() - defer v.Unlock() + v.cc.RLock() + defer v.cc.RUnlock() - return v.Wrap(v.Unwrap().LookupPath(cueStringsToCuePath(path...))) + return v.Wrap(v.Value.LookupPath(cueStringsToCuePath(path...))) } func (v *Value) LookupPath(p cue.Path) *Value { - v.Lock() - defer v.Unlock() - return v.Wrap(v.Unwrap().LookupPath(p)) + v.cc.RLock() + defer v.cc.RUnlock() + + return v.Wrap(v.Value.LookupPath(p)) } // FIXME: deprecated by Get() @@ -150,9 +137,13 @@ func (v *Value) Merge(x interface{}, path ...string) (*Value, error) { if xval.Compiler() != v.Compiler() { return nil, fmt.Errorf("can't merge values from different compilers") } - x = xval.Unwrap() + x = xval.Value } - result := v.Wrap(v.Unwrap().Fill(x, path...)) + + v.cc.Lock() + result := v.Wrap(v.Value.Fill(x, path...)) + v.cc.Unlock() + return result, result.Validate() } @@ -226,8 +217,6 @@ func (v *Value) IsConcreteR() bool { // Contrast with cue.Value.MarshalJSON which requires all values // to be concrete. func (v *Value) JSON() JSON { - v.Lock() - defer v.Unlock() var out JSON v.Walk( func(v cue.Value) bool { @@ -267,7 +256,7 @@ func (v *Value) Save(fs FS, filename string) (FS, error) { } func (v *Value) Validate(defs ...string) error { - if err := v.Unwrap().Validate(); err != nil { + if err := v.Value.Validate(); err != nil { return err } if len(defs) == 0 { @@ -289,13 +278,14 @@ func (v *Value) Validate(defs ...string) error { // This is the only method which changes the value in-place. // FIXME this co-exists awkwardly with the rest of Value. func (v *Value) Fill(x interface{}) error { + v.cc.Lock() + defer v.cc.Unlock() + v.Value = v.Value.Fill(x) return v.Validate() } func (v *Value) Source() ([]byte, error) { - v.Lock() - defer v.Unlock() return cueformat.Node(v.Eval().Syntax()) } @@ -313,9 +303,6 @@ func (v *Value) CueInst() *cue.Instance { } func (v *Value) Compiler() *Compiler { - // if v.cc == nil { - // return &Compiler{} - // } return v.cc } @@ -323,10 +310,6 @@ func (v *Value) Wrap(v2 cue.Value) *Value { return wrapValue(v2, v.inst, v.cc) } -func (v *Value) Unwrap() cue.Value { - return v.Value -} - func wrapValue(v cue.Value, inst *cue.Instance, cc *Compiler) *Value { return &Value{ Value: v,