Golang testing/synctest: improve interactions between Wait, time, and the race detector

This is an offshoot of #74327.

The following program contains a data race as reported by the race detector:

func Test(t *testing.T) {
        synctest.Test(t, func(t *testing.T) {
                var v int
                go func() {
                        time.Sleep(1)
                        v = 0 // data race: write
                }()
                _ = v // data race: read
                time.Sleep(2)
        })
}

It should be obvious that the access to v is racy when executed outside of a synctest bubble: The time.Sleep(1) call does not establish a happens-before relationship between the read and the write. Within the fake time environment of a synctest bubble, the time.Sleep(1) will not return until after the read has occurred. However, we deliberately do not tell the race detector to establish a happens-before relationship here, to allow it to detect racing operations in a system under test that might be obscured by the use of fake time.

The synctest.Wait function establishes a happens-before relationship between all activity in a bubble and the Wait call returning. Therefore, this program does not contain a data race:

func Test(t *testing.T) {
        synctest.Test(t, func(t *testing.T) {
                var v int
                go func() {
                        time.Sleep(1)
                        v = 0
                }()
                time.Sleep(2)
                synctest.Wait() // v = 0 happens-before Wait returns
                _ = v
        })
}

However, perhaps surprisingly, the race detector does report a race in this program:

func Test(t *testing.T) {
        synctest.Test(t, func(t *testing.T) {
                var v int
                go func() {
                        time.Sleep(1)
                        v = 0
                }()
                synctest.Wait()
                _ = v
                synctest.Wait()
                time.Sleep(2)
        })
}

The user's intent in this test seems clear, but nothing in this program establishes a happens-before relationship between the two accesses to v.

Perhaps there's some tweak we can make to the happens-before relationships established by Wait that would allow us to avoid reporting a data race in this case. I'm not sure exactly what that would be.

We do want to take care not to make a change that masks data races in goroutines which do not call Wait, such as in this example:

func Test(t *testing.T) {
        synctest.Test(t, func(t *testing.T) {
                var v int
                go func() {
                        time.Sleep(1)
                        v = 0
                }()
                go func() {
                        time.Sleep(2)
                        _ = v
                }()
                // None of these Waits should establish a happens-before relationship
                // between the two goroutines above.
                synctest.Wait()
                time.Sleep(1)
                synctest.Wait()
                time.Sleep(1) 
        })
}

Comment From: gabyhelp

Related Issues

• runtime/race: running with -race misses races (mismatch with memory model) #37355 (closed)
• sync: WaitGroup showing up with race detected #8054 (closed)
• sync/atomic: data races in tests #8389 (closed)
• runtime/race: race detector sometimes misses race on []byte copy #36794 (closed)
• testing/synctest: unexpected data race after synctest.Wait #74327 (closed)
• http/httptest: waitgroup race detection seems to give false positives relating to httptest.Server.Close #7660 (closed)
• internal/synctest: failure of TestHappensBefore on race builder #72750 (closed)
• runtime/race: data race hidden by fake synchronization #8320 (closed)
• testing: data race in FailNow #7401 (closed)
• testing: race detected in goroutine can report an error in an unnamed test #60083 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}