DevTeam / Pure.DI

Pure DI for .NET
MIT License
506 stars 22 forks source link

Disposable Instances Handling #44

Closed genment closed 5 months ago

genment commented 6 months ago

Hi there,

I have a question regarding the disposable instances created by the DI. I've noticed that these disposable instances are only disposed automatically if their lifetime is set to singleton. Is this behavior intended or is it a bug?

Additionally, I've been exploring workarounds and came across this readme, but I'm unsure where to integrate the following code:

// Disposal of instances in reverse order
while (disposables.TryPop(out var disposable))
{
    disposable.Dispose();
}

Any guidance on where to incorporate this code would be greatly appreciated.

Thank you in advance for your assistance!

NikolayPianikov commented 6 months ago

Hi @genment

Out of the box, it is possible to dispose instances with lifetime Singleton and Scoped. This is expected behavior, since the number of instances with other lifetime may be very large, may require a lot of memory to store them and a lot of CPU resources to register them in some collection.

And answering your second question, if instances with lifetimes Singleton and Scoped have strictly defined lifetimes, what are the lifetimes of other instances? It depends on the specific scenario. If you would describe your scenario in more detail, I would be able to give recommendations.

NikolayPianikov commented 6 months ago

Added another example.

genment commented 6 months ago

Thanks for replying.

I encountered a scenario where I wanted to bind a DownloadService from the Downloader package to DI with a Transient lifetime. Since the download functionality is only needed once in my project, it made sense to use Transient lifetime.

However, I ran into a problem: DownloadService implements IDisposable, and when I bind it with a Transient lifetime, its Dispose() method isn't called automatically.

After some consideration, I've decided to modify my approach. Instead of managing the DownloadService instance's lifetime via DI, I'm now using the using statement to handle its disposal.

ekalchev commented 6 months ago

Disposables are the hard to get right in DI. Autofac has a nice feature that deals with that https://autofac.readthedocs.io/en/latest/advanced/owned-instances.html

Currently you have the option to inject a factory to your class and call

using(var instance = myfactory.Create())

and you need to make sure myfactory implementation will always creturn transient lifetime instance. If you return a singleton that will fuck up everything.

This is where Owned< T > comes into play by to ensure Owned Dispose method is no-op when the instance is registered with singleton lifetime.

@NikolayPianikov Owned< T > can be a great addition to Pure.DI

NikolayPianikov commented 6 months ago

Disposables are the hard to get right in DI. Autofac has a nice feature that deals with that https://autofac.readthedocs.io/en/latest/advanced/owned-instances.html

Yeah, I read it, thanks. I've updated the example.

@NikolayPianikov Owned< T > can be a great addition to Pure.DI

Yes, the current approach looks complicated and not user friendly. I'll try to figure out a way to do something similar to Owned. The main difficulty is to make it as productive as possible in terms of speed and efficient in terms of memory consumption. There's also a question about using it in delegates. Since some instances (PerResolve) can be shared in a composition and it depends on the particular dependency tree.

NikolayPianikov commented 6 months ago

A new feature has been added:

Example 1 Example 2

Still testing, but it'll look something like this. IAsynEnumerable will work the same way.

So there is also an extension point that allows you to work with other types, for example:

 .Accumulate<IMyDisposable, MyOwned>(Lifetime.Transient)
NikolayPianikov commented 6 months ago

Another one example using custom accumulators

ekalchev commented 4 months ago

I am trying to figure out how Owned is implemented.

Lets say I have an assembly Library that has this class

internal class ServiceDescriptor
{
    public ServiceDescriptor(Func<Owned<IService>> owned)
    {

    }
}

and then in assembly Executable

        .Bind<IService>.To<MyService>()
        .Bind().To(ctx => 
        {
            ctx.Inject(out Func<Owned<IService>> fact);
            return new ServiceDescriptor(fact);
        })

I can see Owned class is generated by the source generator? if so how the type Owned in Library assembly will be the same as the Executable assembly?

I am trying to use it with 2 assemblies and I am getting errors. I can see Owned class and interface are generated in both assemblies, which means they are different types.

NikolayPianikov commented 4 months ago

I can see Owned class is generated by the source generator?

Yes, the Owned class is part of the Pure.DI API.

if so how the type Owned in Library assembly will be the same as the Executable assembly?

Yes you are right.

I am trying to use it with 2 assemblies and I am getting errors. I can see Owned class and interface are generated in both assemblies, which means they are different types.

I have created the issue #59 and will fix that soon.

I would suggest adding the reference to Pure.DI to the root assembly only and use your own accumulator to register the disposing objects, here is an example. This will allow your business entities to not know anything about DI.

NikolayPianikov commented 4 months ago

59 fixed in 2.1.22

NikolayPianikov commented 4 months ago

The same question is about Tag, Type and Ordinal attributes. The Pure.DI API is supposed to be used only when configuring a composition, so the Pure.DI API is internal. It is recommended that other asmblies use their own Accumulators and their own attributes, this will make them completely independent of the infrastructure (DI).

ekalchev commented 3 months ago

I am trying to understand changes you've made with Owned

    internal interface IDependency : IDisposable
    {
        void DoWork();
    }

    public class Dependency : IDependency
    {
        private bool _disposed;
        public void Dispose()
        {
            _disposed = true;
        }

        public void DoWork()
        {
            if (_disposed) throw new ObjectDisposedException(GetType().Name);
        }
    }

internal class Service
{
    private readonly Func<IDependency> dependencyFactory;

    public Service(Func<IDependency> dependencyFactory)
    {
        this.dependencyFactory = dependencyFactory;
    }

    public void Run()
    {
        var instance1 = dependencyFactory();

        using (var instance2 = dependencyFactory())
        {
            instance2.DoWork();
        }

        instance1.DoWork();
    }
}

private void Setup() => DI.Setup(nameof(Composition))
     .Bind<IDependency>().As(Lifetime.Singleton).To<Dependency>()
      .Root<Service>("Root")
       ;

Composition composition = new Composition();
composition.Root.Run();  

I was left with the impression that you are trying to solve the above problem. When you have a factory that returns IDisposable instance. In this situation you MUST dispose the instance because you've created it. Unfortunatelly the problem is that Service class is not aware of the lifetime of IDependency instance and in the current situation it is singleton and when you dispose it, you dispose also instance1. Are there solution for this situation?

I am reading the code but I can't see a solution using Owned for this case.

NikolayPianikov commented 3 months ago

In your case, Dependency is a disposable singleton. So you don't know in advance how many instances will use it. In this case, you can dispose it by disposing composition. Change one line to using var composition = new Composition(); If there is even one disposable singleton, the entire composition becomes disposable.

By the way, this interface IDependency : IDisposable looks like a leaky abstraction. Maybe it would be better to do it like this class Dependency : IDependency, IDisposable.

using Pure.DI;

DI.Setup(nameof(Composition))
    .Bind().As(Lifetime.Singleton).To<Dependency>()
    .Root<Service>("Root");

using var composition = new Composition();
composition.Root.Run();

internal interface IDependency
{
    void DoWork();
}

public class Dependency : IDependency, IDisposable
{
    private bool _disposed;

    public void Dispose()
    {
        _disposed = true;
        Console.WriteLine("Disposed");
    }

    public void DoWork()
    {
        if (_disposed) throw new ObjectDisposedException(GetType().Name);
    }
}

internal class Service(Func<IDependency> dependencyFactory)
{
    public void Run() => dependencyFactory().DoWork();
}
ekalchev commented 3 months ago

The service class should not be aware of the lifetime of the IDependency and the code in Service class should not assume what is the lifetime of IDependency. I should be able to switch from Singleton to Transient in the registration and the code in Service class should not be updated because of that.

Probably you'll suggest using Accumulator (If I correctly understand the usage) that is not a solution if IDependency uses a lot of resources and need to be disposed immediately after use. Also, accumulators keep reference to the instance and doesn't allow GC to collect the instances. I was left with the impression that the introduction of Owned < T > was added to address this problem, because this is how it is done in AutoFac, but after reading the code, It seems I thought wrong.

Here a potential solution of the problem.

You should be able to register a user provided class to manage Owned instances.

class MyOwned<T> : IDisposable where T : IDisposable
{
    private readonly T instance;
    private long refCount;

    public MyOwned(T instance)
    {
        this.instance = instance;
        Interlocked.Increment(ref refCount);
    }

    public T Instance => instance;

    public void AddRef()
    {
        Interlocked.Increment(ref refCount);
    }

    public void Dispose()
    {
        if (Interlocked.Decrement(ref refCount) == 0)
        {
            instance.Dispose();
        }
    }
}

    internal class Service
    {
        private readonly Func<MyOwned<IDependency>> dependencyFactory;

        public Service(Func<MyOwned<IDependency>> dependencyFactory)
        {
            this.dependencyFactory = dependencyFactory;
        }

        public void Run()
        {
            var instance1 = dependencyFactory();

            using (var instance2 = dependencyFactory())
            {
                instance2.Instance.DoWork();
            }

            instance1.Instance.DoWork();
        }
    }

     internal partial class Composition
 {
     private void Setup() => DI.Setup(nameof(Composition))
         .Bind<IDependency>().As(Lifetime.Transient).To<Dependency>()
         .Bind().As(Lifetime.Transient).To<MyOwned<IDependency>>(ctx =>
         {
             ctx.Inject(out IDependency dependency);

             if (myOwnedInstanceLifetime == Lifetime.Transient)
             {
                 return new MyOwned<IDependency>(dependency);
             }
             else
             {
                 // lock for thread safery
                 if(singletonInstance == null)
                 {
                     singletonInstance = new MyOwned<IDependency>(dependency);
                 }
                 else
                 {
                     singletonInstance.AddRef();
                 }

                 return singletonInstance;
             }

         })
         .Root<Service>("Root")
         ;

     private static MyOwned<IDependency>? singletonInstance;
     private static readonly Lifetime myOwnedInstanceLifetime = Lifetime.Transient; // this lifetime must match Bind<IDependency>().As(Lifetime.Transient).To<Dependency>() IDependency lifetime
 }

Now you can change the lifetime of IDependency in registration code and the Service will work correctly for singleton or transient lifetime without the need Service class to know about the IDependency lifetime.

Of course the code is not great but Pure.DI can take care of two static fields, because it already has knowledge of those values and may be that part too

if (myOwnedInstanceLifetime == Lifetime.Transient)
             {
                 return new MyOwned<IDependency>(dependency);
             }
             else
             {
                 // lock for thread safery
                 if(singletonInstance == null)
                 {
                     singletonInstance = new MyOwned<IDependency>(dependency);
                 }
                 else
                 {
                     singletonInstance.AddRef();
                 }

                 return singletonInstance;
             }

It is possible to notify Pure.DI for custom user provided MyOwned class similar to Accumulator<T1, T2> registration code. This is most elegant way I've seen dealing with IDisposable instances in DI containers.

NikolayPianikov commented 3 months ago

I get your idea. It looks interesting. There are a few things:

In my opinion objects can be divided into 2 types:

In the general case for any lifetime my answer above can be rewritten as follows:

using Pure.DI;

DI.Setup(nameof(Composition))
    .Bind().As(Lifetime.Singleton).To<Dependency>()
    .Bind(2).As(Lifetime.Transient).To<Dependency>()
    .Root<Owned<Service>>("Root");

using var composition = new Composition();

using (var rootA = composition.Root)
{
    rootA.Value.Run();
    Console.WriteLine("Dispose root A:");
}

// Dispose root A:
// Disposed
// Disposed
// Disposed

using (var rootB = composition.Root)
{
    rootB.Value.Run();
    Console.WriteLine("Dispose root B:");
}

// Dispose root B:
// Disposed
// Disposed
// Disposed

Console.WriteLine("Dispose others:");

// Dispose others:
// Disposed

internal interface IDependency
{
    void DoWork();
}

public class Dependency : IDependency, IDisposable
{
    private bool _disposed;

    public void Dispose()
    {
        _disposed = true;
        Console.WriteLine("Disposed");
    }

    public void DoWork()
    {
        if (_disposed) throw new ObjectDisposedException(GetType().Name);
    }
}

internal class Service(Func<IDependency> dependencyFactory1, [Tag(2)] Func<IDependency> dependencyFactory2)
{
    public void Run()
    {
        for (var i = 0; i < 3; i++)
        {
            dependencyFactory1().DoWork();
            dependencyFactory2().DoWork();   
        }
    }
}