Skip to content

Add a channel to close the go routine for updating ring metrics #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 12, 2021
Merged

Add a channel to close the go routine for updating ring metrics #62

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
429ff43
Add a channel to close the go routine for updating ring metrics
Oct 12, 2021
2286b72
Fix comment
Oct 12, 2021
171a0ab
refactor ticker creation to put it in the go routine
Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions ring/ring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ type Ring struct {
totalTokensGauge prometheus.Gauge
numTokensGaugeVec *prometheus.GaugeVec
oldestTimestampGaugeVec *prometheus.GaugeVec
metricsUpdateTicker *time.Ticker
metricsUpdateCloser chan struct{}

logger log.Logger
}
Expand Down Expand Up @@ -284,11 +284,20 @@ func (r *Ring) starting(ctx context.Context) error {
r.updateRingState(value.(*Desc))
r.updateRingMetrics()

// Start metrics update ticker, and give it a function to update the ring metrics.
r.metricsUpdateTicker = time.NewTicker(10 * time.Second)
// Use this channel to close the go routine to prevent leaks.
r.metricsUpdateCloser = make(chan struct{})
go func() {
for range r.metricsUpdateTicker.C {
Copy link
Contributor

@pracucci pracucci Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the previous implementation wasn't good enough? I would expect the goroutine to end when the ticker channel is closed. However, I think the issue (which I believe persists even after this PR) is that the stopping() function isn't waiting until the goroutine is effectively terminated, so the goroutines leaker test may fail anyway.

Copy link
Contributor

@pracucci pracucci Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive the comment about the ticker. I just read the doc and learned ticker.Stop() doesn't close the channel. In case it's still leak the goroutine, then the 2nd part of my comment may be valid.

Copy link
Contributor

@56quarters 56quarters Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's also been a mistake: the previous version of this used a Ticker while this version uses a Timer. Was that the intention?

Copy link
Contributor Author

@treid314 treid314 Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this should be a ticker. 😞

Copy link
Contributor

@56quarters 56quarters Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #63 for this

r.updateRingMetrics()
// Start metrics update ticker to update the ring metrics.
ticker := time.NewTimer(10 * time.Second)
defer ticker.Stop()

for {
select {
case <-ticker.C:
r.updateRingMetrics()
case <-r.metricsUpdateCloser:
return
}
}
}()
return nil
Expand All @@ -309,8 +318,8 @@ func (r *Ring) loop(ctx context.Context) error {

func (r *Ring) stopping(_ error) error {
// Stop Metrics ticker.
if r.metricsUpdateTicker != nil {
r.metricsUpdateTicker.Stop()
if r.metricsUpdateCloser != nil {
close(r.metricsUpdateCloser)
}
return nil
}
Expand Down