Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: race condition on Controller.Satisfied #101

Merged
merged 1 commit into from Oct 11, 2023

Conversation

davidharrigan
Copy link
Contributor

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
==================

This commit fixes the potential race condition that can arise from
calling Controller.Satified from a goroutine.
@CLAassistant
Copy link

CLAassistant commented Oct 4, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks good to me.

@sywhang sywhang merged commit 892b665 into uber-go:main Oct 11, 2023
3 checks passed
@davidharrigan davidharrigan deleted the fix-satisfied-race-condition branch November 8, 2023 18:29
@davidharrigan
Copy link
Contributor Author

@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: #88, if that is a blocker I'm happy to help if you can provide a description of what you're looking for? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants