Part of duplicate code analysis: #1882

Summary

A read-then-write double-check locking pattern (RLock → check → RUnlock → Lock → double-check → create) appears 3+ times across internal/launcher/ and internal/server/, representing ~45 lines of duplicated concurrent-map management code.

Duplication Details

Pattern: Double-Check Locking for Lazy Cache Initialisation

• Severity: Medium-High
• Occurrences: 3 distinct locations
• Locations:
  • internal/launcher/launcher.go — GetOrLaunch (lines ~70–90) and GetOrLaunchForSession (lines ~141–154)
  • internal/launcher/connection_pool.go — SessionConnectionPool.Get (lines ~208–237)
  • internal/server/routed.go — filteredServerCache.getOrCreate (lines ~46–74)

Code Sample (launcher.go GetOrLaunch):

l.mu.RLock()
if conn, ok := l.connections[serverID]; ok {
    l.mu.RUnlock()
    return conn, nil
}
l.mu.RUnlock()

l.mu.Lock()
defer l.mu.Unlock()
if conn, ok := l.connections[serverID]; ok {
    return conn, nil
}
// ... create conn
l.connections[serverID] = conn
return conn, nil

Same skeleton in routed.go:

c.mu.RLock()
if server, exists := c.servers[key]; exists {
    c.mu.RUnlock()
    return server
}
c.mu.RUnlock()

c.mu.Lock()
defer c.mu.Unlock()
if server, exists := c.servers[key]; exists {
    return server
}
// ... create server
c.servers[key] = server
return server

Impact Analysis

• Maintainability: Any correctness fix to the locking strategy (e.g., adding a sync.Once per key) must be applied to 3+ independent locations
• Bug Risk: High — subtle differences between copies (e.g., one copy missing the second check inside the write lock) can introduce data races
• Code Bloat: ~45 lines of near-identical concurrent control flow

Refactoring Recommendations

1. Generic getOrCreate helper — introduce a type-parameterised helper (Go 1.18+):

   // internal/syncutil/cache.go
   func GetOrCreate[K comparable, V any](
       mu *sync.RWMutex,
       m map[K]V,
       key K,
       create func(K) (V, error),
   ) (V, error)

   Each call site becomes a single syncutil.GetOrCreate(...) call.

2. sync.Map — evaluate whether sync.Map (stdlib) is sufficient for these use cases; it handles double-check locking internally and is idiomatic for append-mostly maps.

3. Document the pattern — if a custom implementation is retained, extract it to a named type (lockedMap[K, V]) with a single getOrCreate method, used by both launcher and server packages.

Implementation Checklist

• Identify all instances of the read-check-unlock-lock-check pattern in the codebase
• Write a shared generic helper or adopt sync.Map
• Replace each call site
• Run the race detector: go test -race ./internal/...
• Verify integration tests pass: make test-integration

Parent Issue

See parent analysis report: #1882
Related to #1882

Generated by Duplicate Code Detector · ◷

• expires on Mar 21, 2026, 2:57 AM UTC