Data race when handling SIGINT

[nodo]

I found the following data race in ginkgo. It happens when a suite receives a SIGINT (or Ctrl-C).

Step to reproduce

1. Create a suite with a test that sleeps for a period of time. E.g.

It("succeeds", func() {
  time.Sleep(1000 * time.Second)
  Expect(1).To(Equal(1))
})

2. Run ginkgo -race
3. Hit Ctrl-C
4. You should see the data race

Example output

$ ginkgo -race playground
Running Suite: Playground Suite
===============================
Random Seed: 1517654437
Will run 1 of 1 specs

^C==================
WARNING: DATA RACE
Read at 0x00c42018a030 by goroutine 9:
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).suiteDidEndSummary.func1()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/spec/spec.go:72 +0x48
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).countSpecsThatRanSatisfying()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:327 +0xb2
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).suiteDidEndSummary()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:336 +0x51
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).reportSuiteDidEnd()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:316 +0x45
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).registerForInterrupts()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:237 +0x246

Previous write at 0x00c42018a030 by goroutine 8:
  github.com/onsi/ginkgo/internal/spec.(*Spec).runSample()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/spec/spec.go:142 +0x62
  github.com/onsi/ginkgo/internal/spec.(*Spec).Run()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/spec/spec.go:133 +0x160
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpec()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:198 +0x17c
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpecs()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:168 +0x62d
  github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).Run()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:64 +0x103
  github.com/onsi/ginkgo/internal/suite.(*Suite).Run()
      /Users/andrea/go/src/github.com/onsi/ginkgo/internal/suite/suite.go:62 +0x3d0
  github.com/onsi/ginkgo.RunSpecsWithCustomReporters()
      /Users/andrea/go/src/github.com/onsi/ginkgo/ginkgo_dsl.go:220 +0x324
  github.com/onsi/ginkgo.RunSpecs()
      /Users/andrea/go/src/github.com/onsi/ginkgo/ginkgo_dsl.go:201 +0x367
  github.com/nodo/ginkgo-playground/playground_test.TestPlayground()
      /Users/andrea/go/src/github.com/nodo/ginkgo-playground/playground/playground_suite_test.go:12 +0x88
  testing.tRunner()
      /usr/local/Cellar/go/1.9.3/libexec/src/testing/testing.go:746 +0x16c

The problem

I think the problem is the following.

Ginkgo register for interrupts in the registerForInterrupts function, which is called in a goroutine here.

When it receives a SIGINT, registerForInterrupts calls the reportSuiteDidEnd function here, which in turn, calls suiteDidEndSummary.

The latter filters the specs by state in order to create a summary. However, the state is also being accessed by the "main" spec runner here.

Hence the data race reported by go:

• Read https://github.com/onsi/ginkgo/blob/master/internal/spec/spec.go#L72
• Write https://github.com/onsi/ginkgo/blob/master/internal/spec/spec.go#L142

The solution

The simplest solution would be adding a mutex, with getter and setter to the state and use it to syncronise the goroutines when accessing the state.
E.g.

func (spec *Spec) getState() types.SpecState {
	spec.mutex.Lock()
	defer spec.mutex.Unlock()
	return spec.state
}

func (spec *Spec) setState(s types.SpecState) {
	spec.mutex.Lock()
	defer spec.mutex.Unlock()
	spec.state = s
}

[...]
func (spec *Spec) Passed() bool {
	return spec.getState() == types.SpecStatePassed
}

What do you think?

If this solution looks good to you I can open a PR fairly quickly.