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

[alexmg]

From kendallb...@gmail.com on December 07, 2013 13:44:15

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 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.

Attachment: CircularCrashTest.zip

Original issue: http://code.google.com/p/autofac/issues/detail?id=477

[alexmg]

From kendallb...@gmail.com on December 07, 2013 14:57:20

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.

[alexmg]

From kendallb...@gmail.com on December 07, 2013 15:56:44

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>());
    }

[alexmg]

From kendallb...@gmail.com on December 07, 2013 16:09:38

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?

[alexmg]

From nblumha...@nblumhardt.com on December 07, 2013 17:01:51

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.

[alexmg]

From kendallb...@gmail.com on December 07, 2013 18:41:31

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.

[alexmg]

From travis.illig on December 09, 2013 09:43:15

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."

Status: WontFix
Owner: travis.illig
Labels: Module-Core

[alexmg]

From kenda...@amainhobbies.com on December 09, 2013 14:19:03

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.

[alexmg]

From travis.illig on December 09, 2013 15:29:06

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.

[alexmg]

From alex.meyergleaves on December 09, 2013 17:46:23

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.

[alexmg]

From kendallb...@gmail.com on December 10, 2013 19:32:47

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.

[alexmg]

From kendallb...@gmail.com on December 10, 2013 19:35:25

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.

[alexmg]

From kendallb...@gmail.com on December 10, 2013 20:18:39

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=11ac5c16ff7d2203632a8fff3ad4f50fea74901e

[alexmg]

From travis.illig on December 10, 2013 21:04:48

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.

Status: Accepted
Cc: alex.meyergleaves

[alexmg]

From kendallb...@gmail.com on December 11, 2013 22:12:58

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 :)

[alexmg]

From kendallb...@gmail.com on December 12, 2013 12:45:01

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().As().PropertiesAutowired().InstancePerLifetimeScope();

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...

[alexmg]

From travis.illig on December 12, 2013 12:53:28

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. :)

[alexmg]

From kendallb...@gmail.com on December 12, 2013 13:44:52

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...

[alexmg]

From kendallb...@gmail.com on December 12, 2013 15:08:25

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 ...

[alexmg]

From travis.illig on December 17, 2013 08:07:18

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.

Labels: Milestone-Release4.0

[alexmg]

From travis.illig on December 17, 2013 08:07:27

Owner: ---
Cc: -alex.meyergleaves

[tillig]

This may or may not tie into the newly opened design-based issue #798.

[tillig]

This thing has been open for nearly four years now. Reading back through it I think it may just be something we have to live with, known-issue style. Circular dependencies, which should largely be avoided anyway, plus property injection... seems pretty edge case, like something to be fixed in the consuming code more than something we should work around in Autofac. I can imagine even if we tried to fix it, there will still be weird issues. For example:

public class A
{
  public B B { get; set; }
}

public class B
{
  public B(A dep)
  {
    // Uh oh, the circular dependency trying to be consumed
    // is actually the thing being constructed _right now_
    dep.B.DoSomething();
  }
}

This sort of falls into the same thing - the constructor parameter in a circular dependency hasn't been 100% initialized.

Given all that, I think I'm going to close this for now. If folks run into additional issues, Chip in on the discussion at #798.