Blog / Go

678 words

  • go
  • mutex
  • concurrency
  • data-race
  • go-vet
  • debugging

Why a Mutex-Protected Go Counter Still Races After a Struct Copy

You put a sync.Mutex next to the field it guards, you lock in every method, and the race detector still complains. The usual culprit is a single character: a method with a value receiver, or an innocent-looking assignment, has copied the struct, mutex and all.

Here is the smallest version of the bug.

package main

import (
	"fmt"
	"sync"
)

type Counter struct {
	mu sync.Mutex
	n  int
}

func (c *Counter) Inc() {
	c.mu.Lock()
	defer c.mu.Unlock()
	c.n++
}

// Bug: value receiver, so the whole Counter is copied on the call.
func (c Counter) Value() int {
	c.mu.Lock()
	defer c.mu.Unlock()
	return c.n
}

func main() {
	var c Counter
	var wg sync.WaitGroup
	for i := 0; i < 100; i++ {
		wg.Add(1)
		go func() {
			defer wg.Done()
			c.Inc()
			_ = c.Value()
		}()
	}
	wg.Wait()
	fmt.Println(c.Value())
}

Run it with go run -race . and you get a data race report, even though every access appears to take the lock.

The copy happens before the lock does

With a value receiver, Go copies the struct into the method's receiver before a single line of the method runs. That copy reads mu and n from the original, and nothing is holding the original's lock while it does so.

So the sequence is: goroutine A is inside Inc, writing n. Goroutine B calls Value, and the compiler-generated copy reads n at the same moment. That is an unsynchronised read against a write. The lock inside Value then locks the copy's mutex, which nobody else can see, so it protects nothing.

It can deadlock too

The copy also takes the mutex's state. If the original happens to be locked at that instant, the copy is born locked, and nobody will ever unlock it.

c.mu.Lock()
snapshot := c // snapshot.mu is now locked as well
snapshot.mu.Lock() // blocks forever

In a real program the copy usually happens on another goroutine while the lock is held elsewhere, so the hang is intermittent. Those are the worst kind to find in production.

Quick detour: why does copying a mutex even compile?

Hang on, why doesn't the compiler stop this? Because Go has no "non-copyable" type concept. sync.Mutex is an ordinary struct of integers, and assignment of any struct is a plain memory copy.

The standard library's answer is a convention plus tooling. Types that must not be copied have pointer-receiver Lock and Unlock methods, and go vet has a copylocks check that looks for them. The sync docs say a Mutex must not be copied after first use.

Where copies sneak in

The value receiver is the classic one, but it is not the only one. All of these copy the lock:

  • A value receiver on any method of the type.
  • Passing the struct by value to a function.
  • Assigning it, for example snapshot := c.
  • Ranging over a slice of them with for _, c := range counters.
  • Returning the struct by value from a constructor after it has been used.
  • Embedding it in another struct that is then copied.

The range case is a nasty one, because the loop variable is a copy and any Lock you call on it locks the copy.

Let vet find it

You do not need to spot these by eye. go vet runs copylocks by default, and it flags the example above.

$ go vet ./...
./main.go:20:9: Value passes lock by value: Counter contains sync.Mutex

The exact wording and line numbers vary, but the message names the type and the lock it contains. Run vet in CI and this whole class of bug mostly stops reaching review.

The fix: pointers all the way

Make every method a pointer receiver, and hand out *Counter rather than Counter. Mixing receiver kinds on one type is what caused this, and Go's own guidance is to stay consistent.

func (c *Counter) Value() int {
	c.mu.Lock()
	defer c.mu.Unlock()
	return c.n
}

func NewCounter() *Counter { return &Counter{} }

If callers need a point-in-time copy of the data, return the data, not the struct. An int copied out while holding the lock is perfectly safe, as Value above shows. For bigger state, return a separate plain struct without the mutex in it.

Checking it worked

  1. Run go vet ./...; the copylocks warning should be gone.
  2. Run go test -race ./... with a test that hammers Inc and Value from many goroutines.
  3. Keep the race flag on in CI. It only reports races that actually happen during the run, so a clean pass is evidence, not proof.

Note that for a plain counter, atomic.Int64 sidesteps all this, and it carries its own no-copy marker so vet complains about copies of that too. A mutex earns its place once the lock guards more than one field.

← Back to the blog