hadashiA / VContainer

The extra fast, minimum code size, GC-free DI (Dependency Injection) library running on Unity Game Engine.
https://vcontainer.hadashikick.jp
MIT License
1.89k stars 165 forks source link

Undesirable behavior of a registered entity #553

Closed Bezarius closed 11 months ago

Bezarius commented 12 months ago

When we use registration like this:

builder.Register<IObservableConnection>(x => x.Resolve<IMetaClient>().ObservableConnection(), Lifetime.Singleton) .AsSelf();

or like this:

builder.RegisterFactory<IObservableConnection>( x => { return () => x.Resolve<IMetaClient>().ObservableConnection(); }, Lifetime.Singleton) .AsSelf();

It would be registered with: `FuncRegistrationBuilder'. This would lead to incorrect behavior, because we asked to register only the interface, but the container will also register the IDisposable, and when the scope is removed, it also wiil call Dispose of this object.

IObservableConnection does not contain IDisposable interface, but it's implementation have.

In my opinion, this would be the correct behavior if the registration was AsImplementedInterfaces()

Problem here:

    `[MethodImpl(MethodImplOptions.AggressiveInlining)]
    object CreateTrackedInstance(Registration registration)
    {
        var lazy = sharedInstances.GetOrAdd(registration, createInstance);
        var created = lazy.IsValueCreated;
        var instance = lazy.Value;
        if (!created && instance is IDisposable disposable && !(registration.Provider is ExistingInstanceProvider))
        {
            disposables.Add(disposable);
        }
        return instance;
    }`

When we do this check: instance is IDisposable we check implementation that have IDisposable, and instance provided with FuncRegistrationBuilder, so the instance of it will be added to disposables.

I'm not sure about a good solution, so I decided to start with a bug.

Mb we should add something like this: builder.RegisterInstance<IObservableConnection>(x => x.Resolve<IMetaClient>().ObservableConnection()) .AsSelf();

Or should I register in another way?

hadashiA commented 11 months ago

I think it is common for a DI container to keep track of the instances it creates and dispose of them with the destruction of scope. So, it makes sense to "DIspose" the scope. This is a specification.

Why would you want the life of an instance created by a DI container to be longer than that of the DI container? There is no guarantee that the dependent object will survive after the DI container is disposed.

Also, we do not recommend DI a generic interface like IObservableConnection. You can DI a more application-specific type that wraps it and not implement IDIsposable if you so choose.