grim/convey

Fix #126 - do not let extend modify state concurrently.

2017-10-01, Eric Fritz
5eb3d9373cf9
Parents 64cd62a316ab
Children 65b7657346f9
Fix #126 - do not let extend modify state concurrently.
3 files changed, 72 insertions(+), 83 deletions(-)
  • +5 -3
    intrinsic/extend.go
  • +40 -40
    state/state.go
  • +27 -40
    state/state_test.go
    • --- a/intrinsic/extend.go Sun Oct 01 21:04:31 2017 -0500
    +++ b/intrinsic/extend.go Sun Oct 01 21:31:03 2017 -0500
    @@ -45,10 +45,12 @@
    // While extending, certain environment variables can be expanded into
    // lists. We store this meta information as a stack on the state, which
    // is passed into the inner task execute method (and so on).
    - st.PushExtendFrame(e.Environment, e.Expand, e.ExpandDelimiter)
    - defer st.PopExtendFrame()
    - return e.InnerTask.Execute(name, logger, env, st)
    + return e.InnerTask.Execute(name, logger, env, st.WrapWithExpandableEnv(
    + e.Environment,
    + e.Expand,
    + e.ExpandDelimiter,
    + ))
    }
    func (e *Extend) New() tasks.Task {
    --- a/state/state.go Sun Oct 01 21:04:31 2017 -0500
    +++ b/state/state.go Sun Oct 01 21:31:03 2017 -0500
    @@ -18,7 +18,6 @@
    package state
    import (
    - "errors"
    "fmt"
    "os"
    "strings"
    @@ -38,23 +37,25 @@
    EnableSSHAgent bool
    TaskTimeout time.Duration
    Environment []string
    - ExtendFrames []extendFrame
    DockerConfig string
    CPUShares string
    Memory string
    DetachedContainers []string
    +
    + // States have the ability to "wrap" another one without
    + // changing the underlying state. This is used by the
    + // extends intrinsic in order to modify the stat without
    + // requiring unique access to the state object during the
    + // execution of the extended task. A past implementation
    + // had modified the stack directly, but that causes an
    + // extended task to clobber other tasks in concurrent mode.
    + parent *State
    + expandables []string
    + expandableDelimiter string
    }
    -type extendFrame struct {
    - expandable []string
    - delimiter string
    - stashedEnv []string
    -}
    -
    -var errEmptyExtendStack = errors.New("empty extend stack")
    -
    func (st *State) Valid() error {
    if st.EnableSSHAgent {
    if val := os.Getenv("SSH_AUTH_SOCK"); val == "" {
    @@ -88,46 +89,45 @@
    // GetDelimiter returns the highest (outermost extend task) delimiter registered
    // with a given expandable environment variable. Returns true if found by name.
    func (st *State) getDelimiter(name string) (string, bool) {
    - for i := len(st.ExtendFrames) - 1; i >= 0; i-- {
    - frame := st.ExtendFrames[i]
    + if st.parent == nil {
    + return "", false
    + }
    - for _, expandable := range frame.expandable {
    - if expandable == name {
    - return frame.delimiter, true
    - }
    + for _, expandable := range st.expandables {
    + if expandable == name {
    + return st.expandableDelimiter, true
    }
    }
    - return "", false
    + return st.parent.getDelimiter(name)
    }
    -// PushExtendFrame will store the set of expandable environment variables
    -// and the current set of environments which are restored on pop. These frames
    -// are used to map a slice within an extended task.
    -func (st *State) PushExtendFrame(env, expandable []string, delimiter string) {
    - st.ExtendFrames = append(st.ExtendFrames, extendFrame{
    - expandable: expandable,
    - delimiter: delimiter,
    - stashedEnv: st.Environment,
    - })
    -
    +// WrapWithExpandableEnv will create a shallow clone of the state with a reference
    +// to the current state as "parent" with a modified environment. This creates a local
    +// stack of states which do not interfere with other goroutines. A pop operation is
    +// the same as ignoring the wrapped values and using the underlying state. This stack
    +// is used to map a slice within an extended task.
    +func (st *State) WrapWithExpandableEnv(env, expandable []string, delimiter string) *State {
    // Merge the environment into this map, but do NOT override anything that
    // is currently in the state's enviornment - this has higher precedence.
    - st.Environment = environment.Merge(env, st.Environment)
    -}
    + env = environment.Merge(env, st.Environment)
    -// PopExtendFrame will remove one frame from the top of the stack.
    -func (st *State) PopExtendFrame() error {
    - idx := len(st.ExtendFrames) - 1
    - if idx < 0 {
    - return errEmptyExtendStack
    + return &State{
    + Network: st.Network,
    + Workspace: st.Workspace,
    + KeepWorkspace: st.KeepWorkspace,
    + ForceSequential: st.ForceSequential,
    + EnableSSHAgent: st.EnableSSHAgent,
    + TaskTimeout: st.TaskTimeout,
    + Environment: env,
    + DockerConfig: st.DockerConfig,
    + CPUShares: st.CPUShares,
    + Memory: st.Memory,
    + DetachedContainers: st.DetachedContainers,
    + parent: st,
    + expandables: expandable,
    + expandableDelimiter: delimiter,
    }
    -
    - last := st.ExtendFrames[idx]
    - st.ExtendFrames = st.ExtendFrames[:idx]
    - st.Environment = last.stashedEnv
    -
    - return nil
    }
    // getName returns the name of the variable `var` if the string is of the
    --- a/state/state_test.go Sun Oct 01 21:04:31 2017 -0500
    +++ b/state/state_test.go Sun Oct 01 21:31:03 2017 -0500
    @@ -38,59 +38,46 @@
    }
    func (e *stateSuite) TestMap(t sweet.T) {
    - st := &State{}
    - st.Environment = []string{"FOO=BAR"}
    -
    - mapEnv := func(val string) []string {
    + mapEnv := func(st *State, val string) []string {
    mapped, err := st.MapSlice([]string{val}, st.Environment)
    Expect(err).To(BeNil())
    return mapped
    }
    - Expect(mapEnv("$X")).To(Equal([]string{"$X"}))
    - Expect(mapEnv("$FOO")).To(Equal([]string{"BAR"}))
    + st1 := &State{}
    + st1.Environment = []string{"FOO=BAR"}
    - st.PushExtendFrame([]string{"X=A;B;C"}, []string{"X"}, ";")
    - Expect(mapEnv("$X")).To(Equal([]string{"A", "B", "C"}))
    - Expect(mapEnv("$FOO")).To(Equal([]string{"BAR"}))
    + Expect(mapEnv(st1, "$X")).To(Equal([]string{"$X"}))
    + Expect(mapEnv(st1, "$FOO")).To(Equal([]string{"BAR"}))
    - st.PushExtendFrame([]string{"BAR=B;A;R::B;A;Z", "FOO=SAME"}, []string{"BAR"}, "::")
    - Expect(mapEnv("$X")).To(Equal([]string{"A", "B", "C"}))
    - Expect(mapEnv("$FOO")).To(Equal([]string{"BAR"}))
    - Expect(mapEnv("$BAR")).To(Equal([]string{"B;A;R", "B;A;Z"}))
    + st2 := st1.WrapWithExpandableEnv([]string{"X=A;B;C"}, []string{"X"}, ";")
    + Expect(mapEnv(st2, "$X")).To(Equal([]string{"A", "B", "C"}))
    + Expect(mapEnv(st2, "$FOO")).To(Equal([]string{"BAR"}))
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(mapEnv("$X")).To(Equal([]string{"$X"}))
    - Expect(mapEnv("$FOO")).To(Equal([]string{"BAR"}))
    + st3 := st2.WrapWithExpandableEnv([]string{"BAR=B;A;R::B;A;Z", "FOO=SAME"}, []string{"BAR"}, "::")
    + Expect(mapEnv(st3, "$X")).To(Equal([]string{"A", "B", "C"}))
    + Expect(mapEnv(st3, "$FOO")).To(Equal([]string{"BAR"}))
    + Expect(mapEnv(st3, "$BAR")).To(Equal([]string{"B;A;R", "B;A;Z"}))
    }
    -func (e *stateSuite) TestPopExtendFrameSize(t sweet.T) {
    - st := &State{}
    - st.PushExtendFrame(nil, nil, "")
    - Expect(st.ExtendFrames).To(HaveLen(1))
    - st.PushExtendFrame(nil, nil, "")
    - Expect(st.ExtendFrames).To(HaveLen(2))
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(st.ExtendFrames).To(HaveLen(1))
    - Expect(st.PopExtendFrame()).To(BeNil())
    - Expect(st.ExtendFrames).To(HaveLen(0))
    +func (e *stateSuite) TestWrapParent(t sweet.T) {
    + st1 := &State{}
    + st1.WrapWithExpandableEnv(nil, nil, "")
    + Expect(st1.parent).To(BeNil())
    +
    + st2 := st1.WrapWithExpandableEnv(nil, nil, "")
    + Expect(st2.parent).To(BeIdenticalTo(st1))
    }
    -func (e *stateSuite) TestPopExtendFrameRestoresEnv(t sweet.T) {
    - st := &State{}
    - st.Environment = []string{"FOO=BAR", "BAR=BAZ"}
    - st.PushExtendFrame([]string{"FOO=BONK", "BAZ=BONK"}, nil, "")
    - Expect(st.Environment).To(HaveLen(3))
    - Expect(st.Environment).To(ConsistOf([]string{"FOO=BAR", "BAR=BAZ", "BAZ=BONK"}))
    - st.PopExtendFrame()
    - Expect(st.Environment).To(HaveLen(2))
    - Expect(st.Environment).To(ConsistOf([]string{"FOO=BAR", "BAR=BAZ"}))
    -}
    +func (e *stateSuite) TestWrapWithExpandableEnvMap(t sweet.T) {
    + st1 := &State{}
    + st1.Environment = []string{"FOO=BAR", "BAR=BAZ"}
    + Expect(st1.Environment).To(HaveLen(2))
    + Expect(st1.Environment).To(ConsistOf([]string{"FOO=BAR", "BAR=BAZ"}))
    -func (e *stateSuite) TestPopExtendFrameEmpty(t sweet.T) {
    - st := &State{}
    - Expect(st.PopExtendFrame()).To(Equal(errEmptyExtendStack))
    + st2 := st1.WrapWithExpandableEnv([]string{"FOO=BONK", "BAZ=BONK"}, nil, "")
    + Expect(st2.Environment).To(HaveLen(3))
    + Expect(st2.Environment).To(ConsistOf([]string{"FOO=BAR", "BAR=BAZ", "BAZ=BONK"}))
    }
    func (e *stateSuite) TestGetName(t sweet.T) {