dotnet / orleans

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

Orleans DI - Use startup object instead of startup class #3157

Closed nehmebilal closed 6 years ago

nehmebilal commented 7 years ago

Hi guys,

I am starting to play with Orleans DI and finding myself in need to override some dependencies that are setup in the StartUp class for testing.

With ASP.NET, I can do the following:

        public void StartService(IStartup startup)
        {
            var builder = new WebHostBuilder()
                .UseContentRoot(Directory.GetCurrentDirectory())
                .UseUrls(url)
                .ConfigureServices(services =>
                {
                    // this instance of IStartup below will be used as a Startup class
                    services.AddSingleton(startup);
                });

            // start the web server
        }

This allows me to pre-configure my startup object with dependencies that are already instantiated in my test (e.g. I want to replace IStorage with InMemoryStorage which I am sharing with my test code).

Is there a way to achieve this with Orleans DI?

Note: I do definitely not want to add static members to my Startup class for pre-configuring dependencies :)

nehmebilal commented 7 years ago

Another alternative would be to provide dependency overrides API as follows:

            var overridesCollection = new ServiceCollection();
            overridesCollection.AddSingleton(inMemoryStorage); // used for testing

            siloHost = new SiloHost(Dns.GetHostName());
            siloHost.LoadOrleansConfig();
            siloHost.Config.UseStartupType<StartUp>();

            // set the dependency overrides
            siloHost.Config.DependencyOverrides = overridesCollection;

            siloHost.InitializeOrleansSilo();

In this case, SiloHost will still call the StartUp class first and then apply the dependency overrides (if any).

jdom commented 7 years ago

Hi @nehmebilal, thanks for the feedback. We were considering extending the Startup functionality as defined in #2936 to align a lot closer with ASP.NET. After syncing with them, it seems like for the generic host builder that they would like to write (separate from ASP.NET's one) they are actually moving away from the Startup type entirely, and prefer to just have a way to register services by configuring them directly in the host builder. Having said that, this is indeed a scenario that we want to cover, but probably through a different approach, and likely by dropping the Startup type too, since the whole idea is to align with them so that we don't have to design, develop, maintain and evolve a completely new startup and extensibility framework.

An additional concern separate from the way you configure stuff, is that you mentioned you wanted to share instances with the running test, but remember that the silos run in separate AppDomains, so it can be very tricky to share these, even if you use MarshalByRefObject proxies.

nehmebilal commented 7 years ago

An additional concern separate from the way you configure stuff, is that you mentioned you wanted to share instances with the running test, but remember that the silos run in separate AppDomains, so it can be very tricky to share these, even if you use MarshalByRefObject proxies.

Hey @jdom. Yes, I remembered that later on 😄

+1 For dropping StartUp class entirely, configuring services directly gives a lot more flexibility.

Btw, the AppDomains limitation is temporary right? Once the .Net Core version is ready it would be possible to start an Orleans silo in process without the need for a separate AppDomain?

jdom commented 7 years ago

Btw, the AppDomains limitation is temporary right? Once the .Net Core version is ready it would be possible to start an Orleans silo in process without the need for a separate AppDomain?

yes, hopefully, although there is some more work needed to get the silos to not rely on statics too. It's almost there, but not quite.

sergeybykov commented 6 years ago

Closing as this seems resolved.