Allow configuration of circular dependency max resolve depth in the container builder.

[bstewart00]

This comes from my comments in #798 regarding problems I have with the current circular dependency implementation. Hopefully the unit test demonstrates the problem clearly.

The current circular dependency stack limit is set to 50, however in practice depending on how your scopes are configured this limit is lower due to how inner scopes are implemented. The web application I work with creates a separate scope for each request, with the majority of component registrations coming from the root container. I noticed that resolving a component from an inner scope that has dependencies registered in an outer scope creates an internal DelegateActivator for each dependency as an intermediate between the two scopes. This is in addition to the normal ReflectionActivator, so in effect the stack limit can be reduced to 25 in this situation. If your application has a very deep dependency graph, it is quite easy to exceed this 25 limit.

What I propose is a low-friction workaround by making this limit configurable in the container builder so I can at least increase the limit for my application without having to resort to adding Lazy<> registrations to arbitrary places in the dependency tree to "reset" the stack.

[flin-8]

Please consider accepting the PR or making this setting customizable. When we hit this issue, IIS also crashes due to a stackoverflow while the exception message is computed

[tillig]

• Could this be an extension method instead of something changing the ContainerBuilder public API?
• We need logic to ensure folks don't set this to zero, a negative number, or something that will shoot themselves in the foot. Perhaps the minimum is the current default of 50?

[tillig]

Something that worries me here is the lack of caching. The call to MaxResolveDepth to query the set of properties is fast, but it's a tiny bit of overhead on every resolve call all the way down the stack. Basically it adds overhead to the 99% case where people don't override it.

Is there a use case where you'd set this differently for one part of the app but not another? Or for one container but not another? I'm wondering about whether it'd be better to have some sort of internal static that gets set rather than a property that has to be looked up.

I dunno, maybe it's not that big of a deal. Have you tried benchmarking it to see the difference? What other options did you consider?

[bstewart00]

I think it would be unusual to need to configure this on a per-container basis and the main use case is something you would set for the root container.

The performance impact was negligible while cursory testing over the same environment I have this problem with. The dictionary lookups are less than 1ms each. That said I would prefer to use a static property actually but thought the properties infrastructure would be nicer. Caching would make it more complex as it would need to be cached per-scope. So perhaps a static property is better here.

I am thinking of changing it to an extension method like

public static ContainerBuilder WithMaxResolveStackDepth(this ContainerBuilder builder, int maximumStackDepth)
        {
            if (maximumStackDepth < CircularDependencyDetector.DefaultMaxResolveDepth)
            {
                maximumStackDepth = CircularDependencyDetector.DefaultMaxResolveDepth;
            }

            CircularDependencyDetector.MaxResolveStackDepth = maximumStackDepth;
            return builder;
        }

This might still suggest you can configure this per-container. Would you consider a public static method on the container builder?

[tillig]

The static method on ContainerBuilder - I'd say, if possible, not to change the contract on ContainerBuilder if possible. Almost like the CircularDependencyDetector depth setting itself should just be accessible.

But... I don't know. Hmmm. Figuring out the right place this should be is a tough one. I could see, for example, that folks might want to be able to set this in a module (like, configure the container settings as well as register types). Having static accessors getting set in modules seems weird.

I'm also wondering if the logic in how we detect circular dependencies may be off. We just had an issue (#927) about a circular dependency that was inadvertently caught not by depth of resolve chain but by an Owned<T> having another identical Owned<T> later in the stack.

I'm curious if the logic needs to be updated, and maybe @alexmg has some ideas. We already have #798 open for discussing ways to improve this detection. I'll post a thought I just had in there.

Point being... if we add something to the API we need to support it for a long time to come; if we end up refactoring to remove that later, it's technically a breaking change, which isn't good. I want to solve the problem, but I want to do it in a way that won't introduce a break later.

[tillig]

I'm also thinking absolute worst case scenario, we can make the value a static instead of constant so you could tweak it through reflection. Not awesome, but at least it'd get around the challenges.

[bstewart00]

What's the rationale behind having the fixed limit anyway? I think the logic looking for the same registration is correct, it's just whether or not the reference equality can be trusted here. Is there a stronger way to compare two registrations? I think if we can fix that a limit isn't necessary at all.

I can understand wanting to avoid API impact for something like this, but before it can be fixed properly I think even just making the limit a static would be an improvement.

[tillig]

The fixed limit is so we can catch it before the program hits a StackOverflowException and there's nothing we can do about it. If we can catch it first, we can throw our own exception that makes it easier to troubleshoot. No limit, things just blow up and we can't always catch the StackOverflowException to wrap it.

IComponentRegistration does have an Id property and we could assume that if you're visiting the same registration twice in the same resolution stack that it's circular. We'd just have to keep the stack of visited registrations around through each resolve op and check it at each level. I don't think it'll be hard, but it'll be more than a five minute fix.

OK, so... short term, let me make this a static. I appreciate the help on the PR but I'd like to see if there's a way to remove limits entirely and still accomplish the goal. You can hack it through reflection, just put some good error handling code around it so if/when we actually do switch from a limit it won't totally break you and give you a bad day.