A potential goroutine memory leak

[xuxiaofan1203]

Hello, I found a potential bug when I used the project, I'm not sure I'm right, we can discuss the problem to avoid a potential trouble possibly.

Blocking position:

client_golang/prometheus/registry.go

Line 472 in 26e3055

wg.Wait()

At line 449 call the wg.Add(goroutineBudget), and wg.Wait() is blocked until wg.Done() is called the number of goroutineBudget times to awaken the wg.Wait().
But if the select statement chooses the default path, return the function, maybe the wg.Done() has not executed enough times to awaken the wg.Wait(), which can result in a goroutine leak

client_golang/prometheus/registry.go

Lines 451 to 463 in 26e3055

	collectWorker := func() {
	for {
	select {
	case collector := <-checkedCollectors:
	collector.Collect(checkedMetricChan)
	case collector := <-uncheckedCollectors:
	collector.Collect(uncheckedMetricChan)
	default:
	return
	}
	wg.Done()
	}
	}

Complete codes of the part

client_golang/prometheus/registry.go

Lines 449 to 475 in 26e3055

	wg.Add(goroutineBudget)
	collectWorker := func() {
	for {
	select {
	case collector := <-checkedCollectors:
	collector.Collect(checkedMetricChan)
	case collector := <-uncheckedCollectors:
	collector.Collect(uncheckedMetricChan)
	default:
	return
	}
	wg.Done()
	}
	}
	// Start the first worker now to make sure at least one is running.
	go collectWorker()
	goroutineBudget--
	// Close checkedMetricChan and uncheckedMetricChan once all collectors
	// are collected.
	go func() {
	wg.Wait()
	close(checkedMetricChan)
	close(uncheckedMetricChan)
	}()

[ArthurSens]

I guess we could test this behavior using the runtime package? More precisely, using runtime.NumGoroutine()(https://pkg.go.dev/runtime#NumGoroutine)

Verify the number of goroutines, execute the function you believe there is a leak, and verify the number of go routines again. If numbers are different, it means we have a leak :)

[bwplotka]

As @kakkoyun suggested, we can double check with https://github.com/uber-go/goleak. Help wanted to add this (if not added before) (:

[surinkim]

Analysis of Potential Goroutine Leak in Registry.Gather

After a detailed review of the code, I found no evidence of goroutine leaks in the current implementation of Registry.Gather(). The logic ensures that all collectors are processed exactly once and that the WaitGroup counter reaches zero as expected.

Key Observations

checkedCollectors := make(chan Collector, len(r.collectorsByID))
uncheckedCollectors := make(chan Collector, len(r.uncheckedCollectors))

// All collectors are added to their respective channels
for _, collector := range r.collectorsByID {
    checkedCollectors <- collector
}
for _, collector := range r.uncheckedCollectors {
    uncheckedCollectors <- collector
}

• The sizes of checkedCollectors and uncheckedCollectors channels exactly match the number of registered collectors.
• All collectors are inserted into their respective channels before any goroutine starts.
• Each collectWorker goroutine processes at least one collector before hitting the default case.
• The default case is only reached when both channels are empty, which implies all collectors have already been picked up.
• Every processed collector leads to a wg.Done() call, maintaining WaitGroup correctness.

Verification

To verify this behavior, I used two approaches:

1. Compared the goroutine count before and after multiple Gather() calls using runtime.NumGoroutine()
2. Added a test using go.uber.org/goleak to formally check for leaks

Both methods confirmed that the current implementation does not leak goroutines.

Conclusion

The current design of Registry.Gather() correctly ensures that all work is done and cleaned up.
To ensure this remains true even after future changes to the internal logic, I will submit a PR #1797 with a goleak-based test that verifies goroutine hygiene.

[xuxiaofan1203]

Thank you for your reply and careful review. Just as you said, the default case is only reached when both channels are empty, which implies all collectors have already been picked up. Every processed collector leads to a wg.Done() call, maintaining WaitGroup correctness. So the potential goroutine leak will not appear. I agree with you.

Analysis of Potential Goroutine Leak in Registry.Gather

After a detailed review of the code, I found no evidence of goroutine leaks in the current implementation of Registry.Gather(). The logic ensures that all collectors are processed exactly once and that the WaitGroup counter reaches zero as expected.

Key Observations

checkedCollectors := make(chan Collector, len(r.collectorsByID))
uncheckedCollectors := make(chan Collector, len(r.uncheckedCollectors))

// All collectors are added to their respective channels
for _, collector := range r.collectorsByID {
checkedCollectors <- collector
}
for _, collector := range r.uncheckedCollectors {
uncheckedCollectors <- collector
}

• The sizes of checkedCollectors and uncheckedCollectors channels exactly match the number of registered collectors.
• All collectors are inserted into their respective channels before any goroutine starts.
• Each collectWorker goroutine processes at least one collector before hitting the default case.
• The default case is only reached when both channels are empty, which implies all collectors have already been picked up.
• Every processed collector leads to a wg.Done() call, maintaining WaitGroup correctness.

Verification

To verify this behavior, I used two approaches:

1. Compared the goroutine count before and after multiple Gather() calls using runtime.NumGoroutine()
2. Added a test using go.uber.org/goleak to formally check for leaks

Both methods confirmed that the current implementation does not leak goroutines.

Conclusion

The current design of Registry.Gather() correctly ensures that all work is done and cleaned up. To ensure this remains true even after future changes to the internal logic, I will submit a PR #1797 with a goleak-based test that verifies goroutine hygiene.