uber-go / mock

GoMock is a mocking framework for the Go programming language.
Apache License 2.0
1.82k stars 104 forks source link

fix: race condition on Controller.Satisfied #101

Closed davidharrigan closed 9 months ago

davidharrigan commented 9 months ago

This PR is a proposal to add mutex.Lock on Controller.Satisfied to solve a race condition that can arise when two goroutines are involved in checking whether mock conditions have been satisfied.

The use case is illustrated in the sample/concurrent package - what I'm hoping to accomplish is where the test scenario contains one goroutine that runs indefinitely that calls mocked functions, I would like to use Satisfied as one of the conditions to trigger test assertions.

Tests added in sample/concurrent fails data race check without this change:

$ go test -race go.uber.org/mock/sample/concurrent
==================
WARNING: DATA RACE
Read at 0x00c000156350 by goroutine 9:
  go.uber.org/mock/gomock.(*Call).satisfied()
      /Users/dave/work/src/mock/gomock/call.go:307 +0x204
  go.uber.org/mock/gomock.callSet.Satisfied()
      /Users/dave/work/src/mock/gomock/callset.go:157 +0x247
  go.uber.org/mock/gomock.(*Controller).Satisfied()
      /Users/dave/work/src/mock/gomock/controller.go:249 +0x244
  go.uber.org/mock/sample/concurrent.waitForMocks()
      /Users/dave/work/src/mock/sample/concurrent/concurrent_test.go:35 +0x247
  go.uber.org/mock/sample/concurrent.TestCancelWhenMocksSatisfied()
      /Users/dave/work/src/mock/sample/concurrent/concurrent_test.go:85 +0x33b
  testing.tRunner()
      /usr/local/Cellar/go/1.20.3/libexec/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /usr/local/Cellar/go/1.20.3/libexec/src/testing/testing.go:1629 +0x47

Previous write at 0x00c000156350 by goroutine 10:
  go.uber.org/mock/gomock.(*Call).call()
      /Users/dave/work/src/mock/gomock/call.go:433 +0x611
  go.uber.org/mock/gomock.(*Controller).Call.func1()
      /Users/dave/work/src/mock/gomock/controller.go:220 +0x5d6
  go.uber.org/mock/gomock.(*Controller).Call()
      /Users/dave/work/src/mock/gomock/controller.go:225 +0xcd
  go.uber.org/mock/sample/concurrent/mock.(*MockMath).Sum()
      /Users/dave/work/src/mock/sample/concurrent/mock/concurrent_mock.go:43 +0x16d
  go.uber.org/mock/sample/concurrent.TestCancelWhenMocksSatisfied.func1()
      /Users/dave/work/src/mock/sample/concurrent/concurrent_test.go:76 +0x4c

Goroutine 9 (running) created at:
  testing.(*T).Run()
      /usr/local/Cellar/go/1.20.3/libexec/src/testing/testing.go:1629 +0x805
  testing.runTests.func1()
      /usr/local/Cellar/go/1.20.3/libexec/src/testing/testing.go:2036 +0x8d
  testing.tRunner()
      /usr/local/Cellar/go/1.20.3/libexec/src/testing/testing.go:1576 +0x216
  testing.runTests()
      /usr/local/Cellar/go/1.20.3/libexec/src/testing/testing.go:2034 +0x87c
  testing.(*M).Run()
      /usr/local/Cellar/go/1.20.3/libexec/src/testing/testing.go:1906 +0xb44
  main.main()
      _testmain.go:51 +0x2e9

Goroutine 10 (running) created at:
  go.uber.org/mock/sample/concurrent.TestCancelWhenMocksSatisfied()
      /Users/dave/work/src/mock/sample/concurrent/concurrent_test.go:74 +0x324
  testing.tRunner()
      /usr/local/Cellar/go/1.20.3/libexec/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /usr/local/Cellar/go/1.20.3/libexec/src/testing/testing.go:1629 +0x47
==================
CLAassistant commented 9 months ago

CLA assistant check
All committers have signed the CLA.

davidharrigan commented 6 months ago

@sywhang any chance we could cut a new release containing this fix / several others in the main branch? I see an issue to fix release actions: https://github.com/uber-go/mock/issues/88, if that is a blocker I'm happy to help if you can provide a description of what you're looking for? Thanks!