Improve circular dependency handling (general discussion / design)

[tillig]

We have seen issues and PRs come in about changes desired for circular dependency handling. We had every intention of making some improvements in time for 4.0 but, unfortunately, .NET Core "ate our lunch," as it were.

Much has changed since v4.0 and the .NET Core support was released so we're closing the following PRs so they can be later reimplemented (if desired) and/or used for ideas for improvement:

• Bug in CircularDependency tracking #579: This PR moved the check for circular dependencies to after where the instance lookup occurs. However, there was no unit test associated that could show the issue that this was intended to fix. It appears innocuous, but without understanding the specific case this fixes we declined to take it.
• Better circular dependency handling #575: This PR improved circular dependency handling to always work on properties. It also changed the default behavior of PropertyWiringOptions and broke the IComponentRegistration interface.

There are comments in #575 indicating ideas on better ways to handle improvement of the circular dependency handling. It is potentially non-trivial and breaking.

We invite folks interested in improving or changing the circular dependency handling to comment on this issue and let us know challenges they've seen or improvements they'd like to see. Note a failing unit test or two would help so we can understand in a more concrete fashion the challenges being seen.

[JackGrinningCat]

Hi, I have an circular dependency (A<->B) and expected that A.OnActivated() would give me fully initialized component back as instance pointer with B as property set.
I reread the documentation and i know that 'fully constructed' is not 'fully resolved dependency' here.
I will come up with another solution but as far as I'm concerned i wish for such a feature. But maybe I'm not done with some principle of Dos and Don'ts of DI.

Edit:
Well I solved my problem with an construction dependency that both (A and B) shares (C). As improvement to 'OnActivated' I use IEnumerable. Then any extending methods can be just attached. And it seems to work like a charm.
Well here a thanks for the good work.

class A
{
A( IEnumerable cs)
prop B
}

class B
{
B( IEnumerable cs)
prop A
}

class C
{
MethodForA
MethodForB
}

[bstewart00]

The biggest pain point for me with the current circular detection design is the hard-coded stack limit of 50. I work on a large application with a very deep dependency graph that frequently hits this limit despite no actual circular dependencies.

We've hacked around it so far by "resetting" the stack limit by injecting a Lazy registration in the middle of the stack somewhere. I would like the limit to at least be configurable in the container builder instead of it being arbitrary.

[tillig]

Based on some recent discussion in #924 and issue #927 I was thinking about how we might change the circular dependency detection to remove the need for any sort of hard-coded stack limit.

For example, I'm curious if each resolve operation might carry with it a context that includes the set of services that have already been resolved during that operation. If you see the same service twice in the same operation, it's a circular dependency. Maybe even if you see the same component (concrete class, etc) rather than just the same service then it's circular. The "allow circular dependencies" behavior would then change to basically be "if you see this twice, just don't throw."

I think the new decorator mechanism @alexmg is working on also has a similar tracking thing that was added. Perhaps that whole resolve context thing could be used in a more widespread manner.

Something like that would be a big improvement as far as circular dependency detection but I can see these possible drawbacks:

• Performance overhead: tracking and checking the list of services in the chain would be pretty fast but not free.
• Possibly breaking for edge cases: people can get into some crazy areas where they "work around" the circular dependency detection or inadvertently circumvent it by accident; this would likely catch things that weren't previously caught and may cause a few existing things to break because of it.

[tillig]

IComponentRegistration does carry an Id property with it. I wonder if just tracking the list of GUIDs over the course of the resolve operation would be enough. That would at least keep the memory consumption down - no flowing contexts and things, just a list of GUIDs.

[tillig]

Aggregating some things from other issues to consider as we look at this...

• Circular component dependency detected with manual IEnumerable<T> registration #648: If someone specifically registers IEnumerable<T> that doesn't play nicely with current support for collections and can cause a circular dependency.
• Instance Creation Issue with Property injection and lazy instantiation #718: Use of Lazy<T> and Func<T> defer resolution of values and can break or challenge circular dependency detection.
• InvalidOperationException from Owned<T> lifetime scope circular dependency #927: Use of Owned<T> creates a nested lifetime scope that can break or challenge circular dependency detection.

[bluetentacle]

Is there an easy way of determining how deep a dependency graph is? I could use reflection on the final dependency graph but it's not always reliable. It would be nice to have a way of how many resolutions are performed when I call Resolve. Thanks.

[tillig]

@bluetentacle This may help.

[VonOgre]

I have no idea what the performance implications would be, but could RuntimeHelpers.EnsureSufficientExecutionStack() be leveraged to remove the hardcoded static? I'm assuming it's possible if someone was deep in recursion before calling Resolve(), that the limit of 50 could even be larger than the remaining stack capacity before a StackOverflowException is thrown?

[tillig]

Hmmm, I didn't even know that was a thing, but it appears interesting.

It looks like it just determines if a single function call can be made, where we do have more of a recursion where it's like A -> B -> C -> A in the resolution chain, but there may be some sort of lever there or thread we can pull to be more dynamic about it. Nice find!

[alistairjevans]

It's an interesting method, but I suspect that by the time you get down to the point where that method throws, Autofac should maybe have already stopped?

Is it cheaper than incrementing and checking a counter? (Knowing the C# compiler team, it probably does some native stuff that means it actually is...)

At least it gives us a hard limit though.

[VonOgre]

I hadn't yet looked at the circular dependency detector in any sort of detail, but was looking into how one might be able to measure remaining call stack space and stumbled across that and thought that it (or possibly the Constrained Execution Regions) could be an interesting avenue to pursue 🤷‍♂️

I might poke around a bit more over the weekend, in case there are some hidden goodies to learn about :)

[alistairjevans]

The stack size limit is potentially less problematic than the fact we have to search the stack of in-progress resolves each time we resolve to check for the same registration.

There may be more efficient ways to check that.

[VonOgre]

That definitely makes sense. I only brought up the method as a potential alternative to investigate to the magic number limit before failing, since it seemed that was mostly to ensure that a StackOverflowException wasn't encountered, which is a thread-killing exception. If it can be calculated or discerned on the fly, based on the context where Resolve() is initially called, at least it's more an environmental constraint than a programmatic one.

I wouldn't be surprised if there were more efficient routes to checking whether the overall resolution chain will circle back on itself and is the more worthwhile end to pursue

[alistairjevans]

What are the thoughts on whether there is anything more to do on this? #1148 introduced changes to how stack depth protections are applied, and how circular dependencies are detected when decorators are involved, introducing the segmented stack.

I'd propose that we close this issue, draw a line under the current history of our circular dependency work, and revisit it if/when there is a problem or performance improvement needed.

[tillig]

I agree, I think the v6 changes will address this about as much as we can, at least for now. If someone sees something additional that can be improved we can open a new issue with that specific proposal.

[kendallb]

It does not. My original crash test code I submitted a decade ago STILL crashes.

#575

I will work on seeing how things are done today with an eye towards resolving it yet again, but we just got bitten by this bug in production and I now have to quickly find a solution and roll our own version of Autofac because we have new code we cannot roll out into production due to complaints from Autofac about circular dependencies.

The entire point of doing property injection is to allow circular dependencies, and the solution I wrote a decade ago was pretty simple and entirely fixed the problem for me. Its just a matter of making sure that the properties are always properly injected prior to being passed to the constructor of a constructor injection.

Our code base is riddled with circular dependencies and its impossible to remove them. Believe me, a decade ago everyone got on my case about how you should never need circular dependencies, but honestly I think anyone that truly believes that has never worked in a large production code base before.

[kendallb]

Shoot, debugging it I see the problem. Our code is still stuck on .NET 4.8 (its a big project to migrate to .NET 6+ which has been in progress for a few years now but it's gonna take time). Anyway, the problem is here:

Autofac/src/Autofac/Core/Resolving/Middleware/CircularDependencyDetectorMiddleware.cs

Line 61 in ec4907f

// Pre .NET Standard 2.1 we just end at 50.

For .NET Standard 2.1 and later it keeps trying until it runs out of stack space, but since that function does not exist in .NET Standard 2.0 (which unfortunately is what .NET 4.6.2 and later targets) it just stops at 50. So as our project has gotten bigger we suddenly hit this limit.

Clearly using a larger value will fix the issue, but short of submitting a patch to change the random choice of 50 to something larger, is there a way I can override the default CircularDependencyDetectorMiddleware myself to construct it with a different default?

It is referenced here as a static instance of the CircularDependencyDetectorMiddleware.Default property, but I don't see a way to replace that?

Autofac/src/Autofac/Core/Resolving/Pipeline/ServicePipelines.cs

Line 18 in ec4907f

CircularDependencyDetectorMiddleware.Default,

[kendallb]

Note that the original issue I was trying to resolve still exists as you can see in this branch:

kendallb@9324aaa

But that was also trying to solve the issue of letting constructor injected instances be able to use property injection properties in the constructor. That was something I wrestled with a decade ago, but we remove that issue but its still something that can crop up in practice. So that test is still valid and will fail, but the core issue is the hard coded limit of 50 for the resolution stack which we have now hit.

[alistairjevans]

There's no clean way to replace the default circular dependency detector, 50 is a pretty deep circular resolution graph already. If you're already at the stage where you are considering forking Autofac, you could reflect into the backing field behind the 'Default' static property and swap it out I suppose? Do it before Autofac builds the container though.

Also, I'm not sure if you noticed, but we added detailed resolve graph tracing a while back, including support for generating DOT graphs of the dependency chain. Might help you discover/adjust the apparently quite deep circular dependencies you have.

[kendallb]

Yes, I have written code to adjust it via reflection and that is working. We have quite a large list of dependencies and it's surprising to me that it managed to get that deep also. I will play with the DOT chain tooling to see if I can determine where it's getting so deep.

[kendallb]

Ahh, that was enlightening to use that tool. The variable _maxResolveDepth is not really measuring the DEPTH at all, but also the BREADTH. So in the case where we had an issue I was able to log the resolution of that class and the issue is not that we have dependencies super deep at all (most are only a couple of layers deep) but we have classes that bring in a lot of dependencies in total. So they are quite wide. Since this circular dependency checker is tracking all classes in the resolution chain depth and breadth, that is where we run into an issue as some of our classes might inject 10 or so dependencies as we try split stuff up a lot to minimize coupling and when you do that you end up with classes that need to bring in a good number of dependencies to get their work done. Then if you have a circular chain somewhere along the way, the total it needs to check gets large real fast.

This will not be an issue for anyone who has moved on from .NET 4.8 as it will keep going until it runs out of stack space.

So I do think a higher default value would make a lot more sense there still.