castleproject / Windsor

Castle Windsor is a best of breed, mature Inversion of Control container available for .NET
http://www.castleproject.org
Apache License 2.0
1.52k stars 456 forks source link

Resolve<> cannot pick parameter-less constructor #364

Open davidshen84 opened 6 years ago

davidshen84 commented 6 years ago

In my project, I used the DatabaseProviderFactory class from Microsoft.Practices.EnterpriseLibrary.Data library. It has a parameter-less constructor, and another two constructor with different parameters.

I register the component as

container.Register(Component.For<DatabaseProviderFactory>());

But when I try to resolve this component, I got an exception saying it cannot resolve IConfigurationSource which is a parameter type of the other constructor.

ghost commented 6 years ago

Windsor always tries to pick the ctor with the highest amount of parameters.

https://github.com/castleproject/Windsor/blob/master/docs/how-constructor-is-selected.md

davidshen84 commented 6 years ago

But the doc said:

It will then see how many parameters each satisfiable constructor has, and pick the one with most parameters (the greediest one).

In my case, the parameter-less constructor satisfies while the the other one which depends on a IConfigurationSource component does not. But Windsor still picked the unsatisfied one and threw an error.

Did I misunderstood the rule, or the rule was updated?

ghost commented 6 years ago

Is there a way you can reproduce this in a small code sample? Maybe we can use it to create a failing unit test to try and isolate the problem. To keep you going for now use the DoNotSelect attribute. Will re-open...

davidshen84 commented 6 years ago

@fir3pho3nixx

It is strange that I cannot reproduce it in a demo code. Actually, I remember my code was working without this constructor resolution conflict. I suspect that some extra packages I added into my project caused this problem.

Unfortunately, I cannot show you my company's code. I hope below stack trace is helpful.

Castle.MicroKernel.ComponentNotFoundException occurred
  HResult=0x80131500
  Message=No component for supporting the service System.Configuration.ConfigurationSection was found
  Source=Castle.Windsor
  StackTrace:
   at Castle.MicroKernel.DefaultKernel.Castle.MicroKernel.IKernelInternal.Resolve(Type service, IDictionary arguments, IReleasePolicy policy)
   at Castle.Facilities.TypedFactory.TypedFactoryComponentResolver.Resolve(IKernelInternal kernel, IReleasePolicy scope)
   at Castle.Facilities.TypedFactory.Internal.TypedFactoryInterceptor.Resolve(IInvocation invocation)
   at Castle.Facilities.TypedFactory.Internal.TypedFactoryInterceptor.Intercept(IInvocation invocation)
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at Castle.Proxies.Func`2Proxy.Invoke(String arg)
   at Microsoft.Practices.EnterpriseLibrary.Data.Configuration.DatabaseSyntheticConfigSettings.GetConnectionStrings()
   at Microsoft.Practices.EnterpriseLibrary.Data.Configuration.DatabaseSyntheticConfigSettings.GetConnectionStringSettings(String name)
   at Microsoft.Practices.EnterpriseLibrary.Data.Configuration.DatabaseSyntheticConfigSettings.GetDatabase(String name)
   at Microsoft.Practices.EnterpriseLibrary.Data.DatabaseProviderFactory.DatabaseConfigurationBuilder.<Create>b__3(String n)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Microsoft.Practices.EnterpriseLibrary.Data.DatabaseProviderFactory.DatabaseConfigurationBuilder.Create(String name)
   at Microsoft.Practices.EnterpriseLibrary.Data.DatabaseProviderFactory.Create(String name)
   at Some.Company.WindsorInstaller.<>c.<Install>b__0_1(DatabaseProviderFactory factory) in C:\some\project\WindsorInstaller.cs:line 169
   at Castle.MicroKernel.Registration.ComponentRegistration`1.<>c__DisplayClass87_0`2.<UsingFactory>b__0(IKernel kernel)
   at Castle.MicroKernel.Registration.ComponentRegistration`1.<>c__DisplayClass89_0`1.<UsingFactoryMethod>b__0(IKernel k, ComponentModel m, CreationContext c)
   at Castle.MicroKernel.ComponentActivator.FactoryMethodActivator`1.Instantiate(CreationContext context)
   at Castle.MicroKernel.ComponentActivator.DefaultComponentActivator.InternalCreate(CreationContext context)
   at Castle.MicroKernel.ComponentActivator.AbstractComponentActivator.Create(CreationContext context, Burden burden)
   at Castle.MicroKernel.Lifestyle.AbstractLifestyleManager.CreateInstance(CreationContext context, Boolean trackedExternally)
   at Castle.MicroKernel.Lifestyle.SingletonLifestyleManager.Resolve(CreationContext context, IReleasePolicy releasePolicy)
   at Castle.MicroKernel.Handlers.DefaultHandler.ResolveCore(CreationContext context, Boolean requiresDecommission, Boolean instanceRequired, Burden& burden)
   at Castle.MicroKernel.Handlers.DefaultHandler.Resolve(CreationContext context, Boolean instanceRequired)
   at Castle.MicroKernel.Handlers.AbstractHandler.Resolve(CreationContext context)
   at Castle.MicroKernel.Resolvers.DefaultDependencyResolver.ResolveFromKernelByType(CreationContext context, ComponentModel model, DependencyModel dependency)
   at Castle.MicroKernel.Resolvers.DefaultDependencyResolver.ResolveFromKernel(CreationContext context, ComponentModel model, DependencyModel dependency)
   at Castle.MicroKernel.Resolvers.DefaultDependencyResolver.ResolveCore(CreationContext context, ISubDependencyResolver contextHandlerResolver, ComponentModel model, DependencyModel dependency)
   at Castle.MicroKernel.Resolvers.DefaultDependencyResolver.Resolve(CreationContext context, ISubDependencyResolver contextHandlerResolver, ComponentModel model, DependencyModel dependency)
   at Castle.MicroKernel.ComponentActivator.DefaultComponentActivator.CreateConstructorArguments(ConstructorCandidate constructor, CreationContext context)
ghost commented 6 years ago

This does not help much.

Here is maybe something you can try. Once your container is full setup on application startup, could you set a breakpoint and inspect the debugger views for your windsor container and see you have any potentially misconfigured components? This is not a silver bullet but it might turn up something.

image

davidshen84 commented 6 years ago

@fir3pho3nixx

I checked my configuration like you said. There were a bunch of components listed in the misconfigured section, all are waiting for dependency for the typed factory facility. But the DatabaseProviderFactory class is not in this list.

I found the DatabaseProviderFactory class in the all components section. It has 3 constructors, the first of which depends on the System.Configuration.ConfigurationSection component which is not registered; whilist the parameter-less constructor is the last of the three.

I think the resolver blindly picked up the first one.

ghost commented 6 years ago

Interesting, then something that reproduces this would need to involve a typed factory somehow, could you factor this into your repro? It could yield a failing test which I can try and debug for you.

davidshen84 commented 6 years ago
using System;
using Castle.Facilities.TypedFactory;
using Castle.MicroKernel.Registration;
using Castle.Windsor;
using Microsoft.Practices.EnterpriseLibrary.Data;

namespace ConsoleAppWindsor364
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            var container = new WindsorContainer()
                    .AddFacility<TypedFactoryFacility>()
                    .Register(Component.For<DatabaseProviderFactory>())
                    .Register(Component.For<Database>()
                        .UsingFactoryMethod(kernel => kernel.Resolve<DatabaseProviderFactory>().Create("test")))
                    .Register(Component.For<IDummy>().ImplementedBy<Dummy>())
                    .Register(Component.For<IFactory>().AsFactory())
                ;

//            var database = container.Resolve<Database>(); // uncomment this to show the bug.
            var dummy = container.Resolve<IFactory>().Create();
            Console.WriteLine(dummy.GetName());
        }
    }

    public interface IDummy
    {
        string GetName();
    }

    public class Dummy : IDummy
    {
        public string GetName()
        {
            return "bug";
        }
    }

    public interface IFactory
    {
        IDummy Create();
    }
}

Indeed the TypedFactoryFacility is causing the error.

ghost commented 6 years ago

Let me add this as a failing test and get back to you. Thanks for the repro.

ghost commented 6 years ago

@davidshen84 - Have not had time to look at this. There are a fair few problems with TypedFactories in Windsor and we no longer have original committers with us. I have to choose my battles as it were, if there is anything you can do that would result in a pull request in the mean time that would be awesome.

ghost commented 6 years ago

@davidshen84 - I am going to collate an issue with everything I find wrong with typed factories. I am just working through the all the problems to understand it better.

I can see you are using Microsoft.Practices.EnterpriseLibrary.Data. I would prefer not to add this to the CastleTests library if possible. Here are my reasons why:

Could you kindly revise your example without the dependency?

davidshen84 commented 6 years ago

First, Microsoft.Practices.EnterpriseLibrary.Data is not official, but EnterpriseLibrary.Data is...they like to mess up with their users. :)

I created below code to try to reproduce this issue. The setup tries to create a environment that a factory class used in the UsingFactoryMethod clause has multiple constructors, one of which has unsatisfied dependency. However, below code does not reproduce this issue. Maybe the NuGet package is really to blame?

using System;
using Castle.Facilities.TypedFactory;
using Castle.MicroKernel.Registration;
using Castle.Windsor;

namespace WindsorBug
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            var container = new WindsorContainer()
                    .AddFacility<TypedFactoryFacility>()
                    .Register(Component.For<BugTriggerFactory>())
                    .Register(Component.For<Bug>()
                        .UsingFactoryMethod(kernel => kernel.Resolve<BugTriggerFactory>().GetBug()))
                    .Register(Component.For<IDummy>().ImplementedBy<Dummy>())
                    .Register(Component.For<IDummyFactory>().AsFactory())
                ;

            var bug = container.Resolve<Bug>();
            var dummy = container.Resolve<IDummyFactory>().Create();
            Console.WriteLine("cannot reproduce :(");
        }
    }

    public class BugDependency
    {
    }

    public class BugTriggerFactory
    {
        private readonly BugDependency _dep;

        public BugTriggerFactory(BugDependency dep)
        {
            _dep = dep;
        }

        public BugTriggerFactory()
        {
        }

        public Bug GetBug()
        {
            return new Bug();
        }
    }

    public class Bug
    {
    }

    public interface IDummy
    {
        void Test();
    }

    internal class Dummy : IDummy
    {
        public void Test()
        {
            throw new NotImplementedException();
        }
    }

    public interface IDummyFactory
    {
        IDummy Create();
    }
}
ghost commented 6 years ago

@davidshen84 - Update: we are prepping a release for AspNet DesktopClr facilities from November last year.

I need to move on to AspNet Core and then do the V5 deprecation/facility issues.

Any chance you could repro this without the dependencies? I would appreciate any help and guide you through the PR.

davidshen84 commented 6 years ago

@fir3pho3nixx, I finally have some time to look into this issue. Here is what I found:

When trying to resolve DatabaseProviderFactory type, the constructor resolver:

Also, the resolver stopped evaluating the 3rd constructor which has zero dependencies due the logic in Windsor\src\Castle.Windsor\MicroKernel\ComponentActivator\DefaultComponentActivator.cs, at around line 218. In the comment it says:

since the constructors are sorted greedier first, we know there's no way any other .ctor is going to beat us here

But I do not remember seeing anywhere in the code to sort the constructors. Please point me to the right file that has this sorting logic.

ghost commented 6 years ago

I think it is applying a weighting algorithm here: https://github.com/castleproject/Windsor/blob/master/src/Castle.Windsor/MicroKernel/ComponentActivator/DefaultComponentActivator.cs#L207

davidshen84 commented 6 years ago

@fir3pho3nixx,

I think I finally pinned down the cause of this issue to the logic in the DelegateFactory class but still have no clue how to solve this puzzle.

Firstly, it is very uncommon to have a function parameter in a constructor, which I think is why I am the only one complaining here.

    public DatabaseProviderFactory(Func<string, ConfigurationSection> configurationAccessor)
    {
      Guard.ArgumentNotNull((object) configurationAccessor, nameof (configurationAccessor));
      this.builder = new DatabaseProviderFactory.DatabaseConfigurationBuilder(configurationAccessor);
    }

When trying to resolve the input parameter configurationAccessor, the DefaultKernel went through great length to find a matching handler and eventually had to resort to an ILazyComponentLoader component, which is the DelegateFactory class.

The ILazyComponentLoader, which is a DelegateFactory instance, will try to automatically create a registration for the unknown service, which is the Func2<> generic function (please see line 52 at DelegateFactory.cs file).

  1. Why should we try to register an unknown service automatically? Isn't it risky?
  2. How the DelegateFactory managed to create a registration for an unknown service. It doesn't even know how the service is implemented.
ghost commented 6 years ago

Good work!

I think it is registering the unbound func because it assumes the generic parameter can be passed via the resolve. This does make things a little harder to reason about but is still valid.

I would need to look at the delegate factory a little more. Will get back to you on this.

davidshen84 commented 6 years ago

@fir3pho3nixx, I created a test case for this issue. Test_Bug_364 (could not come up with a better name).

In the test case, I created class Bug364ProviderFactory and UnregisteredClass to simulate the environment that DatabaseProviderFactory and ConfigurationSection had created that triggered this issue. However, the simulators do not trigger this issue. I think by comparing the differences of the two sets of classes, we can find out what's wrong with the resolver.

ghost commented 6 years ago

@fir3pho3nixx, I created a test case for this issue. Test_Bug_364 (could not come up with a better name).

Awesome.

I think by comparing the differences of the two sets of classes, we can find out what's wrong with the resolver.

Yes, a bit more time spent debugging would reveal a problem in the SelectEligibleConstructor method.

ghost commented 6 years ago

Another thing you can try if you find Windsor internals daunting is to write a custom component activator.

Please see: https://github.com/castleproject/Windsor/blob/master/docs/component-activators.md

What these docs don't tell you, is that you can change:

.Register(Component.For<Bug>()
.UsingFactoryMethod(kernel => kernel.Resolve<BugTriggerFactory>().GetBug())

To

.Register(Component.For<Bug>()
.UsingFactoryMethod(kernel => kernel.Resolve<BugTriggerFactory>().GetBug())
.Activator<MyActivatorThatFixesTheBugTriggerFactory>()

We should really fix the docs to make that obvious.

What do you think?

abbotware commented 5 years ago

I think I just hit this same issue - I had a ctor with 2 parameters... then I added 2nd ctor introducing a new Func<IQueryable<TEntity>, IQueryable<TEntity>> parameter and now all the typed factory does is complain about a missing unregistered service... even though the first ctor it has all the parameters for...

I was planning that anyone that wanted to use this additional parameter could use .DependsOn(Dependency.OnValue<Func<IQueryable<Type>, IQueryable<Type>>>( ... )) during registration...

now however, I have to register the seemingly optional parameter in all cases.

I supposed I can do the following:

  1. make the parameter optional instead of overloading the ctor
  2. introduce a 'class' dependency instead of a Func<T,T>

still though, it seems like the ctor resolver code has a flawed / greedy selector ?

abbotware commented 5 years ago

it would seem that using an optional parameter ie ctor(Func<T,T>= null) does not work ...

I actually came up with a 'third' option - create a derived class that basically serves no purpose other than a way to 'overload' the ctor... not the most ideal solution, but a work around for now...

davidshen84 commented 5 years ago

I think more people will hit this bug as generic lambda expression is getting increasingly popular in c#. However, from a Design Pattern's point of view, I think we should not have functions injected into classes. Simply replace the function injection with an interface injection could avoid this issue.

abbotware commented 5 years ago

It seems like there still is a bug in the resolver... so it should get fixed right?

I think we should not have functions injected into classes

Not sure I agree entirely with that... what about the typed factory? you have the ability to wire up ctor parameters directly...

factory.Create(x => something(x))

could create a class that had the following:

Ctor(Action<int> a, ILogger l)