go-patterns/smells/common-mistakes.md

# Common Go Mistakes and Code Smells

Patterns that Go programmers get wrong, extracted from what the Go stdlib and Kubernetes intentionally **avoid** or handle carefully.

---

## 1. Interface Pollution

### Pattern/Smell Name: Premature/Over-broad Interfaces

**Source citation:** `/tmp/go-src/src/io/io.go` lines 86-307 (22 interfaces, each 1-2 methods), `/tmp/go-src/src/net/http/server.go` lines 89-95 (Handler: 1 method)

**What they do:** Every stdlib interface has 1-2 methods. `io.Reader` has one method. `http.Handler` has one method. Interfaces are defined at the **consumer** site (where they're used), not the producer site (where they're implemented).

**What they avoid:** Large interfaces, interfaces defined by the implementer, interfaces defined "just in case."

**Why:** Small interfaces are easy to implement and compose. Large interfaces force all implementations to satisfy every method — most of which the caller doesn't need. The Go proverb: "The bigger the interface, the weaker the abstraction."

**Anti-pattern (the smell):**
```go
// BAD: "Java-style" interface defined by the implementor
type UserService interface {
    GetUser(id string) (*User, error)
    CreateUser(u *User) error
    UpdateUser(u *User) error
    DeleteUser(id string) error
    ListUsers(filter Filter) ([]*User, error)
    GetUserByEmail(email string) (*User, error)
    ValidateUser(u *User) error
    // 15 more methods...
}
```

**Correct Go pattern:**
```go
// GOOD: Interface defined at the consumer with only what it needs
type UserGetter interface {
    GetUser(id string) (*User, error)
}

// The handler only declares what it actually calls
func NewHandler(users UserGetter) *Handler { ... }
```

**Key rules from stdlib:**
- Accept interfaces, return structs
- Define interfaces where they're *consumed*, not where they're *produced*
- 1-2 methods per interface is ideal; 3 is suspicious; 5+ is almost certainly wrong
- Use `var _ Interface = (*Struct)(nil)` to verify implementations at compile time (server.go line 1802)

---

## 2. Error Swallowing

### Pattern/Smell Name: Ignoring Returned Errors

**Source citation:** `/tmp/go-src/src/net/http/server.go` (18 `if err != nil` blocks, every one handles the error), `/tmp/go-src/src/net/http/transport.go` (25 `if err != nil` blocks, all handled)

**What they do:** Every error returned from a function call is either:
1. Returned to the caller (propagated)
2. Logged and the operation retried/aborted
3. Wrapped with context via `fmt.Errorf("...: %w", err)`

The stdlib **never** does `_ = someFunc()` on functions that return errors (with rare, documented exceptions).

**Why:** Swallowed errors create silent failures. A database write that silently fails. A file close that leaks a descriptor. A network send that drops data. These manifest as data corruption or resource exhaustion hours later.

**Anti-pattern (the smell):**
```go
// BAD: Error thrown away
f.Close()                          // Close can fail (unflushed writes)
json.Unmarshal(data, &result)      // silently leaves result zero-valued
resp, _ := http.Get(url)           // panic on nil resp

// BAD: "log and forget" where the caller can't tell it failed
func SaveUser(u *User) {
    if err := db.Save(u); err != nil {
        log.Printf("failed to save user: %v", err)
        // caller thinks it succeeded!
    }
}
```

**Correct Go pattern:**
```go
// GOOD: Error propagated with context
func SaveUser(u *User) error {
    if err := db.Save(u); err != nil {
        return fmt.Errorf("save user %s: %w", u.ID, err)
    }
    return nil
}

// GOOD: Close errors handled for writes
defer func() {
    if cerr := f.Close(); cerr != nil && err == nil {
        err = cerr
    }
}()
```

---

## 3. Naked Goroutines Without Shutdown

### Pattern/Smell Name: Goroutine Leaks from Unmanaged Spawning

**Source citation:** `/tmp/go-src/src/net/http/transport.go` lines 2111-2112 (readLoop/writeLoop with closech), `/tmp/go-src/src/net/http/server.go` lines 3553 (serve with context cancellation), `/tmp/go-src/src/net/http/main_test.go` (goroutine leak detector)

**What they do:** Every goroutine spawned in the stdlib has a clear shutdown path:
- A channel that signals termination (`closech`, `done`)
- A context that gets cancelled
- A `sync.WaitGroup` that tracks completion
- Tests that verify no goroutines leak

The stdlib HTTP package spawns goroutines (readLoop, writeLoop, serve) but each one:
1. Has a `closech` channel or context for signaling
2. Has a `writeLoopDone` channel to confirm exit
3. Is tracked by `sync.WaitGroup` (in httptest.Server)
4. Is verified by `afterTest` goroutine leak detection

**Why:** A goroutine without a shutdown path runs forever. In a server handling 10K connections, that's 10K leaked goroutines per restart cycle. Go's garbage collector cannot collect goroutines — they must exit.

**Anti-pattern (the smell):**
```go
// BAD: Goroutine with no way to stop it
func StartWorker(ch <-chan Task) {
    go func() {
        for task := range ch {
            process(task)
        }
    }()
    // Who closes ch? What if nobody does? Goroutine leaks forever.
}

// BAD: Fire-and-forget goroutine
func HandleRequest(r *Request) {
    go sendAnalytics(r)  // What if this blocks? No timeout, no tracking.
}
```

**Correct Go pattern (from stdlib transport.go):**
```go
// GOOD: Goroutine with explicit lifecycle
type persistConn struct {
    reqch         chan requestAndChan  // communication
    closech       chan struct{}        // signal shutdown
    writeLoopDone chan struct{}        // confirm exit
}

go pconn.readLoop()   // reads from closech to know when to stop
go pconn.writeLoop()  // closes writeLoopDone on exit

// Shutdown:
func (pc *persistConn) close(err error) {
    close(pc.closech)      // signal both loops
    <-pc.writeLoopDone     // wait for confirmation
}
```

---

## 4. sync.Mutex Where atomic Suffices

### Pattern/Smell Name: Over-synchronization with Mutexes

**Source citation:** `/tmp/go-src/src/net/http/server.go` lines 298-300 (atomic for simple state), `/tmp/go-src/src/net/http/transport.go` line 786-787 (comment explaining why NOT atomic)

**What they do:** Use `atomic.Bool`, `atomic.Pointer`, `atomic.Uint64` for simple flags and state that is read/written independently. Reserve `sync.Mutex` for guarding multi-field invariants.

The stdlib explicitly documents the decision: `didRead bool // not atomic.Bool because only one goroutine (the user's) should be accessing` (transport.go line 786).

**Why:** Mutexes serialize all access. For a single boolean flag read by many goroutines (like `inShutdown`), atomic operations are lock-free and orders of magnitude faster under contention.

**Anti-pattern (the smell):**
```go
// BAD: Mutex for a single boolean
type Server struct {
    mu       sync.Mutex
    shutdown bool
}

func (s *Server) IsShutdown() bool {
    s.mu.Lock()
    defer s.mu.Unlock()
    return s.shutdown
}
```

**Correct Go pattern (from stdlib):**
```go
// GOOD: Atomic for independent flag (server.go line 3144)
type Server struct {
    inShutdown atomic.Bool
}

func (s *Server) shuttingDown() bool {
    return s.inShutdown.Load()
}

// GOOD: Packed state in atomic (server.go line 300)
curState atomic.Uint64 // packed (unixtime<<8|uint8(ConnState))

// GOOD: Mutex only when multiple fields must be consistent together
mu sync.Mutex  // guards hijackedv
hijackedv bool
```

**Rule of thumb:**
- Single value read/written independently → `atomic`
- Multiple values that must be consistent together → `sync.Mutex`
- Read-heavy, rare writes → `sync.RWMutex`

---

## 5. Channel Misuse

### Pattern/Smell Name: Channels for Simple Synchronization

**Source citation:** `/tmp/go-src/src/net/http/server.go` (uses channels for signaling/coordination, never for simple shared state), `/tmp/go-src/src/net/http/transport.go` lines 2242-2259 (channels with clear ownership documentation)

**What they do:**
- Channels for **signaling** events (closech, done)
- Channels for **passing ownership** of data between goroutines (reqch, writech)
- Comments documenting which goroutine reads/writes each channel
- Buffered channels with explicit reasoning about buffer size

**What they avoid:**
- Channels for sharing mutable state
- Unbuffered channels where buffered would prevent deadlock
- Channels where a mutex would be simpler

**Why:** Rob Pike's "Don't communicate by sharing memory; share memory by communicating" is often misunderstood as "always use channels." The stdlib uses mutexes freely when appropriate. Channels are for goroutine coordination, not data sharing.

**Anti-pattern (the smell):**
```go
// BAD: Channel as a glorified mutex
type Counter struct {
    ch chan int
}

func NewCounter() *Counter {
    c := &Counter{ch: make(chan int, 1)}
    c.ch <- 0
    return c
}

func (c *Counter) Increment() {
    val := <-c.ch
    c.ch <- val + 1
}

// BAD: Unbuffered channel causing goroutine leak
func doWork() <-chan Result {
    ch := make(chan Result)  // unbuffered!
    go func() {
        result := expensiveWork()
        ch <- result  // blocks forever if nobody reads
    }()
    return ch
}
```

**Correct Go pattern (from stdlib transport.go):**
```go
// GOOD: Channels with documented ownership and correct buffering
type persistConn struct {
    reqch     chan requestAndChan  // written by roundTrip; read by readLoop
    writech   chan writeRequest    // written by roundTrip; read by writeLoop
    closech   chan struct{}        // closed when conn closed
    writeErrCh chan error          // buffer 1: passes write error from writeLoop to readLoop
}

// GOOD: Error channel buffered to 1 — writer never blocks
writeErrCh: make(chan error, 1)
```

---

## 6. init() Abuse

### Pattern/Smell Name: Overusing init() for Side Effects

**Source citation:** `/tmp/go-src/src/net/http/http2.go` lines 37-47 (one of the few init() uses — for package-level wiring that cannot be done any other way)

**What they do:** The stdlib uses `init()` sparingly and only for:
1. Registering protocol handlers with other packages (http2.go)
2. Setting up test hooks (export_test.go — only compiled during tests)
3. Package-level wiring that has no other option

The entire `net/http` package has only 3 `init()` functions in production code, and each has a comment explaining why it's necessary.

**Why:**
- `init()` runs implicitly — no one calls it, you can't control when, you can't skip it
- Makes testing harder (can't test without side effects)
- Creates import order dependencies
- Makes programs slow to start (all inits run at startup)
- Impossible to pass configuration to

**Anti-pattern (the smell):**
```go
// BAD: Database connection in init
func init() {
    db, err := sql.Open("postgres", os.Getenv("DATABASE_URL"))
    if err != nil {
        log.Fatal(err)  // crashes the program at import time!
    }
    globalDB = db
}

// BAD: Configuration in init
func init() {
    cfg = loadConfig("/etc/myapp/config.yaml")  // hard-coded path, untestable
}

// BAD: Registering handlers in init
func init() {
    http.HandleFunc("/api/users", handleUsers)  // global state mutation
}
```

**Correct Go pattern:**
```go
// GOOD: Explicit initialization with error handling
func NewApp(cfg Config) (*App, error) {
    db, err := sql.Open("postgres", cfg.DatabaseURL)
    if err != nil {
        return nil, fmt.Errorf("open database: %w", err)
    }
    return &App{db: db}, nil
}

// GOOD: Only use init() for unavoidable package wiring (like stdlib does)
func init() {
    // Must set these at init time because the types are in different packages
    // and there's no other way to wire them without import cycles.
    http2.NoBody = NoBody
}
```

---

## 7. Premature Concurrency

### Pattern/Smell Name: Goroutines Where Sequential Code Suffices

**Source citation:** `/tmp/go-src/src/encoding/json/` (zero goroutines in production code — purely sequential), `/tmp/go-src/src/net/http/server.go` (goroutines only at the connection accept level, not in handlers)

**What they do:** The stdlib only introduces concurrency where it's structurally necessary (handling multiple connections). JSON encoding, HTTP header parsing, cookie handling — all purely sequential despite being hot paths.

**Why:** Goroutines have overhead (stack allocation, scheduler pressure, synchronization). Sequential code is easier to reason about, debug, and profile. Concurrency should solve a structural problem (waiting for I/O, handling multiple clients) — not speed up CPU-bound work (that's `GOMAXPROCS`'s job).

**Anti-pattern (the smell):**
```go
// BAD: Goroutines for CPU-bound work that shares nothing
func ProcessItems(items []Item) []Result {
    results := make([]Result, len(items))
    var wg sync.WaitGroup
    for i, item := range items {
        wg.Add(1)
        go func(i int, item Item) {
            defer wg.Done()
            results[i] = transform(item)  // transform is a pure function!
        }(i, item)
    }
    wg.Wait()
    return results
}
// This spawns 10000 goroutines for 10000 items. A simple loop is faster.
```

**Correct Go pattern:**
```go
// GOOD: Sequential unless concurrency is structurally needed
func ProcessItems(items []Item) []Result {
    results := make([]Result, len(items))
    for i, item := range items {
        results[i] = transform(item)
    }
    return results
}

// GOOD: Concurrency only when I/O-bound (as in the HTTP server)
for {
    rw, err := l.Accept()  // blocks waiting for connection (I/O)
    go c.serve(connCtx)    // each connection needs its own goroutine because it blocks on I/O
}
```

---

## 8. Interface Satisfaction Checks (What They DO)

### Pattern/Smell Name: Compile-Time Interface Verification

**Source citation:** `/tmp/go-src/src/net/http/server.go` line 1802, `/tmp/go-src/src/net/http/transport.go` line 2141

**What they do:** Use `var _ Interface = (*Struct)(nil)` to verify at compile time that a type implements an interface.

**Why:** Without this, you discover a missing method at runtime (when someone calls the interface method). The blank identifier assignment costs nothing at runtime but catches missing methods at compile time.

**Code example (stdlib):**
```go
// server.go line 1802
var _ closeWriter = (*net.TCPConn)(nil)

// transport.go line 2141
var _ io.ReaderFrom = (*persistConnWriter)(nil)

// server.go line 4071
var _ Pusher = (*timeoutWriter)(nil)
```

**Anti-pattern:** Relying on runtime panics to discover interface mismatches.

---

## 9. Error Wrapping Without Context

### Pattern/Smell Name: Bare Error Propagation

**Source citation:** `/tmp/go-src/src/net/http/transport.go` line 2395, `/tmp/go-src/src/net/http/request.go` line 96

**What they do:** Wrap errors with `fmt.Errorf("context: %w", err)` to add information about what operation failed. Each layer adds its context.

**Why:** A bare `return err` from deep in a call stack gives you "connection refused" with no indication of what was being connected to, for what purpose, with what parameters. Wrapped errors create a trace: `"save user alice: write to database: connection refused"`.

**Anti-pattern (the smell):**
```go
// BAD: Bare propagation
func GetUser(id string) (*User, error) {
    data, err := db.Query(...)
    if err != nil {
        return nil, err  // caller sees "connection refused" — useless
    }
}

// BAD: Losing the original error
func GetUser(id string) (*User, error) {
    data, err := db.Query(...)
    if err != nil {
        return nil, fmt.Errorf("database error")  // original error is gone
    }
}
```

**Correct Go pattern (from stdlib):**
```go
// GOOD: Wrapping with %w preserves the chain
// transport.go line 2395
return fmt.Errorf("net/http: HTTP/1.x transport connection broken: %w", err)

// GOOD: Custom error type with full context
// request.go line 96
func badStringError(what, val string) error {
    return fmt.Errorf("%s %q", what, val)
}
```

---

## 10. Kubernetes-Scale Anti-Patterns

### Pattern/Smell Name: Patterns That Work at Scale but Smell at Small Scale

**Source citation:** `/tmp/kubernetes-src/pkg/util/iptables/iptables_test.go` (fakeexec), `/tmp/kubernetes-src/staging/src/k8s.io/client-go/` (testify usage)

**What Kubernetes does that you probably shouldn't:**

1. **Assertion libraries (testify)** — Kubernetes uses `assert.Equal`, `require.NoError` extensively. The Go team explicitly avoids these. At Kubernetes scale (4M+ lines), the consistency might help. At normal scale, plain `if/t.Errorf` gives better error messages.

2. **Fake executors** — Kubernetes fakes the entire `exec.Command` interface for testing iptables rules. This is necessary because they can't run iptables in CI. For most codebases, integration tests with real dependencies are more valuable than elaborate fakes.

3. **Generated deepcopy methods** — Kubernetes generates `DeepCopy()` on every API type. At 500+ types, this is necessary. At 10 types, just write the copy manually.

4. **Interface-everything for testing** — Kubernetes wraps system calls, time, filesystem behind interfaces purely for testability. At their scale, you can't spin up real infrastructure. At small scale, use `httptest.Server` and real databases in Docker.

**The lesson:** Patterns born from scale requirements become anti-patterns when applied at the wrong scale. Every abstraction has a cost; only pay it when you have the problem it solves.

---

## 11. Context Misuse

### Pattern/Smell Name: Storing Values in context.Context

**Source citation:** `/tmp/go-src/src/net/http/server.go` lines 1060-1080 (context used for cancellation, NOT for passing data between layers)

**What they do:** Use context for:
- Cancellation signals (`WithCancel`, `WithTimeout`)
- Deadline propagation
- Request-scoped values that cross API boundaries (only via well-typed keys)

**What they avoid:** Using context as a grab-bag for function parameters, replacing explicit arguments with context values.

**Anti-pattern (the smell):**
```go
// BAD: Context as parameter smuggling
ctx = context.WithValue(ctx, "userID", userID)
ctx = context.WithValue(ctx, "db", database)
ctx = context.WithValue(ctx, "logger", logger)

func HandleRequest(ctx context.Context) {
    userID := ctx.Value("userID").(string)  // type assertion panic risk
    db := ctx.Value("db").(*sql.DB)         // invisible dependency
}
```

**Correct Go pattern (from stdlib):**
```go
// GOOD: Context for cancellation, explicit params for data
func HandleRequest(ctx context.Context, userID string, db *sql.DB) {
    // ...
}

// GOOD: If you must use context values, use typed keys
type contextKey struct{}
var serverContextKey = contextKey{}

// Only for cross-cutting concerns that truly cross API boundaries
// (tracing, auth tokens, request IDs)
```

---

## 12. Using fmt.Sprintf in Hot Paths

### Pattern/Smell Name: Allocation in Performance-Critical Code

**Source citation:** `/tmp/go-src/src/net/http/header.go` (hand-rolled header writing, no fmt), `/tmp/go-src/src/encoding/json/encode.go` (manual byte buffer manipulation)

**What they do:** The stdlib avoids `fmt.Sprintf`, `string concatenation (+)`, and `[]byte(string)` conversions in hot paths. Instead, they write directly to `[]byte` buffers or use `strings.Builder`.

**Why:** Each `fmt.Sprintf` allocates. In a server handling 100K requests/second, that's 100K allocations/second for a single log line. The GC notices.

**Anti-pattern (the smell):**
```go
// BAD: Allocation per request
func (h Header) Get(key string) string {
    return fmt.Sprintf("%s", h[key][0])  // unnecessary allocation
}

// BAD: String concatenation in a loop
var result string
for _, item := range items {
    result += item.Name + ","  // O(n²) allocations
}
```

**Correct Go pattern:**
```go
// GOOD: Direct buffer writes (like stdlib does)
var buf strings.Builder
for _, item := range items {
    buf.WriteString(item.Name)
    buf.WriteByte(',')
}
return buf.String()  // single allocation for final string
```