Why does golang race detector complains about the following code:
package main
import (
"fmt"
"sync"
)
type Counter struct {
value int
mtx *sync.Mutex
}
func NewCounter() *Counter {
return &Counter {0, &sync.Mutex{}}
}
func (c *Counter) inc() {
c.mtx.Lock()
c.value++
c.mtx.Unlock()
}
func (c Counter) get() int {
c.mtx.Lock()
res := c.value
c.mtx.Unlock()
return res
}
func main() {
var wg sync.WaitGroup
counter := NewCounter()
max := 100
wg.Add(max)
// consumer
go func() {
for i := 0; i < max ; i++ {
value := counter.get()
fmt.Printf("counter value = %d
", value)
wg.Done()
}
}()
// producer
go func() {
for i := 0; i < max ; i++ {
counter.inc()
}
}()
wg.Wait()
}
When I run the code above with -race I'm getting the following warnings:
==================
WARNING: DATA RACE
Read at 0x00c0420042b0 by goroutine 6:
main.main.func1()
main.go:39 +0x72
Previous write at 0x00c0420042b0 by goroutine 7:
main.(*Counter).inc()
main.go:19 +0x8b
main.main.func2()
main.go:47 +0x50
Goroutine 6 (running) created at:
main.main()
main.go:43 +0x167
Goroutine 7 (running) created at:
main.main()
main.go:49 +0x192
==================
If I change func (c Counter) get() int to func (c *Counter) get() int then everything is working fine. It turns out that the receiver type for get() function should be a pointer. And I'm confused why that is. I'm aware of "-copylocks" but in this case mtx is a pointer, not value. If I change 'mtx' to be value and run program with vet -copylocks I get this warning:
main.go:23: get passes lock by value: main.Counter contains sync.Mutex`
That makes sense.
note: This question is not about how to implement thread safe counter
The race is because of the value receiver for the get() method. In order to call the get() method, a copy of the struct must be passed to the method expression. The method call without the syntactic sugar looks like:
value := Counter.get(*counter)
Copying the struct entails reading the value field, which happens before the method can take the lock, which is why the race is reported on the line of the method call, rather than within the method.
This is why changing the receiver to a pointer receiver will fix the issue. Also, since all receivers need to be pointers, the mtx can be left as a sync.Mutex value so it doesn't need to be initialized.
As @JimB points out, in the case of the get() method a copy is passed in which case the field value is read first and then copied, without any locking and since the same variable is mutated in inc(), the race is detected.
To further illustrate this point, you could also change the type of the field value to a pointer i.e. value *int in which case you should no longer see a race as now only the pointer is copied and not the underlying value. That said, to make intentions clearer, it is cleaner to change the get() receiver type to be a pointer.
Here is a good wiki around the same - https://github.com/golang/go/wiki/CodeReviewComments#receiver-type
A brief commentary on methods: https://golang.org/ref/spec#Method_values