Circular dependency resolution does not fully resolve classes passed to constructor injection

[GoogleCodeExporter]

I have noticed an anomaly in the way that Autofac handlers wiring of property 
injection compared to how I was used it being done in Ninject. For a lot of our 
stuff we use property injection because of circular references, as there is no 
way to resolve those without using property injection (I have tried!). So as a 
rule most of our primary data access, business logic and service classes use 
property injection, and they need to be wired up such that dependency can be 
circular. Now sometimes we have classes that need to be constructed on the fly, 
and in Ninjet we used the factory extension, but for Autofac it is conveniently 
built in using Func<IMyInterace> members. A lot of those classes were 
originally designed to only use constructor injection and relied on the 
interfaces injected into it being wired up when the constructor is called. 
However with Autofac that is NOT the case if those interfaces were wired 
internally with property injection.

Basically what would happen is I would have a constructor like this:

public MyClass(string arg1, int arg2, ISomeDepdency dep1, IAnothterDep dep2)
{
    // Do something with dep1 and dep2
}

But if dep1 and dep2 are set up to use property injection (in my case also 
using circular dependencies), dep1 and dep2 may not be actually wired up yet, 
so if I make calls in the constructor to dep1 or dep2 methods that internally 
use property injected dependencies, they will crash. This appears to happen if 
MyClass is the first class to consume dep1 and dep2 in the current scope, so 
they had to be created as part of creating the instance of MyClass. If they 
already existed, then it works correctly.

This is clearly a leaky abstraction layer because now MyClass has to care about 
HOW dep1 and dep2 were wired up, and hence I cannot do anything useful in the 
constructor. All I can do is save off all the parameters and then move the 
business logic that would have occurred in the constructor into a Load() 
function that is called whenever internal methods or properties are accessed.

In reality Autofac should be intelligent enough to know that it needs to 
COMPLETELY resolve dep1 and dep2 before passing them to MyClass. Unless there 
is a circular dependency to be found, this should be completely doable.

To illustrate the issue I have built a project that demonstrates it clearly, so 
you can compile and run it to see the problem.

Original issue reported on code.google.com by kendallb...@gmail.com on 7 Dec 2013 at 3:44

Attachments:

• CircularCrashTest.zip

[GoogleCodeExporter]

Ok, this is a super easy bug to fix. One line of code. At line 124 of 
ResolveOperation.cs it has the following:

    if (_activationStack.Count == 0)
        CompleteActivations();

For some reason it looks like someone decided it was a good idea to optimize 
the activation of completed objects until the very END of the class stack, ie: 
after the root class in a resolve operation has been created. This is where it 
all goes wrong, because we MUST complete the activation for every class when it 
is first created, so that if it is used in constructor injection the graph is 
totally complete for all dependent interfaces injected into the constructor. 
The code has already been written to work this way, in that it removes those 
objects it is completing the activation for so it only does each object once, 
and it fixes this problem.

Original comment by kendallb...@gmail.com on 7 Dec 2013 at 10:57

[GoogleCodeExporter]

A couple of unit tests fail if you make this change, specifically this one:

Autofac.Tests.Core.Resolving.ResolveOperationTests.CtorPropDependencyOkOrder2()

This test consists of the following classes:

    public class DependsByCtor
    {
        public DependsByCtor(DependsByProp o)
        {
            Dep = o;
        }

        public DependsByProp Dep { get; private set; }
    }

    public class DependsByProp
    {
        public DependsByCtor Dep { get; set; }
    }

This test will fail if you try to resolve DependsByCtor as DependsByProp will 
attempt to create DependsByCtor again as a dependency, but it requires itself 
as a parameter. IMHO this is a REAL circular dependency problem and SHOULD 
fail. Autofac should NOT allow for circular dependencies to be resolved to ANY 
classes where they are doing constructor injection, so I think this test should 
be changed to be a completely valid failure, and in fact it should test that 
this fails.

A new test should be written for my test case to ensure that circular 
dependencies within property injected classes will work correctly when those 
are passed into constructor injection classes, and will be fully resolved when 
the constructor is called.

I have re-written this test to pass like this:

        [Test]
        public void CtorPropDependencyFailsOrder2()
        {
            var cb = new ContainerBuilder();
            cb.RegisterType<DependsByCtor>().SingleInstance();
            cb.RegisterType<DependsByProp>().SingleInstance().PropertiesAutowired(PropertyWiringOptions.AllowCircularDependencies);

            var c = cb.Build();
            Assert.Throws<DependencyResolutionException>(() => c.Resolve<DependsByCtor>());
        }

Original comment by kendallb...@gmail.com on 7 Dec 2013 at 11:56

[GoogleCodeExporter]

It does break the MEF code though, because it needs that particular resolution 
code to run for it's own activation handlers for setting imports. The correct 
solution is to move the handling of property injection out of the 
Activated/Activating handlers I think and handle it directly in the resolution 
code?

Original comment by kendallb...@gmail.com on 8 Dec 2013 at 12:09

[GoogleCodeExporter]

Thanks for all the info, the repro, and the suggested fixes - great to have 
such a thorough analysis.

Autofac's been around for quite a long time - there's 300k downloads just on 
NuGet, with a lot of products also bundling it, and many, many built before 
NuGet came into vogue.

Making a change in this behaviour, while I think you can make some very good 
arguments for it, is a very risky proposition. There are almost an infinite 
number of ways that object graphs can be put together (some of them sane and 
others, well...)

I hope from that perspective it sounds reasonable that I'd vote to *won't fix* 
this issue; the benefit of closing it is that your code, which you already 
understand well and can adjust to suit the container, will work unmodified. The 
down-side is that potentially thousands of applications that might rely on the 
current behaviour could be broken, and for their maintainers there could be 
much higher barriers to understanding the problem and fixing 
already-established codebases.

I think the only viable way to approach "heart surgery" like this would be if 
we can guarantee that the only change will be failing code now working, and not 
for working code to break or change behaviour subtly.

There may be other ways around this that I'm not seeing, or elements of your 
solution that I'm misunderstanding, so please chime in if so!

Alex/Travis, just my 2c: have to jump in since I've spent a long time wrangling 
with these parts of the code :) Let me know if I can help with resolving this 
one in any way.

Original comment by nblumha...@nblumhardt.com on 8 Dec 2013 at 1:01

[GoogleCodeExporter]

I honestly do not think doing a wont-fix on this is a good solution, because 
IMHO this behavior is broken. Anyone coming from other IoC framework such as 
Ninject to Autofac will be expecting that dependencies are always fully 
resolved when they are passed into constructors via injection.

I do understand this is documented behavior but this kind of behavior can lead 
to some really subtle and difficult to track down bugs, such as stuff where you 
code works in one context but not in another, depending on whether the 
dependencies were already created in some other code prior to being needed. It 
is one thing for your code to ALWAYS crash with a circular dependency exception 
if you code is wrong at runtime, but it is another for it to work in some 
instances and fail in others. Then you can easily have situations where it 
works in your nicely constructed unit tests, and then fails in production.

Since this would be a breaking change for some people, can I suggest that this 
behaviour be added somehow such that it can be activated as needed? And then it 
could be documented on the wiki in the circular dependencies section how to 
turn this on and off, and exactly what it changes. I would be happy to write 
the docs if this change is live :)

Personally I would love to know specifically why this was done this way, and 
why someone would ever want to handle circular dependencies for constructor 
injection in normal use. In normal use if you need circular dependencies, you 
use property inject, and it fixes it completely.

I can see that with this change the MEF code unit test break when doing 
circular dependencies, but it is not clear to me how the MEF code works and why 
it even breaks. Someone more familiar with the code than me could probably 
figure that out.

Original comment by kendallb...@gmail.com on 8 Dec 2013 at 2:41

[GoogleCodeExporter]

Apologies for the response delay here; I rarely have access to a dev machine on 
weekends (work/life balance and Spouse Acceptance Factor issues). Plus, on 
really-deep internal stuff like this I feel it's important to give some good 
thought on it prior to making changes. I've been noodling on this since it 
first showed up in the forums.

As noticed, making this change will affect some of the unit tests and 
downstream functionality. If it was just us, I wouldn't mind as much, but as 
Nick points out, we have a lot of consumers that could break in insidious ways 
if we change this right now. By way of example, check out Issue #462. We added 
an *optional* parameter to an existing method and it broke some integration 
with another system.

We have to be really careful about API and functionality changes.

The other thing I've learned while working on the project is that doing simple 
things like this can have unpredictable effects. An example of that is Issue 
#397. I tried to add automatic disposal of child lifetime scopes when a parent 
scope is disposed (something we don't currently support) and inadvertently 
introduced a non-trivial memory leak issue. When we stepped back and looked at 
how it *really* needed to be fixed, it turned out to be better not to fix it, 
even though it logically makes sense to have the feature available.

Given all that, I have to defer to Nick's experience with this issue and 
augment that with my own experience in trying to fix some of these 
longer-established deep internal issues. For better or worse, this is how it 
works and I'm not sure we're able to change that.

I don't think I'd add it as an option since not only would it potentially be a 
gotcha for people trying to use the option (see above "unpredictable effects"), 
but it'd also be additional support burden (people running into issues when 
they switch up the activation behavior in certain edge cases, bugs/questions 
arising from the option, etc.).

I respect the fact that it's causing trouble in your codebase, though. 
Something I might suggest is making use of an OnActivating()/OnActivated() 
handler (see the LifetimeEvents wiki page: 
https://code.google.com/p/autofac/wiki/LifetimeEvents) to do the initialization 
calls rather than doing it in the constructor. That could bypass the problem 
you're having in the constructor where the object graph isn't completed to the 
state you like and still allow that initialization to happen prior to the 
object being used.

Anyway, I think I'm with Nick here. Alex can chime in if he feels differently, 
but for now I'm going to mark this one "won't fix."

Original comment by travis.illig on 9 Dec 2013 at 5:43

• Changed state: WontFix
• Added labels: Module-Core

[GoogleCodeExporter]

I can look into OnActivated to initialize the objects. It is not a clean 
solution, but at least we could code around it. 

What concerns me the most with this is that this bug already bit me in the ass 
once, and then it bit another of my developers in the ass as well. So unless I 
properly train our developers on NOT EVER relying on injected parameters to a 
constructor to be fully initialized, it WILL come back and bite us again. 
Someone is going to do it again because it makes sense sometimes to do it that 
way. That is what concerns me the most about this.

And as much as I would like to never use property injection and circular 
dependencies, in large projects like ours you just cannot get away from it. 
Allowing circular dependencies allows you the freedom to build loosely coupled 
code that just depends in interfaces that can mirror how code would have been 
written prior to using an IoC container. Not everything fits cleanly into a 
single inheritance object oriented model, which is what requiring constructor 
injection forces you into.

As for making it an option, it could easily be an option to the builder by 
adding a new API call to enable it, which would mean it would not affect any 
code unless someone makes the call to turn it on. This would fix it 100% for 
me, and should have zero impact on anyone else. And I suspect you will find 
over time a LOT of people will want this option. 

Is there anything I can do to convince you guys to add this option in there for 
me?? I would code it myself but I don't want to maintain my own fork of Autofac.

Original comment by kenda...@amain.com on 9 Dec 2013 at 10:19

[GoogleCodeExporter]

Looking at what the lines you mentioned do...

   if (_activationStack.Count == 0)
        CompleteActivations();

What this chains into meaning is: Once all the components are initially 
created, do all the activation events (which includes property injection). 
Without switching Autofac over and doing a lot of investigative testing to see 
what the long-term effect of this is, I can imagine, for example, that 
completing the activations (running those events before all the activations are 
done) could result in, say, some properties on objects not getting injected 
when they should. Or it could inadvertently affect the way some folks' custom 
handlers work. (Again, I've not verified that, but it's the kind of thing I can 
guess, off the top of my head, that could happen.)

Adding it as an option, as mentioned before, increases the support and testing 
effort in a non-trivial and hard-to-quantify way. Even if you're the first 
person to take the option and use it, we would need to implement it and do 
detailed analysis of the effects of using that option; we'd need to document 
the option and gotchas around it, if any; and we'd need to be ready to start 
fixing defects people out there find as they start using it. And, of course, 
once it's out there and part of the public API, it's a breaking change to pull 
it back out if it turns out to be problematic in a way we didn't foresee.

I recognize you've run into troubles with this, and I am sorry for that. By way 
of comparison, however, this is honestly the first time in all my years of 
working with Autofac that I've heard of anyone having trouble with this. I 
personally work on large distributed banking systems - millions of lines of 
code, a few hundred developers, huge institutions, apps of all types from MVC 
to WCF and back - and we've never run into this, either.

At this time I think it's best to leave this as Won't Fix. Again, if Alex or 
Nick want to chime in with more insight, that's cool; but it sounds like Nick 
fought this fairly diligently for quite some time and ended up here, so it 
would probably be up to Alex to vote for further investigation or a fix.

Original comment by travis.illig on 9 Dec 2013 at 11:29

[GoogleCodeExporter]

This feels like a bugberg to me: it looks small on the surface but there is 
actually a lot more to it below. I don’t think changing the behaviour at this 
point is in the interest of the wider Autofac user base. We will never get all 
the behaviours to be exactly what everyone wants all of the time. The fact that 
this has never come up before suggests to me that it is not a common case. I 
also use Autofac in complex industrial automation software and haven’t run 
into this issue, even retrofitting it into a large and existing code base.

I acknowledge that Kendall has a legitimate point but agree this one needs to 
stay as won’t fix. Making it optional still opens surface area for potential 
issues and confusion that would impact other users. As much as I love Autofac 
and personally wouldn't use anything else, if there is another container that 
better meets your specific needs, it may be worth using that instead of 
fighting against the current.

Original comment by alex.meyergleaves on 10 Dec 2013 at 1:46

[GoogleCodeExporter]

I would like to re-open this bug as I have a different proposed solution, that 
passes all existing unit tests. All the code will shortly be checked into my 
fork in Mercurial. 

Basically it dawned on me when looking at the code some more that injecting the 
properties in ALL cases to support property-property dependencies just needs to 
be done AFTER the object is activated, but before the completion call is made. 
I modified my tree to use a different event handler (I called it 
InjectProperties) that does nothing but inject properties and is only set up by 
the PropertiesAutowired fluent function when setting up the registration. It 
now uses that for the default case, and only uses the Activated handler if you 
specifically request circular dependencies. If you do that way, the code 
actually works for ALL cases in the default case, and handles circular 
dependencies just fine. So ResolveOperation now looks basically like this:

    CircularDependencyDetector.CheckForCircularDependency(registration, _activationStack, ++_callDepth);

    var activation = new InstanceLookup(registration, this, currentOperationScope, parameters);

    _activationStack.Push(activation);

    var handler = InstanceLookupBeginning;
    if (handler != null)
        handler(this, new InstanceLookupBeginningEventArgs(activation));

    var instance = activation.Execute();
    _successfulActivations.Add(activation);

    _activationStack.Pop();

    activation.InjectProperties();

    if (_activationStack.Count == 0)
        CompleteActivations();

So the old behavior of not injecting properties until after all objects in the 
graph are activated is still supported, which means it also correctly supports 
property-constructor dependencies the same as before. However the default 
behavior moves the injection of properties from inside the Execute() (basically 
one of the last things it does in there) to immediately after it. But if you do 
it _after_ the call to _activationStack.Pop(), the circular dependencies are 
all handled neatly in the normal case of property-property dependencies. I even 
modified the unit tests to make sure that is indeed the case, and it is. No use 
code else can run in between Execute() and the InjectProperties() call so the 
existing behavior is not changed. It just means I no longer have to write any 
of my classes with PropertyWiringOptions.AllowCircularDependencies, because you 
ONLY need that if you want the old behavior of supporting property->constructor 
circular dependencies.

The end result is that this change will completely fix my problem, will have 
ZERO impact on existing developers not needing circular dependencies and 
developers who use PropertyWiringOptions.AllowCircularDependencies get the old 
behavior so nothing changes there either.

Note that the documentation on circular dependencies will need to be updated, 
but if you accept my changes I will gladly update that page for you.

Original comment by kendallb...@gmail.com on 11 Dec 2013 at 3:32

[GoogleCodeExporter]

I don't think this system supports comment editing. This:

No use code else can run in between Execute() and the InjectProperties() call 
so the existing behavior is not changed.

should read:

No user code can run in between Execute() and the InjectProperties() call so 
the existing behavior is not changed.

Original comment by kendallb...@gmail.com on 11 Dec 2013 at 3:35

[GoogleCodeExporter]

Ok, I finally figured out Mercurial and got my changes pushed to my clone on 
Google Code. You can see the change set here:

http://code.google.com/r/kendallb1015-autofac/source/detail?r=11ac5c16ff7d220363
2a8fff3ad4f50fea74901e

Original comment by kendallb...@gmail.com on 11 Dec 2013 at 4:18

[GoogleCodeExporter]

It looks like a nice solution, and I don't see on the surface that it would 
cause any issues. It appears to just boil down to a slightly better isolation 
of exactly when property injection takes place, though it doesn't change the 
overall order of operations. Very slick.

I'd like to hear from Alex and/or Nick on it before taking it in, though. When 
changing stuff this deep and subtle I'd like more eyes on it.

I'll switch the bug to an open status so we can be sure to check it out.

Original comment by travis.illig on 11 Dec 2013 at 5:04

• Changed state: Accepted

[GoogleCodeExporter]

Thanks. My analysis so far is nothing else runs that would be affected between 
where I moved property injection from to where it is now. Hopefully I did not 
miss anything :)

Original comment by kendallb...@gmail.com on 12 Dec 2013 at 6:12

[GoogleCodeExporter]

Ok, so there is one snag I have run into. The fix itself works great, but it 
does not actually fix all cases. The problem is when you are using interfaces, 
because internally Autofac registers the defining class as well as the 
interface, so if you have a class ISettings and it is registered like this:

builder.RegisterType<Settings>().As<ISettings>().PropertiesAutowired().InstanceP
erLifetimeScope();

If you request ISettings internally Autofac resolves the Settings class when 
ISettings is requested, and the properties are injected into the Settings class 
not ISettings. So ISettings is still in the resolution stack when Settings is 
being injected, and hence if someone is requesting ISettings as part of a 
circular dependency being injected into the Settings class, it will fail 
because ISettings has not yet been fully resolved. This is different to Ninject 
because the defining class is not actually resolved itself in Ninject, only the 
interface (keeps it cleaner as you can then ONLY resolve interfaces, not 
defining classes which you really don't want anyone actually resolving unless 
you explicitly define it).

Of course this all works if you set the circular references flag, but that does 
not help me solve my problem :)

I can build a unit test to make this fail in this case so I will work on that 
first, so I can then investigate how this can be fixed. I have a sneaking 
suspicion that the only issues is the way circular dependencies are checked for 
in the activation stack, and if I disable this everything will run correctly. 
So the fix might be simply finding a better way to track down circular 
dependencies and somehow ignoring Interface->Instance boundaries...

Original comment by kendallb...@gmail.com on 12 Dec 2013 at 8:45

[GoogleCodeExporter]

Sounds like this needs to be looked into some more before we implement the fix. 
Keep looking at it and let us know what you come up with. Subtle issues are 
fun. :)

Original comment by travis.illig on 12 Dec 2013 at 8:53

[GoogleCodeExporter]

Hmm, I was wrong about the defining class being accessible via Resolve. And I 
cannot seem to build a simple unit test that fails, but I am getting a failure 
in my real code. So now I have to dig into what is failing in the real code and 
try to replicate it in a unit test so I can make it fail and then look into why 
it fails. It could be something to do with how I have registered everything...

Original comment by kendallb...@gmail.com on 12 Dec 2013 at 9:44

[GoogleCodeExporter]

Something is odd in how I am seeing things work in my code. All the unit tests 
work, and in fact I can resolve my ISettings interface just fine from the root 
scope (with everything set to InstancePerLifetimeScope(), but when I try to 
resolve the same ISettings interface from request scope inside a web request, 
suddenly it is causing a circular reference (only without turning on circular 
references of course). It is not clear at all what is different in these two 
cases? I will keep debugging and figure it out ...

Original comment by kendallb...@gmail.com on 12 Dec 2013 at 11:08

[GoogleCodeExporter]

Per our email discussions, I'm going to mark this as a Milestone 4.0 issue so 
we know to come back to it at the right time.

Original comment by travis.illig on 17 Dec 2013 at 4:07

• Added labels: Milestone-Release4.0

[GoogleCodeExporter]

Original comment by travis.illig on 17 Dec 2013 at 4:07

[GoogleCodeExporter]

Moved issue to GitHub: https://github.com/autofac/Autofac/issues/477

Subsequent issue management will be held there; closing the issue on Google 
Code as "WontFix" because we will handle issue resolution on GitHub.

Original comment by travis.illig on 11 Feb 2014 at 12:01

• Changed state: WontFix