Fix data races, simplify

[titanous]

I've simplified the basic runner to check for cancellation before each step and use the close of a goroutine to synchronize waiting on Cancel().

Before this patch go test -race indicates data races:

==================
WARNING: DATA RACE
Read by goroutine 11:
  github.com/mitchellh/multistep.func·001()
      $GOPATH/src/github.com/mitchellh/multistep/basic_runner.go:39 +0x17a
  gosched0()
      $GOROOT/src/pkg/runtime/proc.c:1218 +0x9f

Previous write by goroutine 12:
  github.com/mitchellh/multistep.(*BasicRunner).Cancel()
      $GOPATH/src/github.com/mitchellh/multistep/basic_runner.go:103 +0x9a
  github.com/mitchellh/multistep.func·003()
      $GOPATH/src/github.com/mitchellh/multistep/basic_runner_test.go:96 +0x50
  gosched0()
      $GOROOT/src/pkg/runtime/proc.c:1218 +0x9f

Goroutine 11 (running) created at:
  github.com/mitchellh/multistep.(*BasicRunner).Run()
      $GOPATH/src/github.com/mitchellh/multistep/basic_runner.go:44 +0x477
  gosched0()
      $GOROOT/src/pkg/runtime/proc.c:1218 +0x9f

Goroutine 12 (running) created at:
  github.com/mitchellh/multistep.TestBasicRunner_Cancel()
      $GOPATH/src/github.com/mitchellh/multistep/basic_runner_test.go:98 +0x5b5
  testing.tRunner()
      $GOROOT/src/pkg/testing/testing.go:353 +0x12f
  gosched0()
      $GOROOT/src/pkg/runtime/proc.c:1218 +0x9f

==================
==================
WARNING: DATA RACE
Write by goroutine 11:
  runtime.mapassign1()
      $GOROOT/src/pkg/runtime/hashmap.c:1289 +0x0
  github.com/mitchellh/multistep.func·001()
      $GOPATH/src/github.com/mitchellh/multistep/basic_runner.go:40 +0x1e7
  gosched0()
      $GOROOT/src/pkg/runtime/proc.c:1218 +0x9f

Previous read by goroutine 9:
  runtime.mapaccess2_faststr()
      /Users/titanous/src/go-tip/src/pkg/runtime/hashmap.c:473 +0x0
  github.com/mitchellh/multistep.TestBasicRunner_Cancel()
      $GOPATH/src/github.com/mitchellh/multistep/basic_runner_test.go:101 +0x5e1
  testing.tRunner()
      $GOROOT/src/pkg/testing/testing.go:353 +0x12f
  gosched0()
      $GOROOT/src/pkg/runtime/proc.c:1218 +0x9f

Goroutine 11 (running) created at:
  github.com/mitchellh/multistep.(*BasicRunner).Run()
      $GOPATH/src/github.com/mitchellh/multistep/basic_runner.go:44 +0x477
  gosched0()
      $GOROOT/src/pkg/runtime/proc.c:1218 +0x9f

Goroutine 9 (running) created at:
  testing.RunTests()
      $GOROOT/src/pkg/testing/testing.go:433 +0xaef
  testing.Main()
      $GOROOT/src/pkg/testing/testing.go:365 +0xab
  main.main()
      github.com/mitchellh/multistep/_test/_testmain.go:55 +0xda
  runtime.main()
      $GOROOT/src/pkg/runtime/proc.c:182 +0x91

==================
Pausing after run of step 'foo'. Press any key to continue.
PASS
Found 2 data race(s)
exit status 66
FAIL    github.com/mitchellh/multistep  1.150s

After, there are none:

Pausing after run of step 'foo'. Press any key to continue.
PASS
ok      github.com/mitchellh/multistep  1.130s

[titanous]

Hmm, looking at your usage in Packer, it looks like checking the cancelled state in the middle of a step run is a feature. In that case, we need to either wrap a Mutex or RWMutex around all state access everywhere, or put the cancelled flag in a separate variable that is properly protected.

[titanous]

What do you think about passing a channel in the state bag that gets closed if/when the run is cancelled?

func (s ExStep) Run(state map[string]interface{}) StepAction {
    for {
        select {
        case <-state[CancelChan].(<-chan struct{}):
            return
        default:
            // work...
        }
    }
}

We could also change the type of state to have a nice wrapper that does the type assertion:

func (s ExStep) Run(state StateBag) StepAction {
    for {
        select {
        case <-state.CancelNotify():
            return
        default:
            // work...
        }
    }
}

[mitchellh]

Whoops sorry I forgot to "watch" this repo. Haha.

So I was initially going to use channels to do this. The issue, that I think is still present in this pull, is what if multiple people call Cancel(). Does that work properly?

[titanous]

Yep, receiving from a closed channel returns the zero value instantly, so close can be used to signal all receivers on a channel simultaneously.

[mitchellh]

Looks great, finally merging. Thanks. :)

[mitchellh]

@titanous re: cancel access and stuff: Yeah, I think breaking API and introducing a state bag is the way to go. I didn't realize when I first wrote this the dangers of data races.

[titanous]

@mitchellh Cool. I don't have time to implement that, but let me know if you have any questions.