From 7e27df2d44aa4b147b1d65cf31197a80f9f75ba3 Mon Sep 17 00:00:00 2001 From: Nathan Wallace Date: Sun, 16 Nov 2025 14:11:31 -0500 Subject: [PATCH] Fix #4791: Use idiomatic for-range pattern for error channel closes #4791 (#4835) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add test demonstrating bug #4791: Goroutine leak in executeUpdateSetsInParallel 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude * Fix #4791: Use idiomatic for-range pattern for error channel Replace the for-select-nil-check pattern with the idiomatic for-range pattern when consuming from the error channel. The for-range pattern: - Automatically exits when the channel is closed - Doesn't require manual nil checks - Is more maintainable and less error-prone - Follows Go best practices for channel consumption This eliminates the potential for goroutine leaks if the nil check were accidentally removed or modified in future maintenance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --------- Co-authored-by: Claude --- pkg/connection/connection_lifecycle_test.go | 80 +++++++++++++++++++++ pkg/connection/refresh_connections_state.go | 20 ++---- 2 files changed, 87 insertions(+), 13 deletions(-) diff --git a/pkg/connection/connection_lifecycle_test.go b/pkg/connection/connection_lifecycle_test.go index 3203f3945..d5c3be373 100644 --- a/pkg/connection/connection_lifecycle_test.go +++ b/pkg/connection/connection_lifecycle_test.go @@ -2,6 +2,8 @@ package connection import ( "context" + "errors" + "runtime" "sync" "sync/atomic" "testing" @@ -279,3 +281,81 @@ func TestLogRefreshConnectionResultsTypeAssertion(t *testing.T) { state.logRefreshConnectionResults() }) } + +// TestExecuteUpdateSetsInParallelGoroutineLeak tests for goroutine leak in executeUpdateSetsInParallel +// This test demonstrates issue #4791 - potential goroutine leak with non-idiomatic channel pattern +// +// The issue is in refresh_connections_state.go:519-536 where the goroutine uses: +// for { select { case connectionError := <-errChan: if connectionError == nil { return } } } +// +// While this pattern technically works when the channel is closed (returns nil, then returns from goroutine), +// it has several problems: +// 1. It's not idiomatic Go - the standard pattern for consuming until close is 'for range' +// 2. It relies on nil checks which can be error-prone +// 3. It's harder to understand and maintain +// 4. If the nil check is accidentally removed or modified, it causes a goroutine leak +// +// The idiomatic pattern 'for range errChan' automatically exits when channel is closed, +// making the code safer and more maintainable. +func TestExecuteUpdateSetsInParallelGoroutineLeak(t *testing.T) { + // Get baseline goroutine count + runtime.GC() + time.Sleep(100 * time.Millisecond) + baselineGoroutines := runtime.NumGoroutine() + + // Test the CURRENT pattern from refresh_connections_state.go:519-536 + // This pattern has potential for goroutine leaks if not carefully maintained + errChan := make(chan *connectionError) + var errorList []error + var mu sync.Mutex + + // Simulate the current (non-idiomatic) pattern + go func() { + for { + select { + case connectionError := <-errChan: + if connectionError == nil { + return + } + mu.Lock() + errorList = append(errorList, connectionError.err) + mu.Unlock() + } + } + }() + + // Send some errors + testErr := errors.New("test error") + errChan <- &connectionError{name: "test1", err: testErr} + errChan <- &connectionError{name: "test2", err: testErr} + + // Close the channel (this should cause goroutine to exit via nil check) + close(errChan) + + // Give time for the goroutine to process and exit + time.Sleep(200 * time.Millisecond) + runtime.GC() + time.Sleep(100 * time.Millisecond) + + // Check for goroutine leak + afterGoroutines := runtime.NumGoroutine() + goroutineDiff := afterGoroutines - baselineGoroutines + + // The current pattern SHOULD work (goroutine exits via nil check), + // but we're testing to document that the pattern is risky + if goroutineDiff > 2 { + t.Errorf("Goroutine leak detected with current pattern: baseline=%d, after=%d, diff=%d", + baselineGoroutines, afterGoroutines, goroutineDiff) + } + + // Verify errors were collected + mu.Lock() + if len(errorList) != 2 { + t.Errorf("Expected 2 errors, got %d", len(errorList)) + } + mu.Unlock() + + t.Logf("BUG #4791: Current pattern works but is non-idiomatic and error-prone") + t.Logf("The for-select-nil-check pattern at refresh_connections_state.go:520-535") + t.Logf("should be replaced with idiomatic 'for range errChan' for safety and clarity") +} diff --git a/pkg/connection/refresh_connections_state.go b/pkg/connection/refresh_connections_state.go index 78a1a480d..0b7afc9c2 100644 --- a/pkg/connection/refresh_connections_state.go +++ b/pkg/connection/refresh_connections_state.go @@ -531,20 +531,14 @@ func (s *refreshConnectionState) executeUpdateSetsInParallel(ctx context.Context sem := semaphore.NewWeighted(maxParallel) go func() { - for { - select { - case connectionError := <-errChan: - if connectionError == nil { - return - } - errors = append(errors, connectionError.err) - conn, poolErr := s.pool.Acquire(ctx) - if poolErr == nil { - if err := s.tableUpdater.onConnectionError(ctx, conn.Conn(), connectionError.name, connectionError.err); err != nil { - log.Println("[WARN] failed to update connection state table", err.Error()) - } - conn.Release() + for connectionError := range errChan { + errors = append(errors, connectionError.err) + conn, poolErr := s.pool.Acquire(ctx) + if poolErr == nil { + if err := s.tableUpdater.onConnectionError(ctx, conn.Conn(), connectionError.name, connectionError.err); err != nil { + log.Println("[WARN] failed to update connection state table", err.Error()) } + conn.Release() } } }()