diff --git a/.editorconfig b/.editorconfig index a2a479130e..01deda3d42 100644 --- a/.editorconfig +++ b/.editorconfig @@ -40,6 +40,9 @@ indent_size = 2 # Sort using and Import directives with System.* appearing first dotnet_sort_system_directives_first = true +# Keep using directives at the top of the file, outside of the namespace +csharp_using_directive_placement = outside_namespace + # Avoid "this." and "Me." if not necessary dotnet_style_qualification_for_field = false : warning dotnet_style_qualification_for_property = false : warning diff --git a/src/Sentry.EntityFramework/SentryDatabaseLogging.cs b/src/Sentry.EntityFramework/SentryDatabaseLogging.cs index c61ff996d2..174af13873 100644 --- a/src/Sentry.EntityFramework/SentryDatabaseLogging.cs +++ b/src/Sentry.EntityFramework/SentryDatabaseLogging.cs @@ -1,3 +1,5 @@ +using Sentry.Internal; + namespace Sentry.EntityFramework; /// @@ -5,14 +7,14 @@ namespace Sentry.EntityFramework; /// internal static class SentryDatabaseLogging { - private static int Init; + private static InterlockedBoolean Init; internal static SentryCommandInterceptor? UseBreadcrumbs( IQueryLogger? queryLogger = null, bool initOnce = true, IDiagnosticLogger? diagnosticLogger = null) { - if (initOnce && Interlocked.Exchange(ref Init, 1) != 0) + if (initOnce && Init.Exchange(true)) { diagnosticLogger?.LogWarning("{0}.{1} was already executed.", nameof(SentryDatabaseLogging), nameof(UseBreadcrumbs)); diff --git a/src/Sentry.Profiling/SampleProfilerSession.cs b/src/Sentry.Profiling/SampleProfilerSession.cs index 6cd9a9242a..f3de9c2186 100644 --- a/src/Sentry.Profiling/SampleProfilerSession.cs +++ b/src/Sentry.Profiling/SampleProfilerSession.cs @@ -56,19 +56,12 @@ private SampleProfilerSession(SentryStopwatch stopwatch, EventPipeSession sessio public TraceLog TraceLog => EventSource.TraceLog; - // default is false, set 1 for true. - private static int _throwOnNextStartupForTests = 0; + private static InterlockedBoolean _throwOnNextStartupForTests = false; internal static bool ThrowOnNextStartupForTests { - get { return Interlocked.CompareExchange(ref _throwOnNextStartupForTests, 1, 1) == 1; } - set - { - if (value) - Interlocked.CompareExchange(ref _throwOnNextStartupForTests, 1, 0); - else - Interlocked.CompareExchange(ref _throwOnNextStartupForTests, 0, 1); - } + get { return _throwOnNextStartupForTests; } + set { _throwOnNextStartupForTests.Exchange(value); } } public static SampleProfilerSession StartNew(IDiagnosticLogger? logger = null) @@ -77,7 +70,7 @@ public static SampleProfilerSession StartNew(IDiagnosticLogger? logger = null) { var client = new DiagnosticsClient(Environment.ProcessId); - if (Interlocked.CompareExchange(ref _throwOnNextStartupForTests, 0, 1) == 1) + if (_throwOnNextStartupForTests.CompareExchange(false, true) == true) { throw new Exception("Test exception"); } diff --git a/src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs b/src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs index 5f0d54453d..1d3d03c5f3 100644 --- a/src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs +++ b/src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs @@ -6,14 +6,11 @@ namespace Sentry.Profiling; internal class SamplingTransactionProfilerFactory : IDisposable, ITransactionProfilerFactory { // We only allow a single profile so let's keep track of the current status. - internal int _inProgress = FALSE; + internal InterlockedBoolean _inProgress = false; // Whether the session startup took longer than the given timeout. internal bool StartupTimedOut { get; } - private const int TRUE = 1; - private const int FALSE = 0; - // Stop profiling after the given number of milliseconds. private const int TIME_LIMIT_MS = 30_000; @@ -50,12 +47,12 @@ public SamplingTransactionProfilerFactory(SentryOptions options, TimeSpan startu public ITransactionProfiler? Start(ITransactionTracer _, CancellationToken cancellationToken) { // Start a profiler if one wasn't running yet. - if (!_errorLogged && Interlocked.Exchange(ref _inProgress, TRUE) == FALSE) + if (!_errorLogged && !_inProgress.Exchange(true)) { if (!_sessionTask.IsCompleted) { _options.LogWarning("Cannot start a sampling profiler, the session hasn't started yet."); - _inProgress = FALSE; + _inProgress = false; return null; } @@ -63,7 +60,7 @@ public SamplingTransactionProfilerFactory(SentryOptions options, TimeSpan startu { _options.LogWarning("Cannot start a sampling profiler because the session startup has failed. This is a permanent error and no future transactions will be sampled."); _errorLogged = true; - _inProgress = FALSE; + _inProgress = false; return null; } @@ -72,13 +69,13 @@ public SamplingTransactionProfilerFactory(SentryOptions options, TimeSpan startu { return new SamplingTransactionProfiler(_options, _sessionTask.Result, TIME_LIMIT_MS, cancellationToken) { - OnFinish = () => _inProgress = FALSE + OnFinish = () => _inProgress = false }; } catch (Exception e) { _options.LogError(e, "Failed to start a profiler session."); - _inProgress = FALSE; + _inProgress = false; } } return null; diff --git a/src/Sentry/Ben.BlockingDetector/DetectBlockingSynchronizationContext.cs b/src/Sentry/Ben.BlockingDetector/DetectBlockingSynchronizationContext.cs index 2e4aeb08ca..701465b5d3 100644 --- a/src/Sentry/Ben.BlockingDetector/DetectBlockingSynchronizationContext.cs +++ b/src/Sentry/Ben.BlockingDetector/DetectBlockingSynchronizationContext.cs @@ -10,8 +10,8 @@ internal sealed class DetectBlockingSynchronizationContext : SynchronizationCont internal int _isSuppressed; - internal void Suppress() => Interlocked.Exchange(ref _isSuppressed, _isSuppressed + 1); - internal void Restore() => Interlocked.Exchange(ref _isSuppressed, _isSuppressed - 1); + internal void Suppress() => Interlocked.Increment(ref _isSuppressed); + internal void Restore() => Interlocked.Decrement(ref _isSuppressed); public DetectBlockingSynchronizationContext(IBlockingMonitor monitor) { diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index 5170a64233..5b0a71bd7f 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -24,15 +24,16 @@ internal class Hub : IHub, IDisposable private readonly MemoryMonitor? _memoryMonitor; #endif - private int _isPersistedSessionRecovered; + private InterlockedBoolean _isPersistedSessionRecovered; // Internal for testability internal ConditionalWeakTable ExceptionToSpanMap { get; } = new(); internal IInternalScopeManager ScopeManager { get; } - private int _isEnabled = 1; - public bool IsEnabled => _isEnabled == 1; + private InterlockedBoolean _isEnabled = true; + + public bool IsEnabled => _isEnabled; internal SentryOptions Options => _options; @@ -356,7 +357,7 @@ public TransactionContext ContinueTrace( public void StartSession() { // Attempt to recover persisted session left over from previous run - if (Interlocked.Exchange(ref _isPersistedSessionRecovered, 1) != 1) + if (_isPersistedSessionRecovered.Exchange(true) != true) { try { @@ -835,7 +836,7 @@ public void Dispose() { _options.LogInfo("Disposing the Hub."); - if (Interlocked.Exchange(ref _isEnabled, 0) != 1) + if (!_isEnabled.Exchange(false)) { return; } diff --git a/src/Sentry/Internal/InterlockedBoolean.cs b/src/Sentry/Internal/InterlockedBoolean.cs new file mode 100644 index 0000000000..61e7f0968d --- /dev/null +++ b/src/Sentry/Internal/InterlockedBoolean.cs @@ -0,0 +1,54 @@ +#if NET9_0_OR_GREATER +using TBool = System.Boolean; +#else +using TBool = System.Int32; +#endif + +namespace Sentry.Internal; + +internal struct InterlockedBoolean +{ + private volatile TBool _value; + + [Browsable(false)] + internal TBool ValueForTests => _value; + +#if NET9_0_OR_GREATER + private const TBool True = true; + private const TBool False = false; +#else + private const TBool True = 1; + private const TBool False = 0; +#endif + + public InterlockedBoolean() { } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public InterlockedBoolean(bool value) { _value = value ? True : False; } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static implicit operator bool(InterlockedBoolean @this) => (@this._value != False); + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static implicit operator InterlockedBoolean(bool @this) => new InterlockedBoolean(@this); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public bool Exchange(bool newValue) + { + TBool localNewValue = newValue ? True : False; + + TBool localReturnValue = Interlocked.Exchange(ref _value, localNewValue); + + return (localReturnValue != False); + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public bool CompareExchange(bool value, bool comparand) + { + TBool localValue = value ? True : False; + TBool localComparand = comparand ? True : False; + + TBool localReturnValue = Interlocked.CompareExchange(ref _value, localValue, localComparand); + + return (localReturnValue != False); + } +} diff --git a/src/Sentry/Threading/ScopedCountdownLock.cs b/src/Sentry/Threading/ScopedCountdownLock.cs index f3d992f893..1cf38678fb 100644 --- a/src/Sentry/Threading/ScopedCountdownLock.cs +++ b/src/Sentry/Threading/ScopedCountdownLock.cs @@ -1,3 +1,5 @@ +using Sentry.Internal; + namespace Sentry.Threading; /// @@ -13,12 +15,13 @@ namespace Sentry.Threading; internal sealed class ScopedCountdownLock : IDisposable { private readonly CountdownEvent _event; - private volatile int _isEngaged; + + private InterlockedBoolean _isEngaged; internal ScopedCountdownLock() { _event = new CountdownEvent(1); - _isEngaged = 0; + _isEngaged = false; } /// @@ -31,13 +34,13 @@ internal ScopedCountdownLock() /// Gets the number of remaining required to exit in order to set/signal the event while a is active. /// When and while a is active, no more can be entered. /// - internal int Count => _isEngaged == 1 ? _event.CurrentCount : _event.CurrentCount - 1; + internal int Count => _isEngaged ? _event.CurrentCount : _event.CurrentCount - 1; /// /// Returns when a is active and the event can be set/signaled by reaching . /// Returns when the can only reach the initial count of when no is active any longer. /// - internal bool IsEngaged => _isEngaged == 1; + internal bool IsEngaged => _isEngaged; /// /// No will be entered when the has reached , or while the lock is engaged via an active . @@ -79,7 +82,7 @@ private void ExitCounterScope() /// internal LockScope TryEnterLockScope() { - if (Interlocked.CompareExchange(ref _isEngaged, 1, 0) == 0) + if (_isEngaged.CompareExchange(true, false) == false) { Debug.Assert(_event.CurrentCount >= 1); _ = _event.Signal(); // decrement the initial count of 1, so that the event can be set with the count reaching 0 when all entered 'CounterScope' instances have exited @@ -94,7 +97,7 @@ private void ExitLockScope() Debug.Assert(_event.IsSet); _event.Reset(); // reset the signaled event to the initial count of 1, so that new 'CounterScope' instances can be entered again - if (Interlocked.CompareExchange(ref _isEngaged, 0, 1) != 1) + if (_isEngaged.CompareExchange(false, true) != true) { Debug.Fail("The Lock should have not been disengaged without being engaged first."); } diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs index 767ccf977c..1f965deef6 100644 --- a/src/Sentry/TransactionTracer.cs +++ b/src/Sentry/TransactionTracer.cs @@ -12,9 +12,10 @@ public class TransactionTracer : IBaseTracer, ITransactionTracer private readonly IHub _hub; private readonly SentryOptions? _options; private readonly Timer? _idleTimer; - private long _cancelIdleTimeout; private readonly SentryStopwatch _stopwatch = SentryStopwatch.StartNew(); + private InterlockedBoolean _cancelIdleTimeout; + private readonly Instrumenter _instrumenter = Instrumenter.Sentry; bool IBaseTracer.IsOtelInstrumenter => _instrumenter == Instrumenter.OpenTelemetry; @@ -247,7 +248,7 @@ internal TransactionTracer(IHub hub, ITransactionContext context, TimeSpan? idle // Set idle timer only if an idle timeout has been provided directly if (idleTimeout.HasValue) { - _cancelIdleTimeout = 1; // Timer will be cancelled once, atomically setting this back to 0 + _cancelIdleTimeout = true; // Timer will be cancelled once, atomically setting this back to false _idleTimer = new Timer(state => { if (state is not TransactionTracer transactionTracer) @@ -362,7 +363,7 @@ public void Clear() public void Finish() { _options?.LogDebug("Attempting to finish Transaction {0}.", SpanId); - if (Interlocked.Exchange(ref _cancelIdleTimeout, 0) == 1) + if (_cancelIdleTimeout.Exchange(false) == true) { _options?.LogDebug("Disposing of idle timer for Transaction {0}.", SpanId); _idleTimer?.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); diff --git a/test/Sentry.Tests/Internals/InterlockedBooleanTests.cs b/test/Sentry.Tests/Internals/InterlockedBooleanTests.cs new file mode 100644 index 0000000000..49c967a30b --- /dev/null +++ b/test/Sentry.Tests/Internals/InterlockedBooleanTests.cs @@ -0,0 +1,152 @@ +#if NET9_0_OR_GREATER +using TBool = System.Boolean; +#else +using TBool = System.Int32; +#endif + +namespace Sentry.Tests.Internals; + +public class InterlockedBooleanTests +{ +#if NET9_0_OR_GREATER + private const TBool True = true; + private const TBool False = false; +#else + private const TBool True = 1; + private const TBool False = 0; +#endif + + private TBool ToTBool(bool value) => value ? True : False; + private bool FromTBool(TBool value) => (value != False); + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void InterlockedBoolean_Constructor_ConstructsExpected(bool value) + { + // Arrange + var expected = ToTBool(value); + + // Act + var actual = new InterlockedBoolean(value); + + // Assert + actual.ValueForTests.Should().Be(expected); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void InterlockedBoolean_ImplicitToBool_ReturnsExpected(bool value) + { + // Arrange + var sut = new InterlockedBoolean(value); + var expected = value; + + // Act + bool actual = sut; + + // Assert + actual.Should().Be(expected); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void InterlockedBoolean_ImplicitFromBool_ReturnsExpected(bool value) + { + // Arrange + var expected = ToTBool(value); + + // Act + InterlockedBoolean actual = value; + + // Assert + actual.ValueForTests.Should().Be(expected); + } + + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public void InterlockedBoolean_Exchange_ReturnsExpected(bool initialState, bool newValue) + { + // Arrange + var sut = new InterlockedBoolean(initialState); + var expected = initialState; + + // Act + var result = sut.Exchange(newValue); + + // Assert + result.Should().Be(expected); + } + + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public void InterlockedBoolean_Exchange_SetsExpectedNewState(bool initialState, bool newValue) + { + // Arrange + var sut = new InterlockedBoolean(initialState); + + var expected = ToTBool(newValue); + + // Act + var _ = sut.Exchange(newValue); + + // Assert + sut.ValueForTests.Should().Be(expected); + } + + [Theory] + [InlineData(false, false, false)] + [InlineData(false, false, true)] + [InlineData(false, true, false)] + [InlineData(false, true, true)] + [InlineData(true, false, false)] + [InlineData(true, false, true)] + [InlineData(true, true, false)] + [InlineData(true, true, true)] + public void InterlockedBoolean_CompareExchange_ReturnsExpected(bool initialState, bool comparand, bool newValue) + { + // Arrange + var sut = new InterlockedBoolean(initialState); + var expected = initialState; + + // Act + var result = sut.CompareExchange(newValue, comparand); + + // Assert + result.Should().Be(expected); + } + + [Theory] + [InlineData(false, false, false)] + [InlineData(false, false, true)] + [InlineData(false, true, false)] + [InlineData(false, true, true)] + [InlineData(true, false, false)] + [InlineData(true, false, true)] + [InlineData(true, true, false)] + [InlineData(true, true, true)] + public void InterlockedBoolean_CompareExchange_SetsExpectedNewState(bool initialState, bool comparand, bool newValue) + { + // Arrange + var sut = new InterlockedBoolean(initialState); + + var expected = ToTBool( + initialState == comparand + ? newValue + : initialState); + + // Act + sut.CompareExchange(newValue, comparand); + + // Assert + sut.ValueForTests.Should().Be(expected); + } +}