Skip to content
This repository has been archived by the owner on Oct 20, 2023. It is now read-only.

Double initialization when resolving strongly typed singleton registrations using concrete type #138

Closed
BrunoZell opened this issue Mar 21, 2020 · 7 comments · Fixed by #139
Assignees
Labels
bug Something isn't working

Comments

@BrunoZell
Copy link

BrunoZell commented Mar 21, 2020

Describe the bug
One feature of Singularity is "Supports resolving unregistered concrete types". I may misunderstand how to use it, but I think those two registrations should be equivalent (where WritingSingleton implements ISingleton):

builder.Register<ISingleton, WritingSingleton>(c => c
    .With(Lifetimes.PerContainer));

builder.Register(typeof(WritingSingleton), c => c
    .As(typeof(ISingleton))
    .With(Lifetimes.PerContainer));

However, I find that in the first, strongly typed registration the singleton will be initialized once when resolving ISingleton, and another time when resolving WritingSingleton.

To Reproduce

using System;

namespace Singularity.DoubleInit
{
    public interface ISingleton { }

    public class WritingSingleton : ISingleton
    {
        public WritingSingleton() => Console.WriteLine("Initialized");
    }

    class Program
    {
        static void Main(string[] args)
        {
            var container = new Container(builder =>
            {
                // This registration does result in two initializations of WritingSingleton
                //builder.Register<ISingleton, WritingSingleton>(c => c.With(Lifetimes.PerContainer));

                // This registration does result in one initialization of WritingSingleton
                builder.Register(typeof(WritingSingleton), c => c.As(typeof(ISingleton)).With(Lifetimes.PerContainer));
            });

            var instance1 = container.GetInstance<ISingleton>();
            var instance2 = container.GetInstance<ISingleton>();
            var instance3 = container.GetInstance<WritingSingleton>();
        }
    }
}

Expected behavior
Both registrations should result in Initialized being written to the console only once.

@BrunoZell BrunoZell added the bug Something isn't working label Mar 21, 2020
@BrunoZell
Copy link
Author

After a little more investigation I found that adding .As<WritingSingleton>() to the strongly typed registration does the trick. These two now behave equivalent:

builder.Register<ISingleton, WritingSingleton>(c => c
    .As<WritingSingleton>()
    .With(Lifetimes.PerContainer));

builder.Register(typeof(WritingSingleton), c => c
    .As(typeof(ISingleton))
    .With(Lifetimes.PerContainer));

I'm not sure if this is by design. Can you please verify @Barsonax?.

Also, is there a reason why there aren't any generic constraints in place on the strongly typed .As<> method? Those would point the programmer in the right direction immediately if they accidentally fill in the wrong type.

@BrunoZell
Copy link
Author

With the working registrations, we can get a duplicated initialization again when using a child container:

using System;

namespace Singularity.DoubleInit
{
    public interface ISingleton { }

    public class WritingSingleton : ISingleton
    {
        public WritingSingleton() => Console.WriteLine("Initialized");
    }

    class Program
    {
        static void Main(string[] args)
        {
            var container = new Container(builder =>
            {
                // Both registrations result in two initializations of WritingSingleton
                builder.Register<ISingleton, WritingSingleton>(c => c.As<WritingSingleton>().With(Lifetimes.PerContainer));
                //builder.Register(typeof(WritingSingleton), c => c.As(typeof(ISingleton)).With(Lifetimes.PerContainer));
            });

            var childContainer = container.GetNestedContainer();

            var instance1 = childContainer.GetInstance<ISingleton>();
            var instance2 = childContainer.GetInstance<ISingleton>();
            var instance5 = childContainer.GetInstance<WritingSingleton>();
        }
    }
}

The documentation on Lifetime.PerContainer states: The same instance will be returned as long as it is requested in the same Container or a child of this container.

So I would interpret this as that there should be an instance created only once, since the child container inherits the very same instance. Am I mistaken or is the implementation mistaken?

@BrunoZell
Copy link
Author

A workaround is to remove ConcreteServiceBindingGenerator in the Singularity settings:

builder.ConfigureSettings(settings =>
{
    settings.ConfigureServiceBindingGenerators(generators =>
    {
        generators.Remove(x => x is ConcreteServiceBindingGenerator);
    });
});

@Barsonax
Copy link
Owner

Barsonax commented Mar 21, 2020

Thanks for making a issue.

When you register a type you are registering a mapping from a service type to a implementation type. This means if you use the concrete implementation type to resolve the type it cannot find the mapping and thus Singularity will create a new separate registration as you already found out. This is confusing behavior.

Not registering the implementation as a service in the mapping was done on purpose to make things more explicit but that kinda backfired here. Thinking of changing this now.

Your childcontainer example is indeed a bug which I will fix.

A generic constraints on the As method would be nice but iam afraid C# doesn't support that specific kind of constraint since where TInstance : TService does not compile.

@BrunoZell
Copy link
Author

The documentation states "Supports resolving unregistered concrete types". What does that mean then when this is expected behavior (everything needs to be registered explicitly)?

Regarding the generic constraint, check again. The ASP.NET Core's default dependency injection does use generic constraints, which are where TImplementation : class, TService (source). This definitely helps preventing rookie mistakes.

Additionally, registering implementation types that are not assignable from the used dependency type does not result in a runtime error either.

@Barsonax
Copy link
Owner

Barsonax commented Mar 22, 2020

The documentation states "Supports resolving unregistered concrete types". What does that mean then when this is expected behavior (everything needs to be registered explicitly)?

It means that when Singularity cannot find a registration for a given service type it will create a default one for you. This is what ConcreteServiceBindingGenerator does. However I think that when you register a type Singularity should register that type for both the implementation and the service type as that makes more sense.

Regarding the generic constraint, check again. The ASP.NET Core's default dependency injection does use generic constraints, which are where TImplementation : class, TService (source). This definitely helps preventing rookie mistakes.

Thats a different kind of constraint there. Lets view the code of the StronglyTypedServiceConfigurator:

public StronglyTypedServiceConfigurator<TDependency, TInstance> As<TService>()
{
   // more code
}

Here we don't want TService to derive/implement TInstance but we want TInstance to derive/implement from TService. Such a constraint does not compile in C#. The StronglyTypedServiceConfigurator class itself does have the constraints though and while it would be nice if we could also do that for the As method it would require changing the API quite a bit to make it work with C#. See #140

Additionally, registering implementation types that are not assignable from the used dependency type does not result in a runtime error either.

Seems a runtime check was missing in the As method. Added it on my branch along with unit tests to verify this.

EDIT: prerelease package which should fix your issues: https://www.nuget.org/packages/Singularity/0.17.4-fixlifetimeinchi0008

@BrunoZell
Copy link
Author

It means that when Singularity cannot find a registration for a given service type it will create a default one for you.

I see, now I understand.

However I think that when you register a type Singularity should register that type for both the implementation and the service type as that makes more sense.

+1. That was my original expectation.

That new prerelease package works in my case, thank you 👍

@Barsonax Barsonax linked a pull request Mar 24, 2020 that will close this issue
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants