runtime: race detector: more aggressive scheduler perturbations

[bcmills]

A program was posted to golang-nuts, with the question “is this code thread [safe]?”

The program has a race: it executes a select with a default case many times in parallel, and the default case calls close on a channel. (You can see the race by inserting a runtime.Gosched(): https://play.golang.org/p/_PWTTCwPgi.)

To my surprise, the program runs without error even with the race detector enabled.

The Go Memory Model defines an order between sends and receives but not between closes and sends or closes and other closes, so I think this is technically a data race (and not just a logic race). The race detector should report it.

src/racy/main.go:

package main

import (
	"fmt"
	"sync/atomic"
	"time"
)

func main() {
	die := make(chan struct{})
	i := int32(0)
	closed := func() {
		select {
		case <-die:
			atomic.AddInt32(&i, 1)
		default:
			close(die)
			fmt.Println("closed")
		}
	}
	N := 100000
	for i := 0; i < N; i++ {
		go closed()
	}
	time.Sleep(10 * time.Second)
	fmt.Println(atomic.LoadInt32(&i))
}

$ go version
go version devel +1852573521 Wed Nov 1 16:58:36 2017 +0000 linux/amd64
$ go run -race src/racy/main.go
closed
99999

[ianlancetaylor]

CC @dvyukov

[dvyukov]

Calling recv and close concurrently is perfectly fine and in fact a common idiom. Behavior is defined and is useful.
It's calling close and send concurrently is problematic.

[bcmills]

Calling recv and close concurrently is perfectly fine and in fact a common idiom. Behavior is defined and is useful.

Yes, that's true. However, a select does not induce a happens-before relationship if its default case is taken (i.e., if the receive operation does not execute).

In this particular example, that makes the existence of the race itself nondeterministic: if goroutine scheduling is favorable, the first default case executes before any other goroutines are scheduled. Then subsequent goroutines take the receive case, and no race occurs.

However, if goroutine scheduling is unfavorable, the default case is taken by multiple goroutines before the close executes, and there is a close-after-close race.

---

Perhaps one way to make the race detector more useful for select-with-default races would be to make the scheduler more aggressive (i.e., less deterministic) when the race detector is active.

A more deterministic option might be to check whether the first (or last) send or close event on the channel happens before the select statement itself: if not, evaluate the default case up to the first observable write (or next synchronization point) in addition to whatever branch was actually taken.

[dvyukov]

The problem is more general and is not specific to selects. Consider:

if atomic.LoadUint32(&x) == 0 {
  atomic.StoreUint32(&x, 1)
  y++
}

If 2 goroutines sneak into the if before one of them sets x=1, there will be a race on y. But most of the time there won't be a race.
In another race detector (Relacy) I implemented systematic checking of all possible thread interleavings, but that requires executing the same code gazillions of times and is not generally feasible. Our best bet is randomizing execution order more. In the old tsan I implemented a feature called "scheduler shake" for this, but I never got around to implementing it in the new tsan.