fix: stop cached DNS refresh goroutine on close

CHANGELOG.md

@@ -21,6 +21,8 @@ The following emojis are used to highlight certain changes:
 
 ### Fixed
 
+- stop the cached DNS refresh goroutine on `Close()` to avoid leaking a goroutine ([#431](https://github.com/ipfs/rainbow/pull/431))
+
 ### Removed
 
 ### Security

dns_cache.go

@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"net"
+	"sync"
 	"time"
 
 	"github.com/rs/dnscache"
@@ -15,12 +16,21 @@ const dnsCacheRefreshInterval = 5 * time.Minute
 type cachedDNS struct {
 	resolver  *dnscache.Resolver
 	refresher *time.Ticker
+
+	// done is closed by Close to signal the refresh goroutine to exit.
+	// time.Ticker.Stop does not close Ticker.C, so we cannot rely on
+	// `for range cdns.refresher.C` to terminate.
+	done chan struct{}
+	// closeOnce guarantees done is closed and the goroutine is joined exactly once.
+	closeOnce sync.Once
+	wg        sync.WaitGroup
 }
 
 func newCachedDNS(refreshInterval time.Duration) *cachedDNS {
 	cache := &cachedDNS{
 		resolver:  &dnscache.Resolver{},
 		refresher: time.NewTicker(refreshInterval),
+		done:      make(chan struct{}),
 	}
 
 	// Configure DNS cache to not remove stale records to protect gateway from
@@ -31,10 +41,16 @@ func newCachedDNS(refreshInterval time.Duration) *cachedDNS {
 
 	// Every refreshInterval we check for updates, but if there is
 	// none, or if domain disappears, we keep the last cached version
+	cache.wg.Add(1)
 	go func(cdns *cachedDNS) {
-		defer cdns.refresher.Stop()
-		for range cdns.refresher.C {
-			cdns.resolver.RefreshWithOptions(options)
+		defer cdns.wg.Done()
+		for {
+			select {
+			case <-cdns.refresher.C:
+				cdns.resolver.RefreshWithOptions(options)
+			case <-cdns.done:
+				return
+			}
 		}
 	}(cache)
 
@@ -62,7 +78,13 @@ func (cdns *cachedDNS) dialWithCachedDNS(ctx context.Context, network string, ad
 	return
 }
 
+// Close stops the refresh goroutine and waits for it to exit. It is safe to
+// call multiple times.
 func (cdns *cachedDNS) Close() error {
-	cdns.refresher.Stop()
+	cdns.closeOnce.Do(func() {
+		cdns.refresher.Stop()
+		close(cdns.done)
+		cdns.wg.Wait()
+	})
 	return nil
 }

dns_cache_test.go

@@ -0,0 +1,54 @@
+package main
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestCachedDNS_CloseStopsRefreshGoroutine verifies that Close terminates the
+// background refresh goroutine and is safe to call more than once.
+//
+// The refresh interval is set very long so the goroutine is parked waiting on
+// either a tick (which would not arrive for an hour) or the done channel.
+// With a long interval, the only way Close can return promptly AND the worker
+// can have exited is if Close actually signals the worker to stop. A buggy
+// implementation that only calls time.Ticker.Stop lets Close return while the
+// goroutine stays blocked on `for range refresher.C` forever.
+//
+// We assert goroutine termination via the same sync.WaitGroup the production
+// code uses (this is a same-package white-box test), which is deterministic
+// and does not rely on the flaky runtime.NumGoroutine.
+func TestCachedDNS_CloseStopsRefreshGoroutine(t *testing.T) {
+	cdns := newCachedDNS(time.Hour)
+
+	closed := make(chan struct{})
+	go func() {
+		_ = cdns.Close()
+		close(closed)
+	}()
+
+	select {
+	case <-closed:
+		// Close returned promptly.
+	case <-time.After(5 * time.Second):
+		t.Fatal("Close() did not return; refresh goroutine appears blocked on the ticker channel")
+	}
+
+	// The worker goroutine must have actually exited by now.
+	workerExited := make(chan struct{})
+	go func() {
+		cdns.wg.Wait()
+		close(workerExited)
+	}()
+	select {
+	case <-workerExited:
+		// Worker exited: the goroutine leak is fixed.
+	case <-time.After(5 * time.Second):
+		t.Fatal("refresh goroutine did not exit after Close(); cachedDNS still leaks a goroutine")
+	}
+
+	// A second Close must be safe and idempotent (no panic from double close).
+	require.NoError(t, cdns.Close())
+}