dispose of client should only flush

CHANGELOG.md

@@ -7,6 +7,10 @@
 - Add CaptureLastError as an extension method to the Server class on ASP.NET ([#1411](https://github.com/getsentry/sentry-dotnet/pull/1411))
 - Add IsDynamicCode* to events ([#1418](https://github.com/getsentry/sentry-dotnet/pull/1418))
 
+### Fixes
+
+- Dispose of client should only flush ([#1354](https://github.com/getsentry/sentry-dotnet/pull/1354))
+
 ## 3.12.3
 
 ### Fixes

src/Sentry/Internal/Hub.cs

@@ -391,9 +391,8 @@ public void Dispose()
                 }
             }
 
-            (_ownedClient as IDisposable)?.Dispose();
-            _rootScope.Dispose();
-            ScopeManager.Dispose();
+            _ownedClient.FlushAsync(_options.ShutdownTimeout).GetAwaiter().GetResult();
+            //Dont dispose of _rootScope and ScopeManager since we want dangling transactions to still be able to access tags.
         }
 
         public SentryId LastEventId

src/Sentry/SentryClient.cs

@@ -20,7 +20,6 @@ namespace Sentry
     /// <inheritdoc cref="IDisposable" />
     public class SentryClient : ISentryClient, IDisposable
     {
-        private volatile bool _disposed;
         private readonly SentryOptions _options;
 
         // Internal for testing.
@@ -62,11 +61,6 @@ internal SentryClient(
         /// <inheritdoc />
         public SentryId CaptureEvent(SentryEvent? @event, Scope? scope = null)
         {
-            if (_disposed)
-            {
-                throw new ObjectDisposedException(nameof(SentryClient));
-            }
-
             if (@event == null)
             {
                 return SentryId.Empty;
@@ -86,11 +80,6 @@ public SentryId CaptureEvent(SentryEvent? @event, Scope? scope = null)
         /// <inheritdoc />
         public void CaptureUserFeedback(UserFeedback userFeedback)
         {
-            if (_disposed)
-            {
-                throw new ObjectDisposedException(nameof(SentryClient));
-            }
-
             if (userFeedback.EventId.Equals(SentryId.Empty))
             {
                 // Ignore the user feedback if EventId is empty
@@ -104,11 +93,6 @@ public void CaptureUserFeedback(UserFeedback userFeedback)
         /// <inheritdoc />
         public void CaptureTransaction(Transaction transaction)
         {
-            if (_disposed)
-            {
-                throw new ObjectDisposedException(nameof(SentryClient));
-            }
-
             if (transaction.SpanId.Equals(SpanId.Empty))
             {
                 _options.LogWarning(
@@ -156,11 +140,6 @@ public void CaptureTransaction(Transaction transaction)
         /// <inheritdoc />
         public void CaptureSession(SessionUpdate sessionUpdate)
         {
-            if (_disposed)
-            {
-                throw new ObjectDisposedException(nameof(SentryClient));
-            }
-
             CaptureEnvelope(Envelope.FromSession(sessionUpdate));
         }
 
@@ -299,19 +278,13 @@ private bool CaptureEnvelope(Envelope envelope)
         /// Disposes this client
         /// </summary>
         /// <inheritdoc />
+        [Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in version 4.")]
         public void Dispose()
         {
-            _options.LogDebug("Disposing SentryClient.");
-
-            if (_disposed)
-            {
-                return;
-            }
-
-            _disposed = true;
+            _options.LogDebug("Flushing SentryClient.");
 
             // Worker should empty it's queue until SentryOptions.ShutdownTimeout
-            (Worker as IDisposable)?.Dispose();
+            Worker.FlushAsync(_options.ShutdownTimeout).GetAwaiter().GetResult();
         }
     }
 }

test/Sentry.DiagnosticSource.Tests/ApiApprovalTests.Run.Core3_1.verified.txt

@@ -341,6 +341,8 @@ namespace Sentry
         public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
         public void CaptureTransaction(Sentry.Transaction transaction) { }
         public void CaptureUserFeedback(Sentry.UserFeedback userFeedback) { }
+        [System.Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in" +
+            " version 4.")]
         public void Dispose() { }
         public System.Threading.Tasks.Task FlushAsync(System.TimeSpan timeout) { }
     }

test/Sentry.DiagnosticSource.Tests/ApiApprovalTests.Run.DotNet5_0.verified.txt

@@ -341,6 +341,8 @@ namespace Sentry
         public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
         public void CaptureTransaction(Sentry.Transaction transaction) { }
         public void CaptureUserFeedback(Sentry.UserFeedback userFeedback) { }
+        [System.Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in" +
+            " version 4.")]
         public void Dispose() { }
         public System.Threading.Tasks.Task FlushAsync(System.TimeSpan timeout) { }
     }

test/Sentry.DiagnosticSource.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt

@@ -341,6 +341,8 @@ namespace Sentry
         public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
         public void CaptureTransaction(Sentry.Transaction transaction) { }
         public void CaptureUserFeedback(Sentry.UserFeedback userFeedback) { }
+        [System.Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in" +
+            " version 4.")]
         public void Dispose() { }
         public System.Threading.Tasks.Task FlushAsync(System.TimeSpan timeout) { }
     }

test/Sentry.Tests/ApiApprovalTests.Run.Core2_1.verified.txt

@@ -341,6 +341,8 @@ namespace Sentry
         public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
         public void CaptureTransaction(Sentry.Transaction transaction) { }
         public void CaptureUserFeedback(Sentry.UserFeedback userFeedback) { }
+        [System.Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in" +
+            " version 4.")]
         public void Dispose() { }
         public System.Threading.Tasks.Task FlushAsync(System.TimeSpan timeout) { }
     }

test/Sentry.Tests/ApiApprovalTests.Run.Core3_0.verified.txt

@@ -341,6 +341,8 @@ namespace Sentry
         public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
         public void CaptureTransaction(Sentry.Transaction transaction) { }
         public void CaptureUserFeedback(Sentry.UserFeedback userFeedback) { }
+        [System.Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in" +
+            " version 4.")]
         public void Dispose() { }
         public System.Threading.Tasks.Task FlushAsync(System.TimeSpan timeout) { }
     }

test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt

@@ -341,6 +341,8 @@ namespace Sentry
         public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
         public void CaptureTransaction(Sentry.Transaction transaction) { }
         public void CaptureUserFeedback(Sentry.UserFeedback userFeedback) { }
+        [System.Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in" +
+            " version 4.")]
         public void Dispose() { }
         public System.Threading.Tasks.Task FlushAsync(System.TimeSpan timeout) { }
     }

test/Sentry.Tests/ApiApprovalTests.Run.DotNet4_6.verified.txt

@@ -340,6 +340,8 @@ namespace Sentry
         public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
         public void CaptureTransaction(Sentry.Transaction transaction) { }
         public void CaptureUserFeedback(Sentry.UserFeedback userFeedback) { }
+        [System.Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in" +
+            " version 4.")]
         public void Dispose() { }
         public System.Threading.Tasks.Task FlushAsync(System.TimeSpan timeout) { }
     }

test/Sentry.Tests/ApiApprovalTests.Run.DotNet5_0.verified.txt

@@ -341,6 +341,8 @@ namespace Sentry
         public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
         public void CaptureTransaction(Sentry.Transaction transaction) { }
         public void CaptureUserFeedback(Sentry.UserFeedback userFeedback) { }
+        [System.Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in" +
+            " version 4.")]
         public void Dispose() { }
         public System.Threading.Tasks.Task FlushAsync(System.TimeSpan timeout) { }
     }

test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt

@@ -341,6 +341,8 @@ namespace Sentry
         public void CaptureSession(Sentry.SessionUpdate sessionUpdate) { }
         public void CaptureTransaction(Sentry.Transaction transaction) { }
         public void CaptureUserFeedback(Sentry.UserFeedback userFeedback) { }
+        [System.Obsolete("Sentry client should no be explicitly disposed of. This method will be removed in" +
+            " version 4.")]
         public void Dispose() { }
         public System.Threading.Tasks.Task FlushAsync(System.TimeSpan timeout) { }
     }

test/Sentry.Tests/HubTests.cs

@@ -849,17 +849,18 @@ public void Dispose_IsEnabled_SetToFalse()
     public void Dispose_CalledSecondTime_ClientDisposedOnce()
     {
         var client = Substitute.For<ISentryClient, IDisposable>();
-        var hub = new Hub(new SentryOptions
+        var options = new SentryOptions
         {
             Dsn = DsnSamples.ValidDsnWithSecret
-        }, client);
+        };
+        var hub = new Hub(options, client);
 
         // Act
         hub.Dispose();
         hub.Dispose();
 
         // Assert
-        ((IDisposable)client).Received(1).Dispose();
+        client.Received(1).FlushAsync(options.ShutdownTimeout);
     }
 
     [Fact]

test/Sentry.Tests/SentryClientTests.cs

@@ -1,4 +1,5 @@
 using System.Net.Http;
+#pragma warning disable CS0618
 
 namespace Sentry.Tests;
 
@@ -372,11 +373,12 @@ public void CaptureEvent_Release_SetFromOptions()
     }
 
     [Fact]
-    public void CaptureEvent_DisposedClient_ThrowsObjectDisposedException()
+    public void CaptureEvent_DisposedClient_DoesNotThrow()
     {
         var sut = _fixture.GetSut();
         sut.Dispose();
-        _ = Assert.Throws<ObjectDisposedException>(() => sut.CaptureEvent(null));
+        var @event = new SentryEvent();
+        sut.CaptureEvent(@event);
     }
 
     [Fact]
@@ -419,13 +421,28 @@ public void CaptureUserFeedback_EventIdEmpty_FeedbackIgnored()
         //Assert
         _ = sut.Worker.DidNotReceive().EnqueueEnvelope(Arg.Any<Envelope>());
     }
+    [Fact]
+    public void Dispose_should_only_flush()
+    {
+        // Arrange
+        var client = new SentryClient(new SentryOptions
+        {
+            Dsn = DsnSamples.ValidDsnWithSecret,
+        });
+
+        // Act
+        client.Dispose();
+
+        //Assert is still usable
+        client.CaptureEvent(new SentryEvent { Message = "Test" });
+    }
 
     [Fact]
-    public void CaptureUserFeedback_DisposedClient_ThrowsObjectDisposedException()
+    public void CaptureUserFeedback_DisposedClient_DoesNotThrow()
     {
         var sut = _fixture.GetSut();
         sut.Dispose();
-        _ = Assert.Throws<ObjectDisposedException>(() => sut.CaptureUserFeedback(null));
+        sut.CaptureUserFeedback(new UserFeedback(SentryId.Empty, "name", "email", "comment"));
     }
 
     [Fact]
@@ -557,34 +574,35 @@ public void CaptureTransaction_NotFinished_Sent()
     }
 
     [Fact]
-    public void CaptureTransaction_DisposedClient_ThrowsObjectDisposedException()
+    public void CaptureTransaction_DisposedClient_DoesNotThrow()
     {
         var sut = _fixture.GetSut();
         sut.Dispose();
-        _ = Assert.Throws<ObjectDisposedException>(() => sut.CaptureTransaction(null));
+        sut.CaptureTransaction(
+            new Transaction(
+                "test name",
+                "test operation")
+            {
+                IsSampled = true,
+                EndTimestamp = null // not finished
+            });
     }
 
     [Fact]
-    public void Dispose_Worker_DisposeCalled()
+    public void Dispose_Worker_FlushCalled()
     {
-        _fixture.GetSut().Dispose();
-        (_fixture.BackgroundWorker as IDisposable)?.Received(1).Dispose();
+        var client = _fixture.GetSut();
+        client.Dispose();
+        _fixture.BackgroundWorker?.Received(1).FlushAsync(_fixture.SentryOptions.ShutdownTimeout);
     }
 
     [Fact]
-    public void Dispose_MultipleCalls_WorkerDisposedOnce()
+    public void Dispose_MultipleCalls_WorkerFlushedTwice()
     {
         var sut = _fixture.GetSut();
         sut.Dispose();
         sut.Dispose();
-        (_fixture.BackgroundWorker as IDisposable).Received(1).Dispose();
-    }
-
-    [Fact]
-    public void Dispose_WorkerDoesNotImplementDispose_DoesntThrow()
-    {
-        _fixture.BackgroundWorker = Substitute.For<IBackgroundWorker>();
-        _fixture.GetSut().Dispose();
+        _fixture.BackgroundWorker?.Received(2).FlushAsync(_fixture.SentryOptions.ShutdownTimeout);
     }
 
     [Fact]