From 5725dda6b72a04793939e0e6409ac9dc02aca7df Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 29 Sep 2023 16:11:47 -0700 Subject: [PATCH 01/10] Fix memory leaks in TracerProvider.GetTracer API. --- .../Stable/net462/PublicAPI.Unshipped.txt | 1 + .../Stable/net6.0/PublicAPI.Unshipped.txt | 1 + .../netstandard2.0/PublicAPI.Unshipped.txt | 1 + src/OpenTelemetry.Api/Trace/Tracer.cs | 8 ++- src/OpenTelemetry.Api/Trace/TracerProvider.cs | 65 ++++++++++++++++++- 5 files changed, 71 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry.Api/.publicApi/Stable/net462/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Api/.publicApi/Stable/net462/PublicAPI.Unshipped.txt index e69de29bb2d..b3db39f4c47 100644 --- a/src/OpenTelemetry.Api/.publicApi/Stable/net462/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry.Api/.publicApi/Stable/net462/PublicAPI.Unshipped.txt @@ -0,0 +1 @@ +override OpenTelemetry.Trace.TracerProvider.Dispose(bool disposing) -> void diff --git a/src/OpenTelemetry.Api/.publicApi/Stable/net6.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Api/.publicApi/Stable/net6.0/PublicAPI.Unshipped.txt index e69de29bb2d..b3db39f4c47 100644 --- a/src/OpenTelemetry.Api/.publicApi/Stable/net6.0/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry.Api/.publicApi/Stable/net6.0/PublicAPI.Unshipped.txt @@ -0,0 +1 @@ +override OpenTelemetry.Trace.TracerProvider.Dispose(bool disposing) -> void diff --git a/src/OpenTelemetry.Api/.publicApi/Stable/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Api/.publicApi/Stable/netstandard2.0/PublicAPI.Unshipped.txt index e69de29bb2d..b3db39f4c47 100644 --- a/src/OpenTelemetry.Api/.publicApi/Stable/netstandard2.0/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry.Api/.publicApi/Stable/netstandard2.0/PublicAPI.Unshipped.txt @@ -0,0 +1 @@ +override OpenTelemetry.Trace.TracerProvider.Dispose(bool disposing) -> void diff --git a/src/OpenTelemetry.Api/Trace/Tracer.cs b/src/OpenTelemetry.Api/Trace/Tracer.cs index af626aafc8b..880d9d7d232 100644 --- a/src/OpenTelemetry.Api/Trace/Tracer.cs +++ b/src/OpenTelemetry.Api/Trace/Tracer.cs @@ -28,7 +28,7 @@ namespace OpenTelemetry.Trace; /// Tracer is a wrapper around class. public class Tracer { - internal readonly ActivitySource ActivitySource; + internal ActivitySource? ActivitySource; internal Tracer(ActivitySource activitySource) { @@ -213,7 +213,9 @@ private TelemetrySpan StartSpanHelper( IEnumerable? links = null, DateTimeOffset startTime = default) { - if (!this.ActivitySource.HasListeners()) + var activitySource = this.ActivitySource; + + if (!(activitySource?.HasListeners() ?? false)) { return TelemetrySpan.NoopInstance; } @@ -230,7 +232,7 @@ private TelemetrySpan StartSpanHelper( try { - var activity = this.ActivitySource.StartActivity(name, activityKind, parentContext.ActivityContext, initialAttributes?.Attributes ?? null, activityLinks, startTime); + var activity = activitySource.StartActivity(name, activityKind, parentContext.ActivityContext, initialAttributes?.Attributes ?? null, activityLinks, startTime); return activity == null ? TelemetrySpan.NoopInstance : new TelemetrySpan(activity); diff --git a/src/OpenTelemetry.Api/Trace/TracerProvider.cs b/src/OpenTelemetry.Api/Trace/TracerProvider.cs index c7611bff428..2a2ae779bef 100644 --- a/src/OpenTelemetry.Api/Trace/TracerProvider.cs +++ b/src/OpenTelemetry.Api/Trace/TracerProvider.cs @@ -16,7 +16,7 @@ #nullable enable -using System.Diagnostics; +using System.Collections; namespace OpenTelemetry.Trace; @@ -25,6 +25,8 @@ namespace OpenTelemetry.Trace; /// public class TracerProvider : BaseProvider { + private Hashtable? tracers = new(); + /// /// Initializes a new instance of the class. /// @@ -44,5 +46,64 @@ protected TracerProvider() /// Version of the instrumentation library. /// Tracer instance. public Tracer GetTracer(string name, string? version = null) - => new(new ActivitySource(name ?? string.Empty, version)); + { + var tracers = this.tracers + ?? throw new ObjectDisposedException(nameof(TracerProvider)); + + var key = new TracerKey(name, version); + + if (tracers[key] is not Tracer tracer) + { + lock (tracers) + { + tracer = (tracers[key] as Tracer)!; + if (tracer == null) + { + tracer = new(new(key.Name, key.Version)); + tracers[key] = tracer; + } + } + } + + return tracer; + } + + /// + protected override void Dispose(bool disposing) + { + if (disposing) + { + var tracers = Interlocked.CompareExchange(ref this.tracers, null, this.tracers); + if (tracers != null) + { + lock (tracers) + { + foreach (DictionaryEntry entry in tracers) + { + var tracer = (Tracer)entry.Value!; + var activitySource = tracer.ActivitySource; + tracer.ActivitySource = null; + activitySource?.Dispose(); + } + + tracers.Clear(); + } + } + } + + base.Dispose(disposing); + } + + private sealed record class TracerKey + { + public TracerKey(string? name, string? version) + { + this.Name = name ?? string.Empty; + this.Version = version; + } + + public string Name { get; } + + public string? Version { get; } + } } From 0bd778cdf30b97df585448dffa04e08acc4f25fa Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 2 Oct 2023 09:33:50 -0700 Subject: [PATCH 02/10] Tweak. --- src/OpenTelemetry.Api/Trace/Tracer.cs | 2 +- src/OpenTelemetry.Api/Trace/TracerProvider.cs | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Api/Trace/Tracer.cs b/src/OpenTelemetry.Api/Trace/Tracer.cs index 880d9d7d232..91adfb8d38b 100644 --- a/src/OpenTelemetry.Api/Trace/Tracer.cs +++ b/src/OpenTelemetry.Api/Trace/Tracer.cs @@ -30,7 +30,7 @@ public class Tracer { internal ActivitySource? ActivitySource; - internal Tracer(ActivitySource activitySource) + internal Tracer(ActivitySource? activitySource) { this.ActivitySource = activitySource; } diff --git a/src/OpenTelemetry.Api/Trace/TracerProvider.cs b/src/OpenTelemetry.Api/Trace/TracerProvider.cs index 2a2ae779bef..023effc69a8 100644 --- a/src/OpenTelemetry.Api/Trace/TracerProvider.cs +++ b/src/OpenTelemetry.Api/Trace/TracerProvider.cs @@ -59,8 +59,15 @@ public Tracer GetTracer(string name, string? version = null) tracer = (tracers[key] as Tracer)!; if (tracer == null) { - tracer = new(new(key.Name, key.Version)); - tracers[key] = tracer; + if (this.tracers != null) + { + tracer = new(new(key.Name, key.Version)); + tracers[key] = tracer; + } + else + { + tracer = new(activitySource: null); + } } } } From 87a1bbcedc4c37a0a1695a6ecde1596b1881a716 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 3 Oct 2023 16:13:20 -0700 Subject: [PATCH 03/10] Added a test for tracer becoming no-op when parent provider is disposed. --- .../Trace/TracerTest.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs b/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs index 115417efba8..5388a23ecbf 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs +++ b/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs @@ -283,6 +283,25 @@ public void CreateSpan_NotSampled() Assert.False(span.IsRecording); } + [Fact] + public void Tracer_Disposed_Becomes_Noop() + { + Tracer tracer = null; + + using (var tracerProvider = Sdk.CreateTracerProviderBuilder() + .AddSource("mytracer") + .Build()) + { + tracer = tracerProvider.GetTracer("mytracer"); + + var span1 = tracer.StartSpan("foo"); + Assert.True(span1.IsRecording); + } + + var span2 = tracer.StartSpan("foo"); + Assert.False(span2.IsRecording); + } + public void Dispose() { Activity.Current = null; From 31b045f1756597e95814c2ac6661daf475d542d7 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 3 Oct 2023 16:16:16 -0700 Subject: [PATCH 04/10] CHANGELOG patch. --- src/OpenTelemetry.Api/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md index 619ad543626..36b39ea19bf 100644 --- a/src/OpenTelemetry.Api/CHANGELOG.md +++ b/src/OpenTelemetry.Api/CHANGELOG.md @@ -6,6 +6,10 @@ trace was running (`Activity.Current != null`). ([#4890](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4890)) +* Added a `Tracer` cache inside of `TracerProvider` to prevent repeated calls to + `GetTracer` from leaking memory. + ([#4906](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4906)) + ## 1.6.0 Released 2023-Sep-05 From 8afee1bccdfa495214f8265db88aa206b1713c2d Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 3 Oct 2023 16:18:51 -0700 Subject: [PATCH 05/10] Test tweak. --- test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs b/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs index 5388a23ecbf..54fa9e5c719 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs +++ b/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs @@ -286,12 +286,14 @@ public void CreateSpan_NotSampled() [Fact] public void Tracer_Disposed_Becomes_Noop() { + TracerProvider provider = null; Tracer tracer = null; using (var tracerProvider = Sdk.CreateTracerProviderBuilder() .AddSource("mytracer") .Build()) { + provider = tracerProvider; tracer = tracerProvider.GetTracer("mytracer"); var span1 = tracer.StartSpan("foo"); @@ -300,6 +302,9 @@ public void Tracer_Disposed_Becomes_Noop() var span2 = tracer.StartSpan("foo"); Assert.False(span2.IsRecording); + + Assert.Throws( + () => provider.GetTracer("mytracer")); } public void Dispose() From 6d84decdd306a642504a4f0b79e80094a7f21fe3 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 3 Oct 2023 16:27:31 -0700 Subject: [PATCH 06/10] Tweak design to avoid any potential breaking changes. --- src/OpenTelemetry.Api/Trace/TracerProvider.cs | 14 +++++++------- test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs | 6 ++++-- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/OpenTelemetry.Api/Trace/TracerProvider.cs b/src/OpenTelemetry.Api/Trace/TracerProvider.cs index 023effc69a8..3c0221a388e 100644 --- a/src/OpenTelemetry.Api/Trace/TracerProvider.cs +++ b/src/OpenTelemetry.Api/Trace/TracerProvider.cs @@ -47,8 +47,12 @@ protected TracerProvider() /// Tracer instance. public Tracer GetTracer(string name, string? version = null) { - var tracers = this.tracers - ?? throw new ObjectDisposedException(nameof(TracerProvider)); + var tracers = this.tracers; + if (tracers == null) + { + // Note: Returns a no-op Tracer once dispose has been called. + return new(activitySource: null); + } var key = new TracerKey(name, version); @@ -64,15 +68,11 @@ public Tracer GetTracer(string name, string? version = null) tracer = new(new(key.Name, key.Version)); tracers[key] = tracer; } - else - { - tracer = new(activitySource: null); - } } } } - return tracer; + return tracer ?? new(activitySource: null); } /// diff --git a/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs b/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs index 54fa9e5c719..14c0e1eef69 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs +++ b/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs @@ -303,8 +303,10 @@ public void Tracer_Disposed_Becomes_Noop() var span2 = tracer.StartSpan("foo"); Assert.False(span2.IsRecording); - Assert.Throws( - () => provider.GetTracer("mytracer")); + var tracer2 = provider.GetTracer("mytracer"); + + var span3 = tracer2.StartSpan("foo"); + Assert.False(span3.IsRecording); } public void Dispose() From 988af800118157401cbc0d7bb18b0fb72c404ec4 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 3 Oct 2023 21:56:00 -0700 Subject: [PATCH 07/10] Code review. --- src/OpenTelemetry.Api/Trace/TracerProvider.cs | 36 ++++++++++++++++--- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry.Api/Trace/TracerProvider.cs b/src/OpenTelemetry.Api/Trace/TracerProvider.cs index 3c0221a388e..ce182194774 100644 --- a/src/OpenTelemetry.Api/Trace/TracerProvider.cs +++ b/src/OpenTelemetry.Api/Trace/TracerProvider.cs @@ -17,6 +17,9 @@ #nullable enable using System.Collections; +#if NET6_0_OR_GREATER +using System.Diagnostics.CodeAnalysis; +#endif namespace OpenTelemetry.Trace; @@ -25,6 +28,8 @@ namespace OpenTelemetry.Trace; /// public class TracerProvider : BaseProvider { + [ThreadStatic] + private static TracerKey? threadTracerKey; private Hashtable? tracers = new(); /// @@ -54,7 +59,9 @@ public Tracer GetTracer(string name, string? version = null) return new(activitySource: null); } - var key = new TracerKey(name, version); + var key = threadTracerKey == null + ? (threadTracerKey = new TracerKey(name, version)) + : threadTracerKey.Update(name, version); if (tracers[key] is not Tracer tracer) { @@ -66,7 +73,10 @@ public Tracer GetTracer(string name, string? version = null) if (this.tracers != null) { tracer = new(new(key.Name, key.Version)); - tracers[key] = tracer; + + // Note: key is copied if we write into the hashtable + // because it must hold onto something immutable. + tracers[key.Copy()] = tracer; } } } @@ -104,13 +114,29 @@ protected override void Dispose(bool disposing) private sealed record class TracerKey { public TracerKey(string? name, string? version) + { + this.Update(name, version); + } + + public string Name { get; private set; } +#if !NET6_0_OR_GREATER + = string.Empty; +#endif + + public string? Version { get; private set; } + +#if NET6_0_OR_GREATER + [MemberNotNull(nameof(Name))] +#endif + public TracerKey Update(string? name, string? version) { this.Name = name ?? string.Empty; this.Version = version; - } - public string Name { get; } + return this; + } - public string? Version { get; } + public TracerKey Copy() + => new(this.Name, this.Version); } } From 8b4895d294c1581108f3dd44b8767e0f31959480 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 3 Oct 2023 22:01:28 -0700 Subject: [PATCH 08/10] Code review. --- test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs b/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs index e83e8ac8931..1e2e1f4579b 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs +++ b/test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs @@ -284,7 +284,7 @@ public void CreateSpan_NotSampled() } [Fact] - public void Tracer_Disposed_Becomes_Noop() + public void TracerBecomesNoopWhenParentProviderIsDisposedTest() { TracerProvider provider = null; Tracer tracer = null; From ca19d281cabf39f555bfb3b99e682a299203af94 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 4 Oct 2023 12:04:41 -0700 Subject: [PATCH 09/10] Switch to ConcurrentDictionary. --- src/OpenTelemetry.Api/Trace/TracerProvider.cs | 72 ++++++++----------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/src/OpenTelemetry.Api/Trace/TracerProvider.cs b/src/OpenTelemetry.Api/Trace/TracerProvider.cs index ce182194774..b0ddcc5b3e6 100644 --- a/src/OpenTelemetry.Api/Trace/TracerProvider.cs +++ b/src/OpenTelemetry.Api/Trace/TracerProvider.cs @@ -16,7 +16,7 @@ #nullable enable -using System.Collections; +using System.Collections.Concurrent; #if NET6_0_OR_GREATER using System.Diagnostics.CodeAnalysis; #endif @@ -28,9 +28,7 @@ namespace OpenTelemetry.Trace; /// public class TracerProvider : BaseProvider { - [ThreadStatic] - private static TracerKey? threadTracerKey; - private Hashtable? tracers = new(); + private ConcurrentDictionary? tracers = new(); /// /// Initializes a new instance of the class. @@ -50,7 +48,12 @@ protected TracerProvider() /// Name identifying the instrumentation library. /// Version of the instrumentation library. /// Tracer instance. - public Tracer GetTracer(string name, string? version = null) + public Tracer GetTracer( +#if NET6_0_OR_GREATER + [AllowNull] +#endif + string name, + string? version = null) { var tracers = this.tracers; if (tracers == null) @@ -59,30 +62,32 @@ public Tracer GetTracer(string name, string? version = null) return new(activitySource: null); } - var key = threadTracerKey == null - ? (threadTracerKey = new TracerKey(name, version)) - : threadTracerKey.Update(name, version); + var key = new TracerKey(name, version); - if (tracers[key] is not Tracer tracer) +Retry: + if (!tracers.TryGetValue(key, out var tracer)) { lock (tracers) { - tracer = (tracers[key] as Tracer)!; - if (tracer == null) + if (this.tracers == null) { - if (this.tracers != null) - { - tracer = new(new(key.Name, key.Version)); + // Note: We check here for a race with Dispose and return a + // no-op Tracer in that case. + return new(activitySource: null); + } - // Note: key is copied if we write into the hashtable - // because it must hold onto something immutable. - tracers[key.Copy()] = tracer; - } + tracer = new(new(key.Name, key.Version)); + if (!tracers.TryAdd(key, tracer)) + { + // Note: This should really never happen due to the + // outer lock. + tracer.ActivitySource!.Dispose(); + goto Retry; } } } - return tracer ?? new(activitySource: null); + return tracer; } /// @@ -95,9 +100,9 @@ protected override void Dispose(bool disposing) { lock (tracers) { - foreach (DictionaryEntry entry in tracers) + foreach (var kvp in tracers) { - var tracer = (Tracer)entry.Value!; + var tracer = kvp.Value; var activitySource = tracer.ActivitySource; tracer.ActivitySource = null; activitySource?.Dispose(); @@ -111,32 +116,15 @@ protected override void Dispose(bool disposing) base.Dispose(disposing); } - private sealed record class TracerKey + private readonly record struct TracerKey { - public TracerKey(string? name, string? version) - { - this.Update(name, version); - } + public readonly string Name; + public readonly string? Version; - public string Name { get; private set; } -#if !NET6_0_OR_GREATER - = string.Empty; -#endif - - public string? Version { get; private set; } - -#if NET6_0_OR_GREATER - [MemberNotNull(nameof(Name))] -#endif - public TracerKey Update(string? name, string? version) + public TracerKey(string? name, string? version) { this.Name = name ?? string.Empty; this.Version = version; - - return this; } - - public TracerKey Copy() - => new(this.Name, this.Version); } } From 7c1ec508efc5d810689efda7f8e2dd2205a9afab Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 4 Oct 2023 15:41:30 -0700 Subject: [PATCH 10/10] Code review. --- src/OpenTelemetry.Api/Trace/TracerProvider.cs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/OpenTelemetry.Api/Trace/TracerProvider.cs b/src/OpenTelemetry.Api/Trace/TracerProvider.cs index b0ddcc5b3e6..50d033cae51 100644 --- a/src/OpenTelemetry.Api/Trace/TracerProvider.cs +++ b/src/OpenTelemetry.Api/Trace/TracerProvider.cs @@ -64,7 +64,6 @@ public Tracer GetTracer( var key = new TracerKey(name, version); -Retry: if (!tracers.TryGetValue(key, out var tracer)) { lock (tracers) @@ -77,13 +76,12 @@ public Tracer GetTracer( } tracer = new(new(key.Name, key.Version)); - if (!tracers.TryAdd(key, tracer)) - { - // Note: This should really never happen due to the - // outer lock. - tracer.ActivitySource!.Dispose(); - goto Retry; - } +#if DEBUG + bool result = tracers.TryAdd(key, tracer); + System.Diagnostics.Debug.Assert(result, "Write into tracers cache failed"); +#else + tracers.TryAdd(key, tracer); +#endif } }