Open ktyl opened 7 months ago
Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection See info in area-owners.md if you want to be subscribed.
Author: | ktyl |
---|---|
Assignees: | - |
Labels: | `untriaged`, `area-Extensions-DependencyInjection` |
Milestone: | - |
Verified repro on v7 - v9P1.
Thanks @steveharter for verifying. Is there a chance that it can get fixed in the future releases of v8 as well? On the other side I can imagine that as this would change the behavior of the runtime it could be considered to be a breaking change in some cases...
I looked at this in greater detail: Calling GetService()
does not check for [ActivatorUtilitiesConstructor]
. It goes through code on CreateConstructorCallSite().
Only ActivatorUtilities.CreateInstance() checks for [ActivatorUtilitiesConstructor]
.
However, changing your repro to use
var foo = ActivatorUtilities.CreateInstance<Foo>(serviceProvider);
instead of
var foo = serviceProvider.GetService<Foo>();
still results in the same issue - the ordering of the constructors affects the check for [ActivatorUtilitiesConstructor]
which it should not.
I assume the semantics of ActivatorUtilities.CreateInstance()
should be:
[ActivatorUtilitiesConstructor]
(if there is one), otherwise we throw.@tarekgh does this sound correct? I may push a PR soon that does this.
@steveharter
Mostly correct. The following comment describing it:
The only difference is, when seeing a constructor with [ActivatorUtilitiesConstructor]
, this will be selected even if we encountered a constructor with longer parameters. But after encountering [ActivatorUtilitiesConstructor]
and then getting a constructor with longer parameters, the one with the longer parameters will be selected. I am not sure if the reflection always enumerate constructors according to the parameters count or this is not guaranteed? If reflection guarantee that, then what you wrote is accurate. if not, then it is not.
CC @eerhardt
I am not sure if the reflection always enumerate constructors according to the parameters count or this is not guaranteed? I
Reflection will return the constructors in the order they are defined.
However it looks like ctor lookup from GetService() does order the constructors.
But after encountering [ActivatorUtilitiesConstructor] and then getting a constructor with longer parameters, the one with the longer parameters will be selected.
So reflection ordering does matter then. That doesn't seem like a good thing IMO. In either case, I will still put up a PR to address the case where ordering with the same number of parameters doesn't always respect [ActivatorUtilitiesConstructor]
.
Aside, ideally, it seems like provider.GetService()
should use ActivatorUtilities.CreateInstance(provider)
when it is time to find and call the constructor for a service. That would also support [ActivatorUtilitiesConstructor]
for both cases; today they have different semantics.
Moving to V10; this area is still broke and\or confusing. The DI GetService() does not use the attribute - see https://github.com/dotnet/runtime/issues/98959#issuecomment-1973525990.
Does anybody know why the semantics between services.GetService<T>
and ActivatorUtilities.CreateInstance<T>
are different like this?
We have a few places in our application that rely on ActivatorUtilities
for adding DI support to some custom frameworks and we got bit by the breaking changes in behavior back when .NET 8 version launched, yet the same breaking change didn't impact normal service resolution.
This always felt like a poor design to me. Feels like there shouldn't be any difference in activating a registered type, vs constructing an unregistered one as long as you didn't register a construction lambda in the registration. Both operations are semantically identical.
Does anybody know why the semantics between services.GetService
and ActivatorUtilities.CreateInstance are different like this?
That is what @steveharter tracking to fix. We are keeping this issue open to address that.
Description
Selecting the preferred constructor by
ActivatorUtilitiesConstructor
attribute works differently depending on the order of constructors.Reproduction Steps
Let's consider the following class:
Let's assume that I want to instantiate this class using the Dependency Injection container:
This obviously fails because
ActivatorUtilities
class finds two matching constructors. Fortunately there is a[ActivatorUtilitiesConstructorAttribute](https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilitiesConstructorAttribute.cs)
so let's adjust the classFoo
:This still throws the same error.
Expected behavior
If
[ActivatorUtilitiesConstructor]
is used the constructor should be prioritized above all other constructors if all its dependencies are satisfied, regardless of the order of the constructors in the source code.Actual behavior
I have digged in the source code and I think the reason is a mistake in
ActivatorUtilities
class. The following snippet of code (https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs#L95)isPreferred
is properly set to true based on the existance of the attribute on the first constructor. But when the second constructor (public Foo(Baz baz)
) is analyzed the condition in line 108 (if (isPreferred || bestLength < length)
) isfalse
, but the else statement in line 114 (else if (bestLength == length)
) is evaluated totrue
and therefore the constructor is also considered to be the best candidate.If the constructors appeared in an opposite order the condition in line
108
would be true and the code would have worked properly.Regression?
No response
Known Workarounds
Add the constructor decorated with
ActivatorUtilitiesConstructorAttribute
after all other constructors in your class.Configuration
No response
Other information
No response