dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

DI : add support to eager load a singleton service #43149

Closed ah1508 closed 1 year ago

ah1508 commented 4 years ago

This adds a mechanism to register services to be activated during startup of a host instead of on first access.

It leverages the new IHostedLifecycleService.StartingAsync() in https://github.com/dotnet/runtime/issues/86511 to do this as early as possible, although for backwards compat it can also hook into the IHostedService.StartAsync() if the new StartingAsync() was not called.

It supports both a simple way to activate a service by type and an advanced case that calls an async callback which is encapsulated in the host's IHost.StartAsync().

It is located in the hosting abstractions assembly and requires no additional work for hosts other than to support IHostedService and optionally the new IHostedLifecycleService interface.

API Proposal

In the hosting abstractions assembly:

namespace Microsoft.Extensions.DependencyInjection;

public static partial class ServiceCollectionHostedServiceExtensions
{
    // Simply activates the service by type
+    public static IServiceCollection AddStartupActivation<TService>
+        (this IServiceCollection services) where TService : class;

    // Support the new keyed services feature
+    public static IServiceCollection AddKeyedStartupActivation<TService>
+        (this IServiceCollection services, object? serviceKey) where TService : class;

    // Activates the service by type and calls an async method that allows access to the service
+    public static IServiceCollection AddStartupActivation<TService>
+        (this IServiceCollection services,
+        Func<IServiceProvider, TService, CancellationToken, Task> activatorFunc)
+        where TService : class;

    // Support the new keyed services feature
+    public static IServiceCollection AddKeyedStartupActivation<TService>
+        (this IServiceCollection services, object? serviceKey,
+        Func<IServiceProvider, TService, CancellationToken, Task> activatorFunc)
+        where TService : class;
}

Usage examples

[Fact]
public async void ActivateByType()
{
    var hostBuilder = CreateHostBuilder(services =>
    {
        services
            .AddSingleton<ActivateByType_Impl>()
            .AddStartupActivation<ActivateByType_Impl>();
    });

    using (IHost host = hostBuilder.Build())
    {
        await host.StartAsync();
        Assert.True(ActivateByType_Impl.Activated);
    }
}

public class ActivateByType_Impl
{
    public static bool Activated = false;
    public ActivateByType_Impl()
    {
        Activated = true;
    }
}

[Fact]
public async void ActivateByTypeWithCallback()
{
    var hostBuilder = CreateHostBuilder(services =>
    {
        services
            .AddSingleton<ActivateByTypeWithCallback_Impl>()
            .AddStartupActivation<ActivateByTypeWithCallback_Impl>(async (service, sp, cancellationToken) =>
        {
            await service.DoSomethingAsync(cancellationToken);
        });
    });

    using (IHost host = hostBuilder.Build())
    {
        await host.StartAsync();
        Assert.True(ActivateByTypeWithCallback_Impl.Activated);
        Assert.True(ActivateByTypeWithCallback_Impl.DidSomethingAsync_Before);
        Assert.True(ActivateByTypeWithCallback_Impl.DidSomethingAsync_After);
    }
}

public class ActivateByTypeWithCallback_Impl
{
    public static bool Activated = false;
    public static bool DidSomethingAsync_Before = false;
    public static bool DidSomethingAsync_After = false;

    public ActivateByTypeWithCallback_Impl()
    {
        Activated = true;
    }

    public async Task DoSomethingAsync(CancellationToken cancellationToken)
    {
        DidSomethingAsync_Before = true;
        await Task.Delay(10, cancellationToken);
        DidSomethingAsync_After = true;
    }
}

Prototype at https://github.com/steveharter/runtime/tree/SingletonWarmup.

Original issue description

AB#1244417 Until now a service is created when GetService method is called on the service provider, or when the service must be injected in a controller responsible for the coming http request.

A first limitation : a service whose instance is never injected or never asked (GetService) will be never instantiated. If this service do some init work (fill a memory cache for instance), this work will never be done.

Even if this service is required somewhere, we should have the option to force eager creation of the instance (when BuildServiceProvider method is called) and not when the service is required for the first time. startup != first use.

Second limitation : if the init work takes 3 seconds, I will have the the "cold start" problem.

It also has consequences on the context in which the initialization work his done. Example :

public class Foo : IFoo 
{
    public Foo() 
    {
        DoSomeInitWork();
    }

    private void DoSomeInitWork()
    {
        // code
    }
}

Somewhere in my application :

using(var tx = new TransactionScope())
{
    IFoo foo = serviceProvider.GetService<IFoo>();
    // use foo
}

Here, Foo will be instantiated inside the transaction. Let's say I cannot receive Foo by injection because the service I need must be obtained dynamically.

In FooTest :

public class FooTest
{
    private IFoo foo;

    public FooTest()
    {
       // service provider initialization
       this.foo = serviceProvider.GetService<IFoo>();
    }
}

Here, Foo will be instantiated outside of the transaction.

If the DoSomeInitWork() method requires a transaction, it should first check if a transaction is ongoing before creating one.

Suggestion : add a property in the ServiceDescriptor class and a parameter for the AddSingleton méthods.

BrennanConroy commented 4 years ago

There are a couple ways to do this. If you're developing the app then you can resolve the service from the constructed service provider manually in Configure() or inject the service in the Configure() method. If you're a library developer you can do this by injecting a IStartupFilter during your features registration, and the filter can resolve the service in its constructor.

ghost commented 4 years ago

Tagging subscribers to this area: @eerhardt, @maryamariyan See info in area-owners.md if you want to be subscribed.

pinkfloydx33 commented 4 years ago

when BuildServiceProvider method is called

That's potentially dangerous. While there's an analyzer that warns about incorrectly calling BuildServiceProvider just to grab some intermediate values, many library authors, including Microsoft currently follow this pattern.

Eager loading of a dependency this way could end up being performed multiple times without you, the user, being aware and then it becoming difficult to track down.

ah1508 commented 4 years ago

Dear all,

Using a IStartupFilter does not respect the "separation of concern" best practice because the ASP.NET app would be responsible for triggering the creation of an object it does not even need. And creating a IStartupFilter implementation is not enough, it also has to be registered as a service. Too much responsibility for the asp.net layer.

In my case, the DoSomeInitWork method is in the data access layer, and fills a second level cache for Nhibernate. But it could something else, it is a very generic case. In my example the data access layer cannot contain a IStartupFilter implementation because it is a classlib which has no relation to asp.net.

And the IStartupFilter would not work in unit tests.

How about adding a boolean eagerLoading in the ServiceDescriptor ? Then , when BuildServiceProvider is called, services registered with eagerLoading would be created (just by calling GetService).

@pinkfloydx33 : Calling BuildServiceProvider several times for the same service collection seems an antipattern to me as well, but to prevent several eager loadings how about adding a "first call" flag in the service collection (false only if BuildServiceProvider has not been called yet). Then multiple calls to BuildServiceProvider would not trigger more than one eager service creation.

davidfowl commented 4 years ago

We already have an options object, and that's where something like this would like this would go. We already have prior art here with various validations (scoped and a dependency check). Eager loading would fit here as well and isn't a bad idea.

There would of course be caveats as this would only work for closed generics and non-generic singletons. Today we do a similar validation but don't actually resolve the service because that could have side effects.

ah1508 commented 3 years ago

@davidfowl : are you talking about the ServiceProviderOptions object ?

Indeed, it would be interesting to enable eager loading for all services at once but I think it should come in addition to a setting at the ServiceDescriptor level.

davidfowl commented 3 years ago

There shouldn't a per service descriptor setting. Object graphs aren't per descriptor but that's not the only problem. Changes to the service descriptor require other container authors to react to any new feature service descriptors have. This change is eagerly resolving the dependency graph and I think something like this should be a bool on the ServiceProviderOptions

dazinator commented 3 years ago

Im missing something. Why not:

var instance = new Foo();
services.AddSingleton(instance);

If its because you want the container to own the instance then this comes back to having an isExternallyOwned (or equivalent) option:

services.AddSingleton(instance, isExternallyOwned: false)

The use case mentions:

If this service do some init work (fill a memory cache for instance), this work will never be done.

Why would you want init work - that could quite possibly be async, to be done in the construction of a dependency like this? Isn't the better pattern to:-

  1. Resolve the dependency synchronously at the point you need it in the application (or better still to have it injected where needed)
  2. Invoke some method on it like myservice.PrimeCacheAsync()

That way this work isn't within the constructor of the dependency. This is important because if I am consuming a dependency its not great if there are unseen side effects due to async work running in the constructor as part of the resolution.

This removes the need to create the instance ahead of time, but puts the emphasis back on having the right place to do any async init work after the container is built and before serving http requests I.e we need the opportunity to do this async init work:

var sp = BuildServiceProvider();
await DoInitWorkAsybc(sp);
StartAcceptingHttpRequests();

The hosting model could provide a hook that it runs after building the container and before serving requests, where you could do initialisation logic like this.

public class AppInitialise : IAsyncInit
{
  private MyCache _cache;
  public AppInitialise(MyCache cache)
  {
    _cache = cache;
  }
   Task async InitialiseAsync()
  {
      // do your async init work.
       _cache.PrimeAsync();
  }
}

services.AddAsyncInit<AppInitialise>();

Another way to think about that is to add an extension method for IServiceCollection called BuildAndInitaliseAsync() That first builds the container but then resolves and invokes any registered IAsyncInit services. Or even just an extension method on IServiceProvider called `InitializeAsync' that does the same. The beauty of this is that it wouldn't require any changes by DI container authors, but async init logic wouldn't run unless something resolves and executes those services. You can't to that as part of BuildServiceProvider because it's synchronous and its reasonable to expect that init logic would want to be async.

ah1508 commented 3 years ago

@dazinator : in my example, the method responsible for cache loading is in the data access layer of a classlib and is not aware of aspnet. And the aspnet application that uses this classlib is not aware of this cache loading mechanism. It is nothing more that separation of concern between data access, business logic and web tier. The classlib can also be called from a test project.

And the class that contains this cache loading mechanism may be not injected anywhere and has no reason to be resolved since no other service needs it. It just exists in the services collection and must be initialized when application starts (i.e. when BuildServiceProvider is called) to have a chance to do the init work. Cache loading is just an example of a common "startup task" use case.

If a BuildAndInitaliseAsync method exists, who calls it ? With ASP.NET, the developer is not responsible for calling BuildServiceProvider.

Doing the init stuff in the constructor is not ideal but for now there is no other option. Non blocking execution can be achieved by not awaiting the task (Task.Run in the constructor). However, in this case a failure will not stop application start.

Implementing a IAsyncInit interface would be interesting. Custom attribute would be ok as well ([OnInit] or [PostConstruct] for instance) and would bring more flexibility regarding the method signature.

But from what has been written here, the eager loading would not be enabled per service but only globally through ServiceProviderOptions :

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.UseStartup<Startup>();
        }).UseDefaultServiceProvider(serviceProviderOptions => {
            // code that enable eager loading
        });

or from a test method

IServiceProvider sp = new ServiceCollection()
// code
.BuildServiceProvider(new ServiceProviderOptions {/*code that enable eager loading*/});
Timovzl commented 3 years ago

@ah1508 as an alternative, would using an IHostedService solve your problem?

Currently, whenever I have a dependency that should perform startup work, I register a custom IHostedService for it.

internal class CacheStartupHostedService
{
    public async Task StartAsync(CancellationToken cancellationToken)
    {
        // Note that CancellationToken indicates that startup is being aborted, NOT application shutdown

        // Snip: Perform startup work

        // Exceptions bubbling up from here prevent application startup (since Core 3.0)
    }

    public Task StopAsync(CancellationToken cancellationToken)
    {
        // Note that CancellationToken indicates that shutdown is taking too long

        return Task.CompletedTask;
    }
}

We can then register the custom hosted service, with complete control of its lifetime and construction:

// By type
services.AddSingleton<IHostedService, CacheStartupHostedService>();

// Or manually constructed on-demand
services.AddSingleton<IHostedService>(serviceProvider =>
    new CacheStartupHostedService(serviceProvider.GetRequiredService<ISubdependency>()));

// Or semi-automatically constructed on-demand
services.AddSingleton<IHostedService>(serviceProvider =>
    ActivatorUtilities.CreateInstance<CacheStartupHostedService>(serviceProvider, someConfigValue));

This solution provides a cleaner place to perform work compared to a constructor, and it supports async.

The approach works well for .NET Core 3.0 and up (if I remember correctly), as of which the following has been true:

dazinator commented 3 years ago

@Timovzl @ah1508 For one time initialisation tasks, here is another method that I use which is a little more straight to the point than using IHostedServices imho (if you don't need something continuously running):

  public class Program
    {
        public static async Task Main(string[] args)
        {
            var host = CreateHostBuilder(args).Build();
            await MigrateDatabaseAsync(host.Services);
            await host.RunAsync();
        }

        private static async Task MigrateDatabaseAsync(IServiceProvider services)
        {         
            using (var scope = services.CreateScope())
            {
                var dbContext = scope.ServiceProvider.GetRequiredService<Data.DbContext>();
                await dbContext.Database.MigrateAsync();
            }
        }

Just seperate out the Build() and the Run() of the host and add your async initialisation stuff in between so you can be sure its done before the host starts accepting http traffic. Ofcouse you don't have to await the task if you don't want to delay the host accepting traffic, and you design your services accordingly.

I appreciate this isn't the ideal you are looking for with all containers supporting a new async init paradigm but its going to be flexible and simple, and could be made into an extension method with your own paradigm at play for discovering and initialising such services using whatever scheme you want.

ah1508 commented 3 years ago

Dear @Timovzl , indeed, IHostedService is a possible workaround since it does not depend on asp.net. However it may require a double registration :

public interface IFoo
{
    void Bar();
    void Baz();
}

public class Foo : IFoo, IHostedService
{
    void IFoo.Bar() {/*code*/ }

    void IFoo.Baz() {/*code*/ }

    Task IHostedService.StartAsync(CancellationToken cancellationToken)
    {
        // load cache
        return Task.CompletedTask;
    }

    Task IHostedService.StopAsync(CancellationToken cancellationToken)
    {
        return Task.CompletedTask;
    }
}

services registration :

serviceCollection.AddSingleton<IFoo, Foo>();
serviceCollection.AddHostedService<Foo>(sp => sp.GetRequiredService<IFoo>() as Foo);

And from unit test, must be called explicitely :

IServiceProvider = new ServiceCollection()
// Add...
.BuildServiceProvider();
serviceProvider.GetServices<IHostedService>().ToList().ForEach(x=>x.StartAsync(new()));

Too bad that integration between DependencyInjection and Unit test is non existent.

Until now it is the best we can do, thanks !!

Timovzl commented 3 years ago

@ah1508 Interesting point.

I would consider startup logic not a part of unit tests by default. After all, these each test a unit, and should avoid other logic as much as possible.

By contrast, integration tests would certainly run such startup logic. For precisely such reasons, I tend to perform integration tests by constructing a HostBuilder, configuring the services exactly as the application would, and starting the resulting IHost.

Anyway, if you want to run the logic even for unit tests, and you truly want to keep it as startup logic, you might choose to put that logic in a constructor after all. In your example, that would be Foo's constructor. Have StartAsync and StopAsync simply return Task.CompletedTask. In your example, Foo is the IHostedService, in which case you are done, since a type registered as IHostedService gets constructed on startup. (If you had separated the IHostedService from Foo, the IHostedService should inject IFoo to ensure its construction as well.)

ah1508 commented 3 years ago

Hi @Timovzl , of course init tasks are not important for unit test where business logic dependencies are mocked. But you may also want to test your service layer (classlib) with its data access layer and only mock the database. The web tier is then not involved but the application should behaves like it will in production. So if performance is bad because cache is empty it is difficult to say "don't worry, once hosted the performance will be better because the StartAsync method will be called".

The following extension method hides the activation of IHostedService :

public static IServiceProvider BuildServiceProvider(this IServiceCollection sc, ServiceProviderOptions options, bool activateHostedServices)
{
    IServiceProvider sp = sc.BuildServiceProvider(options);
    if(activateHostedServices)
    {
        sp.GetServices<IHostedService>().ToList().ForEach(s => s.StartAsync(new()));
    }
    return sp;
}

From test :

IServiceProvider serviceProvider = new ServiceCollection()
// Add
.BuildServiceProvider(new(), true);

For a buit-in support, a new property could be added to the ServiceProviderOptions class, so the code would be :

IServiceProvider serviceProvider = new ServiceCollection()
// Add
.BuildServiceProvider(new() {ActivateHostedServices = true});
Timovzl commented 3 years ago

@ah1508 The following might be a "less custom" way to achieve that:

var hostBuilder = new HostBuilder();
hostBuilder.ConfigureServices(services =>
    services.AddMyStuff()); // Or BuildServiceProvider(...), as you have named it
using var host = hostBuilder.Build();
await host.StartAsync();

The scenario you describe is what I would call an integration test: it tests code in context, i.e. as it integrates with other code.

For integration tests, it makes perfect sense to me to use and start an IHost. Doing so implies starting every potential IHostedService, which happens to be precisely what you want.

Any background service that we might not want to start - say, a periodic archiving process - we would simply mock away, by overwriting the dependency with a mock instance, in ConfigureServices().

steveharter commented 1 year ago

Note the original description was updated to include an API proposal:

rafal-mz commented 1 year ago
[Fact]
public async void ActivateByType()
{
    var hostBuilder = CreateHostBuilder(services =>
    {
        services
            .AddSingleton<ActivateByType_Impl>()
            .AddStartupActivation<ActivateByType_Impl>();
    });

    using (IHost host = hostBuilder.Build())
    {
        await host.StartAsync();
        Assert.True(ActivateByType_Impl.Activated);
    }
}

Do we need to have two calls? I think that you need to know that you will want to activate your class eagerly at the design time, so I don't see a reason for separation.

I would expect something like:

services.AddActivatedSingleton<TImplementation>(...)

Personally, I don't really like the word Activation, since it is not very precise about what does it mean to activate a class. I would call it so, it points directly to the fact that instance will be created eagerly not lazily. Maybe something like AddStartupCreatedSingleton<TImplementation>?

IMHO adding async overload for factory is turbo strange, since there is no parity for this functionality for other lifetimes. I would consider adding async overloads for DI constructors when we are ready to 'fully' support async ctors, which probably mean async service lifetime, and async scoped and transient stuff.

namespace Microsoft.Extensions.DependencyInjection;

public static partial class StartupCreatedServiceCollectionExtensions
{
+    public static IServiceCollection AddStartupCreatedSingleton<TService>
+        (this IServiceCollection services) where TService : class;

+    public static IServiceCollection TryAddStartupCreatedSingleton<TService>
+        (this IServiceCollection services) where TService : class;

+    public static IServiceCollection AddStartupCreatedTransient<TService>
+        (this IServiceCollection services) where TService : class;

+    public static IServiceCollection AddKeyedStartupActivatedSingleton<TService>
+        (this IServiceCollection services, object? serviceKey) where TService : class;
Timovzl commented 1 year ago

From the perspective of not introducing too many ways to do the same thing, I'm still curious what the proposal truly adds that isn't already perfectly available.

// We can implement IHostedService
services
   .AddSingleton<MyService>()
   .AddSingleton<IHostedService>(serviceProvider => serviceProvider.GetRequiredService<MyService>());

// In the rare case where we cannot or should not change MyService to implement IHostedService,
// we can delegate that responsibility to an accompanying starter class
services
   .AddSingleton<MyService>()
   .AddSingleton<IHostedService, MyServiceStarter>();

I believe the proposed API introduces a non-negligible amount of confusion, in return for features that we already have a clean and concise API for.

What's more, the examples I've shown here also work as expected when we use assembly scanning (such as provided by Scrutor). This allows us to omit these manual registrations altogether. The proposed API, on the other hand, encourages manual and customized registrations.

I dread a scenario where half the services are started because they implement IHostedService and the other half because the registration says so.

ah1508 commented 1 year ago

@Timovzl : I see two problem with IHostedService :

1: the class must implement this interface.

But after all why not, even if it requires a requires a Nuget package.

2: this code is not intuitive:

services
   .AddSingleton<MyService>()
   .AddSingleton<IHostedService>(serviceProvider => serviceProvider.GetRequiredService<MyService>());

It is even worse if MyService must be visible as IMyService the declaration in the service collection looks like that:

services
   .AddSingleton<IMyService, MyService>()
   .AddSingleton<IHostedService>(serviceProvider => serviceProvider.GetRequiredService<IMyService>() as MyService);

Hack or best practice ? Maybe the second registration could be built-in if implementation type implements IHostedService

Timovzl commented 1 year ago

Declaring a type both as itself (or its representative interface) and as IHostedService by requesting the former, I would consider best practice.

I agree that the way to do it is not obvious. One has to learn that, for scoped or singleton, registering one thing under multiple types requires some care rather than two naive, unrelated registrations. I'd love to see a way to express that more neatly. Admittedly, it's tough to come up with a sensible overload to do so. AddSingleton<T1, T2> already means that T2 is to be constructed, not resolved from the service provider, so we can't use that.

On a practical note, in application code, I stick to assembly scanning as much as possible:

// Using Scrutor
services.Scan(scanner => scanner
    .FromAssemblies(this.GetType().Assembly)
    .AddClasses(c => c.Where(type => /* Snip: constraints */), publicOnly: false) // Services only
    .AsSelfWithInterfaces()
    .WithSingletonLifetime());

This takes correctly registering a type under multiple types out of our hands, and it avoids the general maintenance work of manual registrations.

steveharter commented 1 year ago

Changing to API-suggestion based on feedback above until there is more consensus.

I would expect something like: services.AddActivatedSingleton(...)

adding async overload for factory is turbo strange, since there is no parity for this functionality for other lifetimes

That was added based on other feedback in this issue requesting async support. The proposal also works on an existing service without having to change it to implement IHostedService.

I would expect more overloads, that accept function factory, 'Try*' semantics for adding items, two generics and so on.

If we combine with the AddSingleton* version then yes there would need to be more overloads.

I'm still curious what the proposal truly adds that isn't already perfectly available.

Like existing extensions methods in DI, these essentially wrap existing 1-3 lines that can be manually written as well.

steveharter commented 1 year ago

Video

Closing this issue as a duplicate of #86075