NotificationsYou must be signed in to change notification settings
Fork1k
Star11.1k

Commitee35ad3

authored

fix: remove hold WaitGroup in TestConcurrentFetch (#19617)

Fixes:coder/internal#950Pretty sure the intention of the `hold` wait group is to try to get the two goroutines that the test starts running at the same time. But, that should be the case for two goroutines started anyway.The use of `hold` doesn't actually guarantee concurrent execution of `Acquire`, just that both goroutines get as far as `Done()` --- the go scheduler could run them serially without incident.So I've chosen to just remove the use of `hold` to simplify.But, for posterity, the data race was due to incrementing by 1 in the loop along with the goroutine that calls Done. You could increment by 1 and then back down to 0 before the second iteration of the loop starts. This then causes a data race with calling `Wait()` in the first goroutine and `Add()` in the second iteration. c.f.https://pkg.go.dev/sync#WaitGroup.Add

1 parent6606d8b commitee35ad3Copy full SHA for ee35ad3

File tree

1 file changed

-9

lines changed

coderd/files
- cache_test.go

1 file changed

-9

lines changed

`‎coderd/files/cache_test.go‎`

Lines changed: 3 additions & 9 deletions

Original file line number	Diff line number	Diff line change
`@@ -100,21 +100,15 @@ func TestConcurrentFetch(t *testing.T) {`
`100`	`100`	`ctx:=dbauthz.AsFileReader(testutil.Context(t,testutil.WaitShort))`
`101`	`101`
`102`	`102`	`// Expect 2 calls to Acquire before we continue the test`
`103`		`-var (`
`104`		`-hold sync.WaitGroup`
`105`		`-wg sync.WaitGroup`
`106`		`-)`
	`103`	`+varwg sync.WaitGroup`
`107`	`104`
	`105`	`+wg.Add(2)`
`108`	`106`	`forrange2 {`
`109`		`-hold.Add(1)`
`110`	`107`	`// TODO: wg.Go in Go 1.25`
`111`		`-wg.Add(1)`
`112`	`108`	`gofunc() {`
`113`	`109`	`deferwg.Done()`
`114`		`-hold.Done()`
`115`		`-hold.Wait()`
`116`	`110`	`_,err:=cache.Acquire(ctx,dbM,fileID)`
`117`		`-require.NoError(t,err)`
	`111`	`+assert.NoError(t,err)`
`118`	`112`	`}()`
`119`	`113`	`}`
`120`	`114`

0 commit comments

Comments

(0)

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Commitee35ad3

File tree

1 file changed

1 file changed

`‎coderd/files/cache_test.go‎`

0 commit comments