Use new Interlocked.Exchange/CompareExchange overloads that support bool values (#4585)

Author: logiclrd

.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

src/Sentry.EntityFramework/SentryDatabaseLogging.cs

@@ -1,18 +1,20 @@
+using Sentry.Internal;
+
 namespace Sentry.EntityFramework;
 
 /// <summary>
 /// Sentry Database Logger
 /// </summary>
 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));

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");
             }

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,20 +47,20 @@ 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;
             }
 
             if (!_sessionTask.IsCompletedSuccessfully)
             {
                 _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;

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)
         {

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<Exception, ISpan> 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;
         }

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);
+    }
+}

src/Sentry/Threading/ScopedCountdownLock.cs

@@ -1,3 +1,5 @@
+using Sentry.Internal;
+
 namespace Sentry.Threading;
 
 /// <summary>
@@ -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;
     }
 
     /// <summary>
@@ -31,13 +34,13 @@ internal ScopedCountdownLock()
     /// Gets the number of remaining <see cref="CounterScope"/> required to exit in order to set/signal the event while a <see cref="LockScope"/> is active.
     /// When <see langword="0"/> and while a <see cref="LockScope"/> is active, no more <see cref="CounterScope"/> can be entered.
     /// </summary>
-    internal int Count => _isEngaged == 1 ? _event.CurrentCount : _event.CurrentCount - 1;
+    internal int Count => _isEngaged ? _event.CurrentCount : _event.CurrentCount - 1;
 
     /// <summary>
     /// Returns <see langword="true"/> when a <see cref="LockScope"/> is active and the event can be set/signaled by <see cref="Count"/> reaching <see langword="0"/>.
     /// Returns <see langword="false"/> when the <see cref="Count"/> can only reach the initial count of <see langword="1"/> when no <see cref="CounterScope"/> is active any longer.
     /// </summary>
-    internal bool IsEngaged => _isEngaged == 1;
+    internal bool IsEngaged => _isEngaged;
 
     /// <summary>
     /// No <see cref="CounterScope"/> will be entered when the <see cref="Count"/> has reached <see langword="0"/>, or while the lock is engaged via an active <see cref="LockScope"/>.
@@ -79,7 +82,7 @@ private void ExitCounterScope()
     /// </remarks>
     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.");
         }

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);