1
0
mirror of https://github.com/golang/go synced 2024-11-21 21:44:40 -07:00

testing: fix racy t.done usage by adding a separate race-canary field

This commit is contained in:
Steven L 2024-06-03 13:51:31 -05:00
parent eaa7d9ff86
commit e6ce5de4a4
2 changed files with 26 additions and 16 deletions

View File

@ -710,6 +710,7 @@ func fRunner(f *F, fn func(*F)) {
// Report after all subtests have finished. // Report after all subtests have finished.
f.report() f.report()
f.doneRaceCanary = true
f.done = true f.done = true
f.setRan() f.setRan()
}() }()

View File

@ -600,6 +600,7 @@ type common struct {
failed bool // Test or benchmark has failed. failed bool // Test or benchmark has failed.
skipped bool // Test or benchmark has been skipped. skipped bool // Test or benchmark has been skipped.
done bool // Test is finished and all subtests have completed. done bool // Test is finished and all subtests have completed.
doneRaceCanary bool // Shadows all interactions with done except test-completion to trigger a race on misbehavior.
helperPCs map[uintptr]struct{} // functions to be skipped when writing file/line info helperPCs map[uintptr]struct{} // functions to be skipped when writing file/line info
helperNames map[string]struct{} // helperPCs converted to function names helperNames map[string]struct{} // helperPCs converted to function names
cleanups []func() // optional functions to be called at the end of the test cleanups []func() // optional functions to be called at the end of the test
@ -949,6 +950,7 @@ func (c *common) Fail() {
c.mu.Lock() c.mu.Lock()
defer c.mu.Unlock() defer c.mu.Unlock()
// c.done needs to be locked to synchronize checks to c.done in parent tests. // c.done needs to be locked to synchronize checks to c.done in parent tests.
_ = c.doneRaceCanary
if c.done { if c.done {
panic("Fail in goroutine after " + c.name + " has completed") panic("Fail in goroutine after " + c.name + " has completed")
} }
@ -960,6 +962,7 @@ func (c *common) Failed() bool {
c.mu.RLock() c.mu.RLock()
defer c.mu.RUnlock() defer c.mu.RUnlock()
_ = c.doneRaceCanary
if !c.done && int64(race.Errors()) > c.lastRaceErrors.Load() { if !c.done && int64(race.Errors()) > c.lastRaceErrors.Load() {
c.mu.RUnlock() c.mu.RUnlock()
c.checkRaces() c.checkRaces()
@ -1015,12 +1018,14 @@ func (c *common) log(s string) {
func (c *common) logDepth(s string, depth int) { func (c *common) logDepth(s string, depth int) {
c.mu.Lock() c.mu.Lock()
defer c.mu.Unlock() defer c.mu.Unlock()
_ = c.doneRaceCanary
if c.done { if c.done {
// This test has already finished. Try and log this message // This test has already finished. Try and log this message
// with our parent. If we don't have a parent, panic. // with our parent. If we don't have a parent, panic.
for parent := c.parent; parent != nil; parent = parent.parent { for parent := c.parent; parent != nil; parent = parent.parent {
parent.mu.Lock() parent.mu.Lock()
defer parent.mu.Unlock() defer parent.mu.Unlock()
_ = parent.doneRaceCanary
if !parent.done { if !parent.done {
parent.output = append(parent.output, parent.decorate(s, depth+1)...) parent.output = append(parent.output, parent.decorate(s, depth+1)...)
return return
@ -1672,9 +1677,13 @@ func tRunner(t *T, fn func(t *T)) {
} }
t.report() // Report after all subtests have finished. t.report() // Report after all subtests have finished.
// Do not lock t.done to allow race detector to detect race in case // Do not lock around t.done's race canary to allow race detector to detect
// the user does not appropriately synchronize a goroutine. // cases where the user does not appropriately synchronize a goroutine...
t.doneRaceCanary = true
t.mu.Lock()
// ...but do lock around t.done so after-done behavior is consistent.
t.done = true t.done = true
t.mu.Unlock()
if t.parent != nil && !t.hasSub.Load() { if t.parent != nil && !t.hasSub.Load() {
t.setRan() t.setRan()
} }