Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Autofac/ContainerBuilder.cs
View file
Open in desktop
Expand Up @@ -30,6 +30,7 @@
using System.Linq;
using Autofac.Builder;
using Autofac.Core;
using Autofac.Core.Resolving;
using Autofac.Features.Collections;
using Autofac.Features.GeneratedFactories;
using Autofac.Features.Indexed;
Expand Down Expand Up @@ -98,6 +99,17 @@ internal ContainerBuilder(IDictionary<string, object> properties)
/// </value>
public IDictionary<string, object> Properties { get; }

/// <summary>
/// Sets the maximum size of the stack used when resolving a single component (and all of its transitive dependencies) from the container.
/// If this limit is exceeded the component will be treated as a circular dependency.
/// </summary>
/// <param name="maximumStackDepth">Maximum size of the stack used for resolve operations.</param>
public ContainerBuilder WithMaxResolveStackDepth(int maximumStackDepth)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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?

{
Properties[CircularDependencyDetector.MaxResolveStackDepthPropertyName] = maximumStackDepth;
return this;
}

/// <summary>
/// Register a callback that will be invoked when the container is configured.
/// </summary>
Expand Down
24 changes: 17 additions & 7 deletions src/Autofac/Core/Resolving/CircularDependencyDetector.cs
View file
Open in desktop
Expand Up @@ -30,12 +30,13 @@

namespace Autofac.Core.Resolving
{
/// <summary>
/// Catch circular dependencies that are triggered by post-resolve processing (e.g. 'OnActivated').
/// </summary>
internal class CircularDependencyDetector
{
/// <summary>
/// Catch circular dependencies that are triggered by post-resolve processing (e.g. 'OnActivated').
/// </summary>
private const int MaxResolveDepth = 50;
internal const string MaxResolveStackDepthPropertyName = "MaxResolveStackDepth";
private const int DefaultMaxResolveDepth = 50;

private static string CreateDependencyGraphTo(IComponentRegistration registration, Stack<InstanceLookup> activationStack)
{
Expand All @@ -53,16 +54,25 @@ private static string Display(IComponentRegistration registration)
return registration.Activator.LimitType.FullName ?? string.Empty;
}

public static void CheckForCircularDependency(IComponentRegistration registration, Stack<InstanceLookup> activationStack, int callDepth)
public static void CheckForCircularDependency(ISharingLifetimeScope currentOperationScope, IComponentRegistration registration, Stack<InstanceLookup> activationStack, int callDepth)
{
if (registration == null) throw new ArgumentNullException(nameof(registration));

if (callDepth > MaxResolveDepth)
throw new DependencyResolutionException(string.Format(CultureInfo.CurrentCulture, CircularDependencyDetectorResources.MaxDepthExceeded, registration));
if (callDepth > MaxResolveDepth(currentOperationScope))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
throw new DependencyResolutionException(string.Format(CultureInfo.CurrentCulture, CircularDependencyDetectorResources.MaxDepthExceeded, registration) + "\r\nGraph: " + CreateDependencyGraphTo(registration, activationStack));
}

// Checks for circular dependency
if (activationStack.Any(a => a.ComponentRegistration == registration))
throw new DependencyResolutionException(string.Format(CultureInfo.CurrentCulture, CircularDependencyDetectorResources.CircularDependency, CreateDependencyGraphTo(registration, activationStack)));
}

private static int MaxResolveDepth(ISharingLifetimeScope currentOperationScope)
{
return currentOperationScope.ComponentRegistry.Properties.ContainsKey(MaxResolveStackDepthPropertyName)
? (int)currentOperationScope.ComponentRegistry.Properties[MaxResolveStackDepthPropertyName]
: DefaultMaxResolveDepth;
}
}
}
2 changes: 1 addition & 1 deletion src/Autofac/Core/Resolving/ResolveOperation.cs
View file
Open in desktop
Expand Up @@ -112,7 +112,7 @@ public object GetOrCreateInstance(ISharingLifetimeScope currentOperationScope, I

++_callDepth;

if (_activationStack.Count > 0) CircularDependencyDetector.CheckForCircularDependency(registration, _activationStack, _callDepth);
if (_activationStack.Count > 0) CircularDependencyDetector.CheckForCircularDependency(currentOperationScope, registration, _activationStack, _callDepth);

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

Expand Down
68 changes: 68 additions & 0 deletions test/Autofac.Test/Core/Resolving/CircularDependencyDetectorTests.cs
View file
Open in desktop
Expand Up @@ -23,6 +23,40 @@ public void OnCircularDependency_MessageDescribesCycle()
Assert.DoesNotContain("System.Object -> System.Object -> System.Object", de.Message);
}

[Fact]
public void MaxStackDepthExceeded_ThrowsCircularDependencyException()
{
var builder = new ContainerBuilder().WithMaxResolveStackDepth(10);
builder.RegisterType<OuterScopeType>();
builder.RegisterType<OuterScopeType2>();
builder.RegisterType<OuterScopeType3>();
builder.RegisterType<OuterScopeType4>();
builder.RegisterType<OuterScopeType5>();
IContainer container = builder.Build();

using (var lifetimeScope = container.BeginLifetimeScope(b => b.RegisterType<InnerScopeType>()))
{
Assert.Throws<DependencyResolutionException>(() => lifetimeScope.Resolve<InnerScopeType>());
}
}

[Fact]
public void MaxStackDepthNotExceeded_CanBeResolved()
{
var builder = new ContainerBuilder().WithMaxResolveStackDepth(11);
builder.RegisterType<OuterScopeType>();
builder.RegisterType<OuterScopeType2>();
builder.RegisterType<OuterScopeType3>();
builder.RegisterType<OuterScopeType4>();
builder.RegisterType<OuterScopeType5>();
IContainer container = builder.Build();

using (var lifetimeScope = container.BeginLifetimeScope(b => b.RegisterType<InnerScopeType>()))
{
Assert.NotNull(lifetimeScope.Resolve<InnerScopeType>());
}
}

[Fact(Skip = "Issue #648")]
public void ManualEnumerableRegistrationDoesNotCauseCircularDependency()
{
Expand Down Expand Up @@ -96,5 +130,39 @@ public RootViewModel(IEnumerable<IPlugin> plugins, PluginsViewModel pluginsViewM
{
}
}

public class OuterScopeType
{
public OuterScopeType(OuterScopeType2 outerScopeType2) { }
}

public class OuterScopeType2
{
public OuterScopeType2(OuterScopeType3 outerScopeType3) { }
}

public class OuterScopeType3
{
public OuterScopeType3(OuterScopeType4 outerScopeType4) { }

}

public class OuterScopeType4
{
public OuterScopeType4(OuterScopeType5 outerScopeType5) { }
}

public class OuterScopeType5 { }

public class InnerScopeType
{
public InnerScopeType(
OuterScopeType outerScopeType,
OuterScopeType4 outerScopeType4,
OuterScopeType5 outerScopeType5
)
{
}
}
}
}