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

Why should UsingFactory be avoided and what improves on it? #130

Open jnm2 opened 8 years ago

jnm2 commented 8 years ago

My application has low level general-purpose services that depend on SqlConnection and DbConnection and a high level UserAccountService with a CreateConnection method. (UserAccountService manages SecureString credentials.)

It took a long time searching through the documentation to figure out a way to make this work. The low-level services cannot take a dependency on UserAccountService and they also cannot take a dependency on a typed factory. They must be injected with SqlConnection and DbConnection.

I finally found the ideal configuration method for this. This is precise and minimalist and I could not have asked for a better config function:

Component.For<DbConnection, SqlConnection>()
    .UsingFactory((UserAccountService userAccountService) => userAccountService.CreateConnection())
    .LifestyleTransient()

My concern is that the documentation also states that UsingFactory should be avoided and is going away in future versions.

Why is this? First, what's the downside to UsingFactory that I should be aware of? Second, is there a paradigm that satisfies the criteria I have in an even more precise and minimalist fashion?

dalefrancis88 commented 8 years ago

Hey,

It says it in the documentation. You should be using "UsingFactoryMethod" instead.

I did remember seeing the reason a while back but can't seem to find it now

jnm2 commented 8 years ago

Duh, misread it. Thank you!

So UsingFactoryMethod feels dirty:

Component.For<DbConnection, SqlConnection>()
    .UsingFactoryMethod(serviceLocator => serviceLocator.Resolve<UserAccountService>().CreateConnection())
    .LifestyleTransient()

Either UserAccountService never gets released because its resolve count goes through the roof, or there is less-than-clear magic going on and Castle Windsor understands that this resolve is being called by a UsingFactoryMethod delegate. Either way this service location feels dirty compared to the implicit goodness of UsingFactory where those details are abstracted away.

For example, how do I know I'm not really supposed to be writing this:

Component.For<DbConnection, SqlConnection>()
    .UsingFactoryMethod(serviceLocator =>
    {
        var dependency = serviceLocator.Resolve<UserAccountService>();
        try
        {
            return dependency.CreateConnection();
        }
        finally
        {
            serviceLocator.ReleaseComponent(dependency);
        }
    })
    .LifestyleTransient()

Which is what I assume UsingFactory would do for me, only it could be even smarter than that and it probably should only release the dependency when the dependent component is released. That's not strictly necessary in this one case with SqlConnection but it would make me more comfortable- it's very likely that the thing you want to inject on its own could be tied to the lifetime of the service it came from.

So in the context of my original concern: what is the downside of UsingFactory and how is UsingFactoryMethod any kind of improvement?

dalefrancis88 commented 8 years ago

Hey,

The way I understand it is that the component uses the method you define as a component resolver within the dependency resolution graph. So its not a service locator either. Kernel is an internal component to castle windsors container.

You have a few other options that you can use that would solve this without the need for factories. The most simple one would be to just inject the UserAccountService into wherever you need the connection and then create the connection and use a using statement instead, not everything has to be tracked by the container. This option is not as bad as you may initially think, connections should be open for only as long as is needed. If thats not doable, i.e. you want the connection for the life of the component, still inject the UserAccountService but have the component that relies on it, implement IDisposable and close and dispose of the connection there. CW will call dispose.

A second option would be to create your own service/wrapper "IConnection" around the connection object. You could have it implement IDisposable and IStartable interfaces and inject the UserAccountService. When started it could create a local instance of the connection object and then obviously close/dispose in the dispose method. You can then inject you new IConnection into any component that needs it. This is mostly moving the logic however it gets away from using the factory method.

There are a few other CW treasures that you could use to solve it but mostly what i am getting at is that if you don't like or want to use something then you can just design around it. Most if not all factory methods can be moved to there own components.

On the down or updside of either, I wouldn't know. I've never had a need to consider it. For the most part i believe they're rarely used, but thats just in my own experiences

IanYates commented 8 years ago

If you dig into the Windsor source, or even just start inspecting the IKernel instance in the debugger (what you've labelled serviceLocator in your lambda) you'll see there's the concept of a Burden for a component. Issue #100 which dug into how these burdens are calculated. Anything the Kernel resolves whilst constructing your instance, such as what you may get it to do in that UsingFactoryMethod() callback, will be added to the burden for you. So lifetime & disposal gets managed that way. You can opt-out by using the managedExternally flag (again discussed in that #100 thread)

jnm2 commented 8 years ago

@dalefrancis88 Those options are not feasible like I said because I can't modify those lower-level services to accept a UserAccountService or an IConnectionFactory because they are in a different codebase. I do actually need to inject a straight-up SqlConnection.

@IanYates that's good to know. I think that should be specifically documented here and everywhere you can explicitly Resolve without pairing with an explicit Release. That's an important rule of thumb to understand and know the boundaries of.

So what you're saying is that it behaves exactly as I would hope- the factory lives as long as the generated component. So far so good. So to deal with my remaining concerns:

I love this. It feels good and is immediately familiar, easy and comprehensible because it is in actuality dependency injection. It purely and almost declaratively defines the relationship between SqlConnection and UserAccountService:

Component.For<DbConnection, SqlConnection>()
    .UsingFactory((UserAccountService userAccountService) => userAccountService.CreateConnection())
    .LifestyleTransient()

This is strongly distasteful because it is in actuality service location. I want to follow the 3 R's pattern; I want to write Resolve one time in my entire codebase. Not here. On top of that, it's just more clunky to type and read:

Component.For<DbConnection, SqlConnection>()
    .UsingFactoryMethod(kernel => kernel.Resolve<UserAccountService>().CreateConnection())
    .LifestyleTransient()

So why avoid UsingFactory? Why is UsingFactory not in fact better recommended, since IKernel could be injected with UsingFactory too if you need it for some other reason?

mario-d-s commented 6 years ago

@jonorossi @fir3pho3nixx This issue has not had activity for two years. I do think @jnm2 raises a valid concern here about UsingFactory vs UsingFactoryMethod.

Why do the docs state not to use UsingFactory, and what is the real intended alternative?

By the way, is there any reason UsingFactoryMethod doesn't have the "releasing delegate" return mechanism that DependsOn has? I have wanted it in the past and it would seem the ideal solution in this case.

EDIT: Trying to code that up, I realize the API is awkward since UsingFactoryMethod already returns the component it constructs.

ghost commented 6 years ago

@mario-d-s Windsor Factories* are on my hit list as soon as I am done with the ASP.NET stuff. They most definitely need a complete re-write as far as I am concerned. The lifestyle violations for factories alone are stacking up on this issue tracker. They are also incredibly hard to solve.

jnm2 commented 5 years ago

Oops, I didn't realize this was tracking work. Sorry!