Skip to content
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

Merged
merged 8 commits into from May 26, 2022

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented May 26, 2022

close #5962

Fixes #5962

Changes

Passes a Cluster instance directly into the SBR constructor in order to avoid same-thread issues with Lazy<Cluster>.

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

@Aaronontheweb Aaronontheweb added bug-reproduction akka-cluster api-change and removed bug-reproduction labels May 26, 2022
Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

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) { }
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

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""
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

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 TimerMsgs 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);
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

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);
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

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:
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

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);
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

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)
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

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)
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

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;
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

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);
Copy link
Member Author

@Aaronontheweb Aaronontheweb May 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bugfix 2.

Copy link
Contributor

@Arkatufus Arkatufus left a comment

LGTM

@Arkatufus Arkatufus merged commit 9260924 into akkadotnet:v1.4 May 26, 2022
13 checks passed
@Aaronontheweb Aaronontheweb deleted the fix-5962-SBR-startup branch May 26, 2022
Arkatufus added a commit to Arkatufus/akka.net that referenced this issue May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants