UNLOCKING REAL-WORLD GO MUTEX USAGE PATTERNS by writing a mutex linter Vladimir Dementyev Evil Martians 

AGENDA Bug's Place 6 Lintersworth 16 Patterns Wilderness 26 

github.com/palkan 

No content

THE CRIME SCENE Realtime server (WebSockets, etc.) with guarantees for any backend OSS · Pro · Managed anycable.io 

THE CRIME buoy2022 ! 

THE CRIME Connection ✅ Ping ✅ Broadcasts ✅ Command acks ❌ Occasional $ — restarts % 

INVESTIGATE RPC rate limiter Circuit breaker Epoll Network ↳ Not reproducible 

GUESS-TIGATE More tests / benchmarks More metrics More logs ↳ 6 releases → 1 clue: rpc_timeout_total 

func (n *Node) Subscribe(s *Session, msg *common.Message) (*common.CommandResult, error) { s.smu.Lock() if ok := s.subscriptions.HasChannel(msg.Identifier); ok { s.smu.Unlock() return nil, fmt.Errorf("already subscribed to %s", msg.Identifier) } res, err := n.controller.Subscribe(s.closeCtx, s.GetID(), s.env, s.GetIdentifiers(), msg.Identifier) if s.IsClosed() { s.Log.Debug("skip subscribe result: closed") return nil, nil } if res == nil && err == nil { return nil, nil } s.Log.Debug("controller subscribe", "response", res, "err", err) var confirmed bool if err != nil { if res == nil || res.Status == common.ERROR { return nil, errorx.Decorate(err, "subscribe failed for %s", msg.Identifier) } } else if res.Status == common.SUCCESS { confirmed = true s.subscriptions.AddChannel(msg.Identifier) s.Log.Debug("subscribed", "identifier", msg.Identifier) } else { s.Log.Debug("subscription rejected", "identifier", msg.Identifier) } s.smu.Unlock() if res != nil { n.handleCommandReply(s, msg, res) n.markDisconnectable(s, res.DisconnectInterest) } if confirmed { if s.IsResumeable() { if berr := n.broker.CommitSession(s.GetID(), s); berr != nil { s.Log.Error("failed to persist session in cache", "error", berr) } } if msg.History.Since > 0 || msg.History.Streams != nil { if err := n.History(s, msg); err != nil { s.Log.Warn("couldn't retrieve history", "identifier", msg.Identifier, "error", err) } } if msg.Presence != nil { if err := n.handlePresenceReply(s, msg.Identifier, common.PresenceJoinType, msg.Presence); err != nil { s.Log.Warn("couldn't process presence join", "identifier", msg.Identifier, "error", err) } } } return res, nil } 

func (n *Node) Subscribe(s *Session, msg *common.Message) (*common.CommandResult, error) { s.smu.Lock() // ... if s.IsClosed() { s.Log.Debug("skip subscribe result: closed") return nil, nil } if err != nil { if res == nil || res.Status == common.ERROR { return nil, errorx.Decorate(err, "subscribe failed for %s", msg.Identifier) } } else if res.Status == common.SUCCESS { // ... } else { s.Log.Debug("subscription rejected", "identifier", msg.Identifier) } s.smu.Unlock() if res != nil { n.handleCommandReply(s, msg, res) } // ... return res, nil } 

func (n *Node) Subscribe(s *Session, msg *common.Message) (*common.CommandResult, error) { s.smu.Lock() & // ... if s.IsClosed() { s.Log.Debug("skip subscribe result: closed") return nil, nil } &‼ if err != nil { if res == nil || res.Status == common.ERROR { return nil, errorx.Decorate(err, "subscribe failed for %s", msg.Identifier) } } else if res.Status == common.SUCCESS { // ... } else { s.Log.Debug("subscription rejected", "identifier", msg.Identifier) } s.smu.Unlock() ' if res != nil { n.handleCommandReply(s, msg, res) } // ... return res, nil } &❗ 

SOMETHING IN THE GIT Underneath the Lock() Return-s began to leak 

THE FIX Fixing was trivial Prevention raised questions 

WHAT IS WRONG WITH ME? 

WHAT IS WRONG WITH ME? WHAT IS WHAT I NEED? 

WHAT IS MUTEX, ANYWAY? 

MUTEX 101 G2 G3 G4 G1 Sync access: one goroutine at a time Not reentrant No owner & E V I T I ⏳ ⏳ ⏳ M I PR Unlock from anywhere CRITICAL SECTION 

THE PATTERN Covers every return path Panic-safe Lock scope is the whole function func DoSmth() { mu.Lock() defer mu.Unlock() // ... } 

THE PATTERN 'S TRAP Lock scope is the whole function, not just critical sections func DoSmth() { mu.Lock() defer mu.Unlock() payload := criticalStuff() // needs & res, err := doReq(data) // IO, non-critical Other go-s wait for your network call, file read, etc. ↳ Lock contention someSideEffects(res) // non-critical updateCriticalStuff(res) // also needs & } 

THE SOLUTION Reduce the lock scope func DoSmth() { mu.Lock() payload, err := criticalStuff() mu.Unlock() res, err := doReq(data) someSideEffects(res) mu.Lock() updateCriticalStuff(res) mu.Unlock() } 

THE SOLUTION? Reduce the lock scope func DoSmth() { mu.Lock() payload, err := criticalStuff() if err != nil { return } mu.Unlock() res, err := doReq(data) ↳ ⚠ Balance yourself someSideEffects(res) mu.Lock() updateCriticalStuff(res) mu.Unlock() } 

THE BUG func DoSmth() { mu.Lock() payload, err := criticalStuff() if err != nil { return } mu.Unlock() func (n *Node) Subscribe(s *Session, msg *common.Message) (*com s.smu.Lock() // ... if s.IsClosed() { return nil, nil } if err != nil { if res == nil || res.Status == common.ERROR { return nil, errorx.Decorate(err, "subscribe failed for % } } else { // ... } res, err := doReq(data) someSideEffects(res) mu.Lock() updateCriticalStuff(res) mu.Unlock() s.smu.Unlock() if res != nil { n.handleCommandReply(s, msg, res) } } // ... } 

SHRINKING THE LOCK SCOPE SHOULDN'T GROW THE BUG SURFACE 

WHAT IS WHAT I NEED? 

WHAT IS WHAT I NEED? A LINTER? 

NOT THEIR BUSINESS go vet only copylocks golangci-lint 10+ plugins enabled, neither watched for locks -race a leaked lock is correctly synchronized runtime deadlock detector all goroutines must be asleep 

BETTER CANDIDATES staticcheck Typo-like problems sasha-s/go-deadlock Custom mutexes with runtime deadlocks detection gnieto/mulint Recursive locks (static) linter 

gnieto/mulint github.com/gnieto/mulint Uses x/tools/go/ analysis Detects some recursive locks 

gnieto/mulint github.com/gnieto/mulint Uses x/tools/go/ analysis Detects some recursive locks Last commit in 2020 ↳ Let's bring it back to life 

THE PLAN Modernize ("it compiles!") Encode bugs as specs 

THE BUG, ENCODED Minimal repro fixture x/tools/go/analysis/ analysistest handles everything else // tests/branching_locks.go func (b *branch) WorkHard(task string) { b.m.Lock() if _, ok := b.data[task]; ok { b.m.Unlock() return // OK } res, err := b.Work(task) if err != nil { if res < 0 { return // want "Mutex lock must be released ..." } } b.m.Unlock() b.doWork(task) } 

THE PLAN Modernize ("it compiles!") Encode bugs as specs Focus on specs, AI "writes" code ↳ Repeat until accurate enough 

ACCURACY: 100% Months of hunting → 2 seconds anycable — zsh $ mulint ./... node/node.go:486:3: Mutex lock must be released before this line node/node.go:475: Lock was acquired here: s.smu.Lock() node/node.go:490:3: Mutex lock must be released before this line node/node.go:475: Lock was acquired here: s.smu.Lock() node/node.go:499:4: Mutex lock must be released before this line node/node.go:475: Lock was acquired here: s.smu.Lock() 

ACCURACY: 100% ...But only for a single project anycable — zsh $ (cd anycable && mulint ./...) (nothing) 

ACCURACY: 87% ...But only for a single project "Go code always looks the same" is a myth whatsmeow — zsh $ (cd whatsmeow && mulint ./...) notification.go:388:4: Mutex lock is acquired on this cli.dispatchEvent(wrapper.Data.Leave) notification.go:386: But the same lock was acquire cli.dispatchEvent(wrapper.Data.Join) (via Client:disp (14 more) 

ACCURACY: 32% ...But only for a single project "Go code always looks the same" is a myth Developers can be creative with mutexes nats-server — zsh $ (cd nats-server && mulint ./...) jetstream_cluster.go:2925:3: Mutex lock is acquired o server/jetstream_cluster.go:2905: But the same loc (143 more) 

ACCURACY: 32% ...But only for a single project "Go code always looks the same" is a myth Developers can be creative with mutexes ↳ Deadlocks are not what they seem , nats-server — zsh $ (cd nats-server && mulint ./...) jetstream_cluster.go:2925:3: Mutex lock is acquired o server/jetstream_cluster.go:2905: But the same loc (143 more) 

THE PLAN, V2 Pick good codebases Encode false positives as specs Fix the linter ↳ Learn new patterns! 

CODEBASES 

TRICKSLIST 1. No-lock windows 2. Hand-off 3. Wrappers 4. Cooperative locking 5. Conditionals & conventionals 

1. NO-LOCK WINDOWS Why: don't hold a lock across slow operations Risks: unbalanced returns (The Bug), stale state // anycable/node/node.go func (n *Node) History(s *Session, msg *Message) error { s.smu.Lock() if ok := s.subs.HasChannel(msg.Identifier); !ok { s.smu.Unlock() // early return return fmt.Errorf("unknown subscription %s", msg.Identifier) } subStreams := s.subs.StreamsFor(msg.Identifier) s.smu.Unlock() backlog, err := n.retreiveHistory(&history, subStreams) if s.IsClosed() { // state invalidation return nil } 

SLOW OPERATIONS I/O (network & disk), blocking syscalls // tailscale/net/dns/direct.go func (m *directManager) checkForFileTrample() { m.mu.Lock() want := m.wantResolvConf lastWarn := m.lastWarnContents m.mu.Unlock() cur, err := m.fs.ReadFile(resolvConf) // disk I/O if bytes.Equal(cur, want) { if lastWarn != nil { m.mu.Lock() m.lastWarnContents = nil m.mu.Unlock() ... } return } m.mu.Lock() m.lastWarnContents = cur m.mu.Unlock() // ... 

SLOW OPERATIONS I/O (network & disk), blocking syscalls // prometheus/tsdb/index/postings.go func (p *MemPostings) PostingsForLabelMatching(ctx context.Context, name string, match func(string) bool) Postings { p.mtx.RLock() readOnlyLabelValues := p.lvs[name] p.mtx.RUnlock() vals := make([]string, 0, len(readOnlyLabelValues)) for i, v := range readOnlyLabelValues { // ... if match(v) { // user-provided regexp matching vals = append(vals, v) } } p.mtx.RLock() e := p.m[name] for i, v := range vals { // ... } p.mtx.RUnlock() return Merge(ctx, its...) CPU-heavy work } 

SLOW OPERATIONS I/O (network & disk), blocking syscalls CPU-heavy work Waiting (channels, other locks) // anycable/hub/hub.go func (h *Hub) broadcastToStream(stream string, data string) { h.streamsMu.RLock() if _, ok := h.streams[stream]; !ok { ctx.Debug("No sessions") h.streamsMu.RUnlock() return } h.streamsMu.RUnlock() h.pool.Schedule(func() { // fixed-size goroutine pool h.streamsMu.RLock() streamSessions := streamSessionsSnapshot(h.streams[stream]) h.streamsMu.RUnlock() // .. }) } 

SLOW OPERATIONS I/O (network & disk), blocking syscalls CPU-heavy work Waiting (channels, other locks) User callbacks // k6/browser/common/frame_manager.go func (m *FrameManager) requestStarted(req *Request) { // ... m.page.routesMu.RLock() defer m.page.routesMu.RUnlock() for _, r := range m.page.routes { // ... func() { // In case routes are updated in the handler m.page.routesMu.RUnlock() defer m.page.routesMu.RLock() err := r.handler(route) // user code }() // ... } } 

SLOW OPERATIONS I/O (network & disk), blocking syscalls CPU-heavy work Waiting (channels, other locks) User callbacks // k6/browser/common/frame_manager.go func (m *FrameManager) requestStarted(req *Request) { // ... m.page.routesMu.RLock() defer m.page.routesMu.RUnlock() for _, r := range m.page.routes { // ... func() { // In case routes are updated in the handler m.page.routesMu.RUnlock() defer m.page.routesMu.RLock() err := r.handler(route) }() // ... ❓ } } 

var mu sync.Mutex func riskyLocked() { mu.Unlock() panic("boom") mu.Lock() // never hit } func main() { mu.Lock() func() { defer func() { recover() }() riskyLocked() }() mu.Unlock() // / sync: unlock of unlocked mutex fmt.Println("ok") } 

var mu sync.Mutex func riskyLocked() { mu.Unlock() defer mu.Lock() // hits even when panics panic("boom") } func main() { mu.Lock() func() { defer func() { recover() }() riskyLocked() }() mu.Unlock() // okay, unlocked ' fmt.Println("ok") // prints! } 

2. HAND-OFF Why: run the caller's code inside the critical section How: closures, handles Risks: miss the release (skip, panic) → lock forever // centrifuge/client_experimental.go func (c *Client) AcquireStorage() (map[string]any, func(map[string]any)) { c.storageMu.Lock() // ... return c.storage, func(updatedStorage map[string]any) { c.storage = updatedStorage c.storageMu.Unlock() } } 

2. HAND-OFF Why: run the caller's code inside the critical section How: closures, handles Risks: miss the release (skip, panic) → lock forever // bbolt/db.go func (db *DB) beginRWTx() (*Tx, error) { // This is released when tx is closed db.rwlock.Lock() // ... if !db.opened { // bbolt/tx.go db.rwlock.Unlock() return nil, berrors.ErrDatabaseNotOpen func (tx *Tx) close() { } // ... // ... if tx.writable { t := &Tx{writable: true} // ... t.init(db) tx.db.rwtx = nil db.rwtx = t tx.db.rwlock.Unlock() // ... // ... return t, nil } } } 

2. HAND-OFF Why: run the caller's code inside the critical section // bbolt/db.go func (db *DB) Update(fn func(*Tx) error) error { t, err := db.Begin(true) // calls beginRWTx() if err != nil { return err } defer func() { if t.db != nil { t.rollback() } // calls close() }() How: closures, handles t.managed = true err = fn(t) t.managed = false Risks: miss the release (skip, panic) → lock forever if err != nil { _ = t.Rollback() // calls close() return err } return t.Commit() // calls close() } 

3. WRAPPERS Why: add what sync.Mutex is missing What: asserts, shards, observability Risks: indirection hurts static analysis // tailscale/util/.../test_source.go func (s *TestStore) Lock() error { s.storeLock.RLock() s.storeLockCount.Add(1) return nil } func (s *TestStore) Unlock() { if s.storeLockCount.Add(-1) < 0 { s.tb.Fatal("negative storeLockCount") } s.storeLock.RUnlock() } 

3. WRAPPERS Why: add what sync.Mutex is missing What: asserts, shards, observability Risks: indirection hurts static analysis // syncthing@v1/sync/sync.go type loggedMutex struct { sync.Mutex holder atomic.Value } func (m *loggedMutex) Lock() { m.Mutex.Lock() m.holder.Store(getHolder()) } func (m *loggedMutex) Unlock() { currentHolder := m.holder.Load().(holder) duration := timeNow().Sub(currentHolder.time) if duration >= threshold { l.Debugf("Mutex held for... %v") } m.holder.Store(holder{}) m.Mutex.Unlock() } 

3. WRAPPERS Why: add what sync.Mutex is missing What: asserts, shards, observability Risks: indirection hurts static analysis // prometheus/sdb/head.go type stripeLock struct { sync.RWMutex // avoid multiple locks being on the same cache line _ [40]byte } // tailscale/syncs/shardedmap.go type mapShard[K comparable, V any] struct { mu sync.Mutex m map[K]V _ cpu.CacheLinePad } 

4. COOPERATIVE LOCKING Why: don't starve the waiters (readers, callers) Risks: stale state, RLock caveat // prometheus/tsdb/index/postings.go func (p *MemPostings) unlockWaitAndLockAgain() { p.mtx.Unlock() // While it's tempting to just do a // `time.Sleep(time.Millisecond)` here, it // wouldn't ensure use that readers actually // were able to get the read lock, // because if there are writes waiting // on same mutex, readers won't be able to get it. // So we just grab one RLock ourselves. p.mtx.RLock() p.mtx.RUnlock() time.Sleep(time.Millisecond) p.mtx.Lock() } 

4. COOPERATIVE LOCKING Why: don't starve the waiters (readers, callers) Risks: stale state, RLock caveat // nats-server/server/raft.go func (n *raft) ResumeApply() { n.Lock() defer n.Unlock() // ... for i := n.commit + 1; i <= n.hcommit; i++ { if err := n.applyCommit(i); err != nil { break } n.Unlock() runtime.Gosched() // let other Go routines run n.Lock() if n.State() == Closed { // check if got closed return } } // ... } 

FALSE POSITIVE ↳ PATTERN 

X. WHAT ELSE BREAKS THE LINTER // telegraf/plugins/inputs/prometheus/kubernetes.go func updateCadvisorPodList(p *Prometheus, req *http.Request) error { // ... telegraf — zsh p.lock.Lock() p.kubernetesPods = make(map[podID]urlAndAddress) $ mulint ./... for _, pod := range pods { if necessaryPodFieldsArePresent(pod) && kubernetes.go:289:4: shouldScrapePod(pod, p) { Mutex lock is acquired on this line: registerPod(pod, registerPod(pod, p) kubernetes.go:282: } } But the same lock was acquired here: p.lock.Lock() p.lock.Unlock() return nil } p) 

X. WHAT ELSE BREAKS THE LINTER: CONDITIONALS Why: same function, two contracts (state- or argument-dependent) // telegraf/plugins/inputs/prometheus/kubernetes.go func registerPod(pod *corev1.Pod, p *Prometheus) { // ... if !p.isNodeScrapeScope { p.lock.Lock() defer p.lock.Unlock() } // ... } 

X. WHAT ELSE BREAKS THE LINTER // nats-server/server/filestore.go func (fs *fileStore) firstSeqForSubj(subj string) (uint64, error) { nats-server — zsh // ... for i := start; i <= stop; i++ { $ mulint ./... // ... fs.mu.Unlock() mb.mu.Lock() filestore.go:5506:5: // ... if mb.fssNotLoaded() { Mutex lock must be released before this mb.mu.Unlock() filestore.go:5505: fs.mu.Lock() Lock was acquired here: fs.mu.Lock() return 0, err } } } line 

X. WHAT ELSE BREAKS THE LINTER: NO STATE HINTS Why: the state invariant is unknown to the linter (known to developers) ↳ Comments are not conventions we can enforce // nats-server/server/filestore.go // Write lock should be held. func (fs *fileStore) firstSeqForSubj(subj string) // Lock should be held. func (fs *fileStore) writeStreamMeta() // Lock for fs should be held. func (mb *msgBlock) checkAndLoadEncryption() // Lock should be held on entry. func (s *Server) registerAccountNoLock(acc *Account) 

X. WHAT ELSE BREAKS THE LINTER: NO STATE HINTS Why: the state invariant is unknown to the linter (known to developers) TODO: call-site inference, annotations (Clang: REQUIRES(fs.mu), Java: @GuardedBy) // nats-server/server/filestore.go // Write lock should be held. func (fs *fileStore) firstSeqForSubj(subj string) // Lock should be held. func (fs *fileStore) writeStreamMeta() // Lock for fs should be held. func (mb *msgBlock) checkAndLoadEncryption() // Lock should be held on entry. func (s *Server) registerAccountNoLock(acc *Account) 

MULINT STILL REPORTS FALSE POSITIVES 

MULINT STILL REPORTS FALSE POSITIVES MULINT ALSO FINDS REAL BUGS 

DEADLOCKS WE FOUND AND FIXED 

WHAT IF AI WALKED WITH ME? 

MODEL: OPUS 4.6 Eval Result anycable: "find deadlocks" "smu held during entire RPC call" (but no missing Unlocks) centrifuge: "find deadlocks" "no active deadlocks found" nats-server: "find deadlocks" "no clear unprotected deadlock was found" anycable: "review commit 3cf30a" "good change." anycable: "review commit 303f6b" "the two `smu` mutex leaks" 

MODEL: FABLE 5 Eval Result anycable: "find deadlocks" "the two most dangerous defects are leaked `s.smu`" centrifuge: "find deadlocks" "no fixable deadlock exists in the current code" nats-server: "find deadlocks" "no concrete deadlock found" anycable: "review commit 3cf30a" "the mechanical conversion introduced one serious bug" anycable: "review commit 303f6b" skip 

stage.Unlock() Lock; defer Unlock for as long as you can Patterns and conventions after that Enforce locking hygiene with tools (or DIY!) 

palkan/mulint github.com/palkan/mulint Modernized gnieto/mulint More shapes, less false positives, ignores go vet, golangci-lint, GitHub Action ↳ Give it a try, report your pattern! 

THANK YOU Project: github.com/palkan/mulint Slides: evilmartians.com/events X/Twitter: @palkan_tula, @evilmartians