diff --git a/CHANGELOG.md b/CHANGELOG.md index 88efba3d31..4a887a8bcc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Fixes + +- Memory leak when finishing an unsampled Transaction that has started unsampled Spans ([#4717](https://github.com/getsentry/sentry-dotnet/pull/4717)) + - backported via ([#4853](https://github.com/getsentry/sentry-dotnet/pull/4853)) + ### Dependencies - Bump Java SDK from v8.24.0 to v8.25.0 ([#4679](https://github.com/getsentry/sentry-dotnet/pull/4679)) diff --git a/src/Sentry/Internal/UnsampledTransaction.cs b/src/Sentry/Internal/UnsampledTransaction.cs index 8f55b7d808..8277262458 100644 --- a/src/Sentry/Internal/UnsampledTransaction.cs +++ b/src/Sentry/Internal/UnsampledTransaction.cs @@ -4,7 +4,7 @@ namespace Sentry.Internal; /// /// 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). /// internal sealed class UnsampledTransaction : NoOpTransaction @@ -12,7 +12,12 @@ 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 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 + } } diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 1f965deef6..2c9e322baf 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -167,7 +167,7 @@ public IReadOnlyList Fingerprint /// public IReadOnlyDictionary Tags => _tags; -#if NETSTANDARD2_1_OR_GREATER +#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER private readonly ConcurrentBag _spans = new(); #else private ConcurrentBag _spans = new(); @@ -429,7 +429,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(); diff --git a/test/Sentry.Tests/Internals/UnsampledSpanTests.cs b/test/Sentry.Tests/Internals/UnsampledSpanTests.cs index a64edc9eed..338eb4e639 100644 --- a/test/Sentry.Tests/Internals/UnsampledSpanTests.cs +++ b/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(); + ITransactionContext context = new TransactionContext("TestTransaction", "TestOperation", + new SentryTraceHeader(SentryId.Create(), SpanId.Create(), false) + ); + var transaction = new UnsampledTransaction(hub, context); + + // Act + var unsampledSpan = transaction.StartChild("Foo"); + + // Assert + Assert.IsType(unsampledSpan); + Assert.Same(transaction, unsampledSpan.GetTransaction()); + } + [Fact] public void GetTraceHeader_CreatesHeaderFromUnsampledTransaction() { diff --git a/test/Sentry.Tests/Internals/UnsampledTransactionTests.cs b/test/Sentry.Tests/Internals/UnsampledTransactionTests.cs new file mode 100644 index 0000000000..679e907826 --- /dev/null +++ b/test/Sentry.Tests/Internals/UnsampledTransactionTests.cs @@ -0,0 +1,40 @@ +namespace Sentry.Tests.Internals; + +public class UnsampledTransactionTests +{ + [Fact] + public void StartChild_CreatesSpan_IsTrackedByParent() + { + // Arrange + var hub = Substitute.For(); + ITransactionContext context = new TransactionContext("TestTransaction", "TestOperation", + new SentryTraceHeader(SentryId.Create(), SpanId.Create(), false) + ); + var transaction = new UnsampledTransaction(hub, context); + + // Act + 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(); + 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); + } +} diff --git a/test/Sentry.Tests/SpanTracerTests.cs b/test/Sentry.Tests/SpanTracerTests.cs index 59becf2740..ee51246f70 100644 --- a/test/Sentry.Tests/SpanTracerTests.cs +++ b/test/Sentry.Tests/SpanTracerTests.cs @@ -10,7 +10,7 @@ public async Task SetExtra_DataInserted_NoDataLoss() { // Arrange var hub = Substitute.For(); - var transaction = new SpanTracer(hub, null, null, SentryId.Empty, ""); + var spanTracer = new SpanTracer(hub, null, null, SentryId.Empty, ""); var evt = new ManualResetEvent(false); var ready = new ManualResetEvent(false); var counter = 0; @@ -27,7 +27,7 @@ public async Task SetExtra_DataInserted_NoDataLoss() for (var i = 0; i < amount; i++) { - transaction.SetExtra(Guid.NewGuid().ToString(), Guid.NewGuid()); + spanTracer.SetExtra(Guid.NewGuid().ToString(), Guid.NewGuid()); } })).ToList(); ready.WaitOne(); @@ -36,7 +36,7 @@ public async Task SetExtra_DataInserted_NoDataLoss() // Arrange // 4 tasks testing X amount should be the same amount as Extras. - Assert.Equal(4 * amount, transaction.Extra.Count); + Assert.Equal(4 * amount, spanTracer.Extra.Count); } } } diff --git a/test/Sentry.Tests/TransactionTracerTests.cs b/test/Sentry.Tests/TransactionTracerTests.cs new file mode 100644 index 0000000000..2c4e282a23 --- /dev/null +++ b/test/Sentry.Tests/TransactionTracerTests.cs @@ -0,0 +1,35 @@ +namespace Sentry.Tests; + +public class TransactionTracerTests +{ + [Fact] + public void StartChild_CreatesSpanTracer_TracksSpans() + { + // Arrange + var hub = Substitute.For(); + var transaction = new TransactionTracer(hub, "op", "name"); + + // Act + var span = transaction.StartChild("operation"); + + // Assert + Assert.IsType(span); + Assert.Collection(transaction.Spans, + element => Assert.Same(span, element)); + } + + [Fact] + public void Finish_WithTrackedSpans_ClearsTrackedSpans() + { + // Arrange + var hub = Substitute.For(); + var transaction = new TransactionTracer(hub, "op", "name"); + _ = transaction.StartChild("operation"); + + // Act + transaction.Finish(); + + // Assert + Assert.Empty(transaction.Spans); + } +}