This repository has been archived by the owner on May 18, 2023. It is now read-only.
Mock: Fix AfterFunc data race #42
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's a data race if we call Mock.AfterFunc and Mock.Add
concurrently.
The included test demonstrates this with `go test -race`.
```
$ go test -race
==================
WARNING: DATA RACE
Read at 0x00c0000a64e8 by goroutine 50:
github.com/benbjohnson/clock.(*internalTimer).Tick()
[...]/clock/clock.go:314 +0xc4
github.com/benbjohnson/clock.(*Mock).runNextTimer()
[...]/clock/clock.go:156 +0x1ce
github.com/benbjohnson/clock.(*Mock).Add()
[...]/clock/clock.go:96 +0x97
github.com/benbjohnson/clock.TestMock_AddAfterFuncRace.func3()
[...]/clock/clock_test.go:687 +0xa7
Previous write at 0x00c0000a64e8 by goroutine 49:
github.com/benbjohnson/clock.(*Mock).AfterFunc()
[...]/clock/clock.go:170 +0x178
github.com/benbjohnson/clock.TestMock_AddAfterFuncRace.func2()
[...]/clock/clock_test.go:677 +0xb9
Goroutine 50 (running) created at:
github.com/benbjohnson/clock.TestMock_AddAfterFuncRace()
[...]/clock/clock_test.go:683 +0x464
testing.tRunner()
[...]/go/1.17.2/libexec/src/testing/testing.go:1259 +0x22f
testing.(*T).Run·dwrap·21()
[...]/go/1.17.2/libexec/src/testing/testing.go:1306 +0x47
Goroutine 49 (finished) created at:
github.com/benbjohnson/clock.TestMock_AddAfterFuncRace()
[...]/clock/clock_test.go:673 +0x347
testing.tRunner()
[...]/go/1.17.2/libexec/src/testing/testing.go:1259 +0x22f
testing.(*T).Run·dwrap·21()
[...]/go/1.17.2/libexec/src/testing/testing.go:1306 +0x47
==================
--- FAIL: TestMock_AddAfterFuncRace (0.01s)
testing.go:1152: race detected during execution of test
FAIL
exit status 1
```
Fix the race by applying the same mutex used while reading `t.fn`
in `internalTimer.Tick`.
abhinav
added a commit
to uber-go/fx
that referenced
this pull request
Nov 18, 2021
This temporarily pins to a fork of benbjohnson/clock to validate the fix while we wait for benbjohnson/clock#42 to be reviewed upstream.
abhinav
added a commit
to uber-go/fx
that referenced
this pull request
Nov 18, 2021
This temporarily pins to a fork of benbjohnson/clock to validate the fix while we wait for benbjohnson/clock#42 to be reviewed upstream.
abhinav
added a commit
to uber-go/fx
that referenced
this pull request
Nov 18, 2021
This temporarily pins to a fork of benbjohnson/clock to validate the fix while we wait for benbjohnson/clock#42 to be reviewed upstream.
abhinav
added a commit
to uber-go/fx
that referenced
this pull request
Nov 19, 2021
This temporarily pins to a fork of benbjohnson/clock to validate the fix while we wait for benbjohnson/clock#42 to be reviewed upstream.
|
I did run with the same issue on my own code. The proposed fix makes it work. |
This was referenced Sep 22, 2022
Closed
|
Closing in favor of #52 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's a data race if we call Mock.AfterFunc and Mock.Add
concurrently.
The included test demonstrates this with
go test -race.Fix the race by applying the same mutex used while reading
t.fnin
internalTimer.Tick.