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.
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
Allow configuration of circular dependency max resolve depth in the container builder. #924
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
{ | ||
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The static method on 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContainerBuilder
public API?