feat(promhttp): add CoalesceGather option to deduplicate concurrent Gather calls

[kakkoyun]

What problem does this solve?

Issue #1477 reports apparent goroutine leaks when using prometheus/client_golang. The root cause is not a WaitGroup bug — Registry.Gather() logic is correct. The real problem:

When a collector's Collect() is slower than the scrape interval, each incoming HTTP scrape triggers an independent Gather(). Each call spawns its own goroutine pipeline. With no upper bound, concurrent pipelines grow proportionally to (scrape rate / collection time) — a goroutine pile-up that looks like a leak but is unbounded concurrent work.

This was originally investigated by @initialed85 in this comment, who validated the issue and prototyped a "piggybacking" (singleflight) approach. This PR implements that idea cleanly inside the HTTP handler, with zero new public types and zero overhead when disabled.

Solution

Add HandlerOpts.CoalesceGather bool. When true, HandlerForTransactional wraps the TransactionalGatherer in an unexported coalescingGatherer that allows only one Gather() to run at a time. Concurrent HTTP requests arriving while a gather is in-flight join the existing cycle and receive the same result.

Usage

http.Handle("/metrics", promhttp.HandlerFor(reg, promhttp.HandlerOpts{
    CoalesceGather: true,
}))

Design decisions

Decision	Rationale
HandlerOpts field, not a public wrapper type	Zero new public types; users opt in with one field
Per-cycle gatherCycle object	Prevents a race where a new cycle overwrites result fields while prior waiters are still reading them
Mutex-based ref counting (not atomic)	Ensures c.cycle = nil is cleared before cy.done() is called, ruling out double-done on a stale pointer
close(cy.ready) as the visibility barrier	Go memory model guarantees cy.mfs/err/done are safely readable after <-cy.ready without additional locking
Default false	Zero overhead; identical code path to today when not enabled

Changes

File	Change
prometheus/promhttp/http.go	Add unexported coalescingGatherer + gatherCycle types; add HandlerOpts.CoalesceGather; wire into HandlerForTransactional
prometheus/promhttp/http_test.go	Add 4 tests: sequential invariant, done-called-exactly-once, goroutine-leak-free (goleak), fresh-cycle-after-release

Test plan

• go test -race -count=1 ./prometheus/promhttp/... — all pass, no data races
• go vet ./prometheus/... — clean
• TestCoalesceGatherGoroutineLeakFree uses goleak.VerifyNone to assert no leaked goroutines under concurrent slow-collector load
• TestCoalesceGatherDoneCalledExactlyOnce verifies the TransactionalGatherer contract: done() calls == Gather() calls regardless of concurrency

Closes #1477

[initialed85]

@kakkoyun LGTM- I tested with the roughly same approach as for my hack implementation (ref.: ncabatoff/process-exporter@master...initialed85:process-exporter:kakkoyun/goroutine_leak)

Worked as expected- with a fake 1s sleep added to the scrape method in process-exporter:

• A request loop with a 600ms timeout would would return a result every other request
• 4 x concurrent request loops with no timeout would get a result for every request (after 1s)
• process_open_fds reported by the process-exporter stayed pretty steady at 11 or 12 (impacted by whether or not the fast request loop was active at the time)
• process_open_fds dropped down to 9 if I killed 2 of the 4 concurrent request loops

So I think we're in good shape- lots of timing out of requests, no climbing FDs; works nicely.

Awesome work!

[vesari]

One question: if c.g.Gather() panics, do the joiners actually see the panic, or do they just unblock and return as if everything was ok?

[kakkoyun]

This is a bit of defensive coding here. They just unblock and clean up. If there is no recover in the chain, it will be printed as any other panic.

Feel free to challenge and propose better alternative. Would you prefer us to capture the error?

[vesari]

I made a suggestion above to capture the error. This is because I was thinking that HandlerForTransactional basically returns an http.Handler and if I’m reading this correctly https://github.com/golang/go/blob/e2fa8596cf7016d080c51d772c3012f2533e2219/src/net/http/server.go#L84, then the panics are recovered in any case.

[vesari]

I left just a small suggestion.

[vesari]

Suggested change

	close(cy.ready)
	cy.err = fmt.Errorf("gather panicked")
	close(cy.ready)

[vesari]

I made a suggestion above to capture the error. This is because I was thinking that HandlerForTransactional basically returns an http.Handler and if I’m reading this correctly https://github.com/golang/go/blob/e2fa8596cf7016d080c51d772c3012f2533e2219/src/net/http/server.go#L84, then the panics are recovered in any case.