dotnet / extensions

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

Question: Correct usage of ActivatorUtilities.CreateInstance and DI to dispose of transient services? #2952

Closed JayJamieson closed 4 years ago

JayJamieson commented 4 years ago

Hi team, Im working creating an event based Short and Long running Job framework using the HostBuilder.

I ahve a question about how to correctly use ActivatorUtilities.CreateInstance in such a way that the services instance are correctly disposed of to prevent memory leaks.

My current prototype approach is as such

  1. Simulated event stream to create many service instances and tasks
  2. Create Instance of task and dispatch them to run 3 ??

Question: When using ActivatorUtilities.CreateInstance and IServiceProvider what is the correct way to dispose of injected services to prevent memory leaks (merely setting reference reference to service to null doesnt work), or is there a better approach to injecting and creating instances in a similar fashion to api controller req/resp life cycle?

IService1 injected as Transient

    public interface IService1 {
        Task Test(int id);
    }
    public class Service1 : IService1 {

        private readonly ILogger<Service1> _logger;
        private readonly IService2 _service2;
        private int _i;
        public TestInterface(ILogger<Service1> logger, IService2 service2) {
            _logger = logger;
            _interface2 = interface2;
        }

        public async Task Test(int i) {
            this._i = i;
            _logger.LogInformation($"It Worked! { _service2.Test(i)}");
            await Task.Delay(i)
        }
    }

IService2 injected as Transient

    public interface IService2 {
        string Test(int id);
    }

    public class Service2 : IService2 {
        private string Id = string.Empty

        public TestInterface2() {
            Id = Guid.NewGuid().ToString();
        }

        public string Test(int i) {
            return Id;
        }
    }

JobSchedulerService

public class JobSchedulerService : BackgroundService {
        private readonly ILogger<JobSchedulerService > _logger;
        private Dictionary<string, IService1> _tasks2 = new Dictionary<string, IService1>();
        private Dictionary<string, Task> _tasks = new Dictionary<string, Task>();
        protected JobSchedulerService (ILogger<JobSchedulerService > logger, IServiceProvider serviceProvider) {
            _logger = logger;
        }

        protected override async Task ExecuteAsync(CancellationToken stoppingToken) {
            await RunAsync(stoppingToken);
            while (!stoppingToken.IsCancellationRequested) {
              // 3. perform statemanagement of tasks and remove from task list when completed and clear transient services
            }

        }

        public Task RunAsync(CancellationToken stoppingToken) {
             // 1.
            for (int i = 0; i < 1000000; i++) {
                // 2.
                var a = (IService1)ActivatorUtilities.CreateInstance(_serviceProvider, typeof(Service1));
                _tasks.Add($"Worker_{i}", a.Test(i));
                _tasks2.Add($"Worker_{i}", a);
            }

             await Task.Delay(TimeSpan.FromSeconds(5));
        }
    }
rcollina commented 4 years ago

Neither class implements IDisposable. That being said, you should be able to test the behaviour with a good dose of printf debugging.

As far as I can tell, and provided I haven’t been doing this wrong so far, it should work like this: Service2 seems to be resolved by the DI container. That should be disposed when the container itself is disposed.

Service1 should be disposed by your code since you created it, rather than having the DI container create it for you.

JayJamieson commented 4 years ago

Neither class implements IDisposable. That being said, you should be able to test the behaviour with a good dose of printf debugging.

As far as I can tell, and provided I haven’t been doing this wrong so far, it should work like this: Service2 seems to be resolved by the DI container. That should be disposed when the container itself is disposed.

Service1 should be disposed by your code since you created it, rather than having the DI container create it for you.

I'll have another test with Disposable interface. Is there no way for Service2 to be disposed of without disposing the service container?

rcollina commented 4 years ago

Not that I know of besides manually disposing the object reference, which is owned by the container.

I mentioned the register resolve release pattern in a previous edit but I’m not sure how it’d help in this case. There’s a mixed ownership scenario and I’m not sure why that’s the case. Perhaps you need a factory rather than activating the instance with ActivatorUtilities.

JayJamieson commented 4 years ago

Not that I know of besides manually disposing the object reference, which is owned by the container.

Apologies if this is not the right forum for discussion around this, what im trying to figure out is what best practice around what im trying to do. Ive looked at how controllers are DI but the process is to complex for me to understand.

Ive implemented IDisposable on Service1 and added logging to test disposing of it which for the most part works but i still have references to the transient Service2.

Im essentially trying to extract the DI process for controllers but instead using a request response pattern a request is received from a queue based on event type i selected the controller type and perform the DI and run its process method as a hot task.

Some direction on how to do this or what i should be looking to build around would be appreciated otherwise I'll go ahead and just close this.

rynowak commented 4 years ago

would ServiceProvider.CreateScope() work for this and at end the task performs self disposal on the scoped service provider?

This is where I would start. Most of our frameworks that have a discrete "unit of work" create a scope with the same lifetime as that "unit of work".

Things like controllers work in an intuitive way because ASP.NET Core creates a scope per request.

JayJamieson commented 4 years ago

This is where I would start. Most of our frameworks that have a discrete "unit of work" create a scope with the same lifetime as that "unit of work".

Things like controllers work in an intuitive way because ASP.NET Core creates a scope per request.

It just seems like there is so much infrastructure code involved (looking at DefaultControllerActivator just for the answer to be use ServiceProvider.CreateScope(). Hence why i was hesitant to use this in the first place.

Cheers.

rcollina commented 4 years ago

I second @rynowak ‘s suggestion to use a scoped service provider.

Use case same as yours, message in, activate some handlers, clean up. As long as you identify where the work happens, where the usercode is called, you can wrap it with additional behaviours (eg TransactionScope, etc).

rynowak commented 4 years ago

It just seems like there is so much infrastructure code involved (looking at DefaultControllerActivator just for the answer to be use ServiceProvider.CreateScope(). Hence why i was hesitant to use this in the first place.

I wouldn't read too much into the design of this class. There's a few things going on that you probably don't need.


In general the right want to manage disposable transients (your original question) is to use a scope. If you can share links to what you're working on, then myself of @davidfowl can try to give you more specific advice.

udlose commented 4 years ago

In general the right want to manage disposable transients (your original question) is to use a scope. @rynowak - can you please clarify your comment? I was under the impression that DI manages the disposal of anything registered thru it - including IDisposable types. Is this not the case?

rynowak commented 4 years ago

That is the case. If you want to manage when things are disposed, you can do it with scopes.

JayJamieson commented 4 years ago

I dont have a sample of code that's worthy, but the concept is akin to creating a pool of long running workers that access db and http services triggered by an event from a key or some other message bus.

If management of the scope and holding the scope created at the entry point of the event is fine then im happy to close this issue and continue on with a design based on suggestions made here.

Cheers 👍 👍