Skip to content

fix: race condition on Controller.Satisfied#101

Merged
sywhang merged 1 commit intouber-go:mainfrom
davidharrigan:fix-satisfied-race-condition
Oct 11, 2023
Merged

fix: race condition on Controller.Satisfied#101
sywhang merged 1 commit intouber-go:mainfrom
davidharrigan:fix-satisfied-race-condition

Conversation

@davidharrigan
Copy link
Copy Markdown
Contributor

@davidharrigan davidharrigan commented Oct 4, 2023

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

@davidharrigan
fix: race condition on Controller.Satisfied
88203ed 
This commit fixes the potential race condition that can arise from
calling Controller.Satified from a goroutine.
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Oct 4, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

sywhang
sywhang approved these changes Oct 10, 2023
View reviewed changes
Copy link
Copy Markdown
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
@davidharrigan davidharrigan deleted the fix-satisfied-race-condition branch November 8, 2023 18:29
@davidharrigan
Copy link
Copy Markdown
Contributor Author

davidharrigan commented Dec 19, 2023

@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

Reviewers

@sywhang sywhang sywhang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@davidharrigan @CLAassistant @sywhang