commit b5d0544fd6489b181589806df31e786193c2ff6a Author: Rodin Date: Thu Apr 30 04:07:41 2026 -0700 docs: idiomatic Go patterns from stdlib + Kubernetes with source citations diff --git a/patterns/concurrency.md b/patterns/concurrency.md new file mode 100644 index 0000000..e9a6858 --- /dev/null +++ b/patterns/concurrency.md @@ -0,0 +1,707 @@ +# Go Concurrency Patterns + +Patterns extracted from the Go standard library source code. + +--- + +## 1. sync.Mutex — The Basic Lock + +### Source: `src/sync/mutex.go:18-34`, `src/sync/mutex.go:42-67` + +```go +// src/sync/mutex.go:18-34 +// A Mutex is a mutual exclusion lock. +// The zero value for a Mutex is an unlocked mutex. +// +// A Mutex must not be copied after first use. +type Mutex struct { + _ noCopy + mu isync.Mutex +} + +// src/sync/mutex.go:36-39 +type Locker interface { + Lock() + Unlock() +} + +// src/sync/mutex.go:43-46 +func (m *Mutex) Lock() { + m.mu.Lock() +} + +// src/sync/mutex.go:64-67 +func (m *Mutex) Unlock() { + m.mu.Unlock() +} +``` + +### Why + +- **Zero value is ready to use** — no constructor needed +- **Must not be copied** — enforced by `noCopy` field (go vet detects copies) +- **Not associated with a goroutine** — one goroutine can Lock, another can Unlock +- **Locker interface** — abstracts over Mutex and RWMutex + +### Idiomatic Usage + +```go +var mu sync.Mutex +mu.Lock() +defer mu.Unlock() +// ... critical section ... +``` + +### Anti-pattern + +```go +// DON'T: Copy a mutex +type Config struct { + mu sync.Mutex + data map[string]string +} +c2 := *c1 // COPIES the mutex — data race waiting to happen + +// DON'T: Forget defer +mu.Lock() +// if this panics, the mutex stays locked forever +doSomething() +mu.Unlock() +``` + +--- + +## 2. sync.Once — Exactly-Once Initialization + +### Source: `src/sync/once.go:12-36`, `src/sync/once.go:56-79` + +```go +// src/sync/once.go:12-23 +type Once struct { + _ noCopy + done atomic.Bool + m Mutex +} + +// src/sync/once.go:56-63 +func (o *Once) Do(f func()) { + if !o.done.Load() { + o.doSlow(f) + } +} + +// src/sync/once.go:65-72 +func (o *Once) doSlow(f func()) { + o.m.Lock() + defer o.m.Unlock() + if !o.done.Load() { + defer o.done.Store(true) + f() + } +} +``` + +### Why + +The implementation reveals a subtle guarantee: **when Do returns, f has finished**. The naive CAS-only approach (documented in the comment at line 56-63) would let the second caller return before f completes. The mutex ensures all callers wait. + +The `done` field is first in the struct for hot-path performance on amd64/386 (noted in comment at line 24-27). + +### Idiomatic Usage + +```go +var ( + instance *DB + once sync.Once +) + +func GetDB() *DB { + once.Do(func() { + instance = connectToDB() + }) + return instance +} +``` + +### Anti-pattern + +```go +// DON'T: Implement once yourself with a bool +var initialized bool +var mu sync.Mutex +func init() { + mu.Lock() + if !initialized { + // ... setup ... + initialized = true + } + mu.Unlock() +} + +// DON'T: Call Do recursively (deadlocks) +var once sync.Once +once.Do(func() { + once.Do(func() { /* deadlock */ }) +}) +``` + +--- + +## 3. sync.WaitGroup — Waiting for Goroutine Completion + +### Source: `src/sync/waitgroup.go:14-43`, `src/sync/waitgroup.go:236-260` + +```go +// src/sync/waitgroup.go:14-43 +// A WaitGroup is a counting semaphore typically used to wait +// for a group of goroutines or tasks to finish. +// +// Typically, a main goroutine will start tasks, each in a new +// goroutine, by calling WaitGroup.Go and then wait for all tasks to +// complete by calling WaitGroup.Wait. For example: +// +// var wg sync.WaitGroup +// wg.Go(task1) +// wg.Go(task2) +// wg.Wait() +type WaitGroup struct { + noCopy noCopy + state atomic.Uint64 + sema uint32 +} +``` + +### Go 1.25+: WaitGroup.Go + +```go +// src/sync/waitgroup.go:236-260 +func (wg *WaitGroup) Go(f func()) { + wg.Add(1) + go func() { + defer func() { + if x := recover(); x != nil { + panic(x) // don't call Done — let panic propagate + } + wg.Done() + }() + f() + }() +} +``` + +### Why + +`WaitGroup.Go` (new in Go 1.25) encapsulates the Add/go/Done pattern. Key design: if `f` panics, it re-panics **without** calling Done, preventing the main goroutine from racing to exit. + +### Classic Pattern (pre-Go 1.25) + +```go +var wg sync.WaitGroup +for _, item := range items { + wg.Add(1) + go func() { + defer wg.Done() + process(item) + }() +} +wg.Wait() +``` + +### Anti-pattern + +```go +// DON'T: Add inside the goroutine (race with Wait) +for _, item := range items { + go func() { + wg.Add(1) // might run after Wait is called! + defer wg.Done() + process(item) + }() +} +wg.Wait() + +// DON'T: Forget Done (Wait blocks forever) +wg.Add(1) +go func() { + process() + // forgot wg.Done() +}() +wg.Wait() // hangs +``` + +--- + +## 4. sync.Pool — Object Reuse for GC Pressure + +### Source: `src/sync/pool.go:44-63` + +```go +// src/sync/pool.go:44-63 +// A Pool is a set of temporary objects that may be individually saved and +// retrieved. +// +// Any item stored in the Pool may be removed automatically at any time without +// notification. If the Pool holds the only reference when this happens, the +// item might be deallocated. +// +// Pool's purpose is to cache allocated but unused items for later reuse, +// relieving pressure on the garbage collector. +type Pool struct { + noCopy noCopy + local unsafe.Pointer + localSize uintptr + victim unsafe.Pointer + victimSize uintptr + New func() any +} +``` + +### Why + +Pool is **not** a general cache. Items can vanish between GC cycles. It's for reducing allocation pressure on hot paths — `fmt` uses it for print buffers, `encoding/json` for encoder state. + +### Idiomatic Usage (from fmt package) + +```go +var ppFree = sync.Pool{ + New: func() any { return new(pp) }, +} + +func newPrinter() *pp { + p := ppFree.Get().(*pp) + p.reset() + return p +} + +func (p *pp) free() { + p.buf = p.buf[:0] // reset before returning + ppFree.Put(p) +} +``` + +### Anti-pattern + +```go +// DON'T: Use Pool for connection pooling (items disappear!) +var connPool = sync.Pool{ + New: func() any { return connectToDB() }, +} +// Connections may be GC'd at any time — use database/sql's pool instead + +// DON'T: Put dirty objects back without resetting +pool.Put(buf) // still has data from last use — memory leak or data leak +``` + +--- + +## 5. Channel as Done Signal (Context Pattern) + +### Source: `src/context/context.go:83-100` (Done channel), `src/io/pipe.go:42-45` + +```go +// src/context/context.go:83-100 +// Done returns a channel that's closed when work done on behalf of this +// context should be canceled. +Done() <-chan struct{} + +// src/io/pipe.go:42-45 +type pipe struct { + wrMu sync.Mutex + wrCh chan []byte + rdCh chan int + once sync.Once + done chan struct{} // closed on pipe close + rerr onceError + werr onceError +} +``` + +### Why + +A `chan struct{}` costs zero bytes per send and closing it broadcasts to all receivers simultaneously. This is the canonical "done" signal in Go. + +```go +select { +case <-ctx.Done(): + return ctx.Err() +case result := <-work: + return result, nil +} +``` + +### Anti-pattern + +```go +// DON'T: Use chan bool for done signals +done := make(chan bool) // wastes 1 byte per signal, true/false meaningless + +// DON'T: Send to done (only works once, only one receiver) +done <- struct{}{} // only unblocks one goroutine + +// DO: Close the channel (broadcasts to all) +close(done) +``` + +--- + +## 6. Context Propagation + +### Source: `src/context/context.go:37-48` (rules), `src/net/http/request.go:368-380` + +From the package doc: +```go +// src/context/context.go:37-48 +// Programs that use Contexts should follow these rules: +// +// Do not store Contexts inside a struct type; instead, pass a Context +// explicitly to each function that needs it. The Context should be the first +// parameter, typically named ctx: +// +// func DoSomething(ctx context.Context, arg Arg) error { +// // ... use ctx ... +// } +// +// Do not pass a nil Context, even if a function permits it. Pass context.TODO +// if you are unsure about which Context to use. +``` + +### Request context in net/http: + +```go +// src/net/http/request.go:368-380 +func (r *Request) WithContext(ctx context.Context) *Request { + if ctx == nil { + panic("nil context") + } + r2 := new(Request) + *r2 = *r + r2.ctx = ctx + return r2 +} +``` + +### Why + +Context flows **down** the call chain, never stored in structs. `WithContext` returns a shallow copy — the original request is not mutated. This is the immutable-context pattern. + +### Anti-pattern + +```go +// DON'T: Store context in a struct +type Server struct { + ctx context.Context // stale context persists beyond request lifecycle +} + +// DON'T: Pass nil context +doWork(nil, data) // use context.TODO() if unsure + +// DON'T: Put context anywhere other than first parameter +func doWork(data Data, ctx context.Context) // wrong position +``` + +--- + +## 7. Context Cancellation (WithCancel/WithTimeout) + +### Source: `src/context/context.go:242-249` (WithCancel), `src/net/http/server.go:4007-4050` (TimeoutHandler) + +```go +// src/context/context.go:242-249 +func WithCancel(parent Context) (ctx Context, cancel CancelFunc) { + c := withCancel(parent) + return c, func() { c.cancel(true, Canceled, nil) } +} +``` + +Real-world use — net/http TimeoutHandler: +```go +// src/net/http/server.go:4011-4014 +func (h *timeoutHandler) ServeHTTP(w ResponseWriter, r *Request) { + ctx, cancelCtx := context.WithTimeout(r.Context(), h.dt) + defer cancelCtx() + r = r.WithContext(ctx) + done := make(chan struct{}) + // ... + go func() { + h.handler.ServeHTTP(tw, r) + close(done) + }() + select { + case <-done: + // handler completed + case <-ctx.Done(): + // timeout + } +} +``` + +### Why + +`defer cancelCtx()` is critical — it releases resources (timers, goroutines) when the parent returns, even if the child hasn't timed out yet. The go vet tool checks for this. + +### Anti-pattern + +```go +// DON'T: Forget to call cancel (leaks goroutines) +ctx, _ := context.WithCancel(parent) // cancel function discarded! + +// DON'T: Cancel before work starts +ctx, cancel := context.WithTimeout(parent, 5*time.Second) +cancel() // immediately cancels — no work can happen +doWork(ctx) +``` + +--- + +## 8. Select with Done Channel + +### Source: `src/context/context.go:83-100` (Done in select), `src/io/pipe.go:51-60` + +```go +// src/io/pipe.go:51-60 +func (p *pipe) read(b []byte) (n int, err error) { + select { + case <-p.done: + return 0, p.readCloseError() + default: + } + + select { + case bw := <-p.wrCh: + nr := copy(b, bw) + p.rdCh <- nr + return nr, nil + case <-p.done: + return 0, p.readCloseError() + } +} +``` + +### Why + +The double-select pattern: first a non-blocking check (with `default`), then a blocking wait. The non-blocking check prevents a race where `done` was closed between the last operation and the current one. + +### Standard Context Select Pattern + +```go +// From context package doc (line 83-100) +func Stream(ctx context.Context, out chan<- Value) error { + for { + v, err := DoSomething(ctx) + if err != nil { + return err + } + select { + case <-ctx.Done(): + return ctx.Err() + case out <- v: + } + } +} +``` + +### Anti-pattern + +```go +// DON'T: Check ctx.Done() in a busy loop +for { + select { + case <-ctx.Done(): + return + default: + } + // busy-spins CPU at 100% +} +``` + +--- + +## 9. Goroutine-per-Connection (net/http Server) + +### Source: `src/net/http/server.go` (conceptual — the serve loop spawns goroutines per connection) + +```go +// The pattern (simplified from server.go serve loop): +for { + conn, err := listener.Accept() + if err != nil { + // handle + continue + } + go srv.handleConn(conn) // one goroutine per connection +} +``` + +### Why + +Go's goroutines are cheap (~2KB initial stack). The server doesn't need a thread pool or async/await — it spawns a goroutine per connection and lets the runtime scheduler handle multiplexing. + +### Anti-pattern + +```go +// DON'T: Limit yourself to a fixed thread pool for I/O-bound work +pool := make(chan struct{}, 10) // artificial limit on connections +for { + pool <- struct{}{} // blocks at 10 + conn := accept() + go func() { + defer func() { <-pool }() + handle(conn) + }() +} +// Only appropriate for CPU-bound work or resource-constrained scenarios +``` + +--- + +## 10. Channel as Synchronous Pipe (io.Pipe) + +### Source: `src/io/pipe.go:38-45`, `src/io/pipe.go:195-205` + +```go +// src/io/pipe.go:38-45 +type pipe struct { + wrMu sync.Mutex + wrCh chan []byte // writer sends data slices + rdCh chan int // reader returns bytes consumed + once sync.Once + done chan struct{} + rerr onceError + werr onceError +} + +// src/io/pipe.go:195-205 +func Pipe() (*PipeReader, *PipeWriter) { + pw := &PipeWriter{r: PipeReader{pipe: pipe{ + wrCh: make(chan []byte), + rdCh: make(chan int), + done: make(chan struct{}), + }}} + return &pw.r, pw +} +``` + +### Why + +`io.Pipe` connects a Writer to a Reader using **unbuffered channels** — each Write blocks until the corresponding Read consumes the data. No internal buffering means backpressure is automatic. The `done` channel signals when either end closes. + +### Pattern: Channel Pipeline + +```go +func generate(ctx context.Context) <-chan int { + out := make(chan int) + go func() { + defer close(out) + for i := 0; ; i++ { + select { + case out <- i: + case <-ctx.Done(): + return + } + } + }() + return out +} +``` + +### Anti-pattern + +```go +// DON'T: Forget to close channels (receivers block forever) +func produce() <-chan int { + ch := make(chan int) + go func() { + for i := 0; i < 10; i++ { + ch <- i + } + // forgot close(ch) — receivers hang on range + }() + return ch +} +``` + +--- + +## 11. database/sql Connection Opener Goroutine + +### Source: `src/database/sql/sql.go:836-843` + +```go +// src/database/sql/sql.go:836-843 +func OpenDB(c driver.Connector) *DB { + ctx, cancel := context.WithCancel(context.Background()) + db := &DB{ + connector: c, + openerCh: make(chan struct{}, connectionRequestQueueSize), + lastPut: make(map[*driverConn]string), + stop: cancel, + } + go db.connectionOpener(ctx) + return db +} +``` + +### Why + +A dedicated background goroutine (`connectionOpener`) processes connection requests from a buffered channel. The goroutine is controlled by a context — calling `cancel()` (stored as `db.stop`) shuts it down cleanly. This is the "long-lived worker goroutine with context shutdown" pattern. + +### Anti-pattern + +```go +// DON'T: Start background goroutines without shutdown mechanism +go func() { + for { + processWork() // runs forever, no way to stop it + } +}() +``` + +--- + +## 12. noCopy — Preventing Value Copies at Vet Time + +### Source: `src/sync/cond.go:120-126` + +```go +// src/sync/cond.go:120-126 +type noCopy struct{} + +// Lock is a no-op used by -copylocks checker from `go vet`. +func (*noCopy) Lock() {} +func (*noCopy) Unlock() {} +``` + +### Why + +Embedding `noCopy` in a struct makes `go vet` report an error when the struct is copied. All sync primitives use this because copying a locked mutex or in-use WaitGroup is always a bug. + +### Anti-pattern + +```go +// DON'T: Pass sync types by value +func doWork(wg sync.WaitGroup) { // copies the WaitGroup! + defer wg.Done() +} + +// DO: Pass by pointer +func doWork(wg *sync.WaitGroup) { + defer wg.Done() +} +``` + +--- + +## Summary: Concurrency Decision Guide + +| Need | Use | +|------|-----| +| Protect shared state | `sync.Mutex` + `defer Unlock()` | +| One-time initialization | `sync.Once` | +| Wait for N goroutines | `sync.WaitGroup` (prefer `.Go()` in 1.25+) | +| Reduce allocation pressure | `sync.Pool` (not for connections!) | +| Signal completion/cancellation | `chan struct{}` + `close()` | +| Deadline/timeout propagation | `context.WithTimeout` / `context.WithCancel` | +| Backpressure between producer/consumer | Unbuffered channels | +| Fan-out with results | Buffered channel + WaitGroup | +| Long-lived background worker | Goroutine + context cancellation | +| Prevent struct copying | Embed `noCopy` field | diff --git a/patterns/error-handling.md b/patterns/error-handling.md new file mode 100644 index 0000000..62b7526 --- /dev/null +++ b/patterns/error-handling.md @@ -0,0 +1,535 @@ +# Go Error Handling Patterns + +Patterns extracted from the Go standard library source code. + +--- + +## 1. Sentinel Errors + +### Source: `src/io/io.go:40-43` (EOF), `src/errors/errors.go:81-83` (ErrUnsupported) + +```go +// src/io/io.go:40-43 +// EOF is the error returned by Read when no more input is available. +// (Read must return EOF itself, not an error wrapping EOF, +// because callers will test for EOF using ==.) +var EOF = errors.New("EOF") + +// src/io/io.go:47-49 +var ErrUnexpectedEOF = errors.New("unexpected EOF") +``` + +```go +// src/errors/errors.go:81-83 +var ErrUnsupported = New("unsupported operation") +``` + +### Why + +Sentinel errors are package-level values that represent specific, well-known error conditions. They enable callers to test for specific failures: + +```go +if err == io.EOF { + // end of input — not an error, just done +} +``` + +**Critical rule from io.EOF's doc comment**: Read must return EOF itself, **not an error wrapping EOF**, because callers test for it with `==`. This is the distinction between sentinel errors (identity-checked) and wrapped errors (tree-checked). + +### Anti-pattern + +```go +// DON'T: Use string matching +if err.Error() == "EOF" { ... } // fragile, not guaranteed + +// DON'T: Return a new error each time for sentinel conditions +func Read() error { + return errors.New("EOF") // every call creates a new value, can't compare with == +} +``` + +--- + +## 2. errors.New — Minimal Error Construction + +### Source: `src/errors/errors.go:62-69` + +```go +// src/errors/errors.go:62-64 +func New(text string) error { + return &errorString{text} +} + +// src/errors/errors.go:66-69 +type errorString struct { + s string +} + +func (e *errorString) Error() string { + return e.s +} +``` + +### Why + +`errors.New` returns a pointer to a private struct. Each call creates a **distinct value** even with identical text — this is intentional for sentinel errors. Two calls to `errors.New("foo")` produce different errors (`!=`). + +The `error` interface itself is the smallest possible: +```go +type error interface { + Error() string +} +``` + +### Anti-pattern + +```go +// DON'T: Export the error type +type MyError string // callers can create values that accidentally == your sentinels + +// DON'T: Use plain strings as errors +func doThing() error { + return "something failed" // doesn't implement error interface +} +``` + +--- + +## 3. Error Wrapping with fmt.Errorf and %w + +### Source: `src/fmt/errors.go:13-23`, `src/fmt/errors.go:70-80` + +```go +// src/fmt/errors.go:13-23 +// Errorf formats according to a format specifier and returns the string +// as a value that satisfies error. +// +// If the format specifier includes a %w verb with an error operand, +// the returned error will implement an Unwrap method returning the operand. +// If there is more than one %w verb, the returned error will implement an +// Unwrap method returning a []error containing all the %w operands. +func Errorf(format string, a ...any) (err error) { ... } + +// src/fmt/errors.go:70-80 +type wrapError struct { + msg string + err error +} + +func (e *wrapError) Error() string { + return e.msg +} + +func (e *wrapError) Unwrap() error { + return e.err +} +``` + +### Why + +`%w` creates an error chain: the returned error wraps the original. `errors.Is` and `errors.As` walk this chain. Use `%w` when callers should be able to inspect the underlying cause. + +```go +// Wraps: callers can detect the original error +return fmt.Errorf("open config: %w", err) + +// Does NOT wrap: hides the original error +return fmt.Errorf("open config: %v", err) +``` + +### When to use %w vs %v + +- **%w**: When the wrapped error is part of your API contract. Callers can depend on it. +- **%v**: When you want to include the error text but NOT let callers depend on the underlying type. Use for implementation details. + +### Anti-pattern + +```go +// DON'T: Lose the original error +return errors.New("failed to open config") // original error vanished + +// DON'T: Wrap errors that aren't part of your contract with %w +return fmt.Errorf("internal: %w", internalErr) // now callers depend on internalErr's type +``` + +--- + +## 4. errors.Is — Checking Error Identity Through Chains + +### Source: `src/errors/wrap.go:30-44` + +```go +// src/errors/wrap.go:30-44 +func Is(err, target error) bool { + if err == nil || target == nil { + return err == target + } + isComparable := reflectlite.TypeOf(target).Comparable() + return is(err, target, isComparable) +} + +func is(err, target error, targetComparable bool) bool { + for { + if targetComparable && err == target { + return true + } + if x, ok := err.(interface{ Is(error) bool }); ok && x.Is(target) { + return true + } + switch x := err.(type) { + case interface{ Unwrap() error }: + err = x.Unwrap() + if err == nil { + return false + } + case interface{ Unwrap() []error }: + for _, err := range x.Unwrap() { + if is(err, target, targetComparable) { + return true + } + } + return false + default: + return false + } + } +} +``` + +### Why + +`errors.Is` walks the entire error tree (depth-first). It checks: +1. Direct equality (`err == target`) +2. Custom `Is(error) bool` method on the error +3. Then unwraps and recurses + +This means wrapped errors are transparent: +```go +err := fmt.Errorf("config: %w", os.ErrNotExist) +errors.Is(err, os.ErrNotExist) // true! walks the chain +``` + +### Anti-pattern + +```go +// DON'T: Use == directly on potentially-wrapped errors +if err == os.ErrNotExist { ... } // fails if err wraps ErrNotExist + +// DO: Use errors.Is +if errors.Is(err, os.ErrNotExist) { ... } // works through wrapping +``` + +--- + +## 5. errors.As — Extracting Error Types Through Chains + +### Source: `src/errors/wrap.go:96-120` + +```go +// src/errors/wrap.go:96-120 +func As(err error, target any) bool { + if err == nil { + return false + } + // ... validation ... + targetType := typ.Elem() + return as(err, target, val, targetType) +} +``` + +The `as` function (line 121+) walks the tree checking `AssignableTo` and custom `As(any) bool` methods. + +### Why + +Extract specific error types from wrapped chains: + +```go +var pathErr *fs.PathError +if errors.As(err, &pathErr) { + fmt.Println("failed path:", pathErr.Path) +} +``` + +### Go 1.24+: errors.AsType (generic version) + +From `src/errors/errors.go:48-56` doc: +```go +if perr, ok := errors.AsType[*fs.PathError](err); ok { + fmt.Println(perr.Path) +} +``` + +### Anti-pattern + +```go +// DON'T: Type-assert directly on potentially-wrapped errors +if pathErr, ok := err.(*fs.PathError); ok { ... } // fails if wrapped + +// DO: Use errors.As +var pathErr *fs.PathError +if errors.As(err, &pathErr) { ... } // works through wrapping +``` + +--- + +## 6. errors.Join — Multi-Error Aggregation + +### Source: `src/errors/join.go:20-39` + +```go +// src/errors/join.go:20-39 +func Join(errs ...error) error { + n := 0 + for _, err := range errs { + if err != nil { + n++ + } + } + if n == 0 { + return nil + } + e := &joinError{ + errs: make([]error, 0, n), + } + for _, err := range errs { + if err != nil { + e.errs = append(e.errs, err) + } + } + return e +} +``` + +The `joinError` type implements `Unwrap() []error`: +```go +// src/errors/join.go:57 (implicit from structure) +func (e *joinError) Unwrap() []error { + return e.errs +} +``` + +### Why + +For operations that can produce multiple errors (closing multiple resources, validating multiple fields), `Join` collects them into a single error. Both `Is` and `As` traverse the tree correctly. + +```go +var errs []error +errs = append(errs, closeDB()) +errs = append(errs, closeCache()) +return errors.Join(errs...) // nil if all nil +``` + +### Anti-pattern + +```go +// DON'T: Return only the last error +var lastErr error +for _, r := range resources { + if err := r.Close(); err != nil { + lastErr = err // silently loses previous errors + } +} +return lastErr +``` + +--- + +## 7. Custom Is() Method — Equivalence Classes + +### Source: `src/errors/wrap.go:42-44` (doc comment), `src/context/context.go:177-179` + +From the `errors.Is` doc: +```go +// An error type might provide an Is method so it can be treated as +// equivalent to an existing error. For example, if MyError defines +// +// func (m MyError) Is(target error) bool { return target == fs.ErrExist } +// +// then Is(MyError{}, fs.ErrExist) returns true. +``` + +Real example from context: +```go +// src/context/context.go:177-179 +type deadlineExceededError struct{} + +func (deadlineExceededError) Error() string { return "context deadline exceeded" } +func (deadlineExceededError) Timeout() bool { return true } +func (deadlineExceededError) Temporary() bool { return true } +``` + +### Why + +Custom `Is` methods let you define error equivalence beyond pointer identity. A `syscall.Errno` can match `fs.ErrExist` through its `Is` method, bridging OS-specific error codes to portable sentinel errors. + +### Anti-pattern + +```go +// DON'T: Make Is() too broad +func (e MyError) Is(target error) bool { + return true // matches everything — defeats the purpose +} +``` + +--- + +## 8. Error Wrapping in Custom Types (Unwrap pattern) + +### Source: `src/encoding/json/encode.go:276-293` + +```go +// src/encoding/json/encode.go:276-282 +type MarshalerError struct { + Type reflect.Type + Err error + sourceFunc string +} + +// src/encoding/json/encode.go:293 +func (e *MarshalerError) Unwrap() error { return e.Err } +``` + +### Why + +Custom error types carry structured data (which type failed, which function) while still participating in the error chain via `Unwrap()`. Callers can use `errors.As` to extract the `MarshalerError` AND use `errors.Is` to check the underlying cause. + +### Pattern Template + +```go +type OpError struct { + Op string + Path string + Err error +} + +func (e *OpError) Error() string { + return e.Op + " " + e.Path + ": " + e.Err.Error() +} + +func (e *OpError) Unwrap() error { + return e.Err +} +``` + +### Anti-pattern + +```go +// DON'T: Store error as string +type MyError struct { + Message string // lost the original error! +} + +// DON'T: Forget to implement Unwrap +type MyError struct { + Err error // has the error but errors.Is can't traverse it +} +func (e *MyError) Error() string { return e.Err.Error() } +// Missing: func (e *MyError) Unwrap() error { return e.Err } +``` + +--- + +## 9. ErrUnsupported — Feature Detection via Errors + +### Source: `src/errors/errors.go:76-83` + +```go +// src/errors/errors.go:76-83 +// ErrUnsupported indicates that a requested operation cannot be performed, +// because it is unsupported. For example, a call to os.Link when using a +// file system that does not support hard links. +// +// Functions and methods should not return this error but should instead +// return an error including appropriate context that satisfies +// +// errors.Is(err, errors.ErrUnsupported) +// +// either by directly wrapping ErrUnsupported or by implementing an Is method. +var ErrUnsupported = New("unsupported operation") +``` + +### Why + +This pattern separates "what happened" (detailed context) from "what kind of failure" (sentinel identity). Return a rich error that *wraps* or *matches* the sentinel: + +```go +return fmt.Errorf("chmod %s: %w", path, errors.ErrUnsupported) +``` + +Callers check the sentinel; the message provides context. + +### Anti-pattern + +```go +// DON'T: Return the sentinel directly without context +return errors.ErrUnsupported // no info about what operation or why +``` + +--- + +## 10. Error Value Patterns in net/http + +### Source: `src/net/http/server.go:39-56` + +```go +// src/net/http/server.go:39-56 +var ( + ErrBodyNotAllowed = internal.ErrBodyNotAllowed + ErrHijacked = errors.New("http: connection has been hijacked") + ErrContentLength = errors.New("http: wrote more than the declared Content-Length") + ErrWriteAfterFlush = errors.New("unused") // Deprecated +) +``` + +### Why + +- Errors are package-level `var` (not `const`) — they're pointer values +- Error strings start with the package name (`"http: ..."`) for disambiguation in logs +- Deprecated errors are kept for backward compatibility but marked clearly +- Internal errors can be aliased (`ErrBodyNotAllowed = internal.ErrBodyNotAllowed`) to share across internal packages + +### Convention: Error String Format + +``` +package: description +``` + +Examples from stdlib: +- `"http: connection has been hijacked"` +- `"sql: unknown driver %q (forgotten import?)"` +- `"json: unsupported type: %s"` + +### Anti-pattern + +```go +// DON'T: Capitalize error strings +errors.New("Connection has been hijacked") // Go convention: lowercase + +// DON'T: End error strings with punctuation +errors.New("connection failed.") // no trailing period +``` + +--- + +## Summary: Error Handling Decision Tree + +``` +Is this a specific, well-known condition? +├── YES → Sentinel error (package-level var) +│ └── Should callers detect it? → errors.Is +└── NO → Is there structured info to convey? + ├── YES → Custom error type with Unwrap() + │ └── Should callers extract it? → errors.As + └── NO → fmt.Errorf with %w (wraps) or %v (doesn't wrap) +``` + +| When to... | Use | +|---|---| +| Create a well-known error condition | `var ErrFoo = errors.New("pkg: foo")` | +| Add context while preserving cause | `fmt.Errorf("doing X: %w", err)` | +| Add context, hide internal cause | `fmt.Errorf("doing X: %v", err)` | +| Check for a specific condition | `errors.Is(err, ErrFoo)` | +| Extract structured error data | `errors.As(err, &target)` | +| Aggregate multiple errors | `errors.Join(err1, err2)` | +| Make custom types traversable | Implement `Unwrap() error` | +| Define error equivalence | Implement `Is(error) bool` | diff --git a/patterns/interfaces.md b/patterns/interfaces.md new file mode 100644 index 0000000..328c203 --- /dev/null +++ b/patterns/interfaces.md @@ -0,0 +1,390 @@ +# Go Interface Design Patterns + +Patterns extracted from the Go standard library source code. + +--- + +## 1. Small Interfaces (1-2 Methods) + +### Source: `src/io/io.go:80-92` (Reader), `93-103` (Writer), `105-109` (Closer) + +Go's most powerful interfaces have exactly **one method**: + +```go +// src/io/io.go:80-92 +type Reader interface { + Read(p []byte) (n int, err error) +} + +// src/io/io.go:93-103 +type Writer interface { + Write(p []byte) (n int, err error) +} + +// src/io/io.go:105-109 +type Closer interface { + Close() error +} +``` + +### Why + +Small interfaces are easy to implement and easy to compose. Any type can satisfy `io.Reader` by implementing a single method. This maximizes the number of types that can participate in the ecosystem — files, network connections, buffers, compressors, encryptors all satisfy `Reader`. + +### Anti-pattern + +```go +// DON'T: Giant interface that tries to cover everything +type FileSystem interface { + Open(name string) (File, error) + Create(name string) (File, error) + Remove(name string) error + Stat(name string) (FileInfo, error) + ReadDir(name string) ([]DirEntry, error) + MkdirAll(path string, perm FileMode) error + // ... 10 more methods +} +``` + +Large interfaces are hard to implement, hard to mock, and couple consumers to capabilities they don't need. + +--- + +## 2. Interface Composition + +### Source: `src/io/io.go:131-155` + +Compose small interfaces into larger ones only when needed: + +```go +// src/io/io.go:131-134 +type ReadWriter interface { + Reader + Writer +} + +// src/io/io.go:136-139 +type ReadCloser interface { + Reader + Closer +} + +// src/io/io.go:141-144 +type WriteCloser interface { + Writer + Closer +} + +// src/io/io.go:146-150 +type ReadWriteCloser interface { + Reader + Writer + Closer +} +``` + +### Why + +Composition lets you express precisely what you need. A function that only reads should accept `Reader`, not `ReadWriteCloser`. Callers provide the minimum; composers build up. + +### Anti-pattern + +```go +// DON'T: Define a big interface, then use only part of it +func processData(rw ReadWriteCloser) { + // only calls Read... why require Write and Close? +} +``` + +--- + +## 3. Accept Interfaces, Return Structs + +### Source: `src/io/io.go:461` (LimitReader), `src/io/io.go:618` (TeeReader) + +```go +// src/io/io.go:461 +func LimitReader(r Reader, n int64) Reader { return &LimitedReader{r, n} } + +// src/io/io.go:467-471 +type LimitedReader struct { + R Reader // underlying reader + N int64 // max bytes remaining +} +``` + +```go +// src/io/io.go:618-620 +func TeeReader(r Reader, w Writer) Reader { + return &teeReader{r, w} +} +``` + +### Why + +- **Accept interfaces**: Maximizes what callers can pass in — any `Reader` works. +- **Return structs** (or concrete types): Gives callers access to the full type, including fields and methods not in any interface. `LimitedReader` exposes `R` and `N` publicly so callers can inspect remaining bytes. + +The return type of `LimitReader` is `Reader` (interface), but the underlying value is `*LimitedReader` (struct). Functions like `io.Copy` can type-assert to `*LimitedReader` to optimize buffer sizes (line 425). + +### Anti-pattern + +```go +// DON'T: Accept concrete types +func LimitReader(r *os.File, n int64) Reader // too restrictive + +// DON'T: Return interfaces when you have useful struct fields +func NewServer() ServerInterface // hides useful config fields +``` + +--- + +## 4. Interface Satisfaction as a Compile-Time Check + +### Source: `src/io/io.go:645`, `src/net/http/server.go:4071` + +```go +// src/io/io.go:645 +var _ ReaderFrom = discard{} + +// src/net/http/server.go:4071 +var _ Pusher = (*timeoutWriter)(nil) +``` + +### Why + +These blank-identifier declarations cause a compile error if the type stops implementing the interface. They're documentation and safety net combined — no runtime cost. + +### Anti-pattern + +```go +// DON'T: Discover interface failures at runtime +func doSomething(w ResponseWriter) { + pusher := w.(Pusher) // panics if w doesn't implement Pusher +} +``` + +--- + +## 5. Interface-Based Polymorphism (sort.Interface) + +### Source: `src/sort/sort.go:16-41` + +```go +// src/sort/sort.go:16-41 +type Interface interface { + Len() int + Less(i, j int) bool + Swap(i, j int) +} +``` + +### Why + +The sort package defines *behavior* it needs, not data it operates on. Any collection — slices, linked lists, database result sets — can be sorted by implementing three methods. The algorithm is decoupled from the data structure. + +### Anti-pattern + +```go +// DON'T: Hardcode to a specific data type +func Sort(data []int) // only works for []int +func Sort(data []interface{}) // loses type safety +``` + +Note: Since Go 1.21, `slices.SortFunc` is preferred for slices (generic + faster), but `sort.Interface` remains the pattern for non-slice collections. + +--- + +## 6. The Adapter Pattern (HandlerFunc) + +### Source: `src/net/http/server.go:2334-2342` + +```go +// src/net/http/server.go:2334-2338 +// The HandlerFunc type is an adapter to allow the use of +// ordinary functions as HTTP handlers. If f is a function +// with the appropriate signature, HandlerFunc(f) is a +// Handler that calls f. +type HandlerFunc func(ResponseWriter, *Request) + +// src/net/http/server.go:2341-2342 +// ServeHTTP calls f(w, r). +func (f HandlerFunc) ServeHTTP(w ResponseWriter, r *Request) { + f(w, r) +} +``` + +### Why + +This bridges functions and interfaces. Any function with the right signature becomes a `Handler` via `HandlerFunc(myFunc)`. You get the simplicity of functions with the composability of interfaces. + +### Anti-pattern + +```go +// DON'T: Force users to create a struct just to implement an interface +type myHandler struct{} +func (h myHandler) ServeHTTP(w ResponseWriter, r *Request) { + // just calls a function anyway + handleHome(w, r) +} +``` + +--- + +## 7. Optional Interfaces (Runtime Feature Detection) + +### Source: `src/net/http/server.go:165-175` (Flusher), `src/net/http/server.go:183-206` (Hijacker) + +```go +// src/net/http/server.go:165-170 +type Flusher interface { + Flush() +} + +// src/net/http/server.go:183-206 +type Hijacker interface { + Hijack() (net.Conn, *bufio.ReadWriter, error) +} +``` + +Usage pattern (from doc comments): +```go +// Handlers should always test for this ability at runtime. +if flusher, ok := w.(Flusher); ok { + flusher.Flush() +} +``` + +### Why + +Not every `ResponseWriter` supports flushing or hijacking (HTTP/2 doesn't support Hijacker). Instead of bloating the main interface, optional capabilities are separate interfaces checked at runtime. This keeps the core interface small while allowing progressive enhancement. + +### Anti-pattern + +```go +// DON'T: Put optional capabilities in the main interface +type ResponseWriter interface { + Write([]byte) (int, error) + WriteHeader(int) + Header() Header + Flush() // not always supported! + Hijack() (net.Conn, *bufio.ReadWriter, error) // not always supported! +} +``` + +--- + +## 8. The Stringer Interface (Convention-Based Behavior) + +### Source: `src/fmt/print.go:63-66` + +```go +// src/fmt/print.go:63-66 +type Stringer interface { + String() string +} +``` + +### Why + +`fmt` checks if a value implements `Stringer` and calls `String()` for its text representation. This is a *convention*: any type in any package can participate without importing `fmt`. The interface acts as a protocol. + +Similarly, `GoStringer` (line 71) controls `%#v` output, and `Formatter` (line 58-61) gives total control over formatting. + +### Anti-pattern + +```go +// DON'T: Use type switches over known types +func printThing(v any) string { + switch v := v.(type) { + case MyType: return v.name + case OtherType: return v.label + // ... must know about every type + } +} +``` + +--- + +## 9. Interface Upgrade Pattern (WriterTo/ReaderFrom in io.Copy) + +### Source: `src/io/io.go:410-417` + +```go +// src/io/io.go:410-417 +func copyBuffer(dst Writer, src Reader, buf []byte) (written int64, err error) { + // If the reader has a WriteTo method, use it to do the copy. + // Avoids an allocation and a copy. + if wt, ok := src.(WriterTo); ok { + return wt.WriteTo(dst) + } + // Similarly, if the writer has a ReadFrom method, use it to do the copy. + if rf, ok := dst.(ReaderFrom); ok { + return rf.ReadFrom(src) + } + // ... fallback to buffer-based copy +} +``` + +### Why + +The core function works with the minimum interface (`Reader`/`Writer`) but *upgrades* behavior when richer interfaces are available. `*os.File` implements `ReadFrom` using `sendfile(2)` — zero-copy kernel transfer. The generic path works, but specialized implementations optimize transparently. + +### Anti-pattern + +```go +// DON'T: Always use the slow generic path +func Copy(dst Writer, src Reader) { + buf := make([]byte, 32*1024) + // always allocates, never leverages kernel optimizations +} +``` + +--- + +## 10. The driver.Driver Pattern (Plugin Interfaces) + +### Source: `src/database/sql/driver/driver.go:85-97`, `104-112` + +```go +// src/database/sql/driver/driver.go:85-97 +type Driver interface { + Open(name string) (Conn, error) +} + +// src/database/sql/driver/driver.go:104-112 +type DriverContext interface { + OpenConnector(name string) (Connector, error) +} +``` + +### Why + +The `database/sql` package defines interfaces that driver authors implement. The base interface (`Driver`) is minimal (one method). Extended capabilities (`DriverContext`, `Pinger`, `Execer`, `Queryer`) are opt-in — the `sql` package checks for them at runtime. This allows the ecosystem to grow without breaking existing drivers. + +### Anti-pattern + +```go +// DON'T: One massive interface all drivers must implement +type Driver interface { + Open(name string) (Conn, error) + OpenConnector(name string) (Connector, error) + Ping(ctx context.Context) error + // ... every driver must implement everything, even if not supported +} +``` + +--- + +## Summary: Go Interface Design Principles + +| Principle | Standard Library Example | +|-----------|------------------------| +| Keep interfaces small (1-2 methods) | `io.Reader`, `io.Writer`, `fmt.Stringer` | +| Compose small interfaces | `io.ReadWriteCloser` | +| Accept interfaces, return structs | `io.LimitReader`, `io.TeeReader` | +| Use adapter types for functions | `http.HandlerFunc` | +| Optional capabilities via separate interfaces | `http.Flusher`, `http.Hijacker` | +| Compile-time interface checks | `var _ Interface = (*Type)(nil)` | +| Runtime interface upgrade for optimization | `io.Copy` → `WriterTo`/`ReaderFrom` | +| Plugin/driver interfaces start minimal | `database/sql/driver.Driver` | diff --git a/patterns/package-design.md b/patterns/package-design.md new file mode 100644 index 0000000..09d3f8b --- /dev/null +++ b/patterns/package-design.md @@ -0,0 +1,551 @@ +# Go Package Design Patterns + +Patterns extracted from the Go standard library source code. + +--- + +## 1. Package-Level Documentation + +### Source: `src/io/io.go:5-13`, `src/sync/mutex.go:5-11`, `src/context/context.go:5-57` + +```go +// src/io/io.go:5-13 +// Package io provides basic interfaces to I/O primitives. +// Its primary job is to wrap existing implementations of such primitives, +// such as those in package os, into shared public interfaces that +// abstract the functionality, plus some other related primitives. +// +// Because these interfaces and primitives wrap lower-level operations with +// various implementations, unless otherwise informed clients should not +// assume they are safe for parallel execution. +package io +``` + +```go +// src/sync/mutex.go:5-11 +// Package sync provides basic synchronization primitives such as mutual +// exclusion locks. Other than the Once and WaitGroup types, most are intended +// for use by low-level library routines. Higher-level synchronization is +// better done via channels and communication. +// +// Values containing the types defined in this package should not be copied. +package sync +``` + +### Why + +The package comment: +1. **States the purpose** in one sentence +2. **Establishes contracts** (not safe for parallel execution, values must not be copied) +3. **Guides users** toward correct usage (prefer channels over mutexes) +4. **Appears before `package` keyword** — becomes `go doc` output + +### Convention + +- First sentence: `"Package X does Y."` or `"Package X provides Y."` +- Subsequent paragraphs: contracts, caveats, links to deeper docs +- For multi-file packages, put the package comment in `doc.go` or the primary file + +### Anti-pattern + +```go +// DON'T: No package comment +package myutil + +// DON'T: Restate the obvious +// Package http provides HTTP stuff. +package http + +// DON'T: Put implementation details in the package comment +// Package auth uses bcrypt with cost 12 and stores hashes in PostgreSQL. +package auth +``` + +--- + +## 2. Package Naming + +### Source: All stdlib packages follow these conventions + +**Stdlib examples:** +- `io` — not `ioutil`, not `ioutils` +- `fmt` — not `format`, not `formatting` +- `sync` — not `synchronization` +- `net/http` — not `net/httpserver` +- `encoding/json` — not `encoding/jsonparser` +- `context` — not `ctx` or `contexts` +- `errors` — not `errs` or `errorhandling` + +### Why + +Go package names are: +- **Short** — one word, lowercase, no underscores or mixedCaps +- **Clear** — the name is the context for everything inside it +- **Singular** (usually) — `context` not `contexts`, `error` exception (`errors` has functions) + +The package name is part of every qualified identifier: `http.Handler`, `json.Marshal`, `context.Context`. Redundancy in naming is wasted keystrokes: + +```go +// Good: package name provides context +http.Server // not http.HTTPServer +json.Encoder // not json.JSONEncoder +context.Context // the type IS the context +``` + +### Anti-pattern + +```go +// DON'T: Stutter (repeat package name in exported identifiers) +package http +type HTTPServer struct{} // http.HTTPServer — redundant +func NewHTTPClient() // http.NewHTTPClient — say "http" twice + +// DON'T: Use utility/helper package names +package utils // what does it DO? +package helpers // grab bag, no cohesion +package common // everything ends up here + +// DON'T: Use plural when singular works +package requests // should be: package request +``` + +--- + +## 3. internal/ Packages — Restricting Visibility + +### Source: `src/net/http/internal/`, `src/encoding/json/internal.go` + +``` +src/net/http/internal/ +├── ascii/ +├── chunked.go +├── common.go +├── http2/ +├── httpcommon/ +├── httpsfv/ +├── sniff.go +└── testcert/ +``` + +### Why + +Packages under `internal/` can only be imported by code rooted at the parent of `internal`. For example: +- `net/http/internal/ascii` can be imported by `net/http` and `net/http/...` +- It **cannot** be imported by `net/url` or any other package + +This lets you share code between sub-packages without making it part of the public API. + +### Usage Guidelines + +``` +myproject/ +├── internal/ # shared across the project, but not importable externally +│ ├── auth/ +│ └── metrics/ +├── cmd/ +│ └── server/ +└── pkg/ # actually public API (if you use this convention) + └── client/ +``` + +### Anti-pattern + +```go +// DON'T: Export implementation details that should be internal +package mylib +func HelperThatOnlyIUse() {} // pollutes API surface + +// DON'T: Put everything in internal/ (nothing is reusable) +// Balance: internal/ for implementation; exported packages for contracts +``` + +--- + +## 4. Export Rules — The Capital Letter Boundary + +### Source: Throughout stdlib — the convention is the language itself + +```go +// src/io/io.go +var EOF = errors.New("EOF") // exported: uppercase +var errInvalidWrite = errors.New(...) // unexported: lowercase + +// src/io/io.go:622-625 +type teeReader struct { // unexported type + r Reader + w Writer +} + +// src/io/io.go:618 +func TeeReader(r Reader, w Writer) Reader { // exported constructor + return &teeReader{r, w} +} +``` + +### Why + +The exported/unexported boundary is Go's encapsulation mechanism. `teeReader` is unexported because: +1. Users don't need to know its implementation +2. The return type is `Reader` (the interface) — maximum flexibility +3. The struct's fields can change without breaking anyone + +### Pattern: Exported Function, Unexported Type + +```go +// Export the constructor, not the type +func NewParser(r io.Reader) *parser { ... } // WRONG: can't return unexported type + +// Correct: return via interface or exported type +func TeeReader(r Reader, w Writer) Reader { return &teeReader{r, w} } +``` + +### Anti-pattern + +```go +// DON'T: Export everything "just in case" +type Parser struct { + Input string // should this be settable? probably not + buffer []byte // internal state — definitely not + pos int +} + +// DON'T: Make internal state accessible +type DB struct { + Pool []*Conn // callers shouldn't manipulate the pool directly +} +``` + +--- + +## 5. init() Functions — Use Sparingly + +### Source: `src/net/http/http2.go:37`, `src/net/http/servemux121.go:31` + +```go +// src/net/http/http2.go:37 +func init() { + // register HTTP/2 protocol implementation +} +``` + +### Why + +`init()` runs automatically at program start, in dependency order. The stdlib uses it for: +- **Driver registration** (database drivers register via init) +- **Protocol negotiation** (HTTP/2 registers its handler) +- **Configuration from build tags** (`servemux121.go` — compatibility shim) + +### Rules + +1. `init()` should have no side effects beyond registration +2. No errors should be possible (can't return error from init) +3. Keep them short — they block program startup +4. Prefer explicit initialization in `main()` when possible + +### Anti-pattern + +```go +// DON'T: Do heavy work in init +func init() { + db = connectToDatabase() // fails silently, crashes later + cache = loadGigabyteFile() // blocks startup +} + +// DON'T: Use init for configuration +func init() { + port = os.Getenv("PORT") // harder to test, implicit dependency +} + +// DO: Prefer explicit setup +func main() { + db, err := connectToDatabase() + if err != nil { + log.Fatal(err) // clear failure point + } +} +``` + +--- + +## 6. Functional Options Pattern + +### Source: Not directly in stdlib, but `net/http.Server` and `database/sql.DB` demonstrate the problem it solves + +The stdlib uses struct-based configuration (Server, Transport, DB config via setters). The functional options pattern emerged from the community to solve the "many optional parameters" problem: + +```go +// The pattern (not in stdlib, but idiom from Rob Pike/Dave Cheney): +type Option func(*Server) + +func WithTimeout(d time.Duration) Option { + return func(s *Server) { + s.timeout = d + } +} + +func WithLogger(l *log.Logger) Option { + return func(s *Server) { + s.logger = l + } +} + +func NewServer(addr string, opts ...Option) *Server { + s := &Server{addr: addr, timeout: 30 * time.Second} + for _, opt := range opts { + opt(s) + } + return s +} +``` + +### What the stdlib uses instead: Config structs + +```go +// src/net/http/server.go (Server struct acts as config) +srv := &http.Server{ + Addr: ":8080", + ReadTimeout: 5 * time.Second, + WriteTimeout: 10 * time.Second, + Handler: mux, +} +``` + +### When to use which + +| Approach | When | +|----------|------| +| Config struct | Few options, all are data (stdlib preference) | +| Functional options | Many options, some involve behavior, public API stability matters | +| Builder pattern | Rare in Go — usually overkill | + +### Anti-pattern + +```go +// DON'T: Long parameter lists +func NewServer(addr string, timeout time.Duration, maxConns int, + logger *log.Logger, tls *tls.Config, handler Handler) *Server + +// DON'T: Use functional options when a simple struct suffices +// (Over-engineering for 2-3 fields) +``` + +--- + +## 7. Constructor Pattern — NewX Functions + +### Source: `src/net/http/server.go:2639`, `src/database/sql/sql.go:836` + +```go +// src/net/http/server.go:2639 +func NewServeMux() *ServeMux { + return new(ServeMux) +} + +// src/database/sql/sql.go:836-843 +func OpenDB(c driver.Connector) *DB { + ctx, cancel := context.WithCancel(context.Background()) + db := &DB{ + connector: c, + openerCh: make(chan struct{}, connectionRequestQueueSize), + lastPut: make(map[*driverConn]string), + stop: cancel, + } + go db.connectionOpener(ctx) + return db +} +``` + +### Why + +- `NewX()` when construction is trivial (just allocate) +- `OpenX()` or `NewXWithConfig()` when construction involves resources, validation, or can fail +- Return `*T` (pointer to concrete type), not an interface + +The zero value should be usable where possible (`sync.Mutex`, `bytes.Buffer`), making constructors unnecessary. + +### Anti-pattern + +```go +// DON'T: Constructor that returns interface (hides useful methods) +func NewWriter() io.Writer { return &myWriter{} } + +// DON'T: Require constructor when zero value works +type Buffer struct { + buf []byte + // ... +} +// var b bytes.Buffer ← just works, no New needed +``` + +--- + +## 8. Package Organization — One Concern Per Package + +### Source: Standard library structure + +``` +src/ +├── io/ # I/O interfaces + helpers +├── os/ # OS operations +├── net/ # network primitives +│ ├── http/ # HTTP protocol +│ └── url/ # URL parsing +├── encoding/ # encoding interfaces +│ ├── json/ # JSON codec +│ └── xml/ # XML codec +├── database/ +│ └── sql/ # SQL database abstraction +│ └── driver/ # SPI for database drivers +└── context/ # cancellation propagation +``` + +### Why + +Each package has a single, clear responsibility: +- `io` defines interfaces; `os` implements them for files +- `encoding/json` handles JSON; `encoding/xml` handles XML +- `database/sql` is the user-facing API; `database/sql/driver` is the implementor-facing SPI + +### Anti-pattern + +```go +// DON'T: Package per type +package user // just has User struct +package order // just has Order struct +package payment // just has Payment struct +// 50 packages with 1 file each — Go prefers fewer, larger packages + +// DON'T: Circular dependencies +package a imports package b +package b imports package a // compile error + +// FIX: Extract shared types into a third package, or merge +``` + +--- + +## 9. API Design — database/sql Separation of Concerns + +### Source: `src/database/sql/sql.go` vs `src/database/sql/driver/driver.go` + +Two distinct APIs in one subsystem: + +**User-facing (database/sql):** +```go +db, _ := sql.Open("postgres", connStr) +rows, _ := db.QueryContext(ctx, "SELECT ...") +defer rows.Close() +for rows.Next() { + rows.Scan(&id, &name) +} +``` + +**Driver-facing (database/sql/driver):** +```go +type Driver interface { + Open(name string) (Conn, error) +} +type Conn interface { + Prepare(query string) (Stmt, error) + Close() error + Begin() (Tx, error) +} +``` + +### Why + +The user never sees `driver.Conn`. The driver never sees `sql.DB`'s pool logic. Clean separation: +- Users get a high-level, safe API with pooling and retry +- Drivers implement a low-level, minimal interface +- The `sql` package mediates between them + +### Anti-pattern + +```go +// DON'T: Expose implementation to users +type DB struct { + driver driver.Conn // users shouldn't touch this +} + +// DON'T: Mix user and implementor APIs in one interface +type Database interface { + Query(sql string) Rows // user method + Open(dsn string) Conn // driver method — different audiences +} +``` + +--- + +## 10. Context Key Pattern — Type-Safe Context Values + +### Source: `src/context/context.go:132-164`, `src/net/http/server.go:244-252` + +```go +// src/context/context.go:132-164 (from doc comment) +// Package user defines a User type that's stored in Contexts. +// package user +// +// import "context" +// +// type key int +// +// var userKey key +// +// func NewContext(ctx context.Context, u *User) context.Context { +// return context.WithValue(ctx, userKey, u) +// } +// +// func FromContext(ctx context.Context) (*User, bool) { +// u, ok := ctx.Value(userKey).(*User) +// return u, ok +// } +``` + +```go +// src/net/http/server.go:244-252 +var ( + ServerContextKey = &contextKey{"http-server"} + LocalAddrContextKey = &contextKey{"local-addr"} +) + +type contextKey struct { + name string +} +``` + +### Why + +- **Unexported key type** prevents other packages from accessing or overwriting your values +- **Type-safe accessors** (`FromContext`) avoid type assertions at every call site +- **Pointer-based keys** (`&contextKey{...}`) guarantee uniqueness even with same string names + +### Anti-pattern + +```go +// DON'T: Use string keys (any package can collide) +ctx = context.WithValue(ctx, "user", user) + +// DON'T: Use exported key types (anyone can access) +type Key string +const UserKey Key = "user" // other packages can use this key + +// DON'T: Store optional parameters in context +ctx = context.WithValue(ctx, "timeout", 5*time.Second) // use function params! +``` + +--- + +## Summary: Package Design Principles + +| Principle | Rule | +|-----------|------| +| Package comment | `"Package X does Y."` before `package` keyword | +| Naming | Short, lowercase, no stutter (`http.Server` not `http.HTTPServer`) | +| Encapsulation | `internal/` for shared-but-private code | +| Exports | Minimum viable surface; unexported by default | +| init() | Only for registration; keep trivial | +| Constructors | `NewX()` → `*T`; prefer usable zero values | +| Organization | One concern per package; no circular deps | +| API layers | Separate user-facing from implementor-facing (SPI) | +| Context values | Unexported key type + typed accessors | +| Configuration | Struct literals (stdlib) or functional options (community) | diff --git a/patterns/testing-advanced.md b/patterns/testing-advanced.md new file mode 100644 index 0000000..ecaba8c --- /dev/null +++ b/patterns/testing-advanced.md @@ -0,0 +1,586 @@ +# Advanced Go Testing Patterns + +Patterns extracted from the Go standard library (`src/net/http/`, `src/encoding/json/`, `src/testing/`) and Kubernetes source code. + +--- + +## 1. Table-Driven Tests + +The canonical Go test style. Every Go stdlib test file uses this pattern. + +### Pattern Name: Anonymous Struct Test Table + +**Source:** `/tmp/go-src/src/net/http/header_test.go` lines 17-108 + +**What they do:** Define test cases as a slice of anonymous structs, iterate with a range loop. + +**Why:** Eliminates repetition, makes adding cases trivial, keeps the assertion logic in one place. Every test case gets the same verification path — no "special" cases hidden in different code paths. + +**Anti-pattern:** Writing individual assertions for each case, or copy-pasting test functions that differ by one input. + +**Code example (stdlib):** +```go +var headerWriteTests = []struct { + h Header + exclude map[string]bool + expected string +}{ + {Header{}, nil, ""}, + { + Header{ + "Content-Type": {"text/html; charset=UTF-8"}, + "Content-Length": {"0"}, + }, + nil, + "Content-Length: 0\r\nContent-Type: text/html; charset=UTF-8\r\n", + }, + // ... more cases +} + +func TestHeaderWrite(t *testing.T) { + var buf strings.Builder + for i, test := range headerWriteTests { + test.h.WriteSubset(&buf, test.exclude) + if buf.String() != test.expected { + t.Errorf("#%d:\n got: %q\nwant: %q", i, buf.String(), test.expected) + } + buf.Reset() + } +} +``` + +--- + +### Pattern Name: Named Table Tests with t.Run (Subtests) + +**Source:** `/tmp/go-src/src/encoding/json/encode_test.go` lines 285-320, `/tmp/go-src/src/encoding/json/scanner_test.go` lines 30-50 + +**What they do:** Combine table-driven tests with `t.Run` for named subtests. Use a `CaseName` struct that captures file/line for error reporting. + +**Why:** Each case gets its own subtest name — visible in `go test -v`, filterable with `-run`, and individually re-runnable. The `CaseName`/`Where` pattern provides precise file:line for failures even in large test tables. + +**Anti-pattern:** Using index-only identification (hard to find which case failed), or creating separate `TestFoo_Case1`, `TestFoo_Case2` functions. + +**Code example (stdlib):** +```go +func TestValid(t *testing.T) { + tests := []struct { + CaseName + data string + ok bool + }{ + {Name(""), `foo`, false}, + {Name(""), `}{`, false}, + {Name(""), `{}`, true}, + {Name("StringDoubleEscapes"), `{"foo":"bar"}`, true}, + } + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + if ok := Valid([]byte(tt.data)); ok != tt.ok { + t.Errorf("%s: Valid(`%s`) = %v, want %v", tt.Where, tt.data, ok, tt.ok) + } + }) + } +} +``` + +--- + +### Pattern Name: CaseName with Caller Position Tracking + +**Source:** `/tmp/go-src/src/encoding/json/internal/jsontest/testcase.go` lines 18-37 + +**What they do:** Create a helper type that captures the caller's file:line at the point of test case declaration, so error messages point back to the exact test case definition. + +**Why:** In a 1000-entry test table, `t.Errorf` points to the assertion line (same for all cases). CaseName makes failures point to the case definition. + +**Code example (stdlib):** +```go +type CaseName struct { + Name string + Where CasePos +} + +func Name(s string) (c CaseName) { + c.Name = s + runtime.Callers(2, c.Where.pc[:]) + return c +} + +type CasePos struct{ pc [1]uintptr } + +func (pos CasePos) String() string { + frames := runtime.CallersFrames(pos.pc[:]) + frame, _ := frames.Next() + return fmt.Sprintf("%s:%d", path.Base(frame.File), frame.Line) +} +``` + +--- + +## 2. Test Helper Patterns + +### Pattern Name: t.Helper() for Clean Stack Traces + +**Source:** `/tmp/go-src/src/testing/testing.go` lines 1415-1435 + +**What they do:** Call `t.Helper()` as the first line in any test utility function. This marks the function as a helper, so test failure messages report the caller's line instead of the helper's line. + +**Why:** Without `t.Helper()`, every failure in a helper function points to the helper itself, not the test case that triggered the failure. Makes debugging test failures require reading the full stack. + +**Anti-pattern:** Writing test utilities that call `t.Fatal`/`t.Error` without marking themselves as helpers. + +**Code example (stdlib):** +```go +// From net/http/clientserver_test.go lines 100-131 +func run[T TBRun[T]](t T, f func(t T, mode testMode), opts ...any) { + t.Helper() + modes := []testMode{http1Mode, http2Mode, http3Mode} + parallel := true + for _, opt := range opts { + switch opt := opt.(type) { + case []testMode: + modes = opt + case testNotParallelOpt: + parallel = false + default: + t.Fatalf("unknown option type %T", opt) + } + } + // ... + for _, mode := range modes { + t.Run(string(mode), func(t T) { + t.Helper() + // ... + f(t, mode) + }) + } +} +``` + +--- + +### Pattern Name: *testing.T as First Argument to Helpers + +**Source:** `/tmp/go-src/src/net/http/serve_test.go` lines 4555-4580 + +**What they do:** Pass `*testing.T` (or `testing.TB`) as the first argument to test helper functions, making the dependency on the test context explicit. + +**Why:** The test object provides `Fatal`, `Error`, `Log`, `Helper`, `Cleanup` — everything a helper needs for reporting. Accepting it as a parameter (rather than capturing it in a closure) makes helpers reusable across tests. + +**Code example (stdlib):** +```go +mustGet := func(url string, headers ...string) { + t.Helper() + req, err := NewRequest("GET", url, nil) + if err != nil { + t.Fatal(err) + } + for len(headers) > 0 { + req.Header.Add(headers[0], headers[1]) + headers = headers[2:] + } + res, err := c.Do(req) + if err != nil { + t.Errorf("Error fetching %s: %v", url, err) + return + } + _, err = io.ReadAll(res.Body) + defer res.Body.Close() +} +``` + +--- + +## 3. t.Cleanup vs defer + +### Pattern Name: t.Cleanup for Test-Scoped Resources + +**Source:** `/tmp/go-src/src/testing/testing.go` lines 1439-1468, `/tmp/go-src/src/net/http/clientserver_test.go` lines 120-127 + +**What they do:** Use `t.Cleanup(fn)` instead of `defer` for resource cleanup in tests. + +**Why:** +1. `defer` runs at the end of the *function*, not the *test*. In subtests launched with `t.Run`, a `defer` in a helper function runs when the helper returns — not when the subtest completes. +2. `t.Cleanup` runs after the test AND all its subtests finish — guaranteeing resources are available for the full test lifetime. +3. `t.Cleanup` is called in reverse order (LIFO), matching `defer` semantics but scoped to the test. + +**Anti-pattern:** Using `defer` for cleanup in test setup functions that return before the test finishes, or in subtests where timing matters. + +**Code example (stdlib):** +```go +// From net/http/clientserver_test.go +func run[T TBRun[T]](t T, f func(t T, mode testMode), opts ...any) { + // ... + for _, mode := range modes { + t.Run(string(mode), func(t T) { + t.Cleanup(func() { + afterTest(t) // Goroutine leak detection — runs AFTER subtest body completes + }) + f(t, mode) + }) + } +} +``` + +--- + +## 4. testdata/ Directory Pattern + +### Pattern Name: testdata/ for Test Fixtures + +**Source:** `/tmp/go-src/src/net/http/testdata/` (contains `file`, `index.html`, `style.css`), `/tmp/go-src/src/net/http/fs_test.go` line 38 + +**What they do:** Store test fixtures in a `testdata/` directory adjacent to the test files. Reference them with relative paths like `"testdata/file"`. + +**Why:** +1. `go build` ignores `testdata/` directories — they never end up in production binaries. +2. `go test` runs with the package directory as CWD — relative paths to `testdata/` work reliably. +3. Fixtures are version-controlled alongside the code they test. +4. Separates test data from test logic. + +**Anti-pattern:** Embedding large test fixtures as string literals in test files, or referencing absolute paths. + +**Code example (stdlib):** +```go +// From net/http/fs_test.go line 38 +const testFile = "testdata/file" + +// Usage in test: +ServeFile(w, r, "testdata/file") +``` + +--- + +## 5. Golden File Testing + +### Pattern Name: Golden Files with -update Flag + +**Source:** `/tmp/go-src/src/cmd/gofmt/gofmt_test.go` lines 18, 113-138 + +**What they do:** Compare test output against `.golden` files. Provide a `-update` flag that regenerates golden files from current output when behavior intentionally changes. + +**Why:** +1. Tests complex output (formatted code, generated HTML, serialized data) without embedding it in test code. +2. The `-update` flag makes intentional changes easy: run `go test -update`, review the diff, commit. +3. Golden files serve as documentation of expected behavior. +4. Reviewers can see exactly what output changed in diffs. + +**Anti-pattern:** Comparing against inline expected strings that span 50+ lines, or manually constructing expected output. + +**Code example (stdlib):** +```go +var update = flag.Bool("update", false, "update .golden files") + +func runTest(t *testing.T, in, out string) { + // ... produce actual output ... + + expected, err := os.ReadFile(out) + if err != nil { + t.Error(err) + return + } + + if got := buf.Bytes(); !bytes.Equal(got, expected) { + if *update { + if in != out { + if err := os.WriteFile(out, got, 0666); err != nil { + t.Error(err) + } + return + } + } + t.Errorf("(gofmt %s) != %s\n%s", in, out, + diff.Diff("expected", expected, "got", got)) + } +} + +func TestRewrite(t *testing.T) { + match, _ := filepath.Glob("testdata/*.input") + for _, in := range match { + name := filepath.Base(in) + t.Run(name, func(t *testing.T) { + out := in[:len(in)-len(".input")] + ".golden" + runTest(t, in, out) + }) + } +} +``` + +--- + +## 6. httptest Patterns + +### Pattern Name: httptest.NewRecorder for Unit-Testing Handlers + +**Source:** `/tmp/go-src/src/net/http/serve_test.go` lines 387-393 + +**What they do:** Use `httptest.NewRecorder()` to test HTTP handlers without starting a server. Captures status code, headers, and body. + +**Why:** Fast, no network, no port allocation, no goroutines. Perfect for unit testing individual handlers in isolation. + +**Anti-pattern:** Spinning up a full server to test handler logic that doesn't need networking. + +**Code example (stdlib):** +```go +func TestServeMuxHandler(t *testing.T) { + mux := NewServeMux() + for _, e := range serveMuxRegister { + mux.Handle(e.pattern, e.h) + } + for _, tt := range serveMuxTests { + r := &Request{Method: tt.method, Host: tt.host, URL: &url.URL{Path: tt.path}} + h, pattern := mux.Handler(r) + rr := httptest.NewRecorder() + h.ServeHTTP(rr, r) + if pattern != tt.pattern || rr.Code != tt.code { + t.Errorf("%s %s %s = %d, %q, want %d, %q", + tt.method, tt.host, tt.path, rr.Code, pattern, tt.code, tt.pattern) + } + } +} +``` + +--- + +### Pattern Name: httptest.NewServer for Integration-Style Tests + +**Source:** `/tmp/go-src/src/net/http/clientserver_test.go` lines 203-280 + +**What they do:** Use `httptest.NewServer` / `httptest.NewUnstartedServer` for end-to-end HTTP testing with a real TCP listener on localhost. + +**Why:** Tests the full HTTP stack including transport, TLS, connection pooling, timeouts. The `clientServerTest` helper in the stdlib runs each test across HTTP/1.1, HTTP/2, and HTTP/3 modes. + +**Code example (stdlib):** +```go +func newClientServerTest(t testing.TB, mode testMode, h Handler, opts ...any) *clientServerTest { + cst := &clientServerTest{t: t, h2: mode == http2Mode, h: h} + cst.ts = httptest.NewUnstartedServer(h) + // ... configure based on mode ... + switch mode { + case http1Mode: + cst.ts.Start() + case http2Mode: + cst.ts.EnableHTTP2 = true + cst.ts.StartTLS() + } + cst.c = cst.ts.Client() + t.Cleanup(cst.close) + return cst +} +``` + +--- + +## 7. Benchmark Patterns + +### Pattern Name: b.ReportAllocs + b.RunParallel + b.SetBytes + +**Source:** `/tmp/go-src/src/encoding/json/bench_test.go` lines 85-101 + +**What they do:** Combine `b.ReportAllocs()` for allocation reporting, `b.RunParallel` for concurrent benchmarks, and `b.SetBytes` for throughput metrics. + +**Why:** +- `b.ReportAllocs()` shows allocations/op — critical for hot paths. +- `b.RunParallel` measures performance under contention (real-world server behavior). +- `b.SetBytes` converts to MB/s throughput — meaningful for serialization benchmarks. + +**Anti-pattern:** Benchmarks that only measure wall time without allocation tracking, or sequential benchmarks for concurrent code. + +**Code example (stdlib):** +```go +func BenchmarkCodeEncoder(b *testing.B) { + b.ReportAllocs() + if codeJSON == nil { + b.StopTimer() + codeInit() + b.StartTimer() + } + b.RunParallel(func(pb *testing.PB) { + enc := NewEncoder(io.Discard) + for pb.Next() { + if err := enc.Encode(&codeStruct); err != nil { + b.Fatalf("Encode error: %v", err) + } + } + }) + b.SetBytes(int64(len(codeJSON))) +} +``` + +--- + +## 8. Integration Test Separation + +### Pattern Name: testing.Short() for Expensive Tests + +**Source:** `/tmp/go-src/src/net/http/serve_test.go` lines 800, 1000, 2212, 2581 + +**What they do:** Skip slow/flaky/network-dependent tests with `testing.Short()`. The Go CI runs with `-short` in fast mode, full tests in thorough mode. + +**Why:** Fast feedback loop for development (`go test -short`), full validation in CI. No custom build tags needed. + +**Anti-pattern:** Separate `_integration_test.go` files with build tags (Go stdlib doesn't do this), or always-slow tests that can't be skipped. + +**Code example (stdlib):** +```go +func TestServerTimeouts(t *testing.T) { + if testing.Short() { + t.Skip("skipping in short mode") + } + // ... expensive test with real timeouts ... +} +``` + +--- + +## 9. No Assertion Libraries in Stdlib + +### Pattern Name: Plain if/t.Errorf Over Assertion Frameworks + +**Source:** Every test file in `/tmp/go-src/src/` (zero imports of `testify`, `gomega`, or any assertion library) + +**What they do:** Use plain Go: `if got != want { t.Errorf(...) }`. Never import assertion libraries. + +**Why:** +1. No implicit control flow — `t.Errorf` continues execution, so you see ALL failures at once. +2. No magic — the test reads like regular Go code. +3. Error messages are custom-crafted for each assertion, providing context that generic `assert.Equal` cannot. +4. One less dependency. + +**Anti-pattern (Kubernetes uses this, stdlib does NOT):** +```go +// Kubernetes style (not stdlib): +assert.Equal(t, expected, actual) +require.NoError(t, err) +``` + +**Stdlib style:** +```go +if got := v.Elem().Interface(); !reflect.DeepEqual(got, tt.out) { + t.Fatalf("%s: Decode:\n\tgot: %#v\n\twant: %#v", tt.Where, got, tt.out) +} +``` + +--- + +## 10. Goroutine Leak Detection + +### Pattern Name: TestMain + afterTest Goroutine Checking + +**Source:** `/tmp/go-src/src/net/http/main_test.go` (entire file) + +**What they do:** `TestMain` runs the test suite and checks for leaked goroutines after all tests complete. `afterTest` checks for goroutine leaks after each individual test. + +**Why:** HTTP code spawns goroutines for connections, background reads, etc. Leaked goroutines indicate resource leaks (connections not closed, servers not shut down). Catching them prevents production OOMs. + +**Code example (stdlib):** +```go +func TestMain(m *testing.M) { + v := m.Run() + if v == 0 && goroutineLeaked() { + os.Exit(1) + } + os.Exit(v) +} + +func goroutineLeaked() bool { + for i := 0; i < 5; i++ { + gs := interestingGoroutines() + if len(gs) == 0 { + return false + } + time.Sleep(100 * time.Millisecond) + } + // Report leaked goroutines + return true +} + +func afterTest(t testing.TB) { + http.DefaultTransport.(*http.Transport).CloseIdleConnections() + // Check for leaked goroutines from this specific test... +} +``` + +--- + +## 11. export_test.go Pattern + +### Pattern Name: Bridge File for Internal Testing + +**Source:** `/tmp/go-src/src/net/http/export_test.go` lines 1-50 + +**What they do:** Create an `export_test.go` file in the package itself (package `http`, not `http_test`) that exports internal symbols to external test packages. Only compiled during testing. + +**Why:** Allows `http_test` (external test package) to access internals needed for white-box testing without polluting the public API. The `_test.go` suffix means it's never included in production builds. + +**Code example (stdlib):** +```go +// export_test.go — package http (not http_test!) +package http + +var ( + DefaultUserAgent = defaultUserAgent + ExportRefererForURL = refererForURL + ExportServerNewConn = (*Server).newConn + ExportErrRequestCanceled = errRequestCanceled +) +``` + +--- + +## 12. Multi-Mode Test Runner + +### Pattern Name: Generic Test Runner Across Protocol Modes + +**Source:** `/tmp/go-src/src/net/http/clientserver_test.go` lines 100-134 + +**What they do:** A generic `run[T]` function that executes every client/server test in HTTP/1.1, HTTP/2, and HTTP/3 modes automatically. Tests opt into specific modes via options. + +**Why:** Ensures behavioral consistency across protocol versions. A single test function covers all modes — no duplication. Bugs in one protocol version are caught immediately. + +**Code example (stdlib):** +```go +// Test declaration (one line runs across 3 protocols): +func TestServerTimeouts(t *testing.T) { run(t, testServerTimeouts, []testMode{http1Mode}) } + +// The runner: +func run[T TBRun[T]](t T, f func(t T, mode testMode), opts ...any) { + t.Helper() + modes := []testMode{http1Mode, http2Mode, http3Mode} + for _, mode := range modes { + t.Run(string(mode), func(t T) { + t.Helper() + t.Cleanup(func() { afterTest(t) }) + f(t, mode) + }) + } +} +``` + +--- + +## 13. testLogWriter — Routing Server Logs to Test Output + +### Pattern Name: io.Writer Adapter for *testing.T + +**Source:** `/tmp/go-src/src/net/http/clientserver_test.go` lines 337-345 + +**What they do:** Implement `io.Writer` backed by `t.Logf`, so server error logs appear in test output (visible with `-v`, suppressed otherwise). + +**Why:** Server logs are crucial for debugging test failures but shouldn't clutter passing output. `t.Log` gives you both: silent on pass, verbose on fail. + +**Code example (stdlib):** +```go +type testLogWriter struct { + t testing.TB +} + +func (w testLogWriter) Write(b []byte) (int, error) { + w.t.Logf("server log: %v", strings.TrimSpace(string(b))) + return len(b), nil +} + +// Usage: +cst.ts.Config.ErrorLog = log.New(testLogWriter{t}, "", 0) +``` diff --git a/patterns/testing.md b/patterns/testing.md new file mode 100644 index 0000000..786e1b2 --- /dev/null +++ b/patterns/testing.md @@ -0,0 +1,670 @@ +# Go Testing Patterns + +Patterns extracted from the Go standard library source code. + +--- + +## 1. Table-Driven Tests with Subtests + +### Source: `src/encoding/json/encode_test.go:405-430` + +```go +// src/encoding/json/encode_test.go:405-430 +func TestUnsupportedValues(t *testing.T) { + tests := []struct { + CaseName + in any + }{ + {Name(""), math.NaN()}, + {Name(""), math.Inf(-1)}, + {Name(""), math.Inf(1)}, + {Name(""), pointerCycle}, + {Name(""), pointerCycleIndirect}, + {Name(""), mapCycle}, + {Name(""), sliceCycle}, + {Name(""), recursiveSliceCycle}, + } + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + if _, err := Marshal(tt.in); err != nil { + if _, ok := err.(*UnsupportedValueError); !ok { + t.Errorf("%s: Marshal error:\n\tgot: %T\n\twant: %T", tt.Where, err, new(UnsupportedValueError)) + } + } else { + t.Errorf("%s: Marshal error: got nil, want non-nil", tt.Where) + } + }) + } +} +``` + +### Source: `src/encoding/json/encode_test.go:270-328` (with inputs and expected outputs) + +```go +// src/encoding/json/encode_test.go:270-328 +func TestRoundtripStringTag(t *testing.T) { + tests := []struct { + CaseName + in StringTag + want string + }{{ + CaseName: Name("AllTypes"), + in: StringTag{ + BoolStr: true, + IntStr: 42, + UintptrStr: 44, + StrStr: "xzbit", + NumberStr: "46", + }, + want: `{ + "BoolStr": "true", + "IntStr": "42", + ... +}`, + }, { + CaseName: Name("StringDoubleEscapes"), + in: StringTag{ + StrStr: "\b\f\n\r\t\"\\", + NumberStr: "0", + }, + want: `{...}`, + }} + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + got, err := MarshalIndent(&tt.in, "", "\t") + if err != nil { + t.Fatalf("%s: MarshalIndent error: %v", tt.Where, err) + } + if got := string(got); got != tt.want { + t.Fatalf("%s: MarshalIndent:\n\tgot: %s\n\twant: %s", tt.Where, ...) + } + // Verify round-trip + var s2 StringTag + if err := Unmarshal(got, &s2); err != nil { + t.Fatalf("%s: Decode error: %v", tt.Where, err) + } + if !reflect.DeepEqual(s2, tt.in) { + t.Fatalf("%s: Decode:\n\tinput: %s\n\tgot: %#v\n\twant: %#v", ...) + } + }) + } +} +``` + +### Why + +Table-driven tests are Go's signature testing pattern: +1. **All cases visible in one place** — easy to add new cases +2. **t.Run creates subtests** — each case runs independently, can be filtered with `-run` +3. **Uniform structure** — input, expected output, test name +4. **Failures identify which case** — via the case name + +### Template + +```go +func TestFoo(t *testing.T) { + tests := []struct { + name string + input InputType + want OutputType + wantErr bool + }{ + {name: "basic", input: ..., want: ...}, + {name: "empty", input: ..., want: ...}, + {name: "error case", input: ..., wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Foo(tt.input) + if (err != nil) != tt.wantErr { + t.Fatalf("Foo() error = %v, wantErr %v", err, tt.wantErr) + } + if got != tt.want { + t.Errorf("Foo() = %v, want %v", got, tt.want) + } + }) + } +} +``` + +### Anti-pattern + +```go +// DON'T: Separate test functions for each case +func TestFoo_Basic(t *testing.T) { ... } +func TestFoo_Empty(t *testing.T) { ... } +func TestFoo_Error(t *testing.T) { ... } +// 50 near-identical functions — hard to maintain + +// DON'T: Tests without names (hard to identify failures) +tests := []struct{ in, want int }{{1, 2}, {3, 4}} +for _, tt := range tests { + // which test failed? index 0 or 1? +} +``` + +--- + +## 2. t.Helper() — Clean Error Reporting + +### Source: `src/testing/testing.go:1415-1435` + +```go +// src/testing/testing.go:1415-1435 +func (c *common) Helper() { + if c.isSynctest { + c = c.parent + } + c.mu.Lock() + defer c.mu.Unlock() + if c.helperPCs == nil { + c.helperPCs = make(map[uintptr]struct{}) + } + var pc [1]uintptr + n := runtime.Callers(2, pc[:]) + if n == 0 { + panic("testing: zero callers found") + } + if _, found := c.helperPCs[pc[0]]; !found { + c.helperPCs[pc[0]] = struct{}{} + c.helperNames = nil + } +} +``` + +### Why + +When a test helper calls `t.Helper()`, failures report the **caller's** line number, not the helper's. Without it, every failure points to the helper function — useless for identifying which test case failed. + +### Idiomatic Usage + +```go +func assertNoError(t *testing.T, err error) { + t.Helper() // failures point to the caller, not this line + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func assertEqual(t *testing.T, got, want any) { + t.Helper() + if !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} +``` + +### Anti-pattern + +```go +// DON'T: Forget t.Helper() in test helpers +func checkResult(t *testing.T, got, want string) { + // Missing t.Helper() + if got != want { + t.Errorf("got %q, want %q", got, want) + // Error points HERE, not the actual test case — confusing + } +} +``` + +--- + +## 3. t.Run() — Subtests and Test Organization + +### Source: `src/testing/testing.go:2204-2260` + +```go +// src/testing/testing.go:2204-2215 +func (t *T) Run(name string, f func(t *T)) bool { + t.hasSub.Store(true) + testName, ok, _ := t.tstate.match.fullName(&t.common, name) + if !ok || shouldFailFast() { + return true + } + // ... + ctx, cancelCtx := context.WithCancel(context.Background()) + t = &T{ + common: common{ + name: testName, + parent: &t.common, + // ... + }, + } + go tRunner(t, f) + // ... +} +``` + +### Why + +Subtests: +1. **Run in separate goroutines** — isolated from each other +2. **Have their own context** — `context.WithCancel(context.Background())` +3. **Can be filtered** — `go test -run TestFoo/subcase` +4. **Can run in parallel** — via `t.Parallel()` inside the subtest +5. **Share setup/teardown** — parent test's defer runs after all subtests + +### Pattern: Setup/Teardown with Subtests + +```go +func TestDB(t *testing.T) { + db := setupTestDB(t) // shared setup + t.Cleanup(func() { db.Close() }) // runs after ALL subtests + + t.Run("Insert", func(t *testing.T) { + // uses db + }) + t.Run("Query", func(t *testing.T) { + // uses db + }) +} +``` + +### Anti-pattern + +```go +// DON'T: Rely on test execution order +func TestInsert(t *testing.T) { ... } // must run before TestQuery +func TestQuery(t *testing.T) { ... } // depends on TestInsert's side effects +// Tests should be independent! +``` + +--- + +## 4. t.Parallel() — Concurrent Test Execution + +### Source: `src/testing/testing.go:1912` + +```go +// src/testing/testing.go:1912 +func (t *T) Parallel() { + // ...marks test as parallel, pauses until parent completes +} +``` + +### Why + +`t.Parallel()` signals that this test can run concurrently with other parallel tests. The test pauses until its parent test function returns, then runs alongside other parallel subtests. + +### Idiomatic Usage + +```go +func TestFoo(t *testing.T) { + tests := []struct{ + name string + input int + }{ + {"small", 1}, + {"large", 1000}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() // runs concurrently with other subtests + result := expensiveOperation(tt.input) + if result != expected { + t.Errorf(...) + } + }) + } +} +``` + +### Anti-pattern + +```go +// DON'T: Parallel tests that share mutable state +var counter int +func TestA(t *testing.T) { + t.Parallel() + counter++ // DATA RACE +} +func TestB(t *testing.T) { + t.Parallel() + counter++ // DATA RACE +} + +// DON'T: Capture loop variable in Go < 1.22 without explicit copy +for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + // In Go < 1.22: tt is shared — always sees last value + // In Go >= 1.22: loop variables are per-iteration (fixed) + }) +} +``` + +--- + +## 5. t.Cleanup() — Deterministic Teardown + +### Source: `src/testing/testing.go:1439` + +```go +// src/testing/testing.go:1439-1442 +// Cleanup registers a function to be called when the test (or subtest) +// and all its subtests complete. Cleanup functions will be called in +// last added, first called order. +func (c *common) Cleanup(f func()) { ... } +``` + +### Why + +`t.Cleanup` is like `defer` but tied to test lifecycle, not function scope. It runs after all subtests complete and works with parallel tests. + +### Idiomatic Usage + +```go +func setupTestDB(t *testing.T) *sql.DB { + t.Helper() + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + db.Close() + }) + return db +} + +// Caller doesn't need to worry about cleanup: +func TestQueries(t *testing.T) { + db := setupTestDB(t) // automatically cleaned up + // ... +} +``` + +### Anti-pattern + +```go +// DON'T: Return cleanup functions (easy to forget) +func setupDB(t *testing.T) (*sql.DB, func()) { + db := ... + return db, func() { db.Close() } +} +// Caller must remember: db, cleanup := setupDB(t); defer cleanup() + +// DO: Use t.Cleanup inside the setup function +``` + +--- + +## 6. t.TempDir() — Automatic Temp Directories + +### Source: `src/testing/testing.go:1575` + +```go +// src/testing/testing.go:1575 +func (c *common) TempDir() string { ... } +``` + +### Why + +Creates a temp directory that's automatically removed when the test completes. No manual cleanup needed, no leftover test artifacts. + +### Idiomatic Usage + +```go +func TestWriteConfig(t *testing.T) { + dir := t.TempDir() // auto-cleaned + path := filepath.Join(dir, "config.json") + + err := WriteConfig(path, myConfig) + if err != nil { + t.Fatal(err) + } + + got, _ := os.ReadFile(path) + // assert contents... +} +``` + +### Anti-pattern + +```go +// DON'T: Create temp dirs manually and forget cleanup +func TestWrite(t *testing.T) { + dir, _ := os.MkdirTemp("", "test") + // forgot os.RemoveAll(dir) — test leaves garbage +} +``` + +--- + +## 7. testdata/ Directory + +### Source: `src/net/http/testdata/`, `src/encoding/json/testdata/` (implicit — exists in the tree) + +``` +src/net/http/testdata/ +├── file +├── index.html +└── style.css +``` + +### Why + +The `testdata/` directory is special in Go: +1. **Ignored by the go tool** — not compiled as a package +2. **Available to tests** — accessed via relative path `"testdata/file.txt"` +3. **Committed to repo** — test fixtures live alongside the code +4. **Portable** — tests work without external dependencies + +### Idiomatic Usage + +```go +func TestParse(t *testing.T) { + input, err := os.ReadFile("testdata/input.json") + if err != nil { + t.Fatal(err) + } + want, err := os.ReadFile("testdata/expected.json") + if err != nil { + t.Fatal(err) + } + + got := Parse(input) + if !bytes.Equal(got, want) { + t.Errorf("Parse mismatch") + } +} +``` + +### Golden File Pattern + +```go +func TestOutput(t *testing.T) { + got := generateOutput() + golden := filepath.Join("testdata", t.Name()+".golden") + + if *update { // -update flag to regenerate + os.WriteFile(golden, got, 0644) + } + + want, _ := os.ReadFile(golden) + if !bytes.Equal(got, want) { + t.Errorf("output mismatch; run with -update to regenerate") + } +} +``` + +### Anti-pattern + +```go +// DON'T: Embed large test fixtures as string literals +var testInput = `{ + "very": "long", + "json": "string", + // 500 lines... +}` + +// DON'T: Depend on external URLs for test data +func TestParse(t *testing.T) { + resp, _ := http.Get("https://example.com/test.json") // flaky! +} +``` + +--- + +## 8. Error Message Formatting + +### Source: `src/encoding/json/encode_test.go` (throughout) + +```go +// Pattern from encode_test.go:304 +t.Fatalf("%s: MarshalIndent error: %v", tt.Where, err) + +// Pattern from encode_test.go:306-307 +t.Fatalf("%s: MarshalIndent:\n\tgot: %s\n\twant: %s", tt.Where, got, want) + +// Pattern from encode_test.go:421-422 +t.Errorf("%s: Marshal error:\n\tgot: %T\n\twant: %T", tt.Where, err, new(UnsupportedValueError)) +``` + +### Why + +The stdlib follows a consistent error format: +- **Context first** — where/what was being tested +- **got/want on separate lines** — easy to diff visually +- **Tab-indented** — aligns the comparison + +### Convention + +``` +t.Errorf("FunctionName(%v) = %v, want %v", input, got, want) +// or for complex values: +t.Errorf("FunctionName(%v):\n\tgot: %v\n\twant: %v", input, got, want) +``` + +### Anti-pattern + +```go +// DON'T: Vague error messages +t.Error("failed") // what failed? what was expected? + +// DON'T: Only show the got value +t.Errorf("got %v", got) // what was expected? + +// DON'T: Use assert libraries that hide the actual comparison +assert.Equal(t, got, want) // when it fails: "not equal" — which is which? +``` + +--- + +## 9. t.Fatal vs t.Error + +### Convention across stdlib + +```go +// Fatal: test cannot continue meaningfully +db, err := sql.Open("sqlite3", ":memory:") +if err != nil { + t.Fatal(err) // no point continuing without a database +} + +// Error: test can continue, report all failures +for _, tt := range tests { + got := fn(tt.input) + if got != tt.want { + t.Errorf(...) // report and keep going + } +} +``` + +### Why + +- `t.Fatal` / `t.Fatalf` — **stops the test immediately**. Use for setup failures where subsequent assertions are meaningless. +- `t.Error` / `t.Errorf` — **reports failure, continues**. Use in loops to collect all failures at once. + +### Anti-pattern + +```go +// DON'T: Fatal in loops (misses subsequent failures) +for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got != tt.want { + t.Fatalf(...) // stops this subtest — fine in subtests actually + } + }) +} +// But without subtests: +for _, tt := range tests { + if got != tt.want { + t.Fatalf(...) // stops ALL remaining cases! + } +} + +// DON'T: Error when you can't continue +file, err := os.Open(path) +if err != nil { + t.Errorf("open: %v", err) // continues to use nil file — panic! +} +file.Read(...) +``` + +--- + +## 10. Test Naming Conventions + +### Source: All stdlib tests follow these patterns + +```go +// Function tests: TestFunctionName +func TestMarshal(t *testing.T) { ... } + +// Method tests: TestTypeName_MethodName +func TestEncoder_Encode(t *testing.T) { ... } + +// Behavior tests: TestDescription +func TestOmitEmpty(t *testing.T) { ... } + +// Edge cases: TestFunctionName_EdgeCase +func TestMarshal_NilSlice(t *testing.T) { ... } + +// Benchmarks: BenchmarkFunctionName +func BenchmarkMarshal(b *testing.B) { ... } + +// Examples: ExampleFunctionName +func ExampleMarshal() { + // ... + // Output: {"name":"Alice"} +} +``` + +### Why + +- Test names are used with `-run` flag for filtering +- They appear in failure output — should be self-explanatory +- Example functions become documentation (shown in godoc) +- Subtests use `/` separator: `TestMarshal/NilSlice` + +### Anti-pattern + +```go +// DON'T: Numbered tests +func Test1(t *testing.T) { ... } +func Test2(t *testing.T) { ... } + +// DON'T: Tests that don't start with Test +func testHelper(t *testing.T) { ... } // won't be run! (lowercase 't') + +// DON'T: Overly verbose names +func TestThatMarshalCorrectlyHandlesNilSliceInputAndReturnsNullJSON(t *testing.T) { ... } +``` + +--- + +## Summary: Testing Best Practices + +| Pattern | When | +|---------|------| +| Table-driven tests | Multiple inputs for same logic | +| t.Run subtests | Isolate cases, enable `-run` filtering | +| t.Helper() | Every test helper function | +| t.Parallel() | Independent tests, speed up suite | +| t.Cleanup() | Resource teardown (replaces defer in helpers) | +| t.TempDir() | Need filesystem for test | +| testdata/ | External test fixtures | +| Golden files | Complex expected output | +| t.Fatal | Setup failures (can't continue) | +| t.Error | Assertion failures (collect all) | +| got/want format | `"got %v, want %v"` or `"\n\tgot: %v\n\twant: %v"` | diff --git a/smells/common-mistakes.md b/smells/common-mistakes.md new file mode 100644 index 0000000..11a5604 --- /dev/null +++ b/smells/common-mistakes.md @@ -0,0 +1,558 @@ +# 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 +```