Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.Sign up
StackOverflowException when using a DI container that supports variance. #686
Describe the bug
I am the author and maintainer of the LightInject DI library and we discovered something strange when upgrading one of our AspNetCore projects to netcoreapp2.2 and setting the compabilitylevel to
Then we started to see a
The object graphs are pretty deep here and we spent a day or so trying to figure out what was going on. I noticed that there are some
A couple of examples just to illustrate what this means.
When requesting a
For instance we can do this
This feature is very powerful and has worked great up until AspNetCore 2.2 where we now have to turn off support for variance in LightInject (through an option) in order to avoid the
I know that this feature is not supported, at least not yet, in MS.DI, but it seems a little strange to require the DI adapter to disable support for variance.
I don't know of this was intentional, but it is maybe something to be aware of as it seems that proper support for open generics might find its way into MS.DI as well according to this PR.
We got around the problem by disabling variance in LightInject, but that also disables a very powerful feature. At the very least there should be a test for this over at the extensions repo that states this requirement. The requirement being that variance is not supported.
The best solution would be to fix this in AspNetCore so that it does not rely on non-variant behaviour.
Steps to reproduce the behavior:
Not to throw a
@pakrym I am not so sure we are hitting the stack size limit here because of a deep graph.
In my repro project I see this before we die
This is outputted from the
It looks like it is recursively trying to resolve that service.
Disable variance here and everything is fine.
The point I am trying to make here is that the MS.DI specification tests should cover this. If the case is that containers must disable variance, there should be a test that checks this.
@pakrym Would it be possible in a relatively easy way for me to fork the whole AspNetCore repo and run all the tests with my adapter. It seems to be the only way to make absolutely sure everything works as expected.
@seesharper I would like to avoid forcing container authors to actively disable features to pass our spec test.
So first I want to figure out how exactly enabling variance is turning
The trouble starts at OptionsFactory
The type being requested is
Injection the following dependencies.
Later down the graph there is a request for
With variance enabled it brings back two types
Maybe this is where it starts.
So the the elefant in the room seems to be
Looks like you are right. Resolution path that breaks is
Worth noting that in 3.0 things work again because registration pattern changed.
Nevertheless, we don't expect or plan for the behaviour of variance resolution so this might happen again with some other component.
I think we should add a spec test for this case.
I don't know. It is kind of a framework level functional test.
My point of view is it is better to put a fence at the top of the cliff, and have test that checks all registered services and prevents this before we release, than wait until something blows up and add fixes and tests as bugs come in. Right now there is zero test coverage of resolving ASP.NET registered services when using variance, correct?
Another option would be to have an app that uses most of the popular features of ASP.NET Core, use a DI framework with variance, and have functional tests run on it.