From a14bb8447b88c7caa20842b1b673197278fb2842 Mon Sep 17 00:00:00 2001 From: Gregorius Soedharmo Date: Tue, 11 Jun 2024 21:10:32 +0700 Subject: [PATCH] Fix Akka.Discovery `ServiceDiscovery` instantiation (#7245) --- .../DiscoveryConfigurationSpec.cs | 115 +++++++++++++++++- src/core/Akka.Discovery/Discovery.cs | 74 ++++++++--- 2 files changed, 167 insertions(+), 22 deletions(-) diff --git a/src/core/Akka.Discovery.Tests/DiscoveryConfigurationSpec.cs b/src/core/Akka.Discovery.Tests/DiscoveryConfigurationSpec.cs index 0d84176b68a..7da0104eff4 100644 --- a/src/core/Akka.Discovery.Tests/DiscoveryConfigurationSpec.cs +++ b/src/core/Akka.Discovery.Tests/DiscoveryConfigurationSpec.cs @@ -14,6 +14,7 @@ using Akka.Util; using FluentAssertions; using Xunit; +using static FluentAssertions.FluentActions; namespace Akka.Discovery.Tests { @@ -113,10 +114,10 @@ class = ""{className}"" }} }}").WithFallback(ConfigurationFactory.Load())); - Action discoveryInstance = () => _ = Discovery.Get(sys).Default; - discoveryInstance + Invoking(() => _ = Discovery.Get(sys).Default) .Should().Throw() - .WithInnerException(); + .WithInnerException() + .WithMessage("oh no"); } [Fact] @@ -131,7 +132,79 @@ public void ServiceDiscovery_should_throw_an_argument_exception_for_not_existing method = ""{className}"" }}").WithFallback(ConfigurationFactory.Load())); - Assert.Throws(() => _ = Discovery.Get(sys).Default); + Invoking(() => _ = Discovery.Get(sys).Default).Should() + .Throw() + .WithMessage($"Could not load discovery config from path [akka.discovery.{className}]"); + } + + [Fact(DisplayName = "Discovery should load ServiceDiscovery class with one parameter")] + public void OneParameterTest() + { + using var sys = ActorSystem.Create( + "DiscoveryConfigurationSpec", + ConfigurationFactory.ParseString($@" + akka.discovery {{ + method = akka-mock-inside + akka-mock-inside {{ + class = ""{typeof(TestDiscoveryWithOneParam).TypeQualifiedName()}"" + }} + }}").WithFallback(ConfigurationFactory.Load())); + + var discovery = Discovery.Get(sys).Default; + discovery.Should().BeAssignableTo(); + } + + [Fact(DisplayName = "Discovery should load ServiceDiscovery class with two parameters")] + public void TwoParametersTest() + { + using var sys = ActorSystem.Create( + "DiscoveryConfigurationSpec", + ConfigurationFactory.ParseString($@" + akka.discovery {{ + method = akka-mock-inside + akka-mock-inside {{ + class = ""{typeof(TestDiscoveryWithTwoParam).TypeQualifiedName()}"" + }} + }}").WithFallback(ConfigurationFactory.Load())); + + var discovery = Discovery.Get(sys).Default; + discovery.Should().BeAssignableTo(); + } + + [Fact(DisplayName = "Discovery should throw if ServiceDiscovery class has invalid parameters")] + public void IllegalParametersTest() + { + using var sys = ActorSystem.Create( + "DiscoveryConfigurationSpec", + ConfigurationFactory.ParseString($@" + akka.discovery {{ + method = akka-mock-inside + akka-mock-inside {{ + class = ""{typeof(TestDiscoveryWithIllegalParam).TypeQualifiedName()}"" + }} + }}").WithFallback(ConfigurationFactory.Load())); + + Invoking(() => _ = Discovery.Get(sys).Default).Should() + .Throw() + .WithMessage("Illegal akka.discovery.akka-mock-inside.class value or incompatible class!*"); + } + + [Fact(DisplayName = "Discovery should throw if ServiceDiscovery class does not have any public constructor")] + public void IllegalConstructorTest() + { + using var sys = ActorSystem.Create( + "DiscoveryConfigurationSpec", + ConfigurationFactory.ParseString($@" + akka.discovery {{ + method = akka-mock-inside + akka-mock-inside {{ + class = ""{typeof(TestDiscoveryWithIllegalCtor).TypeQualifiedName()}"" + }} + }}").WithFallback(ConfigurationFactory.Load())); + + Invoking(() => _ = Discovery.Get(sys).Default).Should() + .Throw() + .WithMessage("Illegal akka.discovery.akka-mock-inside.class value or incompatible class!*"); } } @@ -144,6 +217,40 @@ internal class FakeTestDiscovery : ServiceDiscovery internal class FakeTestDiscovery2 : FakeTestDiscovery { } + internal class TestDiscoveryWithOneParam : ServiceDiscovery + { + public TestDiscoveryWithOneParam(ExtendedActorSystem sys){ } + + public override Task Lookup(Lookup lookup, TimeSpan resolveTimeout)=> + Task.FromResult((Resolved)null); + } + + internal class TestDiscoveryWithTwoParam : ServiceDiscovery + { + public TestDiscoveryWithTwoParam(ExtendedActorSystem sys, Configuration.Config cfg){ } + + public override Task Lookup(Lookup lookup, TimeSpan resolveTimeout)=> + Task.FromResult((Resolved)null); + } + + internal class TestDiscoveryWithIllegalParam : ServiceDiscovery + { + public TestDiscoveryWithIllegalParam(Configuration.Config cfg){ } + + public override Task Lookup(Lookup lookup, TimeSpan resolveTimeout)=> + Task.FromResult((Resolved)null); + } + + internal class TestDiscoveryWithIllegalCtor : ServiceDiscovery + { + private TestDiscoveryWithIllegalCtor(){ } + private TestDiscoveryWithIllegalCtor(ExtendedActorSystem sys){ } + private TestDiscoveryWithIllegalCtor(ExtendedActorSystem sys, Configuration.Config cfg){ } + + public override Task Lookup(Lookup lookup, TimeSpan resolveTimeout)=> + Task.FromResult((Resolved)null); + } + internal class DiscoveryException : Exception { public DiscoveryException() diff --git a/src/core/Akka.Discovery/Discovery.cs b/src/core/Akka.Discovery/Discovery.cs index fceba8f59d5..b6849765342 100644 --- a/src/core/Akka.Discovery/Discovery.cs +++ b/src/core/Akka.Discovery/Discovery.cs @@ -11,7 +11,7 @@ using Akka.Annotations; using Akka.Configuration; using Akka.Event; -using Akka.Util; +using BindingFlags = System.Reflection.BindingFlags; namespace Akka.Discovery { @@ -71,27 +71,65 @@ private ServiceDiscovery CreateServiceDiscovery(string method) if(!config.HasPath("class")) throw new ArgumentException($"akka.discovery.{method} must contain field `class` that is a FQN of an `Akka.Discovery.ServiceDiscovery` implementation"); - Try Create(string typeName) - { - var dynamic = DynamicAccess.CreateInstanceFor(typeName, _system); - return dynamic.RecoverWith(ex => ex is TypeLoadException or MissingMethodException - ? DynamicAccess.CreateInstanceFor(typeName) - : dynamic); - } - var className = config.GetString("class"); _log.Info($"Starting Discovery service using [{method}] method, class: [{className}]"); - var instanceTry = Create(className); - return instanceTry.IsSuccess switch + try + { + return Create(className); + } + catch (Exception ex) + { + if (ex is TypeLoadException or MissingMethodException) + throw new ArgumentException( + message: $"Illegal akka.discovery.{method}.class value or incompatible class!\n" + + "The implementation class MUST extend Akka.Discovery.ServiceDiscovery with:\n" + + " * parameterless constructor, " + + $" * constructor with a single {nameof(ExtendedActorSystem)} parameter, or\n" + + $" * constructor with {nameof(ExtendedActorSystem)} and {nameof(Configuration.Config)} parameters.", + paramName: nameof(method), + innerException: ex); + throw; + } + + ServiceDiscovery Create(string typeName) { - true => instanceTry.Get(), - false when instanceTry.Failure.Value is TypeLoadException or MissingMethodException => - throw new ArgumentException(nameof(method), $"Illegal akka.discovery.{method}.class value or incompatible class! \n" + - "The implementation class MUST extend Akka.Discovery.ServiceDiscovery and take an \n" + - "ExtendedActorSystem as constructor argument."), - _ => throw instanceTry.Failure.Value - }; + var type = Type.GetType(typeName: typeName); + if (type is null || !typeof(ServiceDiscovery).IsAssignableFrom(type)) + throw new TypeLoadException(); + + var bindFlags = BindingFlags.Instance | BindingFlags.Public; + + var ctor = type.GetConstructor( + bindingAttr: bindFlags, + binder: null, + types: new[] + { + typeof(ExtendedActorSystem), + typeof(Configuration.Config) + }, + modifiers: null); + if (ctor is not null) + return (ServiceDiscovery) Activator.CreateInstance(type, _system, config); + + ctor = type.GetConstructor( + bindingAttr: bindFlags, + binder: null, + types: new[] { typeof(ExtendedActorSystem) }, + modifiers: null); + if (ctor is not null) + return (ServiceDiscovery) Activator.CreateInstance(type, _system); + + ctor = type.GetConstructor( + bindingAttr: bindFlags, + binder: null, + types: Array.Empty(), + modifiers: null); + if (ctor is null) + throw new MissingMethodException(); + + return (ServiceDiscovery) Activator.CreateInstance(type); + } } public static Discovery Get(ActorSystem system) => system.WithExtension();