From e58b04185401389f62487592786eb21200611f93 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Wed, 22 Feb 2023 11:59:15 -0600 Subject: [PATCH] Remove unsafe `implicit` conversion operators in `AtomicBoolean` and `AtomicReference` (#6429) * removed unsafe conversion operators from `AtomicBoolean` and `AtomicReference` close https://github.com/akkadotnet/akka.net/issues/6392 * api approvals * fixed PhiAccrualFailureDetector * added safe `implicit` operators back --- .../CoreAPISpec.ApproveCore.Core.verified.txt | 2 -- ...oreAPISpec.ApproveCore.DotNet.verified.txt | 2 -- .../CoreAPISpec.ApproveCore.Net.verified.txt | 2 -- .../StressSpec.cs | 2 +- src/core/Akka.Cluster/Cluster.cs | 4 ++-- .../DefaultFailureDetectorRegistry.cs | 6 ++--- .../Akka.Remote/PhiAccrualFailureDetector.cs | 24 +++++++++---------- .../Implementation/Fusing/GraphStages.cs | 6 ++--- src/core/Akka/Actor/CoordinatedShutdown.cs | 2 +- src/core/Akka/Pattern/CircuitBreakerState.cs | 2 +- src/core/Akka/Util/AtomicBoolean.cs | 14 ----------- src/core/Akka/Util/AtomicReference.cs | 13 ---------- 12 files changed, 23 insertions(+), 56 deletions(-) diff --git a/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.Core.verified.txt b/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.Core.verified.txt index 7c207affec3..4d81f52772a 100644 --- a/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.Core.verified.txt +++ b/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.Core.verified.txt @@ -4958,7 +4958,6 @@ namespace Akka.Util public bool CompareAndSet(bool expected, bool newValue) { } public bool GetAndSet(bool newValue) { } public static bool op_Implicit(Akka.Util.AtomicBoolean atomicBoolean) { } - public static Akka.Util.AtomicBoolean op_Implicit(bool value) { } } public class AtomicReference where T : class @@ -4970,7 +4969,6 @@ namespace Akka.Util public bool CompareAndSet(T expected, T newValue) { } public T GetAndSet(T newValue) { } public static T op_Implicit(Akka.Util.AtomicReference atomicReference) { } - public static Akka.Util.AtomicReference op_Implicit(T value) { } } public class static BitArrayHelpers { diff --git a/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.DotNet.verified.txt b/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.DotNet.verified.txt index 7af8acb0957..b454963884f 100644 --- a/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.DotNet.verified.txt +++ b/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.DotNet.verified.txt @@ -4965,7 +4965,6 @@ namespace Akka.Util public bool CompareAndSet(bool expected, bool newValue) { } public bool GetAndSet(bool newValue) { } public static bool op_Implicit(Akka.Util.AtomicBoolean atomicBoolean) { } - public static Akka.Util.AtomicBoolean op_Implicit(bool value) { } } public class AtomicReference where T : class @@ -4977,7 +4976,6 @@ namespace Akka.Util public bool CompareAndSet(T expected, T newValue) { } public T GetAndSet(T newValue) { } public static T op_Implicit(Akka.Util.AtomicReference atomicReference) { } - public static Akka.Util.AtomicReference op_Implicit(T value) { } } public class static BitArrayHelpers { diff --git a/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.Net.verified.txt b/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.Net.verified.txt index 7c207affec3..4d81f52772a 100644 --- a/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.Net.verified.txt +++ b/src/core/Akka.API.Tests/verify/CoreAPISpec.ApproveCore.Net.verified.txt @@ -4958,7 +4958,6 @@ namespace Akka.Util public bool CompareAndSet(bool expected, bool newValue) { } public bool GetAndSet(bool newValue) { } public static bool op_Implicit(Akka.Util.AtomicBoolean atomicBoolean) { } - public static Akka.Util.AtomicBoolean op_Implicit(bool value) { } } public class AtomicReference where T : class @@ -4970,7 +4969,6 @@ namespace Akka.Util public bool CompareAndSet(T expected, T newValue) { } public T GetAndSet(T newValue) { } public static T op_Implicit(Akka.Util.AtomicReference atomicReference) { } - public static Akka.Util.AtomicReference op_Implicit(T value) { } } public class static BitArrayHelpers { diff --git a/src/core/Akka.Cluster.Tests.MultiNode/StressSpec.cs b/src/core/Akka.Cluster.Tests.MultiNode/StressSpec.cs index 2b490e57805..b0885d61a33 100644 --- a/src/core/Akka.Cluster.Tests.MultiNode/StressSpec.cs +++ b/src/core/Akka.Cluster.Tests.MultiNode/StressSpec.cs @@ -458,7 +458,7 @@ public PhiObserver() _log.Warning("Detected phi value of infinity for [{0}] - ", node); var (history, time) = _cluster.FailureDetector.GetFailureDetector(node) switch { - PhiAccrualFailureDetector fd => (fd.state.History, fd.state.TimeStamp), + PhiAccrualFailureDetector fd => (fd.State.History, fd.State.TimeStamp), _ => (HeartbeatHistory.Apply(1), null) }; _log.Warning("PhiValues: (Timestamp={0}, Mean={1}, Variance={2}, StdDeviation={3}, Intervals=[{4}])",time, diff --git a/src/core/Akka.Cluster/Cluster.cs b/src/core/Akka.Cluster/Cluster.cs index d06b48bcac3..0b4e2e26ce8 100644 --- a/src/core/Akka.Cluster/Cluster.cs +++ b/src/core/Akka.Cluster/Cluster.cs @@ -264,7 +264,7 @@ public void Join(Address address) /// Task which completes, once current cluster node reaches state. public Task JoinAsync(Address address, CancellationToken token = default) { - if (_isTerminated) + if (_isTerminated.Value) throw new ClusterJoinFailedException("Cluster has already been terminated"); if (IsUp) @@ -340,7 +340,7 @@ public void JoinSeedNodes(IEnumerable
seedNodes) /// TBD public Task JoinSeedNodesAsync(IEnumerable
seedNodes, CancellationToken token = default) { - if (_isTerminated) + if (_isTerminated.Value) throw new ClusterJoinFailedException("Cluster has already been terminated"); if (IsUp) diff --git a/src/core/Akka.Remote/DefaultFailureDetectorRegistry.cs b/src/core/Akka.Remote/DefaultFailureDetectorRegistry.cs index c486d58c5e4..84e46972f05 100644 --- a/src/core/Akka.Remote/DefaultFailureDetectorRegistry.cs +++ b/src/core/Akka.Remote/DefaultFailureDetectorRegistry.cs @@ -31,14 +31,14 @@ public DefaultFailureDetectorRegistry(Func factory) private readonly Func _factory; - private AtomicReference> _resourceToFailureDetector = new AtomicReference>(ImmutableDictionary.Empty); + private readonly AtomicReference> _resourceToFailureDetector = new AtomicReference>(ImmutableDictionary.Empty); private readonly object _failureDetectorCreationLock = new object(); private ImmutableDictionary ResourceToFailureDetector { - get { return _resourceToFailureDetector; } - set { _resourceToFailureDetector = value; } + get { return _resourceToFailureDetector.Value; } + set { _resourceToFailureDetector.Value = value; } } #endregion diff --git a/src/core/Akka.Remote/PhiAccrualFailureDetector.cs b/src/core/Akka.Remote/PhiAccrualFailureDetector.cs index 8699403a746..ad2a33b1595 100644 --- a/src/core/Akka.Remote/PhiAccrualFailureDetector.cs +++ b/src/core/Akka.Remote/PhiAccrualFailureDetector.cs @@ -69,7 +69,7 @@ public PhiAccrualFailureDetector(double threshold, int maxSampleSize, TimeSpan m _minStdDeviation = minStdDeviation; _acceptableHeartbeatPause = acceptableHeartbeatPause; _firstHeartbeatEstimate = firstHeartbeatEstimate; - state = new State(FirstHeartBeat, null); + _state = new AtomicReference(new AccrualState(FirstHeartBeat, null)); } /// @@ -90,7 +90,7 @@ public PhiAccrualFailureDetector(Config config, EventStream ev) _minStdDeviation = config.GetTimeSpan("min-std-deviation", null); _acceptableHeartbeatPause = config.GetTimeSpan("acceptable-heartbeat-pause", null); _firstHeartbeatEstimate = config.GetTimeSpan("heartbeat-interval", null); - state = new State(FirstHeartBeat, null); + _state = new AtomicReference(new AccrualState(FirstHeartBeat, null)); EventStream = ev ?? Option.None; } @@ -128,14 +128,14 @@ private HeartbeatHistory FirstHeartBeat /// /// Uses volatile memory and immutability for lockless concurrency. /// - internal class State + internal class AccrualState { /// /// TBD /// /// TBD /// TBD - public State(HeartbeatHistory history, long? timeStamp) + public AccrualState(HeartbeatHistory history, long? timeStamp) { TimeStamp = timeStamp; History = history; @@ -152,12 +152,12 @@ public State(HeartbeatHistory history, long? timeStamp) public long? TimeStamp { get; private set; } } - private AtomicReference _state; + private readonly AtomicReference _state; - internal State state + internal AccrualState State { - get { return _state; } - set { _state = value; } + get { return _state.Value; } + set { _state.Value = value; } } /// @@ -173,7 +173,7 @@ public override bool IsAvailable /// public override bool IsMonitoring { - get { return state.TimeStamp.HasValue; } + get { return State.TimeStamp.HasValue; } } /// @@ -182,7 +182,7 @@ public override bool IsMonitoring public override void HeartBeat() { var timestamp = _clock(); - var oldState = state; + var oldState = State; HeartbeatHistory newHistory; if (!oldState.TimeStamp.HasValue) @@ -208,7 +208,7 @@ public override void HeartBeat() else newHistory = oldState.History; } - var newState = new State(newHistory, timestamp); + var newState = new AccrualState(newHistory, timestamp); //if we won the race then update else try again if(!_state.CompareAndSet(oldState, newState)) HeartBeat(); @@ -236,7 +236,7 @@ internal double CurrentPhi /// TBD internal double Phi(long timestamp) { - var oldState = state; + var oldState = State; var oldTimestamp = oldState.TimeStamp; if (!oldTimestamp.HasValue) diff --git a/src/core/Akka.Streams/Implementation/Fusing/GraphStages.cs b/src/core/Akka.Streams/Implementation/Fusing/GraphStages.cs index 010322f6af7..e8cbeda009b 100644 --- a/src/core/Akka.Streams/Implementation/Fusing/GraphStages.cs +++ b/src/core/Akka.Streams/Implementation/Fusing/GraphStages.cs @@ -549,7 +549,7 @@ public override void PreStart() { _cancelCallback.Value = GetAsyncCallback(_ => CompleteStage()); - if (_cancelled) + if (_cancelled.Value) CompleteStage(); else ScheduleRepeatedly("TickTimer", _stage._initialDelay, _stage._interval); @@ -557,7 +557,7 @@ public override void PreStart() protected internal override void OnTimer(object timerKey) { - if (IsAvailable(_stage.Out) && !_cancelled) + if (IsAvailable(_stage.Out) && !_cancelled.Value) Push(_stage.Out, _stage._tick); } @@ -567,7 +567,7 @@ public void Cancel() _cancelCallback.Value?.Invoke(NotUsed.Instance); } - public bool IsCancellationRequested => _cancelled; + public bool IsCancellationRequested => _cancelled.Value; public CancellationToken Token { get; } diff --git a/src/core/Akka/Actor/CoordinatedShutdown.cs b/src/core/Akka/Actor/CoordinatedShutdown.cs index 153f51be4f2..9b291d485bc 100644 --- a/src/core/Akka/Actor/CoordinatedShutdown.cs +++ b/src/core/Akka/Actor/CoordinatedShutdown.cs @@ -333,7 +333,7 @@ public void AddTask(string phase, string taskName, Func> task) /// A task that will be executed during shutdown. internal void AddClrShutdownHook(Func> hook) { - if (!_clrHooksStarted) + if (!_clrHooksStarted.Value) { _clrShutdownTasks.TryAdd(hook); } diff --git a/src/core/Akka/Pattern/CircuitBreakerState.cs b/src/core/Akka/Pattern/CircuitBreakerState.cs index 77994aed743..a9d68080a30 100644 --- a/src/core/Akka/Pattern/CircuitBreakerState.cs +++ b/src/core/Akka/Pattern/CircuitBreakerState.cs @@ -210,7 +210,7 @@ protected override void EnterInternal() /// TBD public override string ToString() { - return string.Format(CultureInfo.InvariantCulture, "Half-Open currently testing call for success = {0}", (_lock == true)); + return string.Format(CultureInfo.InvariantCulture, "Half-Open currently testing call for success = {0}", (_lock.Value == true)); } } diff --git a/src/core/Akka/Util/AtomicBoolean.cs b/src/core/Akka/Util/AtomicBoolean.cs index 042e934e56f..6f0f5d481a3 100644 --- a/src/core/Akka/Util/AtomicBoolean.cs +++ b/src/core/Akka/Util/AtomicBoolean.cs @@ -71,8 +71,6 @@ public bool GetAndSet(bool newValue) return Interlocked.Exchange(ref _value, newValue ? _trueValue : _falseValue) == _trueValue; } - #region Conversion operators - /// /// Performs an implicit conversion from to . /// @@ -82,18 +80,6 @@ public bool GetAndSet(bool newValue) { return atomicBoolean.Value; } - - /// - /// Performs an implicit conversion from to . - /// - /// The boolean to convert - /// The result of the conversion. - public static implicit operator AtomicBoolean(bool value) - { - return new AtomicBoolean(value); - } - - #endregion } } diff --git a/src/core/Akka/Util/AtomicReference.cs b/src/core/Akka/Util/AtomicReference.cs index 0316068f575..841d4b82528 100644 --- a/src/core/Akka/Util/AtomicReference.cs +++ b/src/core/Akka/Util/AtomicReference.cs @@ -75,7 +75,6 @@ public T GetAndSet(T newValue) return Interlocked.Exchange(ref atomicValue, newValue); } - #region Conversion operators /// /// Performs an implicit conversion from to . @@ -86,18 +85,6 @@ public T GetAndSet(T newValue) { return atomicReference.Value; } - - /// - /// Performs an implicit conversion from to . - /// - /// The reference to convert - /// The result of the conversion. - public static implicit operator AtomicReference(T value) - { - return new AtomicReference(value); - } - - #endregion } }