Better circular dependency handling #575
Conversation
…s with circular dependencies and property injected classes are always properly injected prior to being passed to the constructor of a constructor injection. Hence you can have classes that use constructor injection that can use any instances with circular dependencies passed to them as long as they are using property injection.
|
Can the separate |
| @@ -110,7 +112,14 @@ public object GetOrCreateInstance(ISharingLifetimeScope currentOperationScope, I | |||
|
|
|||
| var activation = new InstanceLookup(registration, this, currentOperationScope, parameters); | |||
|
|
|||
| _activationStack.Push(activation); | |||
| // Do not add to the circular dependency resolution stack unless the activator is a reflection activator (ie: generates the actual instance) | |||
| bool isRealActivation = (activation.ComponentRegistration.Activator is ReflectionActivator); | |||
There was a problem hiding this comment.
I'm not so sure about allowing the ResolveOperation "know" about the activator types. It might be better to have some sort of property on the activator that indicates whether it natively supports property injection and just have the ReflectionActivator be the only one that returns true. Or something like that.
There was a problem hiding this comment.
Yes, I would agree. At the time I wrote it I did not want to get too deep into the bowels but there should be some other mechanism to know if it is real activation or not as opposed to one from reflection. I did not know the best way to do it, so that is what I came up with at the time :)
There was a problem hiding this comment.
Not sure the level of abstraction's quite right - you can do this:
builder.Register(c => new Foo())
.OnActivating((c, e) => e.Instance.SomeProperty = c.Resolve<IBar>());
Although it looks different on the surface, this is not really materially different in needs or capability to using ReflectionActivator - it's just "hand coding" the interactions with the instance being created.
Anything that extends the capabilities available to reflection activated components should probably be available to delegate registrations too.
|
Given that this would be an API change, I definitely think @alexmg should take a look here, too. |
|
Sure the circular test project can be removed. I believe I covered the cases with unit tests already, so mostly it was there for my own sanity to make sure what I needed actually works. I can double check the unit tests to be sure. |
|
Breaking changes are a bit of a concern here and something we need to manage. The changes to the API aren't too bad with I'm less comfortable with changing the default behaviour for |
| @@ -149,6 +150,11 @@ public InstanceSharing Sharing | |||
| public ICollection<EventHandler<ActivatedEventArgs<object>>> ActivatedHandlers { get { return _activatedHandlers; } } | |||
|
|
|||
| /// <summary> | |||
| /// Handlers for the Inject Circular Properties event. | |||
| /// </summary> | |||
| public ICollection<EventHandler<InjectPropertiesEventArgs<object>>> InjectPropertiesHandlers { get { return _injectPropertiesHandlers; } } | |||
There was a problem hiding this comment.
It would be good to try to line this up with the Preparing/Activating/Activated flow that constructs the graph of components.
Although there are cases that it's not ultimately flexible, the phases are somewhat explicable in terms like: preparing is before any activation takes place and prerequisite dependencies are gathered; activating is the phase where prerequisite dependencies (e.g. via ctor) have been filled and non-prerequisite dependencies are gathered; activated is where cycles between non-prerequisite dependencies are completed.
Like I said, it's not perfect, but it works in terms of a dependency graph and the operations made while constructing it. Properties themselves aren't a first-class concept at this level.
Is there a way to name/visualize this in terms of graph operations rather than the underlying instance's features? If so that might also help resolve some of the design points around how it fits in with ReflectionActivator and friends.
(Just in case it sounds a bit theoretical to approach it this way - it's not that uncommon to see components made up of multiple instances, though you sometimes need to squint to see how they work :))
There was a problem hiding this comment.
The way I implemented it was simply due to not wanting to touch too much stuff, and not having a good enough understanding of how all the activation stuff works. The most important property of what I did is making sure that all the properties are properly injected BEFORE anything is passed into a constructor injected object. I am sure there is a better way of doing this, and you guys who know a lot more about the internals can probably do it better and cleaner. The main reason I included my crash test code was so you would have something that explains the scenario I was trying to solve, so my code would work. It really does need to get moved into a better unit test; it is possible the tests I wrote already cover it, but I left the crash code in there as an example.
|
Alex, the primary reason why I changed the default behaviour of PropertyWiringOptions is simply because the single biggest reason why people choose to use property injection over constructor injection, is to get around circular dependencies. There is no other way around it, and in every other IoC framework I have used, property dependencies always get fully resolved prior to calling any constructors for constructor injected objects. Hence I would be extremely surprised if there was any code out that that is using property injection that would somehow break if properties are wired up before constructors are called. If someone is using purely property injection, then the ordering of injecting the property values is immaterial because they will all be wired up when the root of the graph they are requesting is instantiated as no constructors will be called. I do understand that is does change the behaviour, but personally I don't see any situation where this could actually be a breaking change. However for the current code I am find with changing it so it uses a different name for the new behaviour and I can use those calls to wire stuff up myself. But I would highly recommend that for 4.0 the default behaviour is changed to work the same as all other IoC frameworks. |
|
Given this has been open a while and a lot has changed with the .NET Core support and v4.0, I'm going to close it and we will use the great work here as a jumping-off point for potential improvements to the updated code base. I've started an issue at #798 to get some discussion and ideas going around ways to improve circular dependency. |
Better circular dependency handling so property injection ALWAYS works with circular dependencies and property injected classes are always properly injected prior to being passed to the constructor of a constructor injection. Hence you can have classes that use constructor injection that can use any instances with circular dependencies passed to them as long as they are using property injection.