Part of duplicate code analysis: #1824

Summary

The double-check locking pattern (read-lock → check → unlock → write-lock → double-check → create) is implemented independently in at least 2 locations: internal/server/routed.go (filteredServerCache.getOrCreate) and internal/logger/server_file_logger.go (ServerFileLogger.getOrCreateLogger). Each implementation is ~15–20 lines of nearly identical boilerplate.

Duplication Details

Pattern: Read-lock / write-lock / double-check initialization (lazy cache population)

• Severity: Medium
• Occurrences: 2+ distinct implementations
• Locations:
  • internal/server/routed.go (lines ~46–74) — filteredServerCache.getOrCreate
  • internal/logger/server_file_logger.go (lines ~55–95) — ServerFileLogger.getOrCreateLogger

Location 1 — routed.go:

func (c *filteredServerCache) getOrCreate(backendID, sessionID string, creator func() *sdk.Server) *sdk.Server {
    key := fmt.Sprintf("%s/%s", backendID, sessionID)
    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
    }
    server := creator()
    c.servers[key] = server
    return server
}

Location 2 — server_file_logger.go:

func (sfl *ServerFileLogger) getOrCreateLogger(serverID string) (*log.Logger, error) {
    sfl.mu.RLock()
    if logger, exists := sfl.loggers[serverID]; exists {
        sfl.mu.RUnlock()
        return logger, nil
    }
    sfl.mu.RUnlock()
    sfl.mu.Lock()
    defer sfl.mu.Unlock()
    if logger, exists := sfl.loggers[serverID]; exists {
        return logger, nil
    }
    // ... create logic ...
    return logger, nil
}

Impact Analysis

• Maintainability: The pattern is correct but verbose; any subtle bug in the locking protocol must be fixed in multiple places
• Bug Risk: Low — both implementations appear correct, but subtle races could be introduced during future modifications
• Code Bloat: ~30 lines could be reduced to ~10 lines if a generic helper were used

Refactoring Recommendations

1. Use sync.Map for simple key→value caches (Go stdlib): eliminates the need for manual double-check locking in many cases.

2. Alternatively, create a generic getOrCreate helper using Go 1.18+ generics if the project is on a compatible version:

   // getOrCreate safely initializes a value in a map using double-check locking.
   // Returns (value, wasCreated, error).
   func getOrCreate[K comparable, V any](mu *sync.RWMutex, m map[K]V, key K, create func() (V, error)) (V, error) {
       mu.RLock()
       if v, ok := m[key]; ok {
           mu.RUnlock()
           return v, nil
       }
       mu.RUnlock()
       mu.Lock()
       defer mu.Unlock()
       if v, ok := m[key]; ok {
           return v, nil
       }
       v, err := create()
       if err != nil {
           var zero V
           return zero, err
       }
       m[key] = v
       return v, nil
   }

   This could live in internal/sys/ or a new internal/syncutil/ package.

3. Short-term: Add a comment referencing this pattern in both files so future maintainers update both together.

Implementation Checklist

• Decide between sync.Map approach vs. generic helper
• If generic helper: create internal/syncutil/cache.go with getOrCreate
• Refactor filteredServerCache.getOrCreate in internal/server/routed.go
• Refactor ServerFileLogger.getOrCreateLogger in internal/logger/server_file_logger.go
• Run make test to verify no regressions
• Run make agent-finished before completing

Parent Issue

See parent analysis report: #1824
Related to #1824

Generated by Duplicate Code Detector · ◷

• expires on Mar 20, 2026, 2:58 AM UTC