diff --git a/src/Sentry/Internal/ConcurrentBagLite.cs b/src/Sentry/Internal/ConcurrentBagLite.cs new file mode 100644 index 0000000000..353b290724 --- /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 : IReadOnlyCollection +{ + 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/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/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..4a7d077174 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. @@ -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; @@ -278,11 +274,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 +427,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(); @@ -451,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 } /// 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/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(); } 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] 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"",