New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pass Akka.Cluster.Cluster
into IDowningProvider
directly
#5965
pass Akka.Cluster.Cluster
into IDowningProvider
directly
#5965
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the API changes, why they're needed, and why I believe they are safe.
@@ -272,7 +272,7 @@ namespace Akka.Cluster | |||
} | |||
public sealed class SplitBrainResolver : Akka.Cluster.IDowningProvider | |||
{ | |||
public SplitBrainResolver(Akka.Actor.ActorSystem system) { } | |||
public SplitBrainResolver(Akka.Actor.ActorSystem system, Akka.Cluster.Cluster cluster) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking API change, by design - forces the Cluster
to be passed into the SBR directly rather than resolved via Cluster.Get
, which is what triggers this issue.
@@ -25,12 +27,13 @@ public class StartupWithOneThreadSpec : AkkaSpec | |||
akka.actor.default-dispatcher.dedicated-thread-pool.thread-count = 1 | |||
akka.actor.provider = ""Akka.Cluster.ClusterActorRefProvider, Akka.Cluster"" | |||
akka.remote.dot-netty.tcp.port = 0 | |||
akka.cluster.downing-provider-class = ""Akka.Cluster.SBR.SplitBrainResolverProvider, Akka.Cluster"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added SBR to the single thread startup spec to see if the deadletter'd TimerMsg
s would show up here.
// perform a self-join | ||
var cts = new CancellationTokenSource(TimeSpan.FromSeconds((3))); | ||
var selfAddress = cluster.SelfAddress; | ||
await cluster.JoinSeedNodesAsync(new[] { selfAddress }, cts.Token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Force actual cluster formation to complete successfully as part of spec.
@@ -132,7 +132,7 @@ public Cluster(ActorSystemImpl system) | |||
Scheduler = CreateScheduler(system); | |||
|
|||
// it has to be lazy - otherwise if downing provider will init a cluster itself, it will deadlock | |||
_downingProvider = new Lazy<IDowningProvider>(() => Akka.Cluster.DowningProvider.Load(Settings.DowningProviderType, system), LazyThreadSafetyMode.ExecutionAndPublication); | |||
_downingProvider = new Lazy<IDowningProvider>(() => Akka.Cluster.DowningProvider.Load(Settings.DowningProviderType, system, this), LazyThreadSafetyMode.ExecutionAndPublication); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where Cluster
is passed to the IDowningProvider
@@ -60,7 +60,9 @@ akka { | |||
# * if it is set to a duration the `AutoDowning` provider is with the configured downing duration | |||
# | |||
# If specified the value must be the fully qualified class name of a subclass of | |||
# `akka.cluster.DowningProvider` having a public one argument constructor accepting an `ActorSystem` | |||
# `akka.cluster.DowningProvider` having two argument constructor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documented API changes, in case a user has a custom IDowningProvider
implementation. I have never seen one in the wild, which is why I think this breaking API change is probably safe for a v1.4.39 release.
{ | ||
var extendedSystem = system as ExtendedActorSystem; | ||
try | ||
{ | ||
return (IDowningProvider)Activator.CreateInstance(downingProviderType, extendedSystem); | ||
return (IDowningProvider)Activator.CreateInstance(downingProviderType, extendedSystem, cluster); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass this into the new constructor via reflection - this line will throw and fail the Akka.Cluster startup sequence for any IDowningProvider
implementations that don't have a constructor with both an ActorSystem
parameter and a second Akka.Cluster.Cluster
parameter.
@@ -27,22 +27,22 @@ internal class SplitBrainResolver : SplitBrainResolverBase | |||
{ | |||
private Cluster _cluster; | |||
|
|||
public SplitBrainResolver(TimeSpan stableAfter, DowningStrategy strategy) | |||
public SplitBrainResolver(TimeSpan stableAfter, DowningStrategy strategy, Cluster cluster) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modern SBR updates.
|
||
public SplitBrainResolver(ActorSystem system) | ||
public SplitBrainResolver(ActorSystem system, Cluster cluster) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legacy SBR updates.
{ | ||
if (strategy == null) throw new ArgumentNullException(nameof(strategy)); | ||
|
||
_stabilityTimeout = stableAfter; | ||
_strategy = strategy; | ||
_cluster = Cluster.Get(Context.System); | ||
_cluster = cluster; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bugfix 1.
} | ||
|
||
// re-subscribe when restart | ||
protected override void PreStart() | ||
{ | ||
_cluster = Cluster.Get(Context.System); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bugfix 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…Provider directly
close #5962
Fixes #5962
Changes
Passes a
Cluster
instance directly into the SBR constructor in order to avoid same-thread issues withLazy<Cluster>
.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
ChannelTaskScheduler
to work inside Akka.Cluster without causing errors inside/system
actors #5861