From 1a773796b9249a7a663a315d3e965edb85b88758 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Tue, 14 Apr 2026 19:44:03 +1200 Subject: [PATCH 1/4] fix: memory leak when profiling is enabled fixes: #5113 fixes: #4721 --- src/Sentry/Internal/ConcurrentBagLite.cs | 73 ++++++++++ src/Sentry/SdkVersion.cs | 9 +- .../Internals/ConcurrentBagLiteTests.cs | 137 ++++++++++++++++++ test/Sentry.Tests/Protocol/SdkVersionTests.cs | 18 +-- 4 files changed, 224 insertions(+), 13 deletions(-) create mode 100644 src/Sentry/Internal/ConcurrentBagLite.cs create mode 100644 test/Sentry.Tests/Internals/ConcurrentBagLiteTests.cs diff --git a/src/Sentry/Internal/ConcurrentBagLite.cs b/src/Sentry/Internal/ConcurrentBagLite.cs new file mode 100644 index 0000000000..1bf37b2b85 --- /dev/null +++ b/src/Sentry/Internal/ConcurrentBagLite.cs @@ -0,0 +1,73 @@ +namespace Sentry.Internal; + +/// +/// A minimal replacement for . +/// +/// We're using this to avoid the same class of memory leak that +/// was introduced to avoid. See https://github.com/getsentry/sentry-dotnet/issues/5113 +/// +internal class ConcurrentBagLite : IEnumerable +{ + private readonly List _items; + + public ConcurrentBagLite() + { + _items = new List(); + } + + public ConcurrentBagLite(IEnumerable collection) + { + _items = new List(collection); + } + + public void Add(T item) + { + lock (_items) + { + _items.Add(item); + } + } + + public int Count + { + get + { + lock (_items) + { + return _items.Count; + } + } + } + + public bool IsEmpty => Count == 0; + + public void Clear() + { + lock (_items) + { + _items.Clear(); + } + } + + public T[] ToArray() + { + lock (_items) + { + return _items.ToArray(); + } + } + + public IEnumerator GetEnumerator() + { + // Return a snapshot to avoid holding the lock during iteration + // and to prevent InvalidOperationException if the collection is modified. + T[] snapshot; + lock (_items) + { + snapshot = _items.ToArray(); + } + return ((IEnumerable)snapshot).GetEnumerator(); + } + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); +} diff --git a/src/Sentry/SdkVersion.cs b/src/Sentry/SdkVersion.cs index 0ecad1bba7..714fd4c5b7 100644 --- a/src/Sentry/SdkVersion.cs +++ b/src/Sentry/SdkVersion.cs @@ -1,4 +1,5 @@ using Sentry.Extensibility; +using Sentry.Internal; using Sentry.Internal.Extensions; using Sentry.Reflection; @@ -19,8 +20,8 @@ public sealed class SdkVersion : ISentryJsonSerializable internal static SdkVersion Instance => InstanceLazy.Value; - internal ConcurrentBag InternalPackages { get; set; } = new(); - internal ConcurrentBag Integrations { get; set; } = new(); + internal ConcurrentBagLite InternalPackages { get; set; } = new(); + internal ConcurrentBagLite Integrations { get; set; } = new(); /// /// SDK packages. @@ -104,8 +105,8 @@ public static SdkVersion FromJson(JsonElement json) return new SdkVersion { - InternalPackages = new ConcurrentBag(packages), - Integrations = new ConcurrentBag(integrations), + InternalPackages = new ConcurrentBagLite(packages), + Integrations = new ConcurrentBagLite(integrations), Name = name, Version = version }; diff --git a/test/Sentry.Tests/Internals/ConcurrentBagLiteTests.cs b/test/Sentry.Tests/Internals/ConcurrentBagLiteTests.cs new file mode 100644 index 0000000000..3fc8179532 --- /dev/null +++ b/test/Sentry.Tests/Internals/ConcurrentBagLiteTests.cs @@ -0,0 +1,137 @@ +namespace Sentry.Tests.Internals; + +public class ConcurrentBagLiteTests +{ + [Fact] + public void Add_Test() + { + // Arrange + var bag = new ConcurrentBagLite(); + + // Act + bag.Add(1); + + // Assert + bag.Count.Should().Be(1); + + // Act + bag.Add(2); + bag.Add(3); + + // Assert + bag.Count.Should().Be(3); + var items = bag.ToArray(); + items.Should().BeEquivalentTo([1, 2, 3]); + } + + [Fact] + public void Ctor_FromCollection_Test() + { + // Arrange & Act + var bag = new ConcurrentBagLite(new[] { 1, 2, 3 }); + + // Assert + bag.Count.Should().Be(3); + bag.ToArray().Should().BeEquivalentTo([1, 2, 3]); + } + + [Fact] + public void Count_EmptyBag_Test() + { + // Arrange + var bag = new ConcurrentBagLite(); + + // Act & Assert + bag.Count.Should().Be(0); + } + + [Fact] + public void IsEmpty_Test() + { + // Arrange + var bag = new ConcurrentBagLite(); + + // Act & Assert + bag.IsEmpty.Should().BeTrue(); + bag.Add(1); + bag.IsEmpty.Should().BeFalse(); + bag.Clear(); + bag.IsEmpty.Should().BeTrue(); + } + + [Fact] + public void Clear_Test() + { + // Arrange + var bag = new ConcurrentBagLite(); + bag.Add(1); + bag.Add(2); + bag.Add(3); + + // Act + bag.Clear(); + + // Assert + bag.Count.Should().Be(0); + bag.IsEmpty.Should().BeTrue(); + } + + [Fact] + public void GetEnumerator_Test() + { + // Arrange + var bag = new ConcurrentBagLite(); + bag.Add(1); + bag.Add(2); + bag.Add(3); + + // Act + var items = bag.ToList(); + + // Assert + items.Should().BeEquivalentTo([1, 2, 3]); + } + + [Fact] + public void GetEnumerator_Snapshot_DoesNotThrowOnConcurrentModification() + { + // Arrange + var bag = new ConcurrentBagLite(); + bag.Add(1); + bag.Add(2); + + // Act + using var enumerator = bag.GetEnumerator(); + bag.Add(3); // modify after enumerator created — should not throw when iterating snapshot + + var items = new List(); + while (enumerator.MoveNext()) + { + items.Add(enumerator.Current); + } + + // Assert + items.Should().BeEquivalentTo([1, 2]); + } + + [Fact] + public async Task TestConcurrency() + { + // Arrange + var bag = new ConcurrentBagLite(); + var count = 100; + var tasks = new Task[count]; + + // Act + for (var i = 0; i < count; i++) + { + var toAdd = i; + tasks[i] = Task.Run(() => bag.Add(toAdd)); + } + await Task.WhenAll(tasks); + + // Assert + bag.Count.Should().Be(count); + bag.ToArray().Should().BeEquivalentTo(Enumerable.Range(0, count)); + } +} diff --git a/test/Sentry.Tests/Protocol/SdkVersionTests.cs b/test/Sentry.Tests/Protocol/SdkVersionTests.cs index bf29a74dc8..45e8a342f3 100644 --- a/test/Sentry.Tests/Protocol/SdkVersionTests.cs +++ b/test/Sentry.Tests/Protocol/SdkVersionTests.cs @@ -45,13 +45,13 @@ public void SerializeObject_AllPropertiesSetToNonDefault_SerializesValidObject() Assert.Equal(""" { "packages": [ - { - "name": "Sentry", - "version": "1.0" - }, { "name": "Sentry.AspNetCore", "version": "2.0" + }, + { + "name": "Sentry", + "version": "1.0" } ], "name": "Sentry.Test.SDK", @@ -78,7 +78,7 @@ public static IEnumerable TestCases() var sdk = new SdkVersion(); sdk.AddPackage("b", "2"); sdk.AddPackage("a", "1"); - yield return new object[] { (sdk, """{"packages":[{"name":"a","version":"1"},{"name":"b","version":"2"}]}""") }; + yield return new object[] { (sdk, """{"packages":[{"name":"b","version":"2"},{"name":"a","version":"1"}]}""") }; } [Fact] @@ -97,13 +97,13 @@ public void SerializeObject_IgnoresDuplicatePackages() var expected = TrimJson(@" { ""packages"": [ - { - ""name"": ""Bar"", - ""version"": ""Beta"" - }, { ""name"": ""Foo"", ""version"": ""Alpha"" + }, + { + ""name"": ""Bar"", + ""version"": ""Beta"" } ], ""name"": ""Sentry.Test.SDK"", From ba357291d5d76c8cb335ac1e20f61eb84887d690 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Tue, 14 Apr 2026 22:10:40 +1200 Subject: [PATCH 2/4] Replaced remaining uses of ConcurrentBag --- src/Sentry/Internal/ConcurrentBagLite.cs | 2 +- src/Sentry/Internal/UnsampledTransaction.cs | 10 +--------- src/Sentry/Scope.cs | 22 +++++++-------------- src/Sentry/TransactionTracer.cs | 12 ++--------- 4 files changed, 11 insertions(+), 35 deletions(-) diff --git a/src/Sentry/Internal/ConcurrentBagLite.cs b/src/Sentry/Internal/ConcurrentBagLite.cs index 1bf37b2b85..353b290724 100644 --- a/src/Sentry/Internal/ConcurrentBagLite.cs +++ b/src/Sentry/Internal/ConcurrentBagLite.cs @@ -6,7 +6,7 @@ namespace Sentry.Internal; /// We're using this to avoid the same class of memory leak that /// was introduced to avoid. See https://github.com/getsentry/sentry-dotnet/issues/5113 /// -internal class ConcurrentBagLite : IEnumerable +internal class ConcurrentBagLite : IReadOnlyCollection { private readonly List _items; diff --git a/src/Sentry/Internal/UnsampledTransaction.cs b/src/Sentry/Internal/UnsampledTransaction.cs index bc9674262a..be78b2c13a 100644 --- a/src/Sentry/Internal/UnsampledTransaction.cs +++ b/src/Sentry/Internal/UnsampledTransaction.cs @@ -12,11 +12,7 @@ internal sealed class UnsampledTransaction : NoOpTransaction // Although it's a little bit wasteful to create separate individual class instances here when all we're going to // report to sentry is the span count (in the client report), SDK users may refer to things like // `ITransaction.Spans.Count`, so we create an actual collection -#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER - private readonly ConcurrentBag _spans = []; -#else - private ConcurrentBag _spans = []; -#endif + private readonly ConcurrentBagLite _spans = new(); private readonly IHub _hub; private readonly ITransactionContext _context; @@ -117,10 +113,6 @@ public ISpan StartChild(string operation, SpanId spanId) private void ReleaseSpans() { -#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER _spans.Clear(); -#else - _spans = []; -#endif } } diff --git a/src/Sentry/Scope.cs b/src/Sentry/Scope.cs index 8a11e1f975..4b60dd59a1 100644 --- a/src/Sentry/Scope.cs +++ b/src/Sentry/Scope.cs @@ -51,29 +51,29 @@ internal SentryId LastEventId /// internal bool HasEvaluated => _hasEvaluated; - private readonly Lazy> _lazyExceptionProcessors = + private readonly Lazy> _lazyExceptionProcessors = new(LazyThreadSafetyMode.PublicationOnly); /// /// A list of exception processors. /// - internal ConcurrentBag ExceptionProcessors => _lazyExceptionProcessors.Value; + internal ConcurrentBagLite ExceptionProcessors => _lazyExceptionProcessors.Value; - private readonly Lazy> _lazyEventProcessors = + private readonly Lazy> _lazyEventProcessors = new(LazyThreadSafetyMode.PublicationOnly); - private readonly Lazy> _lazyTransactionProcessors = + private readonly Lazy> _lazyTransactionProcessors = new(LazyThreadSafetyMode.PublicationOnly); /// /// A list of event processors. /// - internal ConcurrentBag EventProcessors => _lazyEventProcessors.Value; + internal ConcurrentBagLite EventProcessors => _lazyEventProcessors.Value; /// /// A list of event processors. /// - internal ConcurrentBag TransactionProcessors => _lazyTransactionProcessors.Value; + internal ConcurrentBagLite TransactionProcessors => _lazyTransactionProcessors.Value; /// /// An event that fires when the scope evaluates. @@ -278,11 +278,7 @@ public ITransactionTracer? Transaction /// public IReadOnlyDictionary Tags => _tags; -#if NETSTANDARD2_0 || NETFRAMEWORK - private ConcurrentBag _attachments = new(); -#else - private readonly ConcurrentBag _attachments = new(); -#endif + private readonly ConcurrentBagLite _attachments = new(); /// /// Attachments. @@ -435,11 +431,7 @@ public void Clear() /// public void ClearAttachments() { -#if NETSTANDARD2_0 || NETFRAMEWORK - Interlocked.Exchange(ref _attachments, new()); -#else _attachments.Clear(); -#endif if (Options.EnableScopeSync) { Options.ScopeObserver?.ClearAttachments(); diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 54464668d8..e4ce907607 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -148,7 +148,7 @@ public IReadOnlyList Fingerprint set => _fingerprint = value; } - private readonly ConcurrentBag _breadcrumbs = new(); + private readonly ConcurrentBagLite _breadcrumbs = new(); /// public IReadOnlyCollection Breadcrumbs => _breadcrumbs; @@ -165,11 +165,7 @@ public IReadOnlyList Fingerprint /// public IReadOnlyDictionary Tags => _tags; -#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER - private readonly ConcurrentBag _spans = new(); -#else - private ConcurrentBag _spans = new(); -#endif + private readonly ConcurrentBagLite _spans = new(); /// public IReadOnlyCollection Spans => _spans; @@ -428,11 +424,7 @@ public string? Origin private void ReleaseSpans() { -#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER _spans.Clear(); -#else - _spans = new ConcurrentBag(); -#endif _activeSpanTracker.Clear(); } From a16638f9b2c1774686d0a15389a0941b53665926 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Tue, 14 Apr 2026 22:35:36 +1200 Subject: [PATCH 3/4] Replaced last remaining usage of ConcurrentQueue --- src/Sentry/Internal/ConcurrentQueueLite.cs | 16 +++++++++++++++- src/Sentry/Scope.cs | 11 +---------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/Sentry/Internal/ConcurrentQueueLite.cs b/src/Sentry/Internal/ConcurrentQueueLite.cs index 64c8a93bb3..a8653c52a4 100644 --- a/src/Sentry/Internal/ConcurrentQueueLite.cs +++ b/src/Sentry/Internal/ConcurrentQueueLite.cs @@ -6,7 +6,7 @@ namespace Sentry.Internal; /// We're using this due to a memory leak that happens when using ConcurrentQueue in the BackgroundWorker. /// See https://github.com/getsentry/sentry-dotnet/issues/2516 /// -internal class ConcurrentQueueLite +internal class ConcurrentQueueLite : IReadOnlyCollection { private readonly List _queue = new(); @@ -74,4 +74,18 @@ public T[] ToArray() return _queue.ToArray(); } } + + public IEnumerator GetEnumerator() + { + // Return a snapshot to avoid holding the lock during iteration + // and to prevent InvalidOperationException if the collection is modified. + T[] snapshot; + lock (_queue) + { + snapshot = _queue.ToArray(); + } + return ((IEnumerable)snapshot).GetEnumerator(); + } + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); } diff --git a/src/Sentry/Scope.cs b/src/Sentry/Scope.cs index 4b60dd59a1..4a7d077174 100644 --- a/src/Sentry/Scope.cs +++ b/src/Sentry/Scope.cs @@ -259,11 +259,7 @@ public ITransactionTracer? Transaction /// public IReadOnlyList Fingerprint { get; set; } = Array.Empty(); -#if NETSTANDARD2_0 || NETFRAMEWORK - private ConcurrentQueue _breadcrumbs = new(); -#else - private readonly ConcurrentQueue _breadcrumbs = new(); -#endif + private readonly ConcurrentQueueLite _breadcrumbs = new(); /// public IReadOnlyCollection Breadcrumbs => _breadcrumbs; @@ -443,12 +439,7 @@ public void ClearAttachments() /// public void ClearBreadcrumbs() { -#if NETSTANDARD2_0 || NETFRAMEWORK - // No Clear method on ConcurrentQueue for these target frameworks - Interlocked.Exchange(ref _breadcrumbs, new()); -#else _breadcrumbs.Clear(); -#endif } /// From 1bb843003beb00382cccdb1a1895c087d6d13054 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Wed, 15 Apr 2026 14:22:45 +1200 Subject: [PATCH 4/4] Fixed SqlListener test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was passing accidentally before because ConcurrentBag typically followed a LIFO model for adding/removing items. The SQL listener test had a bug that the LIFO mask would hide. The test did `OrderByDescending(x => x.StartTimestamp).First()` to get the "newest" span, but this fails whenever two spans share the same timestamp — which is common on Windows due to ~15ms clock resolution. Making ConcurrentBagLite LIFO would make the test pass again accidentally, because the newer span would happen to enumerate first and survive the stable sort. But the brittleness is still there — if the spans ever get equal timestamps in a different order (e.g., a background thread adds one), the test breaks again. --- test/Sentry.DiagnosticSource.Tests/SentrySqlListenerTests.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/Sentry.DiagnosticSource.Tests/SentrySqlListenerTests.cs b/test/Sentry.DiagnosticSource.Tests/SentrySqlListenerTests.cs index 1d3eb43770..17049e6f5b 100644 --- a/test/Sentry.DiagnosticSource.Tests/SentrySqlListenerTests.cs +++ b/test/Sentry.DiagnosticSource.Tests/SentrySqlListenerTests.cs @@ -120,11 +120,10 @@ public void OnNext_KnownKey_GetSpanInvoked(string key, bool addConnectionSpan) })); // Assert - var spans = fixture.Spans.Where(s => s.Operation != "abc"); + var spans = fixture.Spans.Where(s => s.Operation != "abc").ToList(); Assert.NotEmpty(spans); - var firstSpan = fixture.Spans.OrderByDescending(x => x.StartTimestamp).First(); - Assert.True(GetValidator(key)(firstSpan)); + Assert.True(GetValidator(key)(spans[0])); } [Theory]