@marcel91200 / caddy Mirror · read-only / commits / 69d6ace3
Fork 0

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS

Mirrored from https://github.com/Slazee/caddy.git

Synced Nov 11, 08:30 UTC

tracing: fix BatchSpanProcessor goroutine leak on config reload (#7826)

* tracing: fix BatchSpanProcessor goroutine leak on config reload When the `tracing` directive is enabled, each config reload leaks one `go.opentelemetry.io/otel/sdk/trace.(*batchSpanProcessor).processQueue` goroutine. On a server that reloads frequently (e.g. polling a remote config source every few seconds) this accumulates into tens of thousands of leaked goroutines over time. A goroutine dump shows many identical stacks: ``` goroutine ... [select]: go.opentelemetry.io/otel/sdk/trace.(*batchSpanProcessor).processQueue(...) .../sdk/trace/batch_span_processor.go:327 go.opentelemetry.io/otel/sdk/trace.NewBatchSpanProcessor.func2() .../sdk/trace/batch_span_processor.go:129 created by go.opentelemetry.io/otel/sdk/trace.NewBatchSpanProcessor .../sdk/trace/batch_span_processor.go:127 ``` `tracing` keeps a global, reference-counted `TracerProvider` so it can be reused across reloads (`tracerProvider.getTracerProvider`). Caddy reloads provision the new config *before* cleaning up the old one, so the counter never drops to 0 and the provider is correctly reused — `Shutdown` is never called, by design. The problem is on the caller side in `newOpenTelemetryWrapper`: ```go traceExporter, err := autoexport.NewSpanExporter(ctx) ... tracerProvider := globalTracerProvider.getTracerProvider( sdktrace.WithBatcher(traceExporter), // evaluated on every reload sdktrace.WithResource(res), ) ``` `sdktrace.WithBatcher(e)` is `WithSpanProcessor(NewBatchSpanProcessor(e))`, and `NewBatchSpanProcessor` **starts its `processQueue` goroutine eagerly at construction time** — not when the option is applied. The option is built on every `Provision` (every reload), but `getTracerProvider` only applies it when it actually creates a new provider (`t.tracerProvider == nil`). On the reuse path the option is silently discarded, so the just-started BatchSpanProcessor goroutine is orphaned: it is never registered with any provider and therefore never shut down. Result: one leaked goroutine (plus a leaked exporter) per reload. Defer construction of the exporter/batcher until a new provider is actually needed. `getTracerProvider` now takes a `buildOpts` factory that is invoked only on the create path, so nothing with a side effect is built on the reuse path. The reference counter is now incremented only after the provider is successfully obtained, preserving the previous semantics where a failed exporter creation did not affect the counter. - `Test_tracersProvider_buildOptsOnlyOnCreate` — asserts `buildOpts` runs exactly once across one create + five reuses (the regression guard). - `Test_tracersProvider_buildOptsError` — asserts that on a build error the provider stays nil and the counter is not incremented. - Existing tracing tests updated for the new signature and still pass. Verified manually with a reload loop: before the fix, 50 reloads leaked 50 `processQueue` goroutines; after the fix, 0 are leaked while the provider is still reused (counter stays at 1). * add test for buldOpts error path --------- Co-authored-by: Zen Dodd <mail@steadytao.com>

wangdongyong committed Jun 19, 2026 at 01:42 UTC 69d6ace32e236f6d6873120254aab6f517a49ab1
3 files changed +98 -18
modules/caddyhttp/tracing/tracer.go
+19 -9
@@ -62,17 +62,27 @@ func newOpenTelemetryWrapper(
62 return ot, fmt.Errorf("creating resource error: %w", err)
63 }
64
65 - traceExporter, err := autoexport.NewSpanExporter(ctx)
66 - if err != nil {
67 - return ot, fmt.Errorf("creating trace exporter error: %w", err)
68 - }
69 -
65 ot.propagators = autoprop.NewTextMapPropagator()
66
72 - tracerProvider := globalTracerProvider.getTracerProvider(
73 - sdktrace.WithBatcher(traceExporter),
74 - sdktrace.WithResource(res),
75 - )
67 + // Defer creation of the exporter (and its batch span processor goroutine)
68 + // until we know a new provider is actually needed. When the global provider
69 + // already exists it is reused and these options are discarded; building them
70 + // here unconditionally would leak the exporter and a BatchSpanProcessor
71 + // goroutine on every config reload.
72 + tracerProvider, err := globalTracerProvider.getTracerProvider(func() ([]sdktrace.TracerProviderOption, error) {
73 + traceExporter, err := autoexport.NewSpanExporter(ctx)
74 + if err != nil {
75 + return nil, fmt.Errorf("creating trace exporter error: %w", err)
76 + }
77 +
78 + return []sdktrace.TracerProviderOption{
79 + sdktrace.WithBatcher(traceExporter),
80 + sdktrace.WithResource(res),
81 + }, nil
82 + })
83 + if err != nil {
84 + return ot, err
85 + }
86
87 ot.handler = otelhttp.NewHandler(http.HandlerFunc(ot.serveHTTP),
88 ot.spanName,
modules/caddyhttp/tracing/tracerprovider.go
+15 -5
@@ -19,20 +19,30 @@ type tracerProvider struct {
19 tracerProvidersCounter int
20 }
21
22 -// getTracerProvider create or return an existing global TracerProvider
23 -func (t *tracerProvider) getTracerProvider(opts ...sdktrace.TracerProviderOption) *sdktrace.TracerProvider {
22 +// getTracerProvider create or return an existing global TracerProvider.
23 +//
24 +// buildOpts is only invoked when a new provider must actually be created.
25 +// This matters because some options (notably sdktrace.WithBatcher) eagerly
26 +// start background goroutines when constructed. Building them unconditionally
27 +// and then discarding them on the reuse path would leak a BatchSpanProcessor
28 +// goroutine on every config reload.
29 +func (t *tracerProvider) getTracerProvider(buildOpts func() ([]sdktrace.TracerProviderOption, error)) (*sdktrace.TracerProvider, error) {
30 t.mu.Lock()
31 defer t.mu.Unlock()
32
27 - t.tracerProvidersCounter++
28 -
33 if t.tracerProvider == nil {
34 + opts, err := buildOpts()
35 + if err != nil {
36 + return nil, err
37 + }
38 t.tracerProvider = sdktrace.NewTracerProvider(
39 opts...,
40 )
41 }
42
35 - return t.tracerProvider
43 + t.tracerProvidersCounter++
44 +
45 + return t.tracerProvider, nil
46 }
47
48 // cleanupTracerProvider gracefully shutdown a TracerProvider
modules/caddyhttp/tracing/tracerprovider_test.go
+64 -4
@@ -1,16 +1,22 @@
1 package tracing
2
3 import (
4 + "errors"
5 "testing"
6
7 + sdktrace "go.opentelemetry.io/otel/sdk/trace"
8 "go.uber.org/zap"
9 )
10
11 +func noOpts() ([]sdktrace.TracerProviderOption, error) {
12 + return nil, nil
13 +}
14 +
15 func Test_tracersProvider_getTracerProvider(t *testing.T) {
16 tp := tracerProvider{}
17
12 - tp.getTracerProvider()
13 - tp.getTracerProvider()
18 + _, _ = tp.getTracerProvider(noOpts)
19 + _, _ = tp.getTracerProvider(noOpts)
20
21 if tp.tracerProvider == nil {
22 t.Errorf("There should be tracer provider")
@@ -24,8 +30,8 @@ func Test_tracersProvider_getTracerProvider(t *testing.T) {
30 func Test_tracersProvider_cleanupTracerProvider(t *testing.T) {
31 tp := tracerProvider{}
32
27 - tp.getTracerProvider()
28 - tp.getTracerProvider()
33 + _, _ = tp.getTracerProvider(noOpts)
34 + _, _ = tp.getTracerProvider(noOpts)
35
36 err := tp.cleanupTracerProvider(zap.NewNop())
37 if err != nil {
@@ -40,3 +46,57 @@ func Test_tracersProvider_cleanupTracerProvider(t *testing.T) {
46 t.Errorf("Tracer providers counter should equal to 1")
47 }
48 }
49 +
50 +// Test_tracersProvider_buildOptsOnlyOnCreate guards against a goroutine leak on
51 +// config reload: buildOpts must be invoked only when a new provider is actually
52 +// created, never on the reuse path. Some options (e.g. sdktrace.WithBatcher)
53 +// eagerly start a BatchSpanProcessor goroutine when constructed, so calling
54 +// buildOpts on every reload would leak one goroutine per reload.
55 +func Test_tracersProvider_buildOptsOnlyOnCreate(t *testing.T) {
56 + tp := tracerProvider{}
57 +
58 + builds := 0
59 + build := func() ([]sdktrace.TracerProviderOption, error) {
60 + builds++
61 + return nil, nil
62 + }
63 +
64 + // First call creates the provider and must build options.
65 + _, _ = tp.getTracerProvider(build)
66 + // Subsequent calls reuse the existing provider and must NOT build options again.
67 + for i := 0; i < 5; i++ {
68 + _, _ = tp.getTracerProvider(build)
69 + }
70 +
71 + if builds != 1 {
72 + t.Errorf("buildOpts should be invoked exactly once (on create), got %d", builds)
73 + }
74 + if tp.tracerProvidersCounter != 6 {
75 + t.Errorf("Tracer providers counter should equal to 6, got %d", tp.tracerProvidersCounter)
76 + }
77 +}
78 +
79 +// Test_tracersProvider_buildOptsError verifies that an error from buildOpts is
80 +// propagated and that no provider is created nor the counter incremented, so a
81 +// failed build leaves the tracerProvider in its initial state.
82 +func Test_tracersProvider_buildOptsError(t *testing.T) {
83 + tp := tracerProvider{}
84 +
85 + wantErr := errors.New("build failed")
86 + build := func() ([]sdktrace.TracerProviderOption, error) {
87 + return nil, wantErr
88 + }
89 +
90 + _, err := tp.getTracerProvider(build)
91 + if !errors.Is(err, wantErr) {
92 + t.Errorf("Expected error %v, got %v", wantErr, err)
93 + }
94 +
95 + if tp.tracerProvider != nil {
96 + t.Errorf("There should be no tracer provider when buildOpts fails")
97 + }
98 +
99 + if tp.tracerProvidersCounter != 0 {
100 + t.Errorf("Tracer providers counter should equal to 0, got %d", tp.tracerProvidersCounter)
101 + }
102 +}