fix: memory leak when finishing unsampled Transactions/Spans (#4717)

Author: Flash0ver

CHANGELOG.md

@@ -20,6 +20,7 @@
 
 - The `Serilog` integration captures _Structured Logs_ (when enabled) independently of captured Events and added Breadcrumbs ([#4691](https://github.com/getsentry/sentry-dotnet/pull/4691))
 - Deliver system breadcrumbs in the main thread on Android ([#4671](https://github.com/getsentry/sentry-dotnet/pull/4671))
+- Memory leak when finishing an unsampled Transaction that has started unsampled Spans ([#4717](https://github.com/getsentry/sentry-dotnet/pull/4717))
 
 ## 6.0.0-preview.2
 

src/Sentry/Internal/NoOpSpan.cs

@@ -88,5 +88,6 @@ public void SetMeasurement(string name, Measurement measurement)
 
     public void Dispose()
     {
+        Finish();
     }
 }

src/Sentry/Internal/UnsampledTransaction.cs

@@ -4,15 +4,20 @@ namespace Sentry.Internal;
 
 /// <summary>
 /// We know already, when starting a transaction, whether it's going to be sampled or not. When it's not sampled, we can
-/// avoid lots of unecessary processing. The only thing we need to track is the number of spans that would have been
+/// avoid lots of unnecessary processing. The only thing we need to track is the number of spans that would have been
 /// created (the client reports detailing discarded events includes this detail).
 /// </summary>
 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 IHub _hub;
     private readonly ITransactionContext _context;
     private readonly SentryOptions? _options;
@@ -79,6 +84,9 @@ public override void Finish()
         _options?.ClientReportRecorder.RecordDiscardedEvent(discardReason, DataCategory.Span, spanCount);
 
         _options?.LogDebug("Finished unsampled transaction");
+
+        // Release tracked spans
+        ReleaseSpans();
     }
 
     public override void Finish(SpanStatus status) => Finish();
@@ -103,4 +111,13 @@ public ISpan StartChild(string operation, SpanId spanId)
         _spans.Add(span);
         return span;
     }
+
+    private void ReleaseSpans()
+    {
+#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER
+        _spans.Clear();
+#else
+        _spans = [];
+#endif
+    }
 }

src/Sentry/TransactionTracer.cs

@@ -168,7 +168,7 @@ public IReadOnlyList<string> Fingerprint
     /// <inheritdoc />
     public IReadOnlyDictionary<string, string> Tags => _tags;
 
-#if NETSTANDARD2_1_OR_GREATER
+#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER
     private readonly ConcurrentBag<ISpan> _spans = new();
 #else
     private ConcurrentBag<ISpan> _spans = new();
@@ -431,7 +431,7 @@ public string? Origin
 
     private void ReleaseSpans()
     {
-#if NETSTANDARD2_1_OR_GREATER
+#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER
         _spans.Clear();
 #else
         _spans = new ConcurrentBag<ISpan>();

test/Sentry.Tests/Internals/UnsampledSpanTests.cs

@@ -2,6 +2,24 @@ namespace Sentry.Tests.Internals;
 
 public class UnsampledSpanTests
 {
+    [Fact]
+    public void StartChild_IsUnsampledSpan_HasReferenceToUnsampledTransaction()
+    {
+        // Arrange
+        var hub = Substitute.For<IHub>();
+        ITransactionContext context = new TransactionContext("TestTransaction", "TestOperation",
+            new SentryTraceHeader(SentryId.Create(), SpanId.Create(), false)
+        );
+        using var transaction = new UnsampledTransaction(hub, context);
+
+        // Act
+        using var unsampledSpan = transaction.StartChild("Foo");
+
+        // Assert
+        Assert.IsType<UnsampledSpan>(unsampledSpan);
+        Assert.Same(transaction, unsampledSpan.GetTransaction());
+    }
+
     [Fact]
     public void GetTraceHeader_CreatesHeaderFromUnsampledTransaction()
     {
@@ -10,8 +28,8 @@ public void GetTraceHeader_CreatesHeaderFromUnsampledTransaction()
         ITransactionContext context = new TransactionContext("TestTransaction", "TestOperation",
             new SentryTraceHeader(SentryId.Create(), SpanId.Create(), false)
         );
-        var transaction = new UnsampledTransaction(hub, context);
-        var unsampledSpan = transaction.StartChild("Foo");
+        using var transaction = new UnsampledTransaction(hub, context);
+        using var unsampledSpan = transaction.StartChild("Foo");
 
         // Act
         var traceHeader = unsampledSpan.GetTraceHeader();

test/Sentry.Tests/Internals/UnsampledTransactionTests.cs

@@ -0,0 +1,58 @@
+namespace Sentry.Tests.Internals;
+
+public class UnsampledTransactionTests
+{
+    [Fact]
+    public void StartChild_CreatesSpan_IsTrackedByParent()
+    {
+        // Arrange
+        var hub = Substitute.For<IHub>();
+        ITransactionContext context = new TransactionContext("TestTransaction", "TestOperation",
+            new SentryTraceHeader(SentryId.Create(), SpanId.Create(), false)
+        );
+        using var transaction = new UnsampledTransaction(hub, context);
+
+        // Act
+        using var unsampledSpan = transaction.StartChild("Foo");
+
+        // Assert
+        var span = Assert.Single(transaction.Spans);
+        Assert.Same(unsampledSpan, span);
+    }
+
+    [Fact]
+    public void Finish_WithTrackedSpans_ClearsTrackedSpans()
+    {
+        // Arrange
+        var hub = Substitute.For<IHub>();
+        ITransactionContext context = new TransactionContext("TestTransaction", "TestOperation",
+            new SentryTraceHeader(SentryId.Create(), SpanId.Create(), false)
+        );
+        var transaction = new UnsampledTransaction(hub, context);
+        _ = transaction.StartChild("Foo");
+
+        // Act
+        transaction.Finish();
+
+        // Assert
+        Assert.Empty(transaction.Spans);
+    }
+
+    [Fact]
+    public void Dispose_WithTrackedSpans_ClearsTrackedSpans()
+    {
+        // Arrange
+        var hub = Substitute.For<IHub>();
+        ITransactionContext context = new TransactionContext("TestTransaction", "TestOperation",
+            new SentryTraceHeader(SentryId.Create(), SpanId.Create(), false)
+        );
+        var transaction = new UnsampledTransaction(hub, context);
+        _ = transaction.StartChild("Foo");
+
+        // Act
+        transaction.Dispose();
+
+        // Assert
+        Assert.Empty(transaction.Spans);
+    }
+}

test/Sentry.Tests/SpanTracerTests.cs

@@ -55,7 +55,7 @@ public void Dispose_IsUnfinished_Finishes()
     }
 
     [Fact]
-    public void Dispose_IsFinsished_DoesNothing()
+    public void Dispose_IsFinished_DoesNothing()
     {
         // Arrange
         var hub = Substitute.For<IHub>();

test/Sentry.Tests/TransactionTracerTests.cs

@@ -30,4 +30,50 @@ public void Dispose_Finished_DoesNothing()
         // Assert
         hub.Received(1).CaptureTransaction(Arg.Is<SentryTransaction>(t => t.SpanId == transaction.SpanId));
     }
+
+    [Fact]
+    public void StartChild_CreatesSpanTracer_TracksSpans()
+    {
+        // Arrange
+        var hub = Substitute.For<IHub>();
+        var transaction = new TransactionTracer(hub, "op", "name");
+
+        // Act
+        var span = transaction.StartChild("operation");
+
+        // Assert
+        Assert.IsType<SpanTracer>(span);
+        Assert.Collection(transaction.Spans,
+            element => Assert.Same(span, element));
+    }
+
+    [Fact]
+    public void Finish_WithTrackedSpans_ClearsTrackedSpans()
+    {
+        // Arrange
+        var hub = Substitute.For<IHub>();
+        var transaction = new TransactionTracer(hub, "op", "name");
+        _ = transaction.StartChild("operation");
+
+        // Act
+        transaction.Finish();
+
+        // Assert
+        Assert.Empty(transaction.Spans);
+    }
+
+    [Fact]
+    public void Dispose_WithTrackedSpans_ClearsTrackedSpans()
+    {
+        // Arrange
+        var hub = Substitute.For<IHub>();
+        var transaction = new TransactionTracer(hub, "op", "name");
+        _ = transaction.StartChild("operation");
+
+        // Act
+        transaction.Dispose();
+
+        // Assert
+        Assert.Empty(transaction.Spans);
+    }
 }