From ee67ed72a4f0561491afc44d4ecfca0dd6475979 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 7 Jul 2025 11:28:02 +1200 Subject: [PATCH 1/6] fix: Remove AppDomain.CurrentDomain.ProcessExit hook on shutdown Resolves #3141 - https://github.com/getsentry/sentry-dotnet/issues/3141 --- .../AppDomainProcessExitIntegration.cs | 7 ++++++- .../Integrations/ISdkIntegrationCleanup.cs | 17 +++++++++++++++++ src/Sentry/Internal/Hub.cs | 18 ++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 src/Sentry/Integrations/ISdkIntegrationCleanup.cs diff --git a/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs b/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs index 9fd9caebe2..0e8ae5f8ec 100644 --- a/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs +++ b/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs @@ -3,7 +3,7 @@ namespace Sentry.Integrations; -internal class AppDomainProcessExitIntegration : ISdkIntegration +internal class AppDomainProcessExitIntegration : ISdkIntegration, ISdkIntegrationCleanup { private readonly IAppDomain _appDomain; private IHub? _hub; @@ -24,4 +24,9 @@ internal void HandleProcessExit(object? sender, EventArgs e) _options?.LogInfo("AppDomain process exited: Disposing SDK."); (_hub as IDisposable)?.Dispose(); } + + public void Cleanup() + { + _appDomain.ProcessExit -= HandleProcessExit; + } } diff --git a/src/Sentry/Integrations/ISdkIntegrationCleanup.cs b/src/Sentry/Integrations/ISdkIntegrationCleanup.cs new file mode 100644 index 0000000000..8cd13c0f8f --- /dev/null +++ b/src/Sentry/Integrations/ISdkIntegrationCleanup.cs @@ -0,0 +1,17 @@ +namespace Sentry.Integrations; + +/// +/// Since we can't add methods to without it being a breaking change, this interface +/// allows us to add some cleanup logic as an internal interface. We can move this to +/// in the next major release. +/// +internal interface ISdkIntegrationCleanup +{ + /// + /// Performs any necessary cleanup for the integration + /// + /// + /// Called when the Hub is disposed + /// + public void Cleanup(); +} diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index ff9d5de43a..bf07f76298 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -1,5 +1,6 @@ using Sentry.Extensibility; using Sentry.Infrastructure; +using Sentry.Integrations; using Sentry.Protocol.Envelopes; using Sentry.Protocol.Metrics; @@ -14,6 +15,7 @@ internal class Hub : IHub, IDisposable private readonly SentryOptions _options; private readonly RandomValuesFactory _randomValuesFactory; private readonly IReplaySession _replaySession; + private readonly List _integrationsToCleanup = new(); #if MEMORY_DUMP_SUPPORTED private readonly MemoryMonitor? _memoryMonitor; @@ -84,6 +86,10 @@ internal Hub( { options.LogDebug("Registering integration: '{0}'.", integration.GetType().Name); integration.Register(this, options); + if (integration is ISdkIntegrationCleanup cleanupIntegration) + { + _integrationsToCleanup.Add(cleanupIntegration); + } } } @@ -772,6 +778,18 @@ public void Dispose() return; } + foreach (var integration in _integrationsToCleanup) + { + try + { + integration.Cleanup(); + } + catch (Exception e) + { + _options.LogError("Failure to cleanup integration {0}: {1}", integration.GetType().Name, e); + } + } + #if MEMORY_DUMP_SUPPORTED _memoryMonitor?.Dispose(); #endif From df5cf45c514d0a80bb94b2fc4bc60bdc5b87dd54 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 7 Jul 2025 12:00:25 +1200 Subject: [PATCH 2/6] Unit tests --- .../AppDomainProcessExitIntegration.cs | 2 +- ...ationCleanup.cs => ITidySdkIntegration.cs} | 2 +- src/Sentry/Internal/Hub.cs | 6 +- test/Sentry.Tests/HubTests.cs | 74 +++++++++++++++++++ 4 files changed, 79 insertions(+), 5 deletions(-) rename src/Sentry/Integrations/{ISdkIntegrationCleanup.cs => ITidySdkIntegration.cs} (90%) diff --git a/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs b/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs index 0e8ae5f8ec..45bcd70c9b 100644 --- a/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs +++ b/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs @@ -3,7 +3,7 @@ namespace Sentry.Integrations; -internal class AppDomainProcessExitIntegration : ISdkIntegration, ISdkIntegrationCleanup +internal class AppDomainProcessExitIntegration : ITidySdkIntegration { private readonly IAppDomain _appDomain; private IHub? _hub; diff --git a/src/Sentry/Integrations/ISdkIntegrationCleanup.cs b/src/Sentry/Integrations/ITidySdkIntegration.cs similarity index 90% rename from src/Sentry/Integrations/ISdkIntegrationCleanup.cs rename to src/Sentry/Integrations/ITidySdkIntegration.cs index 8cd13c0f8f..cc9ee6ea3f 100644 --- a/src/Sentry/Integrations/ISdkIntegrationCleanup.cs +++ b/src/Sentry/Integrations/ITidySdkIntegration.cs @@ -5,7 +5,7 @@ namespace Sentry.Integrations; /// allows us to add some cleanup logic as an internal interface. We can move this to /// in the next major release. /// -internal interface ISdkIntegrationCleanup +internal interface ITidySdkIntegration: ISdkIntegration { /// /// Performs any necessary cleanup for the integration diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index bf07f76298..bd065ec7cd 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -15,7 +15,7 @@ internal class Hub : IHub, IDisposable private readonly SentryOptions _options; private readonly RandomValuesFactory _randomValuesFactory; private readonly IReplaySession _replaySession; - private readonly List _integrationsToCleanup = new(); + private readonly List _integrationsToCleanup = new(); #if MEMORY_DUMP_SUPPORTED private readonly MemoryMonitor? _memoryMonitor; @@ -86,9 +86,9 @@ internal Hub( { options.LogDebug("Registering integration: '{0}'.", integration.GetType().Name); integration.Register(this, options); - if (integration is ISdkIntegrationCleanup cleanupIntegration) + if (integration is ITidySdkIntegration tidyIntegration) { - _integrationsToCleanup.Add(cleanupIntegration); + _integrationsToCleanup.Add(tidyIntegration); } } } diff --git a/test/Sentry.Tests/HubTests.cs b/test/Sentry.Tests/HubTests.cs index 864a92dd2d..b122066400 100644 --- a/test/Sentry.Tests/HubTests.cs +++ b/test/Sentry.Tests/HubTests.cs @@ -1988,6 +1988,80 @@ public void CaptureUserFeedback_InvalidEmail_FeedbackDropped(string email) _fixture.Client.Received(1).CaptureUserFeedback(Arg.Is(f => f.Email.IsNull())); #pragma warning restore CS0618 // Type or member is obsolete } + + [Fact] + public void Dispose_IntegrationsWithCleanup_CleanupCalled() + { + // Arrange + var cleaned = new List(); + var integration1 = Substitute.For(); + integration1.When(i => i.Cleanup()).Do(_ => cleaned.Add(integration1)); + var integration2 = Substitute.For(); + integration2.When(i => i.Cleanup()).Do(_ => cleaned.Add(integration2)); + var integration3 = Substitute.For(); + _fixture.Options.AddIntegration(integration1); + _fixture.Options.AddIntegration(integration2); + _fixture.Options.AddIntegration(integration3); + var hub = _fixture.GetSut(); + + // Act + hub.Dispose(); + + // Assert + cleaned.Should().Contain(integration1); + cleaned.Should().Contain(integration2); + } + + [Fact] + public void Dispose_CleanupThrowsException_ExceptionHandledAndLogged() + { + // Arrange + var cleaned = new List(); + var integration1 = Substitute.For(); + integration1.When(i => i.Cleanup()).Do(_ => cleaned.Add(integration1)); + var integration2 = Substitute.For(); + integration2.When(i => i.Cleanup()).Do(_ => throw new InvalidOperationException("Cleanup failed")); + var integration3 = Substitute.For(); + integration3.When(i => i.Cleanup()).Do(_ => cleaned.Add(integration3)); + _fixture.Options.AddIntegration(integration1); + _fixture.Options.AddIntegration(integration2); + _fixture.Options.AddIntegration(integration3); + _fixture.Options.Debug = true; + _fixture.Options.DiagnosticLogger = Substitute.For(); + _fixture.Options.DiagnosticLogger!.IsEnabled(Arg.Any()).Returns(true); + var hub = _fixture.GetSut(); + + // Act + hub.Dispose(); + + // Assert + cleaned.Should().Contain(integration1); + cleaned.Should().NotContain(integration2); + cleaned.Should().Contain(integration3); + _fixture.Options.DiagnosticLogger.Received(1).Log( + SentryLevel.Error, + Arg.Is(s => s.Contains("Failure to cleanup integration")), + Arg.Any(), + Arg.Any()); + } + + [Fact] + public void Dispose_CalledMultipleTimes_CleanupCalledOnlyOnce() + { + // Arrange + var cleaned = 0; + var integration = Substitute.For(); + integration.When(i => i.Cleanup()).Do(_ => cleaned++); + _fixture.Options.AddIntegration(integration); + var hub = _fixture.GetSut(); + + // Act + hub.Dispose(); + hub.Dispose(); + + // Assert + cleaned.Should().Be(1); + } } #if NET6_0_OR_GREATER From 0e5e8d1f95c266f17caef5d148aa2ec314866a67 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Mon, 7 Jul 2025 12:01:20 +1200 Subject: [PATCH 3/6] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ecd487a840..a73f566bbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ### Fixes - Crontab validation when capturing checkins ([#4314](https://github.com/getsentry/sentry-dotnet/pull/4314)) +- AppDomain.CurrentDomain.ProcessExit hook is now removed on shutdown ([#4323](https://github.com/getsentry/sentry-dotnet/pull/4323)) ### Dependencies From 6af0c0f199e40afadd255b2b8c39815b6f4f8b7c Mon Sep 17 00:00:00 2001 From: Sentry Github Bot Date: Mon, 7 Jul 2025 00:14:02 +0000 Subject: [PATCH 4/6] Format code --- src/Sentry/Integrations/ITidySdkIntegration.cs | 2 +- src/Sentry/Internal/Hub.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Sentry/Integrations/ITidySdkIntegration.cs b/src/Sentry/Integrations/ITidySdkIntegration.cs index cc9ee6ea3f..2eb9489422 100644 --- a/src/Sentry/Integrations/ITidySdkIntegration.cs +++ b/src/Sentry/Integrations/ITidySdkIntegration.cs @@ -5,7 +5,7 @@ namespace Sentry.Integrations; /// allows us to add some cleanup logic as an internal interface. We can move this to /// in the next major release. /// -internal interface ITidySdkIntegration: ISdkIntegration +internal interface ITidySdkIntegration : ISdkIntegration { /// /// Performs any necessary cleanup for the integration diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index bd065ec7cd..4f0646a1be 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -782,7 +782,7 @@ public void Dispose() { try { - integration.Cleanup(); + integration.Cleanup(); } catch (Exception e) { From 6afe45df8134c3338cdb65285ca5226a46e68941 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Wed, 9 Jul 2025 09:28:25 +1200 Subject: [PATCH 5/6] Replaced ITidySdkIntegration with IDisposable --- .../AppDomainProcessExitIntegration.cs | 4 +- .../Integrations/ITidySdkIntegration.cs | 17 ----- src/Sentry/Internal/Hub.cs | 10 +-- test/Sentry.Tests/HubTests.cs | 66 ++++++++++++------- 4 files changed, 50 insertions(+), 47 deletions(-) delete mode 100644 src/Sentry/Integrations/ITidySdkIntegration.cs diff --git a/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs b/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs index 45bcd70c9b..07b222b4dd 100644 --- a/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs +++ b/src/Sentry/Integrations/AppDomainProcessExitIntegration.cs @@ -3,7 +3,7 @@ namespace Sentry.Integrations; -internal class AppDomainProcessExitIntegration : ITidySdkIntegration +internal class AppDomainProcessExitIntegration : ISdkIntegration, IDisposable { private readonly IAppDomain _appDomain; private IHub? _hub; @@ -25,7 +25,7 @@ internal void HandleProcessExit(object? sender, EventArgs e) (_hub as IDisposable)?.Dispose(); } - public void Cleanup() + public void Dispose() { _appDomain.ProcessExit -= HandleProcessExit; } diff --git a/src/Sentry/Integrations/ITidySdkIntegration.cs b/src/Sentry/Integrations/ITidySdkIntegration.cs deleted file mode 100644 index 2eb9489422..0000000000 --- a/src/Sentry/Integrations/ITidySdkIntegration.cs +++ /dev/null @@ -1,17 +0,0 @@ -namespace Sentry.Integrations; - -/// -/// Since we can't add methods to without it being a breaking change, this interface -/// allows us to add some cleanup logic as an internal interface. We can move this to -/// in the next major release. -/// -internal interface ITidySdkIntegration : ISdkIntegration -{ - /// - /// Performs any necessary cleanup for the integration - /// - /// - /// Called when the Hub is disposed - /// - public void Cleanup(); -} diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index 4f0646a1be..67318abc54 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -15,7 +15,7 @@ internal class Hub : IHub, IDisposable private readonly SentryOptions _options; private readonly RandomValuesFactory _randomValuesFactory; private readonly IReplaySession _replaySession; - private readonly List _integrationsToCleanup = new(); + private readonly List _integrationsToCleanup = new(); #if MEMORY_DUMP_SUPPORTED private readonly MemoryMonitor? _memoryMonitor; @@ -86,9 +86,9 @@ internal Hub( { options.LogDebug("Registering integration: '{0}'.", integration.GetType().Name); integration.Register(this, options); - if (integration is ITidySdkIntegration tidyIntegration) + if (integration is IDisposable disposableIntegration) { - _integrationsToCleanup.Add(tidyIntegration); + _integrationsToCleanup.Add(disposableIntegration); } } } @@ -782,11 +782,11 @@ public void Dispose() { try { - integration.Cleanup(); + integration.Dispose(); } catch (Exception e) { - _options.LogError("Failure to cleanup integration {0}: {1}", integration.GetType().Name, e); + _options.LogError("Failed to dispose integration {0}: {1}", integration.GetType().Name, e); } } diff --git a/test/Sentry.Tests/HubTests.cs b/test/Sentry.Tests/HubTests.cs index b122066400..8189952e66 100644 --- a/test/Sentry.Tests/HubTests.cs +++ b/test/Sentry.Tests/HubTests.cs @@ -1989,16 +1989,42 @@ public void CaptureUserFeedback_InvalidEmail_FeedbackDropped(string email) #pragma warning restore CS0618 // Type or member is obsolete } + private class TestDisposableIntegration: ISdkIntegration, IDisposable + { + public int Registered { get; private set; } + public int Disposed { get; private set; } + + public void Register(IHub hub, SentryOptions options) + { + Registered++; + } + + protected virtual void Cleanup() + { + Disposed++; + } + + public void Dispose() + { + Cleanup(); + } + } + + private class TestFlakyDisposableIntegration : TestDisposableIntegration + { + protected override void Cleanup() + { + throw new InvalidOperationException("Cleanup failed"); + } + } + [Fact] public void Dispose_IntegrationsWithCleanup_CleanupCalled() { // Arrange - var cleaned = new List(); - var integration1 = Substitute.For(); - integration1.When(i => i.Cleanup()).Do(_ => cleaned.Add(integration1)); - var integration2 = Substitute.For(); - integration2.When(i => i.Cleanup()).Do(_ => cleaned.Add(integration2)); - var integration3 = Substitute.For(); + var integration1 = new TestDisposableIntegration(); + var integration2 = Substitute.For(); + var integration3 = new TestDisposableIntegration(); _fixture.Options.AddIntegration(integration1); _fixture.Options.AddIntegration(integration2); _fixture.Options.AddIntegration(integration3); @@ -2008,21 +2034,17 @@ public void Dispose_IntegrationsWithCleanup_CleanupCalled() hub.Dispose(); // Assert - cleaned.Should().Contain(integration1); - cleaned.Should().Contain(integration2); + integration1.Disposed.Should().Be(1); + integration3.Disposed.Should().Be(1); } [Fact] public void Dispose_CleanupThrowsException_ExceptionHandledAndLogged() { // Arrange - var cleaned = new List(); - var integration1 = Substitute.For(); - integration1.When(i => i.Cleanup()).Do(_ => cleaned.Add(integration1)); - var integration2 = Substitute.For(); - integration2.When(i => i.Cleanup()).Do(_ => throw new InvalidOperationException("Cleanup failed")); - var integration3 = Substitute.For(); - integration3.When(i => i.Cleanup()).Do(_ => cleaned.Add(integration3)); + var integration1 = new TestDisposableIntegration(); + var integration2 = new TestFlakyDisposableIntegration(); + var integration3 = new TestDisposableIntegration(); _fixture.Options.AddIntegration(integration1); _fixture.Options.AddIntegration(integration2); _fixture.Options.AddIntegration(integration3); @@ -2035,12 +2057,12 @@ public void Dispose_CleanupThrowsException_ExceptionHandledAndLogged() hub.Dispose(); // Assert - cleaned.Should().Contain(integration1); - cleaned.Should().NotContain(integration2); - cleaned.Should().Contain(integration3); + integration1.Disposed.Should().Be(1); + integration2.Disposed.Should().Be(0); + integration3.Disposed.Should().Be(1); _fixture.Options.DiagnosticLogger.Received(1).Log( SentryLevel.Error, - Arg.Is(s => s.Contains("Failure to cleanup integration")), + Arg.Is(s => s.Contains("Failed to dispose integration")), Arg.Any(), Arg.Any()); } @@ -2049,9 +2071,7 @@ public void Dispose_CleanupThrowsException_ExceptionHandledAndLogged() public void Dispose_CalledMultipleTimes_CleanupCalledOnlyOnce() { // Arrange - var cleaned = 0; - var integration = Substitute.For(); - integration.When(i => i.Cleanup()).Do(_ => cleaned++); + var integration = new TestDisposableIntegration(); _fixture.Options.AddIntegration(integration); var hub = _fixture.GetSut(); @@ -2060,7 +2080,7 @@ public void Dispose_CalledMultipleTimes_CleanupCalledOnlyOnce() hub.Dispose(); // Assert - cleaned.Should().Be(1); + integration.Disposed.Should().Be(1); } } From daec7ab0b7b3ac1d0e131ad3121878824f009eb5 Mon Sep 17 00:00:00 2001 From: Sentry Github Bot Date: Tue, 8 Jul 2025 21:40:53 +0000 Subject: [PATCH 6/6] Format code --- test/Sentry.Tests/HubTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Sentry.Tests/HubTests.cs b/test/Sentry.Tests/HubTests.cs index 8189952e66..85bf533df9 100644 --- a/test/Sentry.Tests/HubTests.cs +++ b/test/Sentry.Tests/HubTests.cs @@ -1989,7 +1989,7 @@ public void CaptureUserFeedback_InvalidEmail_FeedbackDropped(string email) #pragma warning restore CS0618 // Type or member is obsolete } - private class TestDisposableIntegration: ISdkIntegration, IDisposable + private class TestDisposableIntegration : ISdkIntegration, IDisposable { public int Registered { get; private set; } public int Disposed { get; private set; }