Closed seesharper closed 3 years ago
This is almost certainly connected to using InProcess mode by default in 2.2. CLR inherits stack size of w3wp.exe which is 512K in 64 bit version and 256K in 32bit one.
Seems like LightInject create much deeper stacks when variance is enabled and hits the limit.
@pakrym I am not so sure we are hitting the stack size limit here because of a deep graph.
Note: This is on a Mac, not on Windows. Not sure if that makes any difference
In my repro project I see this before we die
https://github.com/seesharper/lightinject_aspnetcore2_2
Note: All LightInject sources are included in the repro.
This is outputted from the GetService
method in my adapter.
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Routing.EndpointOptions]
Process is terminating due to StackOverflowException.
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.😀Is there like on or a few places I can replace the default IServiceProvider
@rynowak @JamesNK thoughts?
@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 Microsoft.Extensions.Options.IOptions1[Microsoft.AspNetCore.Routing.EndpointOptions]
into circular reference. Is it possible to dump which type is being activated to satisfy Microsoft.Extensions.Options.IOptions1[Microsoft.AspNetCore.Routing.EndpointOptions]
service request?
Yeah, I'll have a look at this tomorrow 👍
On the top of my head I think it was the OptionsFactory
There are some IEnumerable's being injected there
@pakrym
The trouble starts at OptionsFactory
https://github.com/aspnet/Extensions/blob/release/2.2/src/Options/Options/src/OptionsFactory.cs
The type being requested is OptionsFactory<Microsoft.AspNetCore.Routing.EndpointOptions>
Injection the following dependencies.
System.Collections.Generic.IEnumerable<Microsoft.Extensions.Options.IConfigureOptions<Microsoft.AspNetCore.Routing.EndpointOptions>>
System.Collections.Generic.IEnumerable<Microsoft.Extensions.Options.IPostConfigureOptions<Microsoft.AspNetCore.Routing.EndpointOptions>>
System.Collections.Generic.IEnumerable<Microsoft.Extensions.Options.IValidateOptions<Microsoft.AspNetCore.Routing.EndpointOptions>>
Later down the graph there is a request for
System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Routing.EndpointDataSource>
With variance enabled it brings back two types
Microsoft.AspNetCore.Routing.EndpointDataSource
AND
Microsoft.AspNetCore.Routing.CompositeEndpointDataSource
Maybe this is where it starts.
The Microsoft.AspNetCore.Routing.CompositeEndpointDataSource
again has a
System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Routing.EndpointDataSource>
dependency which is probably where things starts to become recursive.
So the the elefant in the room seems to be System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Routing.EndpointDataSource>
noyt only bringing back Microsoft.AspNetCore.Routing.EndpointDataSource
, but also Microsoft.AspNetCore.Routing.CompositeEndpointDataSource
.
Looks like you are right. Resolution path that breaks is ConfigureEndpointOptions
-> IEnumerable<EndpointDataSource>
--> CompositeEndpointDataSource
-> IOptions<EndpointOptions>
-> ConfigureEndpointOptions
.
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.
Some kind of test that attempts to resolve every registered service with a DI framework that supports variance would be ideal.
Where are we going to put such a test?
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.
@pakrym What's the next action (if any) on this?
We need to decide if registering services that would break in containers that support variance is something we are going to try to avoid and build the required infrastructure to do so.
@seesharper Are there any workarounds at this time for this issue?
For clarification I mean as a LightInject user looking to upgrade to asp.net core 2.2
In my repro project I see this before we die https://github.com/seesharper/lightinject_aspnetcore2_2
Used your repro project and targeting 5.0 the StackOverflowException does not occur anymore, therefore closing the issue.
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
2_2
.Then we started to see a
StackoverflowException
at startup and as fun it is to debug these kind of exceptions, we still managed to track it down toMicrosoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Routing.EndpointOptions>
recursively being requested from the container causing theStackoverflowException
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
IEnumerable<T>
being injected into the services down the object graph and then it suddenly hit me that this might be related to variance.A couple of examples just to illustrate what this means.
When requesting a
IFoo<Bar>
from LightInject, it will return 2 instances,FooWithBar
andFooWithDerivedBar
since they are compatibleFor 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
StackoverflowException
.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.
https://github.com/aspnet/Extensions/pull/536
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.
To Reproduce
Steps to reproduce the behavior:
UseLightInject()
(using the LightInject.Microsoft.AspNetCore.Hosting package)Expected behavior
Not to throw a
StackoverflowException