-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
DI : add support to eager load a singleton service #43149
Comments
There are a couple ways to do this. |
Tagging subscribers to this area: @eerhardt, @maryamariyan |
That's potentially dangerous. While there's an analyzer that warns about incorrectly calling 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. |
Dear all, Using a In my case, the And the How about adding a boolean @pinkfloydx33 : Calling |
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. |
@davidfowl : are you talking about the 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 |
There shouldn't a per service descriptor setting. Object graphs aren't per descriptor but that's not the only problem. Changes to |
Im missing something. Why not:
If its because you want the container to own the instance then this comes back to having an isExternallyOwned (or equivalent) option:
The use case mentions:
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:-
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:
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.
Another way to think about that is to add an extension method for |
@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 If a 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 ( Implementing a But from what has been written here, the eager loading would not be enabled per service but only globally through 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*/}); |
@ah1508 as an alternative, would using an Currently, whenever I have a dependency that should perform startup work, I register a custom 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 The approach works well for .NET Core 3.0 and up (if I remember correctly), as of which the following has been true:
|
@Timovzl @ah1508
Just seperate out the 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. |
Dear @Timovzl , indeed, 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 !! |
@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 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 |
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 The following extension method hides the activation of
From test : IServiceProvider serviceProvider = new ServiceCollection()
// Add
.BuildServiceProvider(new(), true); For a buit-in support, a new property could be added to the
|
@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 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 |
Note the original description was updated to include an API proposal:
|
[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 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;
|
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 |
@Timovzl : I see two problem with 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 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 |
Declaring a type both as itself (or its representative interface) and as 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. 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. |
Changing to API-suggestion based on feedback above until there is more consensus.
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.
If we combine with the AddSingleton* version then yes there would need to be more overloads.
Like existing extensions methods in DI, these essentially wrap existing 1-3 lines that can be manually written as well. |
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 #86511 to do this as early as possible, although for backwards compat it can also hook into theIHostedService.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 newIHostedLifecycleService
interface.API Proposal
In the hosting abstractions assembly:
Usage examples
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 :
Somewhere in my application :
Here,
Foo
will be instantiated inside the transaction. Let's say I cannot receiveFoo
by injection because the service I need must be obtained dynamically.In
FooTest
: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 theAddSingleton
méthods.The text was updated successfully, but these errors were encountered: