Extensions: aspnet core 3.0 DI Bug when constructor value type with default value

[cyrusdargahi]

Description

When a class is registered with service collection as transient and that class constructor has a parameter with a value type, but that parameter has a default value, then when it is resolved in runtime, the first two times it works as expected and the parameter is set to the default value, but on subsequent service resolutions the parameter will be given "bizarre" values that are not declare anywhere within the application code.

I have created a repo with an example and explanation https://github.com/cyrusdargahi/aspnetcore3DIBug

To Reproduce

Build and run the application. On first and the second load in the browser you should see the output 0.01.

Error

On any page reloads after the (exactly) first two, weird thing start to happen. The output will now be a bizarre number taken from somewhere that isn't defined anywhere in the application code.

Expected behavior

Expected result is the page should display the constant 0.01 result on any page (re)load.

[davidfowl]

Value types are unsupported in the DI container.

[cyrusdargahi]

I know that @davidfowl but this is a breaking change from the behavior since asp.net core 2.2 (or even further back). We have a production solution that had similar code and we got very unpleasant result after migrating to asp.net core 3.0 (almost cost us a lot of money due to this small bug/breaking change)

In this case the constructor has a default value so no value need to be injected. But somehow the DI instead injects very strange values, after the second time it gets resolved.

At least it should not behave as it does right now. Either it should throw an exception or it should work consistently (not only the first two times that instance is resolved). Also this should be documented as a breaking change, if no code action is taken.

[davidfowl]

At least it should not behave as it does right now. Either it should throw an exception or it should work consistently (not only the first two times that instance is resolved). Also this should be documented as a breaking change, if no code action is taken.

Agreed.

[brandondahler]

Presumably this is caused by this line 270 in ILEmitResolverBuilder not unboxing the default value for the value type, resulting in undefined behavior occurring. https://github.com/aspnet/Extensions/blob/20dbe660793d5280aead975cd5e6e8d6b280ea3c/src/DependencyInjection/DI/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs#L258-L272

I plan on testing and taking a closer look tonight or over the weekend,

@davidfowl do you have a preference on direction between preserving old behavior versus throwing an exception? I am leaning towards preserving old behavior as long as it doesn't significantly impact performance or lead to a rabbit hole of other problems.

[MichaelJohnBradley]

Has this been fixed? I have downloaded latest net core sdk and using visual studio created a new web project and im still getting random values eg -683792800 when i have set the value in the constructor to have a default value

[davidfowl]

@anurse @Pilchie this was a patch candidate

[Pilchie]

Given that this issue is still open, no it hasn't been fixed. We can consider it for a future 3.1 servicing release, though likely not the next one given holiday deadlines :(

[davidfowl]

It was fixed in 5.x already.

[Pilchie]

Can you tag the PR?

[Pilchie]

Oh, it's #2501, nevermind

[anurse]

Coming back to this, do we still need to consider a backport to 3.1? To do so we need to identify that no workaround is possible and a large number of customers are affected. This thread has been idle for a few months, how have people worked around this so far? (or is this still affecting your app?)

[craigsmitham]

This created quite a headache when we recently moved to .NET Core 3.1. The work-around we have was to manually change our IOC registration to manually construct types this was affecting, adding a fair amount of boilerplate. I think a backport should be strongly considered for any type of LTS release.

[davidfowl]

I think this should be backported as well.

This created quite a headache when we recently moved to .NET Core 3.1. The work-around we have was to manually change our IOC registration to manually construct types this was affecting, adding a fair amount of boilerplate. I think a backport should be strongly considered for any type of LTS release.

What exactly do you mean here? What were you doing before?

[craigsmitham]

@davidfowl to work around this issue, we have to provide an explicit implementation factory for our services that are impacted by this issue. Before we could just use the registration methods that didn't require an implementation factory.

[davidfowl]

@davidfowl to work around this issue, we have to provide an explicit implementation factory for our services that are impacted by this issue. Before we could just use the registration methods that didn't require an implementation factory.

I get it but I'd like to understand how you hit it. Structs on reference types?

[hmacat]

Hello there. I had an issue of the same problem and it is closed, is this problem is planned to be fixed for .net core 3.1? This is a very subtle problem and may cause big issues. If the feature won't be supported, I think at least there should be an exception thrown to make developers aware of this breaking change.
Thanks.

[EamonNerbonne]

I get it but I'd like to understand how you hit it. Structs on reference types?

Isn't this a repro? Or is this something else? https://github.com/dotnet/aspnetcore/issues/22027

[davidfowl]

Are people really injecting value types into constructors directly?

[hmacat]

Are people really injecting value types into constructors directly?

Yes, we were. Until it became broken :)

[davidfowl]

Yes, we were. Until it became broken :)

You were expecting to get the default value right?

[hmacat]

Yes, we were. Until it became broken :)

You were expecting to get the default value right?

Exactly. In my case we were using an enumeration and it becomes corrupted after a while. I opened an issue and it was closed.

Here is the issue and the reproduction example:
https://github.com/dotnet/extensions/issues/3197

[cyrusdargahi]

Are people really injecting value types into constructors directly?

Apparently yes @davidfowl . Although _I don't approve of it_, that is the case.
But that is not the point here. It's more that it was an undocumented breaking change, and in our case if we had not paid attention to it, as mentioned, it would actually had cost us a lot of money, this _tiny little breaking change_.

[davidfowl]

This isn't a breaking change, it's a bug that was fixed (or at least we think it's fixed 😄 ). It needs to be back ported though.

[EamonNerbonne]

It's not just default values; dotnet/aspnetcore#22027 uses a singleton with a non-default-valued constant, and appears to use a version of the code that as best I can tell includes the https://github.com/dotnet/extensions/pull/2501 fix.

It's pretty reasonable to think of all singletons as values and all values as singletons; so that's even really a strange thing to do (assuming immutability, which enums clearly have).

[EamonNerbonne]

Maybe that's the difference to this bug? The non-default value?

[ForNeVeR]

Could we just add an assertion for type to not be a value type, or something like that?

The current situation around this issue is pretty bad, especially since the code only gets optimized on the third run. I could imagine such bug slipping through the tests (if they don't resolve the same type for more than two times), and then suddenly failing in production.

Also, FYI, we've got a good sample that reproduces the issue here.

Symptoms seem to be ArrayTypeMismatchException or InvalidProgramException being thrown at random places of code.