fix: memory leak when profiling is enabled (#5133)

* fix: memory leak when profiling is enabled

fixes: #5113
fixes: #4721

* Replaced remaining uses of ConcurrentBag<T>

* Replaced last remaining usage of ConcurrentQueue<T>

* Fixed SqlListener test

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.

Author: jamescrosswell

src/Sentry/Internal/ConcurrentBagLite.cs

@@ -0,0 +1,73 @@
+namespace Sentry.Internal;
+
+/// <summary>
+/// A minimal replacement for <see cref="ConcurrentBag{T}"/>.
+///
+/// We're using this to avoid the same class of memory leak that <see cref="ConcurrentQueueLite{T}"/>
+/// was introduced to avoid. See https://github.com/getsentry/sentry-dotnet/issues/5113
+/// </summary>
+internal class ConcurrentBagLite<T> : IReadOnlyCollection<T>
+{
+    private readonly List<T> _items;
+
+    public ConcurrentBagLite()
+    {
+        _items = new List<T>();
+    }
+
+    public ConcurrentBagLite(IEnumerable<T> collection)
+    {
+        _items = new List<T>(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<T> 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<T>)snapshot).GetEnumerator();
+    }
+
+    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+}

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
 /// </summary>
-internal class ConcurrentQueueLite<T>
+internal class ConcurrentQueueLite<T> : IReadOnlyCollection<T>
 {
     private readonly List<T> _queue = new();
 
@@ -74,4 +74,18 @@ public T[] ToArray()
             return _queue.ToArray();
         }
     }
+
+    public IEnumerator<T> 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<T>)snapshot).GetEnumerator();
+    }
+
+    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
 }

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<ISpan> _spans = [];
-#else
-    private ConcurrentBag<ISpan> _spans = [];
-#endif
+    private readonly ConcurrentBagLite<ISpan> _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
     }
 }

src/Sentry/Scope.cs

@@ -51,29 +51,29 @@ internal SentryId LastEventId
     /// </summary>
     internal bool HasEvaluated => _hasEvaluated;
 
-    private readonly Lazy<ConcurrentBag<ISentryEventExceptionProcessor>> _lazyExceptionProcessors =
+    private readonly Lazy<ConcurrentBagLite<ISentryEventExceptionProcessor>> _lazyExceptionProcessors =
         new(LazyThreadSafetyMode.PublicationOnly);
 
     /// <summary>
     /// A list of exception processors.
     /// </summary>
-    internal ConcurrentBag<ISentryEventExceptionProcessor> ExceptionProcessors => _lazyExceptionProcessors.Value;
+    internal ConcurrentBagLite<ISentryEventExceptionProcessor> ExceptionProcessors => _lazyExceptionProcessors.Value;
 
-    private readonly Lazy<ConcurrentBag<ISentryEventProcessor>> _lazyEventProcessors =
+    private readonly Lazy<ConcurrentBagLite<ISentryEventProcessor>> _lazyEventProcessors =
         new(LazyThreadSafetyMode.PublicationOnly);
 
-    private readonly Lazy<ConcurrentBag<ISentryTransactionProcessor>> _lazyTransactionProcessors =
+    private readonly Lazy<ConcurrentBagLite<ISentryTransactionProcessor>> _lazyTransactionProcessors =
         new(LazyThreadSafetyMode.PublicationOnly);
 
     /// <summary>
     /// A list of event processors.
     /// </summary>
-    internal ConcurrentBag<ISentryEventProcessor> EventProcessors => _lazyEventProcessors.Value;
+    internal ConcurrentBagLite<ISentryEventProcessor> EventProcessors => _lazyEventProcessors.Value;
 
     /// <summary>
     /// A list of event processors.
     /// </summary>
-    internal ConcurrentBag<ISentryTransactionProcessor> TransactionProcessors => _lazyTransactionProcessors.Value;
+    internal ConcurrentBagLite<ISentryTransactionProcessor> TransactionProcessors => _lazyTransactionProcessors.Value;
 
     /// <summary>
     /// An event that fires when the scope evaluates.
@@ -259,11 +259,7 @@ public ITransactionTracer? Transaction
     /// <inheritdoc />
     public IReadOnlyList<string> Fingerprint { get; set; } = Array.Empty<string>();
 
-#if NETSTANDARD2_0 || NETFRAMEWORK
-    private ConcurrentQueue<Breadcrumb> _breadcrumbs = new();
-#else
-    private readonly ConcurrentQueue<Breadcrumb> _breadcrumbs = new();
-#endif
+    private readonly ConcurrentQueueLite<Breadcrumb> _breadcrumbs = new();
 
     /// <inheritdoc />
     public IReadOnlyCollection<Breadcrumb> Breadcrumbs => _breadcrumbs;
@@ -278,11 +274,7 @@ public ITransactionTracer? Transaction
     /// <inheritdoc />
     public IReadOnlyDictionary<string, string> Tags => _tags;
 
-#if NETSTANDARD2_0 || NETFRAMEWORK
-    private ConcurrentBag<SentryAttachment> _attachments = new();
-#else
-    private readonly ConcurrentBag<SentryAttachment> _attachments = new();
-#endif
+    private readonly ConcurrentBagLite<SentryAttachment> _attachments = new();
 
     /// <summary>
     /// Attachments.
@@ -435,11 +427,7 @@ public void Clear()
     /// </summary>
     public void ClearAttachments()
     {
-#if NETSTANDARD2_0 || NETFRAMEWORK
-        Interlocked.Exchange(ref _attachments, new());
-#else
         _attachments.Clear();
-#endif
         if (Options.EnableScopeSync)
         {
             Options.ScopeObserver?.ClearAttachments();
@@ -451,12 +439,7 @@ public void ClearAttachments()
     /// </summary>
     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
     }
 
     /// <summary>

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<SentryPackage> InternalPackages { get; set; } = new();
-    internal ConcurrentBag<string> Integrations { get; set; } = new();
+    internal ConcurrentBagLite<SentryPackage> InternalPackages { get; set; } = new();
+    internal ConcurrentBagLite<string> Integrations { get; set; } = new();
 
     /// <summary>
     /// SDK packages.
@@ -104,8 +105,8 @@ public static SdkVersion FromJson(JsonElement json)
 
         return new SdkVersion
         {
-            InternalPackages = new ConcurrentBag<SentryPackage>(packages),
-            Integrations = new ConcurrentBag<string>(integrations),
+            InternalPackages = new ConcurrentBagLite<SentryPackage>(packages),
+            Integrations = new ConcurrentBagLite<string>(integrations),
             Name = name,
             Version = version
         };

src/Sentry/TransactionTracer.cs

@@ -148,7 +148,7 @@ public IReadOnlyList<string> Fingerprint
         set => _fingerprint = value;
     }
 
-    private readonly ConcurrentBag<Breadcrumb> _breadcrumbs = new();
+    private readonly ConcurrentBagLite<Breadcrumb> _breadcrumbs = new();
 
     /// <inheritdoc />
     public IReadOnlyCollection<Breadcrumb> Breadcrumbs => _breadcrumbs;
@@ -165,11 +165,7 @@ public IReadOnlyList<string> Fingerprint
     /// <inheritdoc />
     public IReadOnlyDictionary<string, string> Tags => _tags;
 
-#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER
-    private readonly ConcurrentBag<ISpan> _spans = new();
-#else
-    private ConcurrentBag<ISpan> _spans = new();
-#endif
+    private readonly ConcurrentBagLite<ISpan> _spans = new();
 
     /// <inheritdoc />
     public IReadOnlyCollection<ISpan> 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<ISpan>();
-#endif
         _activeSpanTracker.Clear();
     }
 

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]