dotnet / orleans

Cloud Native application framework for .NET
https://docs.microsoft.com/dotnet/orleans
MIT License
10.07k stars 2.03k forks source link

Use attribute instead of fixed name to discover DI initialization method #1164

Closed centur closed 6 years ago

centur commented 8 years ago

public class ConfigureStartupBuilder : IStartupBuilder is using reflection and method lookup by name to discover DI configurator method on Startup classes here :

   private static ConfigureServicesBuilder FindConfigureServicesDelegate(Type startupType)
        {
            var servicesMethod = FindMethod(startupType, "ConfigureServices", typeof(IServiceProvider), required: false);

            return servicesMethod == null ? null : new ConfigureServicesBuilder(servicesMethod);
        }

Why not to use some form of special attribute to mark method to call - this will give more space for refactorings and naming conventions and won't break accidentally if the method name will be changed (including scenarios with auto-formatting tools like CodeFormatter and special naming conventions). Attribute looks less fragile for me.

ReubenBond commented 8 years ago

Why an attribute over an interface? We should be able to provide the startup class instance ourselves instead of have Orleans construct it - that's more flexible.

centur commented 8 years ago

Agree, if it's an interface - it'll be even better. BTW we already have an interface for similar task - IBootstrapProvider. Why not to add extra method to it so it can also configure DI ? Actually this is another question - why it was added as a pretty much different concept instead of extending on IBootstrapProvider ?

jason-bragg commented 8 years ago

IBootstrapProvider runs too late in initialization. DI needs to be setup very early.

centur commented 8 years ago

that specific method of bootstrap provider runs late, if we will extent it with other points - they can be run in a different time in pipeline

gabikliot commented 8 years ago

See my suggestion here https://github.com/dotnet/orleans/issues/1110#issuecomment-162098948 to split IBootstrapProvider into multiple phases. We can tap in to that and configure DI from one of those steps.

jason-bragg commented 8 years ago

I am of the opinion DI should not be loaded by providers. It should (eventually) be used to obtain provider implementations, so provider developers can utilize it to acquire provider dependencies.

jason-bragg commented 8 years ago

IMO: DI should be available to application layer logic and framework extensibility points, this includes grain providers, serializers, and log/metrics consumers.

I agree with @ReubenBond that we should define an interface for the extensible DI hooks. Silo configuration should contain a type that conforms to that interface. If no type is configured, a default implementation should be used. DI should be setup early in the silo initialization, prior to provider creation/initialization.