Go concurrency high-risk patterns: a code review checklist

A review manual built from Uber's study of roughly 46 million lines of Go (about 1,100 production data-race fixes), the LeakProf goroutine-leak study, and Uber's PLDI 2022 paper on real-world data races in Go. Every item is a concrete BAD/GOOD pattern with a checklist you can block a PR on.

---

1. Motivation

Go makes concurrency cheap to write and expensive to get right. Three language-level design decisions cause most real-world data races:

1. Transparent capture-by-reference in closures. A go func() { ... }() silently captures every free variable by reference. Java lambdas, by contrast, capture by value and require effective finality, which rules out this bug class entirely. Uber's data shows Go microservices running roughly 8x more concurrency than equivalent Java services, and closure capture is the single largest source of races that don't involve locks.
2. Value semantics on stateful types. sync.Mutex, sync.WaitGroup, and slice headers are plain structs. The compiler will happily copy them, silently, and a copied lock protects nothing.
3. Non-thread-safe built-in containers with array-like syntax. map[k] = v looks like independent memory access; it is not. Slices carry mutable metadata (ptr/len/cap) that races independently of element data. Slice-related races account for 39% of Uber's fixes, built-in maps for another 3.8%.

So review concurrent Go with suspicion: don't trust the syntactic sugar, reason from the memory model. This manual lists the patterns worth blocking a PR over.

---

2. Go concurrency best practices: a reviewer's primer

Every finding below reduces to one of four rules:

1. Memory visibility over syntactic elegance. For every go statement, enumerate the captured free variables and their lifetimes. Read an anonymous function as a list of shared-memory edges.
2. Type semantics first. Wherever sync.* appears, verify it is never copied: pointer receivers, pointer parameters, a clean copylocks run. A copied lock provides zero mutual exclusion.
3. Never trust implicit behavior. Races hide in compiler-generated writes (named returns), runtime randomness (select fairness), and hidden struct copies (slice headers, interface boxing). In concurrent code, "explicit over implicit" stops being a style preference.
4. Ownership or synchronization: pick exactly one per variable. Either a value is confined to one goroutine (handed over via channel or parameter), or every access, reads included, goes through the same synchronization primitive. Anything in between is a race.

The default posture: share memory by communicating. Use channels for ownership transfer and completion signaling, and mutexes for simple shared state such as counters, caches, and maps. The bugs in module 6 come from mixing both idioms on the same variable.

---

3. Closures and variable capture

Empirical background: Uber's race study identifies transparent reference capture by anonymous goroutines as the dominant non-lock bug class. The three patterns below are its most frequent instances.

3.1 Loop iteration variable capture

• Risk level: CRITICAL
• Empirical background: Before Go 1.22, a for ... range loop allocates one iteration variable and reuses it across all iterations. Every goroutine launched in the loop body captures the same address; by the time the scheduler runs them, the parent has already advanced the variable. The result is a read/write race, and typically all workers observing the final element. Go 1.22 gives each iteration a fresh variable, but the pattern remains live in codebases pinned to older go directives, in captured pointers to loop-scoped data, and in any loop where the variable is reassigned in the body.

// ❌ BAD: every goroutine shares the address of `job`
for _, job := range jobs {
    go func() {
        ProcessJob(job) // race: parent writes `job` each iteration; child reads it
    }()
}

// ✅ GOOD (A): explicit parameter: the value is copied at spawn time
for _, job := range jobs {
    go func(j Job) {
        ProcessJob(j)
    }(job)
}

// ✅ GOOD (B): shadow the variable inside the loop body
for _, job := range jobs {
    j := job // per-iteration copy
    go func() { ProcessJob(j) }()
}

• Checklist: For every go func() inside a loop, verify the iteration variable is passed as an explicit parameter or shadowed (x := x), or that the module's go directive is ≥ 1.22 and no pointer to the variable escapes the iteration. Even read-only use races if the parent keeps writing.

---

3.2 Shared capture of the idiomatic err variable

• Risk level: HIGH
• Empirical background: Go's multi-return idiom concentrates writes onto a single function-scoped err. Because x, err := f() followed by y, err := g() reuses the existing err (only y is newly declared), a goroutine that captures err shares it with every later assignment in the parent. Uber classifies this as one of the most recurrent capture races because it looks like textbook Go.

// ❌ BAD: goroutine writes the enclosing scope's err
x, err := Foo()
go func() {
    var y int
    y, err = Bar() // race: writes the outer err
    log.Println(y)
}()
z, err := Baz() // race: concurrent write to the same err

// ✅ GOOD: goroutine-local error variable
x, err := Foo()
go func() {
    y, gErr := Bar() // independent variable; report via channel/errgroup if needed
    if gErr != nil {
        errCh <- gErr
    }
    log.Println(y)
}()
z, err := Baz()

• Checklist: Inside every go func, search for reads or assignments of an err declared in the enclosing function. Require a locally declared error (gErr, or := inside the goroutine) and an explicit channel or errgroup.Group if the result must propagate.

---

3.3 Named return value captured by a goroutine

• Risk level: HIGH
• Empirical background: A named return value is a variable scoped to the entire function body. return 20 is not a constant return; the compiler lowers it to result = 20; return, an implicit write to that variable. If a still-running goroutine captured result, that compiler-generated write races with the goroutine's access. The deferred-closure variant (defer func() { result = ... }()) combined with an async reader is the same bug.

// ❌ BAD: named return read by a goroutine
func Calculate() (result int) {
    go func() {
        log.Println(result) // race: reads the named return variable
    }()
    return 20 // compiler emits a hidden write: result = 20
}

// ✅ GOOD: sever the reference chain with locals; avoid named returns near `go`
func Calculate() int {
    snapshot := 0
    go func(v int) { // pass a copy; goroutine holds no reference to the return path
        log.Println(v)
    }(snapshot)
    return 20
}

• Checklist: If a function declares named return values and launches goroutines (or registers deferred closures that outlive synchronization), verify no goroutine references the named variables. Prefer converting to anonymous returns with explicit locals.

---

4. Composite types and concurrency safety

Empirical background: Slices are the most racy type in Uber's corpus, at 39% of all fixed races; built-in maps add 3.8%, with map usage in Go running about 1.34x higher than in Java. The root cause in both cases is the illusion that container syntax implies independent, safe memory access.

4.1 Slice header (ptr/len/cap) metadata race

• Risk level: CRITICAL
• Empirical background: A slice value is a three-word header: data pointer, length, capacity. append may rewrite all three on reallocation. Reviewers routinely miss two consequences. First, passing a slice by value into a goroutine copies the header outside any lock, and that copy races with a concurrent locked append. Second, a locked append in one goroutine does not protect unlocked reads (len, indexing, range) elsewhere, because those read the same header words.

// ❌ BAD: safeAppend is locked, but the header copy at spawn time is not
func ProcessAll(uuids []string) {
    var myResults []string
    var mu sync.Mutex
    safeAppend := func(res string) {
        mu.Lock()
        myResults = append(myResults, res)
        mu.Unlock()
    }
    for _, id := range uuids {
        go func(id string, res []string) { // header of myResults copied HERE, outside mu
            _ = res
            safeAppend(Foo(id))
        }(id, myResults)
    }
}

// ✅ GOOD: never pass the mutable slice across the goroutine boundary;
// confine every access (reads included) behind the same mutex
func ProcessAll(uuids []string) []string {
    var (
        myResults []string
        mu        sync.Mutex
        wg        sync.WaitGroup
    )
    for _, id := range uuids {
        wg.Add(1)
        go func(id string) {
            defer wg.Done()
            res := Foo(id)
            mu.Lock()
            myResults = append(myResults, res)
            mu.Unlock()
        }(id)
    }
    wg.Wait()
    return myResults
}

• Checklist: Flag any mutable slice passed by value into a go func while another goroutine may append to it; the header copy itself is the race. Verify all accesses (append, index, len, range) share one mutex, or restructure so each goroutine writes a disjoint pre-allocated index (results[i] = ... with a wg barrier before any read).

---

4.2 Built-in map concurrent access (the "different keys are safe" illusion)

• Risk level: CRITICAL (crashes the runtime; not recoverable)
• Empirical background: m[key] reads like array indexing, so developers assume distinct keys touch distinct memory. A Go map is a sparse hash structure; any insert can trigger bucket growth and re-shuffling that invalidates concurrent readers' positions. The runtime's race-lite detector kills the process with fatal error: concurrent map writes. This is not a recoverable panic; no recover() will save the service.

// ❌ BAD: concurrent writes to *different* keys still crash the runtime
errMap := make(map[string]error)
for _, id := range ids {
    go func(uuid string) {
        errMap[uuid] = GetOrder(uuid) // fatal: concurrent map writes
    }(id)
}

// ✅ GOOD: every access to the map goes through one mutex
var mu sync.Mutex
errMap := make(map[string]error)
for _, id := range ids {
    go func(uuid string) {
        err := GetOrder(uuid)
        mu.Lock()
        errMap[uuid] = err
        mu.Unlock()
    }(id)
}

• Checklist: For every built-in map reachable from more than one goroutine: if there is any write, require sync.Mutex/sync.RWMutex around all accesses (reads too), or sync.Map for append-mostly and read-mostly workloads. "Different keys" is never an accepted justification.

---

5. Sync primitives and semantic pitfalls

Empirical background: Go's primitives are value types with internal state. The bugs in this module are not exotic. They are the compiler silently honoring value semantics on types whose whole purpose depends on identity.

5.1 sync.Mutex copied by value

• Risk level: CRITICAL
• Empirical background: A mutex passed by value, or held in a struct with a value receiver, is duplicated at every call. Each goroutine then locks its own private copy, and mutual exclusion is silently disabled. The failure mode depends on what else gets copied: if the guarded state is a plain field, updates land on the copy and are silently discarded; if the copy still references shared state (a pointer, slice, or map field), the goroutines race on that shared memory under useless private locks. go vet's copylocks catches most instances, yet the pattern persists wherever vet isn't enforced in CI.

// ❌ BAD: the value receiver copies the mutex, so each call locks a private copy,
// while the pointer field still refers to the one shared int: a real data race
type Counter struct {
    mu    sync.Mutex
    count *int
}

func (c Counter) Add() {
    c.mu.Lock()
    defer c.mu.Unlock()
    *c.count++ // race: concurrent writes to the shared int under different locks
}

// ✅ GOOD: pointer receiver: one lock identity guarding one counter
type Counter struct {
    mu    sync.Mutex
    count int
}

func (c *Counter) Add() {
    c.mu.Lock()
    defer c.mu.Unlock()
    c.count++
}

• Checklist: Any struct embedding sync.Mutex/RWMutex/WaitGroup/Cond must use pointer receivers on every method and be passed only by pointer. Enforce go vet -copylocks (and golangci-lint govet) as a merge gate rather than relying on eyeballs.

---

5.2 sync.WaitGroup: Add inside the goroutine

• Risk level: HIGH
• Empirical background: wg.Add(1) placed inside the child goroutine races against the parent's wg.Wait() via scheduling delay: the parent can reach Wait() while the counter is still zero, return immediately, and read incomplete results. The happens-before edge only exists if Add executes in the parent before the go statement.

// ❌ BAD: Add races with Wait via scheduling delay
for _, item := range items {
    go func(id int) {
        wg.Add(1) // may run AFTER wg.Wait() has already returned
        defer wg.Done()
        results[id] = Process(id)
    }(item)
}
wg.Wait()

// ✅ GOOD: count in the parent, before spawning
for _, item := range items {
    wg.Add(1)
    go func(id int) {
        defer wg.Done()
        results[id] = Process(id)
    }(item)
}
wg.Wait()

• Checklist: wg.Add(n) must appear in the spawning goroutine's control flow, lexically before the go statement it accounts for. Reject any Add inside the spawned function body.

---

5.3 defer LIFO ordering vs. wg.Done()

• Risk level: MEDIUM
• Empirical background: Deferred calls run last-in-first-out, so wg.Done() should be registered first: LIFO then runs it last, after every cleanup. Registered last instead, Done fires first, the parent's Wait() unblocks, and the parent observes state the child has not finished cleaning up. The completion signal must be the goroutine's final side effect.

// ❌ BAD: LIFO means Done (registered last) fires first, before cleanup completes
func worker() {
    defer doCleanup() // registered first ⇒ runs last, AFTER Done has signaled
    defer wg.Done()   // registered last ⇒ runs first ⇒ Wait() unblocks too early
    // ... work ...
}

// ✅ GOOD: register Done first so LIFO runs it last, after all cleanup
func worker() {
    defer wg.Done()   // registered first ⇒ executes last
    defer doCleanup() // executes before Done
    // ... work ...
}

• Checklist: In any goroutine with multiple defers, verify wg.Done() (or the equivalent completion signal, such as a channel close or cond broadcast) is registered first, so LIFO guarantees it executes last, after every side effect the parent may observe.

---

6. Mixed concurrency patterns and channel hazards

Empirical background: Uber's data-race study and LeakProf both show that races and leaks cluster where message passing and shared memory are mixed on the same state, and where select timeout idioms interact with worker goroutines. LeakProf found blocked channel sends to be the dominant production leak signature.

6.1 select timeout branch writing shared state (future/context race)

• Risk level: HIGH
• Empirical background: In Future/Promise implementations, select listens on a result channel and ctx.Done(). When the context fires, the parent writes an error field on the shared future, while the background worker, unaware of the cancellation, may be writing the same field. The cancellation path and the completion path race on identical memory.

// ❌ BAD: timeout branch and worker both write f.err
select {
case res := <-f.ch:
    f.err = res.err
case <-ctx.Done():
    f.err = ctx.Err() // race: worker goroutine may be writing f.err right now
}

// ✅ GOOD: consolidate results through message passing; write only parent-local state
var finalErr error
select {
case res := <-f.ch: // worker communicates results ONLY via the channel
    finalErr = res.err
case <-ctx.Done():
    finalErr = ctx.Err()
}
return finalErr

• Checklist: In every select containing a cancellation or timeout branch (ctx.Done(), time.After), verify the branch bodies write only variables owned by the selecting goroutine. A shared struct field may be written by exactly one side; results cross goroutines through the channel, never through fields written from both ends.

---

6.2 Unbuffered channel + early return: a permanent goroutine leak

• Risk level: HIGH (unbounded memory and resource leak)
• Empirical background: LeakProf's production data identifies this as the canonical leak: a worker sends on an unbuffered channel, the parent times out and returns, and no receiver ever arrives. The worker blocks forever on the send. Its stack, its captured references, and anything they pin can never be collected. Each request leaks one goroutine, and the service bleeds memory until it OOMs.

// ❌ BAD: after timeout, nobody receives; the sender blocks forever
ch := make(chan Result) // unbuffered
go func() {
    ch <- DoWork() // permanently blocked once parent has returned
}()
select {
case res := <-ch:
    return res
case <-time.After(timeout):
    return nil // worker goroutine leaked
}

// ✅ GOOD: capacity 1 lets the sender complete and exit even if abandoned
ch := make(chan Result, 1)
go func() {
    ch <- DoWork() // buffered send always succeeds; goroutine terminates
}()
select {
case res := <-ch:
    return res
case <-time.After(timeout):
    return nil // result is dropped into the buffer; worker exits; both are GC'd
}

• Checklist: For every one-shot request/response channel paired with a timeout, cancellation, or early-return path: require capacity ≥ 1, or a guaranteed-drain mechanism (context plumbed into the worker with a select on the send). Ask of every ch <-: "if the receiver has already given up, does this send still complete?"

---

6.3 Non-deterministic select case resolution

• Risk level: MEDIUM

• Empirical background: When multiple select cases are ready at the same time (say the result arrives in the same instant the timeout fires), the runtime chooses uniformly at random for fairness; there is no priority. Code that assumes "completion wins over timeout" harbors an untestable, unreproducible boundary bug. If the branches write competing state, the randomness also masks the race in most runs.

• Technical mechanism: runtime.selectgo shuffles the polling order of ready cases. Any invariant that depends on which ready case wins is unsound by construction. Where priority is required, it must be encoded explicitly, for example with a final non-blocking drain after taking the timeout branch:

// ❌ BAD: assumes the result branch wins when both cases are ready at once
func await(ctx context.Context, ch <-chan *Result) (*Result, error) {
    select {
    case res := <-ch:
        return res, nil
    case <-ctx.Done():
        // If the result and the cancellation arrive in the same instant,
        // the runtime picks a branch at random: a completed result is
        // sometimes dropped here even though it is sitting in the channel.
        return nil, ctx.Err()
    }
}

// ✅ GOOD: explicit priority: after the timeout fires, drain a late result
func await(ctx context.Context, ch <-chan *Result) (*Result, error) {
    select {
    case res := <-ch:
        return res, nil
    case <-ctx.Done():
        select {
        case res := <-ch: // result and cancellation raced; prefer the result
            return res, nil
        default:
            return nil, ctx.Err()
        }
    }
}

• Checklist: (1) Confirm no two select branches write the same variable with different values under the assumption of ordering. (2) Where the design needs "result beats cancellation," require an explicit non-blocking drain as above. (3) Treat every ready case as executable on any run, including in tests.

---

7. Test code concurrency risks

Empirical background: Uber's study calls out test code explicitly: races fixed in production frequently survive in _test.go files, where -race coverage is spotty and t.Parallel() quietly multiplies exposure.

7.1 Table-driven tests + t.Parallel() capture

• Risk level: HIGH
• Empirical background: This is pattern 3.1 wearing a testing idiom. t.Run(name, func(t *testing.T) { t.Parallel(); ... }) returns immediately and defers the subtest body until the parent test function completes. By that point the loop has finished and tc holds the final case (pre-1.22). All parallel subtests then validate the same last row: tests pass vacuously or race on tc.

// ❌ BAD: every parallel subtest captures the same tc
for _, tc := range tests {
    t.Run(tc.Name, func(t *testing.T) {
        t.Parallel()
        process(tc.Data) // race + wrong data: all subtests see the last tc
    })
}

// ✅ GOOD: rebind per iteration (mandatory pre-1.22; harmless documentation after)
for _, tc := range tests {
    tc := tc // per-iteration copy
    t.Run(tc.Name, func(t *testing.T) {
        t.Parallel()
        process(tc.Data)
    })
}

• Checklist: Wherever t.Run and t.Parallel() co-occur in a table loop, verify the case variable is rebound (tc := tc) or the module targets Go ≥ 1.22. Also verify the test actually fails when broken; vacuous last-row-only tests hide behind green CI.

---

7.2 Parallel tests mutating shared or global state

• Risk level: MEDIUM
• Empirical background: t.Parallel() shortens wall-clock time but interleaves subtests that were written assuming isolation. Singletons, package-level variables, os.Setenv, t.Chdir, shared fixtures, and non-thread-safe APIs under test all become cross-test races, producing flaky, order-dependent failures that never reproduce locally.

// ❌ BAD: parallel subtests fight over package-level state
var currentUser string // package-level

func TestAccess(t *testing.T) {
    t.Run("admin", func(t *testing.T) {
        t.Parallel()
        currentUser = "admin" // race: runs concurrently with "guest" below
        checkAccess(t)
    })
    t.Run("guest", func(t *testing.T) {
        t.Parallel()
        currentUser = "guest" // race: both subtests write the same global
        checkAccess(t)
    })
}

// ✅ GOOD: state is injected per subtest, never ambient
func TestAccess(t *testing.T) {
    t.Run("admin", func(t *testing.T) {
        t.Parallel()
        svc := NewService(WithUser("admin")) // per-test instance, no globals
        checkAccess(t, svc)
    })
    t.Run("guest", func(t *testing.T) {
        t.Parallel()
        svc := NewService(WithUser("guest"))
        checkAccess(t, svc)
    })
}

• Checklist: For every test enabling t.Parallel(): trace the call chain for writes to package-level variables, singletons, env vars, or the working directory. If the system under test is not thread-safe, the test must not be parallel; remove t.Parallel() rather than adding ad-hoc test locks.

---

Appendix: automated guardrails (CI/CD enforcement)

Don't leave all of this to human reviewers. Enforce what you can mechanically:

1. Race detector on every CI run (non-negotiable; -count=1 defeats test caching):

go test -race -count=1 ./...

2. Goroutine leak detection in core packages via Uber's goleak:

func TestMain(m *testing.M) {
    goleak.VerifyTestMain(m)
}

3. Static analysis (.golangci.yml): enable govet (with copylocks), staticcheck, bodyclose, contextcheck. copylocks alone catches everything in §5.1.

4. Production-side leak monitoring (the LeakProf signal): export runtime.NumGoroutine() or pprof goroutine profiles and alert on monotonic growth, since §6.2 leaks never show up in unit tests.

---

References

• Data Race Patterns in Go (Uber Engineering)
• LeakProf: Featherlight In-Production Goroutine Leak Detection (Uber Engineering)
• A Study of Real-World Data Races in Golang (Uber, PLDI 2022)
• Go Concurrency Patterns: Timing Out, Moving On (The Go Blog)