Max resolve stack depth is now configurable via extension method

[zamaleev]

My PR based on #924. I carefully read the discussion in this thread. Please let me put my two cents.

First of all, I spent a lot of time investigating such a problem in our codebase. And in the end, I think that we should change the exception message at least. The current message doesn't give any idea of what is really going on.

So my story begins with the decision to replace IoC Container in huge project from old one to Autofac. I can't say that the existing codebase can be called clean architecture but we start working to achieve this goal as a future. As a result of it, when we trying to resolve several controllers we getting a big resolution graph. But this is just one side of the problem. The second one, that we also need to define several child scopes for special registrations based on per scope lifetime.

I thought both of these reasons are played a bad joke with us. I found confirmation that additional scope can be lead to behavior when Autofac will resolve the same service multiply times. Here detail description of what I also can see.

Now about #924. It was published more than two years ago. And how I can see nothing was changed with CircularDependencyDetector.cs so I can say that it wasn't a good idea to reject those request. I saw that Travis Illig suggest as a solution for this problem to use some reflection to change MaxResolveDepth value at runtime but how I can say this is not a good way for me/us - because we will create some dirty hack that we need to remember about and support it in future. And this is not a good way for enterprise code.

TL;DR this problem is real, hard to investigate and we don't have any nice and simple way to fix it.

How I think this simple extension method can provide a clean way to resolve such an issue. If someday Autofac contributes will decide to rewrite CircularDependencyDetector there's no problem to support this extension a few versions after to provide compatibility. The payload of it can be removed and the method can be marked as obsolete.

[alistairjevans]

One problem with this approach is that the extension method you've defined is attached to an instance of the ContainerBuilder, but the MaxResolveDepth value is static, so will apply to all containers in the process, not just the one you called the extension method against; we can't really do that because it will confuse people.

If we really wanted to make MaxResolveDepth configurable via a static method, then perhaps we could add a new static GlobalCircularDependencyOptions class that contains a single (non extension) method to change the value. It's not ideal (we don't like statics much), but might do for now so you don't have to use reflection to manipulate the value.

@tillig, we will probably end up revamping circular dependency tracking as part of #1096, but allowing access via a global class temporarily, until we make the inevitable breaking changes for pipelines, might be ok?

[zamaleev]

so will apply to all containers in the process, not just the one you called the extension method against

But I am expecting that it will affect all child scopes, or I don't understand you... I am new to Autofac.

we can't really do that because it will confuse people.

Right now we have magic number 50 that is hardcoded and zero documentation about it. Also, the exception message that makes you look for a problem in your own code.

then perhaps we could add a new static GlobalCircularDependencyOptions class that contains a single (non extension) method to change the value

Hm... I am not sure that this will be better than the extension method but I can change my request in this way.

[alistairjevans]

It's not a separate scope I'm worried about, it's separate root containers in the same process:

var builder1 = new ContainerBuilder();
// Set custom depth.
builder1.WithMaxResolveStackDepth(100);
var container1 = builder1.Build();

var builder2 = new ContainerBuilder();
// Do not set custom depth.
var container2 = builder2.Build();

// container2 has an implicit stack depth of 100, even though it was set against container1.

If we define a central static method, at least we're being honest about the global nature of the setting.

[zamaleev]

I see. Reasonable.

Most the correct way is to pass this value through ContainerBuilder as a ctor parameter and use it inside CircularDependencyDetector as option but.. this will affect public API.

[alistairjevans]

Yes, long term we would want to scope the MaxResolveDepth as some property of the container, but that's a big change as you say.

[zamaleev]

So.. Let me know if you guys agree, I will update PR.

[tillig]

I saw that Travis Illig suggest as a solution for this problem to use some reflection to change MaxResolveDepth value at runtime but how I can say this is not a good way for me/us - because we will create some dirty hack

Yup, because while "the problem is real" if we add an API to tweak this for the very few people that need to change it, that's something we have to maintain forever. At least two years ago, it wasn't something we were interested in documenting, testing, answering questions about, troubleshooting when people messed it up, and trying to maintain backwards compatibility about.

We've been trying to improve circular dependency handling for quite some time; having a public API like this somewhat hinders that. It means if we actually do fix things to not need that value, instead of being a totally non-breaking 0.0.1 sort of change, it becomes a breaking 1.0.0 change.

Honestly, since I know we're heading toward pipelines and possibly better handling for circular dependencies, I'm still not really a fan of exposing some public, supported API for tweaking this.

However, if we did, it probably should be a global static, shared by all containers, so the lookup is fast and simple.

An alternative might be to just crank the limit up. I don't know why 50 was chosen; seems arbitrary. Maybe 500 or something, as long as that doesn't result in a StackOverflowException.

@radevop If you had the ability to set this number in your code, what would you set it to? I'm not asking what we should set it to internally in Autofac, I'm asking if you got this extension method, what would your code set it to?

[zamaleev]

@tillig hard to say, I think that you can't find any "silver bullet" in this question, in other words there will not be any number that can satisfy everyone, that's why I think that 50 is good until you are hitting this limit and need to understand why this happens. In my personal case I know reasons for it and you right that this is actually not a problem but just symptoms of it. And yes, we will need to fix them, but obviously we can not do this in one moment. So we will need to live for some time with them.

That's why I am suggesting to make a way to configure it. Of course, this option should be well documented and everyone who uses it should fully understand what he are doing and why.

What about your question - I will try to find good value with empiric way. Right now I didn't such investigation because I have a lot of other things to do during my task with replacement IoC. So I left it to the end. Right now it is 300, but for sure it will be less.

[tillig]

I know reasons for it and you right that this is actually not a problem but just symptoms of it. And yes, we will need to fix them, but obviously we can not do this in one moment.

This is another challenge for projects like Autofac - "I have a problem, I know I'm doing something I probably shouldn't be doing, I know I can fix it, but I don't have time... so can you change the common framework to accommodate my project?"

I recognize the challenge and there are things like that I, too, run across. Sometimes it does mean a change in the common framework is needed, sometimes it means I need to tell my customers that the job we originally estimated to be completed in one sprint was incorrectly estimated and we need to extend that time to fix issues we didn't account for in the estimates.

The more I think about it, the more I'm kind of against making this a public API. If it's accessed via reflection, it could be a utility or extension method entirely owned by the consuming project; and while it seems like "a dirty hack," what it further means is that the consuming project is very, very willingly taking on the risk of doing the thing they probably shouldn't be doing, and if we fix the issue internally, the only project that's broken is the one that knowingly took on the risk. Which was the reason it was handled the way it was in the first place. I really want "we fixed circular dependency handling" to not be a breaking API change.

However, again, I'd be curious to know what the number is that solves the issue. Perhaps the solution is just increasing the number and calling it a day.

[zamaleev]

Let's not discuss here about project responsibility. And how we handle it with our clients. I guess that you are also not the first-day on this "rodeo" and you are better than I know that software never becoming good in one single day.

So please take a look at our situation again from our points of view. A huge old project that works for years with existing customers that working with it and using it every day. I don't even care who did old mistakes and why our dependency graph so huge. My job is not to find and blame someone. My job is to find a good way to fix it, clean up it, and make better. And we start to refactor it. In the end, we investigate its a problem with the selected container. And what you suggest doing for us? Instead of making code much more simple and clean as we can you suggest inserting some reflection hack that no one will remember about after one-two months when it will be merged. Or instead of it we will need fork Autofac and keep it in out internal feed for a while (i.e lost any opportunity to use last versions of it without painful merging)?

And another two cents. There are already two ppl who faced this problem at least those who find time to create PR about it. How many passed by? How many even didn't understand that they hit the threshold of some number?

And I understand that it was some "temporary" decision. I also believe that someday you will provide some smart way to detect those circular dependencies. But what we need to do before this happens? All that I am asking about is a clean way to make it configurable of course with a full understanding of our own responsibility for what we are doing.

About Autofac challenge - I guess that placing such logic with internal hiding consts that so critically affecting the not good at all.

Again, I am not asking to break some existing API, just provide a way to make it configurable.

[tillig]

I am not asking to break some existing API, just provide a way to make it configurable.

You're asking me to add an API that will absolutely break in the future. That's the problem. Not that it would be a challenge now or in a month, but when we do fix this then this is another breaking change we'd need to manage.

I would recommend you stop demanding a feature I'm not interested in supporting or allowing to be configured. I've acknowledged your challenge and there's an existing way to handle it that allows the consuming code to take on the known risk. You're asking this project to assume burden for an issue in your code, which isn't cool. I've asked for additional concrete information around which we can make some decisions, however, the more combative you get, the less inclined I am to pursue this.

Please remember that you are getting free software and free support from people who are entirely not funded by day jobs to maintain this. It's personal free time, handed to you. Free.

So: Please take a step back and simply help us make some good decisions by providing the information requested.

[zamaleev]

I didn't want to hurt you in any way. If I did so, I am sorry.

Let's both calm and think about this in a cold way?

Ofc I will inform you about my personal choice. But how can we be sure that this will satisfy all other consumers of your product? You cant leave this as "red button with label 'Don't push me'" someone will do this, absolutely.

You're asking me to add an API that will absolutely break in the future. That's the problem. Not that it would be a challenge now or in a month, but when we do fix this then this is another breaking change we'd need to manage.

Please tell me what problem will be with supporting a single class that provide one variable and alow to change it outside? In a day when you will not need it anymore, you will be able to mark it obsolete and delete it after. Or I miss something? What is absolutely break API here?

And about 'step back'. Please remember that you are getting free PR and free suggestions from person who is entirely not funded by day jobs to maintain this. It's personal free time, handed to you. Free.

And I come for an open-source project discussion board not to solve my personal problems how you probably thinking. I already solved them. I come here to help make the project that you are carrying (thanks a lot for your job!) a bit better.

Anyway, if you do not agree I am getting out of this.

[tillig]

What is absolutely break API here?

Today: We add a public API to change this value. People start using it, and in some cases people who don't actually need to use it see that it can be tweaked so they tweak it.

Tomorrow: We figure out the circular dependency issues and we don't need this value anymore. We remove the value from the public API. People take the upgrade and break. People complain to us because we broke the public API and now their projects - or transitive dependencies of their projects - break when they take the API and that's Autofac's fault.

It doesn't even matter if you personally don't take this. Let's say you use, oh, I dunno, some data access library that has Autofac integration. That data library, at one point, had one person (like this) who needed to tweak this value, so to "help" that data library sets the value. Now Autofac upgrades. You want the upgrade, so you take it. The data library breaks with a hard to debug missing member exception when it tries to set the value that doesn't exist anymore. It wasn't even something that affected you, it affected a transitive dependency.

You submitted a PR without commenting or filing an issue to even discuss if this was a road we were interested in pursuing, so I appreciate the "free PR," but in general, it's best if you file an issue first. That's documented in the contributor guide.

Please remember that you are getting free PR and free suggestions from person who is entirely not funded by day jobs to maintain this.

Except, also

A huge old project that works for years with existing customers that working with it and using it every day.

Which sounds like fixing up your project actually is your day job, for which you are getting paid.

I'm sorry this conversation has devolved like this, but there are reasons this wasn't publicly exposed, and there are reasons I'm pretty strongly against publicly exposing it now. I very strongly believe that while this might fix a problem you are having now, it isn't something that the 80% or even the 90% case hits, and we can't be everything to everyone. And once it fixes it for you, the long term ownership of it isn't with you, it's with us.

It sounds like you already have this solved, so unless you're able to provide the info requested, I think the reflection-based accessor to change the setting is the best it's going to get. If you can provide the info of what you would set this to then we can consider changing the default value.

Regardless, I do think changing the exception message to be more clear is absolutely valuable. If it turns out we end up closing this PR, I think updating the exception message to explain how to take action on reducing the size of the resolve chain is 100% reasonable.

[zamaleev]

It's your project so do whatever you want and what you believe. I am a bit shocked and upset.

The reason that I created PR w/o discussion inside the issue section is that I investigated this problem by my self and found way to fix it. So your "free support" same as my "free PR".

I will come back with value that I selected.

[zamaleev]

We decide to stay at 150... but probably this value will be decreased.