Go

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

All 38 Tricky Output & Code-Review Puzzles questions