Code review: why does Counter always report 0, and what else is wrong?
Question 491MediumGo 1.22 to 1.25
type Counter struct {
mu sync.Mutex
n int
}
func (c Counter) Inc() { // BUG: value receiver
c.mu.Lock()
defer c.mu.Unlock()
c.n++
}
func (c Counter) Value() int { return c.n }
With a value receiver, every call gets a copy of the struct, including a copy of the mutex. Inc increments the copy's n and locks the copy's mutex, so the original is never changed and never protected. Value() always returns 0.
Copying a sync.Mutex, WaitGroup, Once, or Cond after first use is wrong in general. A copy of a locked mutex is itself locked. go vet's copylocks check reports "Inc passes lock by value".
Fix: use pointer receivers for every method, and take the lock in reads as well:
func (c *Counter) Inc() {
c.mu.Lock()
defer c.mu.Unlock()
c.n++
}
func (c *Counter) Value() int {
c.mu.Lock()
defer c.mu.Unlock()
return c.n
}
Also check for the other copy points: for _, c := range counters, passing Counter by value, and returning it by value. For a plain counter, atomic.Int64 is simpler.
More on Tricky Output & Code-Review Puzzles
- Q489Why doesn't this compile, and why does Go refuse it?
- Q490When is the receiver bound for a method value? What prints?
- Q492What happens if you add or delete map entries while ranging over the map?
- Q493Why won't m["a"].count++ compile, and what happens with a nil map?
- Q494Spot the bug: why is err nil after the if block?
- Q495Find the two WaitGroup bugs.