Fix goroutine leak in /probe multi-target endpoint - #1226
Open
danlucioprada wants to merge 1 commit into
Open
Conversation
The Shards and Indices collectors start a background cluster info receive loop in their constructors that only exits when the update channel is closed. In single-target mode a long-lived clusterinfo.Retriever owns that channel for the process lifetime. The /probe handler, however, builds a fresh Shards and Indices per scrape and never registers them with a retriever, so nothing ever closes their channel. Each /probe scrape therefore leaks two goroutines (one per collector). The leaked goroutines retain the collector, its HTTP client and registry, so resident memory grows unbounded until the process is OOMKilled. Add a Close method to both collectors that closes the channel (guarded by sync.Once so it is idempotent) and call it via defer in the /probe handler so the receive-loop goroutines exit when the handler returns. Single-target mode is unchanged and never calls Close. Add a regression test that creates and closes many collectors and asserts the goroutine count returns to its baseline. Signed-off-by: Dan Lucio Prada <dan.prada@effecti.com.br>
Contributor
|
Thanks for the issue and PR. We are actually in the process of deprecating the cluster info package and the channel logic all together. The new way to get the info is through the cluster provider. Check out the new cluster module for details. |
Author
|
So, is something likely already being done for a future version? In that case, I can close the PR |
Contributor
|
Yes, in a future version, we will finish removing the old cluster info package and this bug should go away with that change. |
Contributor
|
@sysadmind could you clarify our plans for this one? Should we close this PR since we're removing this part of the codebase? Should we fix the leak while the code is still here? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1225
Problem
The
ShardsandIndicescollectors start a background cluster-info receive loop in their constructor that only exits when their update channel is closed:In single-target mode a long-lived
clusterinfo.Retrieverowns that channel for the process lifetime, so this is fine. But the/probehandler builds a freshShardsandIndicesper scrape and never registers them with a retriever, so nothing ever closes the channel. Each/probescrape leaks two goroutines, and each leaked goroutine retains the collector, its per-probe HTTP client and registry, so resident memory grows unbounded until the process is OOMKilled.Reproduced on v1.11.0:
go_goroutinesgrows by exactly 2 per/proberequest and never drops (full analysis and pprof dump in #1225).Fix
Close()method (guarded bysync.Once) toShardsandIndicesthat closesclusterInfoCh, letting the receive-loop goroutine return.Close()viadeferin the/probehandler so both goroutines exit when the handler returns.Single-target mode is unchanged, it never calls
Close(), so the retriever keeps owning the channel.Verification
Before/after, hammering
/probeagainst a dead target (the goroutines are started at construction, so a live ES is not needed):Added a regression test (
collector/probe_leak_test.go) that creates and closes 50 of each collector and asserts the goroutine count returns to baseline.go test ./...passes;gofmt/go vetclean.Note
I went with the minimal, lifecycle-based fix that mirrors the existing consumer model. An alternative would be to skip starting the receive-loop goroutine entirely when no retriever is attached (i.e. in
/probemode).