dotnet / extensions

This repository contains a suite of libraries that provide facilities commonly needed when creating production-ready applications.
MIT License
2.59k stars 741 forks source link

AddHostedService() registers service as Transient in the 2.1.1 apart from the 2.0 #553

Closed sguryev closed 5 years ago

sguryev commented 6 years ago

Hello.

According to the documentation for the 2.0 https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/hosted-services?view=aspnetcore-2.0#timed-background-tasks We have to register services as Singleton image

According to the documentation for the 2.1 https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/hosted-services?view=aspnetcore-2.1#timed-background-tasks We should use the AddHostedService extension image which is Transient under the hood now: https://github.com/aspnet/Hosting/blob/master/src/Microsoft.Extensions.Hosting.Abstractions/ServiceCollectionHostedServiceExtensions.cs#L18

So my questions are:

  1. Can it be a problem and why have you changed the lifetime?
  2. What should I do If I have one more interface to reach the the service from the DI? Right now I'm doing the following: Register custom interface as singleton: services.AddSingleton<ICandleSubscriptionHostedService, CandleSubscriptionHostedService>(); Use the custom resolver for the IHostedService registration: services.AddSingleton<IHostedService, CandleSubscriptionHostedService>(provider => (CandleSubscriptionHostedService) provider.GetRequiredService<ICandleSubscriptionHostedService>()); Both are Singletons so the second lifetime doesn't matter a lot actually.

Thank you!

davidfowl commented 6 years ago

Don’t make your hosted service an API. That’s the best bet. Instead use another service that the hosted service interacts with as the public API.

We made it transient because it’s usually only resolved once when used as a hosted service.

sguryev commented 6 years ago

@davidfowl thank you for the answer

I have a worker app service on azure which does some work 24/7. And of course I want to get status\report from the service on demand. That's why I have implemented two interfaces: IHostedService and the custom one for accessing the service instance and getting some data from there.

What solution would you suggest for such cases? Make the hosted service like a black box and interact with it via 3rd service\storage?

davidfowl commented 6 years ago

I'm saying don't implement the interface on the IHostedService itself. Here's an example of a queue:

public interface IQueue<T>
{
    T Dequeue();
    void Enqueue(T item);
}

public class QueueHostedService : IHostedService
{
     public QueueHostedService(IQueue<WorkItem> queue)
     {
     }

     // Write code to Dequeue from the queue and perform some work
}

public class HomeController : Controller
{
     private readonly IQueue<WorkItem> _queue;
     public HomeController(IQueue<WorkItem> queue)
     { 
          _queue = queue;
     }

     [Route("/item")]
     public IActionResult Post(WorkItem item)
     {
           _queue.Enqueue(item);
     }
}

In this example, you could implement the IQueue on the QueueHostedService, or you can implement it on another type that the QueueHostedService uses.

sguryev commented 6 years ago

In this example, you could implement the IQueue on the QueueHostedServic

So will be: public class QueueHostedService<T> : IHostedService, IQueue<T> ?

Sorry,

implement the IQueue on the QueueHostedService

sounds confusing for me

In my case I have to be able to access the QueueHostedService running instance and get some report\status. For example current WorkItem or number of work items in the queue, or number of dequeueed items. I have a service with subscribes on the number of WebSockets on start and I need to have an ability to track the current subscriptions.

davidfowl commented 6 years ago

Maybe I wasn’t clear enough. I want suggesting that you implement the interface on then IHostedService, I was suggesting that you should use another service to store the state. I’m your case, don’t store the state on the hosted service, store it on a different service

sguryev commented 6 years ago

Store the information in the different service

Does it mean in another Singleton which will be used from both hosted service to put and some client to pull data? What the pros of such flow? In the 2.0 I had hosted services as singleton and pulled the data directly from it. So my question is WHY?

davidfowl commented 6 years ago

Keep doing what you’re doing then. We chose transient because we don’t expect people to resolve the hosted service instance and use it like an API.

Drawaes commented 6 years ago

Hmm having a whole other type for the hosted service is a bore and just another class to update or have. I like it as a Singleton otherwise I need 2* the classes for what is a start and end method.

davidfowl commented 6 years ago

Sure but it’s currenrlt transient so you’ll have to avoid using this method.

Drawaes commented 6 years ago

Agreed but the naming is not great. I suspect when most people hear service they equate it to Singleton... I sure did and it caught me out.

davidfowl commented 6 years ago

It only matters if you try to resolve the service for yourself. Ususally when you register system components like this, you aren’t the one responsible for calling it. Similar to how controllers are transient and not scoped. We don’t expect you to resolve the controller instance in your own code.

ygoe commented 6 years ago

Well, you weren't expecting us to actually use the hosted service, but we do. Because we need it. What good is a hosted service that nobody can talk to? Do I really need to create some more communication infrastructure of an unknown structure to make the connection? It's a lot simpler to just get that service and talk to it directly. Just like I can get any other singleton service.

In my case I'm hosting a router component that handles client WebSocket connections and can send data to them. So I resolve that service when I need it. Creating another level of indirection just to tell that service something looks bloated and hard to follow to me.

My suggestion would be to leave this hosted service topic completely out of DI. It's not related. (See my other referenced issue.) Instead, the hosted service should be registered through another API specifically for this purpose.

davidfowl commented 6 years ago

What good is a hosted service that nobody can talk to?

There are lots of reasons to.

Do I really need to create some more communication infrastructure of an unknown structure to make the connection? It's a lot simpler to just get that service and talk to it directly. Just like I can get any other singleton service.

No, in fact, you can just do what @sguryev outlines in the first post. Register using a delegate to resolve the original hosted service. Or you try not using it like an API and use another service for that.

My suggestion would be to leave this hosted service topic completely out of DI. It's not related. (See my other referenced issue.) Instead, the hosted service should be registered through another API specifically for this purpose.

Maybe, we'd then need to handle cases where it comes from DI and where it doesn't.

Tratcher commented 6 years ago

MVC style activate a list of Types.

jeffcj commented 6 years ago

I ran into the same issue as @sguryev today. I wrongly anticipated AddHostedService would result in a singleton. What I'm gathering from @davidfowl here is using a service that is a Singleton to manage state/status within the transient hosted service is the correct approach. Therefore never referencing the hosted service directly after it is started. It makes the implementation a tad more verbose, but I think it enforces a separation of concerns of sorts assuming my understanding here is correct.

dotnetchris commented 5 years ago

This thread is extremely strange. HostedServices should be singletons not transient.

One of the most obvious use cases of why you need a hosted service is you activate a shared resource's backplane and use it to tear it down. It doesn't make sense to have that done else where.

The situtation i'm looking at right now

HostedService StartAsync Start(msgbus)

HostedService StopAsync Stop(msgbus)

and i want my scoped services to get the singleton instance of msgbus only after it's been initialized by the host service.

It's really strange to use a hosted service to capture the msgbus as an injected dependency, manipulate state and then leave it as is with the expectation the container owns it.

I really expected to do something like:

services.AddSingleton<MyBusControl>(provider => provider.GetService<MyHostedService>().Bus);

Which fails with Bus is null because i got a surprise new instance of MyHostedService, when i expect i would get actual instance that's associated with aspnet.

It makes no sense that it doesn't return the correctly associated instance of MyHostedService.

dotnetchris commented 5 years ago

I guess i'll just make Bus be a static property on MyHostedService with the expectation that only Start/Stop will ever touch it.

I don't like it, but it'll work atleast.

services.AddSingleton(typeof(MyBus), MyHostedService.StaticBus);
davidfowl commented 5 years ago

Why does it need to be static?

dotnetchris commented 5 years ago

Because of how IHostedService is designed to register as transient.

I need the instance that is owned by my HostedService, the instance that had Start invoked on it.

With these being transient, they're nearly worthless other than i don't have to write voodoo code to fire on app start to initialize my connections. There needs to be a clear way to initialize a thing in a HostedService and then have that be accessible.

The aspnet core container doesn't seem to support run time modification, so i can't just register the service inside the HostedService. (That also would make perfect sense for me to add additional services into the container during StartAsync) I've used StructureMap since 2008 or earlier, i really want to believe in 10 years Microsoft could build a basic container that has features that have been standard to containers for over a decade now.

public class Foo: IHostedService 
{
    public Task StartAsync(CancellationToken cancellationToken)
    {
        var initializedBar = new Bar();

        return Task.CompletedTask;
    }
}

@davidfowl How would you acquire initializedBar inside a scoped service?

davidfowl commented 5 years ago

I must be missing something obvious so forgive me for being dense. Why can't you add Bar to the container as a singleton and inject it into Foo's constructor?

dotnetchris commented 5 years ago

I must be missing something obvious so forgive me for being dense. Why can't you add Bar to the container as a singleton and inject it into Foo's constructor?

static Bar Singleton;

static void Main() {
Singleton = Bar.Empty;
}

static async Task CallMeMaybe() {
Singleton = await Bar.Intialize();
}

But i guess it's the best option available.

davidfowl commented 5 years ago

I still don't quite understand. Singletons don't need to be exposed statically via a variable.

public class Foo: IHostedService 
{
    public Foo(Bar bar)
    {
    }

    public Task StartAsync(CancellationToken cancellationToken)
    {

        return Task.CompletedTask;
    }
}
dotnetchris commented 5 years ago

@davidfowl my last code block is your design of injecting an uninitialized singleton into a hosted service to be bootstrapped by Start.

It's a clear violation of the Open Closed Principle but that is the design aspnet has landed upon.

This is why HostedServices need to be singletons, so an uninitialized singleton is never ambiently available. Or, HostedServices should be able to modify the service collection during run time (or atleast on Start)

davidfowl commented 5 years ago

I don’t understand how making the hosted service a singleton changes this:

This is why HostedServices need to be singletons, so an uninitialized singleton is never ambiently available

Until Start is called, the service is unitialized

dotnetchris commented 5 years ago

Does AddHostedService do literally nothing?

Tratcher commented 5 years ago

@dotnetchris not much. https://github.com/aspnet/Hosting/blob/f9d145887773e0c650e66165e0c61886153bcc0b/src/Microsoft.Extensions.Hosting.Abstractions/ServiceCollectionHostedServiceExtensions.cs#L16-L18

davidfowl commented 5 years ago

@dotnetchris I understand the reasoning for wanting to use the same object, it seems easier but those other complains you mentioned I don't really buy any of your arguments.

Say we allowed container mutation on the fly (which performant containers usually do not allow), when would you add the service? After StartAsync was called? Does that mean that people consuming the service wouldn't be able to inject it until StartAsync is called? How do you ever coordinate that? Do you get injection errors until the connection to the bus succeeds?

dotnetchris commented 5 years ago

The way AddHostedService() presents itself makes it feel like you're adding it to a special collection ala aspnet filters. It's very non-obvious that all that's actually happening is IHostedService magically adds capability via the container itself and you're just doing AddTransient()... the more i think about this the less it makes sense.

How does this even work? What happens if I either intentionally or inadvertently manually registered Foo: IHostedService as singleton? What would happen if i registered it as scoped?

This really seems to be abusing the container that just dropping an item in it magically causes it to activate with no dependencies to it. Then even stranger is that item then becomes a phantom living in its own reality that nothing else can ever access.


Separate topics, containers in general

Say we allowed container mutation on the fly (which performant containers usually do not allow)

So i had to step back and research this statement and it's really split down the road whether containers allow mutation or not. Pulling up https://github.com/autofac/Autofac/issues/811

Looking at other containers out there, locking a container after it's built and ready to resolve isn't uncommon. Simple Injector, LightInject, and the Microsoft.Extensions.DependencyInjection containers all disallow updating the container post-facto.

For those that do - StructureMap, Ninject, and Windsor to name a few - it appears they actually do all the work mentioned in "easier said than done" - flushing caches, rebuilding the whole container.

If your 'performant' label pretty much means either Light/Simple, it would be accurate. But back to your question at hand, there would be no difference in container behavior to requesting it to provide ICanNeverBeConstructed and ImCurrentlyNotRegisteredButMightBeLater just at some point the latter may actually succeed on future calls.


What IHostedService provides is immense, it just seems really awkwardly bolted onto the container when it should just be an independent collection.

davidfowl commented 5 years ago

It’s an extension method on IServiceCollection, that’s how you know it has something to do with dependency injection.

It’s very simple, the consumer of IHostedServices gets an IEnumerable of them and then calls StsrtAsync and StopAsync at the right time in the life cycle.

We do this in lots of other places as well. If you did it as a singleton it’ll work just fine (you have to add it as an IHostedService explicitly though. This is what the method is for). If you registered it as scoped it would fail. The method exists to register the type as the right service type and to pick the lifetime for you.

Containers that allow mutation after building will have a more complicated design just because of that fact. There’ll be more locking because they have to account for the fact that registrations can change at any point. The added flexibility can result in a much more complex implementation that usually results in poorer performance.

dotnetchris commented 5 years ago

What about making it something more like:

 public static IServiceCollection AddHostedService<THostedService>(this IServiceCollection services, bool singletonLifetime = false) 
     where THostedService : class, IHostedService 
     => singletonLifetime ? services.AddSingleton<IHostedService, THostedService>() : services.AddTransient<IHostedService, THostedService>(); 
Tratcher commented 5 years ago

@dotnetchris doesn't that break down as soon as you have more than one IHostedService? The host will resolve and run all of them, but how do you resolve the right one in your app?

dotnetchris commented 5 years ago

@Tratcher over 9 out of 10 times i don't use interfaces for injection, i depend on concrete types. So in my case I'd just receive Foo directly public Baz(Foo fooSingleton). About the only time i ever inject via an interface is i'm expecting to receive IEnumerable<IHaveManyImplementations>

davidfowl commented 5 years ago

We wouldn't add the bool, we would just make a singleton. @Tratcher lets track this and make it a breaking change in 3.0. It won't break that many people and it'll solve most of the problems described here where people want to use their IHostedService like a public API.

Tratcher commented 5 years ago

@natemcmaster please transfer this to Extensions, it's part of generic host.

schmitch commented 5 years ago

@davidfowl sorry to actually "hijack" such an old issue, but I have a question.

Currently I try to use the IHostedService to actually have a way to Start/Stop Services from a Razor Page. I mean I registered the Services I want to stop/start as a Singleton and I have a special class that is derived from IHostedService, i.e. NamedHostedService that also needs to set a Name Parameter so that when I get all NamedHostedServices, I know which one does what.

So is this a "good" use of IHostedService or shouldn't this be done, from all the Issue's it's unclear if such use cases are ok, I think the documentation should clear this up.

kayjtea commented 5 years ago

AddHostedService should not be a singleton. Please don't make this breaking change, or if you do, introduce an IHostedApi or some such. The hosted service is just a service container -- the main reason to use it is because you need something running in the background with app life time. And in fact, BackgroundService is an IHostedService. You can use BackgroundService as a worker pool (by calling AddHostedService repeatedly), without having to deal with thread pools at all, which is extremely nice. If you need an API just do like @davidfowl suggests and inject a singleton. If you need to use IHostedService as an API then you're going to have to deal with threading issues, and it's much easier to concentrate the thread-safe state in the injected singleton anyway.

Try this example: your app needs to collect and report statistics by sampling every 1 second and reporting every 5 minutes. If you design it the way @davidfowl suggests, the implementation just falls into place very neatly: you have a singleton thread-safe Statistics class that is injected every place where statistics need to collected, for example, in your controllers, and then you have two IHostedService: one to reset the statistics every second and another to report it every 5 minutes. Much cleaner than having the controllers invoke the hosted services directly.

davidfowl commented 5 years ago

Yea that’s not how it’s supposed to work, you should create your own abstraction for that. The hosted service is resolved once by the runtime and shouldn’t be resolved by your code. What did your logic look like before? You can really call AddHostedService once at startup so how does making it a singleton break your logic?

jjxtra commented 5 years ago

Is there a reason AddHostedService doesn't have overloads for an object and func creator?

Tratcher commented 5 years ago

Because it's a helper for what we expect is the most common scenario. If that doesn't suit you then use the DI methods directly.

jjxtra commented 5 years ago

Thanks for clarifying. That was very much not obvious to me, it took me a few days to find an example using AddSingleton with IHostedService. Please consider adding the two overloads in .NET 3.0.

richardgavel commented 5 years ago

One thing I'm not sure of, even with the transient registration is why, in certain cases, the IHostedService is being resolved from the DI container more than once by ASP.NET core. I understand if independent code is resolving an instance, more than one might be created. I'm running this in IIS, any chance app pool recycling could come into play?

Tratcher commented 5 years ago

@richardgavel yes, an app pool recycle would restart your app and re-resolve all DI services, regardless of transient or singleton.

robertjak commented 5 years ago

Workaround to use IHostedService as API in .NET Core 2.1:

services.AddSingleton<BackgroundWorkerService>();
services.AddSingleton<IHostedService>(p => p.GetService<BackgroundWorkerService>());

P.S. I also prefer concrete classes. Entia non sunt multiplicanda sine necessitate :)

ghost commented 5 years ago

Workaround to use IHostedService as API in .NET Core 2.1:

services.AddSingleton<BackgroundWorkerService>();
services.AddSingleton<IHostedService>(p => p.GetService<BackgroundWorkerService>());

P.S. I also prefer concrete classes. Entia non sunt multiplicanda sine necessitate :)

Pages of comments, leads to this decent workaround. Thanks @robertjak