Skip to content

fix: stop cached DNS refresh goroutine on close - #431

Open
kediyalua wants to merge 1 commit into
ipfs:mainfrom
kediyalua:main
Open

kediyalua wants to merge 1 commit into
ipfs:mainfrom
kediyalua:main

Conversation

@kediyalua

Copy link
Copy Markdown

Summary

cachedDNS.Close() does not actually stop the background refresh goroutine.

Close() calls time.Ticker.Stop(), but Stop() does not close Ticker.C. The refresh goroutine loops with for range cdns.refresher.C, which therefore blocks forever after Close() and leaks. This affects the daemon (where cdns is used via defer cdns.Close(), e.g. main.go) and, more visibly, the test suite, which constructs and cleans up a cachedDNS in many tests (setup_test.go, main_test.go).

Fix

  • Add an explicit done chan struct{} and drive the worker with a select on refresher.C and done instead of for range refresher.C.
  • Make Close() close done (once, via sync.Once) and Wait() on a sync.WaitGroup, so it reliably stops and joins the worker. Close() stays safe to call multiple times (idempotent).

Test plan

  • Added TestCachedDNS_CloseStopsRefreshGoroutine (package main, white-box). It uses a 1-hour refresh interval so the worker is parked waiting on the ticker, then asserts:
    • Close() returns promptly,
    • the worker goroutine has actually exited (checked via the same sync.WaitGroup the code uses — deterministic, no runtime.NumGoroutine()),
    • a second Close() is idempotent.
  • Verified locally: go test -run TestCachedDNS_CloseStopsRefreshGoroutine . passes; a negative variant (omitting close(done)) fails, confirming the test catches the bug.

Changelog

  • CHANGELOG.md under ## [Unreleased] → ### Fixed: "stop the cached DNS refresh goroutine on Close() to avoid leaking a goroutine".

Closes #NNNN

Signed-off-by: kediyalua <kediyalua@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant