dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.66k forks source link

ServiceDescriptor.ImplementationType throws exception for Keyed descriptor: System.InvalidOperationException: This service descriptor is keyed. Your service provider may not support keyed services. #95789

Closed amis92 closed 1 month ago

amis92 commented 9 months ago

Description

It looks like an intentional behavior is breaking a lot of existing code. There are a lot of places that inspect/iterate over ServiceCollection and look for a specific ImplementationType as a way of custom TryAdd. This works fine with Microsoft.Extensions.DependencyInjection.Abstractions v8.0.0, until a Keyed descriptor is added before the offending inspection.

This is happening on all frameworks supported by Microsoft.Extensions.DependencyInjection.Abstractions (v8) - it's not framework-dependent.

Now, I understand that this is not a good practice (iterating and inspecting ImplementationType). Sadly, it seems to be a common practice, even including multiple Microsoft products:

Problematic source code:

https://github.com/dotnet/runtime/blob/611934fcd58a6d0ab8ff7000d48f491b56931c08/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs#L158-L168

Reproduction Steps

Program.cs

using Microsoft.Extensions.DependencyInjection;

var services = new ServiceCollection();
services.AddKeyedSingleton<FooService>("key");
services.FirstOrDefault(x => x.ImplementationType == typeof(BarService));

class FooService {}
class BarService {}

dotnet run throws:

Unhandled exception. System.InvalidOperationException: This service descriptor is keyed. Your service provider may not support keyed services.
   at Microsoft.Extensions.DependencyInjection.ServiceDescriptor.ThrowKeyedDescriptor()
   at Microsoft.Extensions.DependencyInjection.ServiceDescriptor.get_ImplementationType()
   at Program.<>c.<<Main>$>b__0_0(ServiceDescriptor x) in C:\Users\amadeusz.sadowski\dev\test-keyed\KeyedExceptionRepro\Program.cs:line 5
   at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)
   at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
   at Program.<Main>$(String[] args) in C:\Users\amadeusz.sadowski\dev\test-keyed\KeyedExceptionRepro\Program.cs:line 5

Expected behavior

Existing libraries keep working - exception is not thrown. Maybe just return null.

Actual behavior

InvalidOperationException is thrown when iterating over registered descriptors' ImplementationType.

Regression?

Definitely a regression. This works with all versions of DependencyInjection up until v8.

Known Workarounds

Attempt to register keyed service after registrations that inspect ServiceCollection.

Configuration

No response

Other information

No response

ghost commented 9 months ago

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection See info in area-owners.md if you want to be subscribed.

Issue Details
### Description It looks like an intentional behavior is breaking a lot of existing code. There are a lot of places that inspect/iterate over ServiceCollection and look for a specific ImplementationType as a way of custom TryAdd. This works fine with `Microsoft.Extensions.DependencyInjection.Abstractions` v8.0.0, until a Keyed descriptor is added before the offending inspection. This is happening on all frameworks supported by `Microsoft.Extensions.DependencyInjection.Abstractions` (v8) - it's not framework-dependent. Now, I understand that this is not a good practice (iterating and inspecting ImplementationType). Sadly, it seems to be a common practice, even including multiple Microsoft products: - https://github.com/microsoft/ApplicationInsights-dotnet/issues/2828 - https://github.com/AzureAD/microsoft-identity-web/issues/2604 Problematic source code: https://github.com/dotnet/runtime/blob/611934fcd58a6d0ab8ff7000d48f491b56931c08/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs#L191-L201 ### Reproduction Steps - `dotnet new console` - `dotnet add package Microsoft.Extensions.DependencyInjection` `Program.cs` ```csharp using Microsoft.Extensions.DependencyInjection; var services = new ServiceCollection(); services.AddKeyedSingleton("key"); services.FirstOrDefault(x => x.ImplementationType == typeof(BarService)); class FooService {} class BarService {} ``` `dotnet run` throws: ``` Unhandled exception. System.InvalidOperationException: This service descriptor is keyed. Your service provider may not support keyed services. at Microsoft.Extensions.DependencyInjection.ServiceDescriptor.ThrowKeyedDescriptor() at Microsoft.Extensions.DependencyInjection.ServiceDescriptor.get_ImplementationType() at Program.<>c.<
$>b__0_0(ServiceDescriptor x) in C:\Users\amadeusz.sadowski\dev\test-keyed\KeyedExceptionRepro\Program.cs:line 5 at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found) at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate) at Program.
$(String[] args) in C:\Users\amadeusz.sadowski\dev\test-keyed\KeyedExceptionRepro\Program.cs:line 5 ``` ### Expected behavior Existing libraries keep working - exception is not thrown. ### Actual behavior `InvalidOperationException` is thrown when iterating over registered descriptors' `ImplementationType`. ### Regression? Definitely a regression. This works with all versions of DependencyInjection up until v8. ### Known Workarounds Attempt to register keyed service after registrations that inspect ServiceCollection. ### Configuration _No response_ ### Other information _No response_
Author: amis92
Assignees: -
Labels: `untriaged`, `area-Extensions-DependencyInjection`
Milestone: -
zhenlei520 commented 9 months ago

I also encountered it, I think this is a bug, KeyedImplementationType needs to check whether KeyedService is true, but ImplementationType should not be checked

steveharter commented 9 months ago

@benjaminpetit PTAL

zhenlei520 commented 9 months ago

Is there a way to make this problem compatible? It makes upgrading net8.0 even more difficult? Now I don’t dare to use the AddKeyedSingleton method in the project anymore

@benjaminpetit @steveharter

turowicz commented 9 months ago

I am having the same problem here. AddServiceByName is gone from Orleans.Runtime 8.0-rc1 and I tried using Keyed Services but it blows up in my reflection methods that are unrelated to Orleans.

https://github.com/dotnet/orleans/issues/8775

private IEnumerable<Type> GetSingletons(IServiceCollection services)
{
    return services
        .Where(descriptor => descriptor.Lifetime == ServiceLifetime.Singleton)
        // exception here: "This service descriptor is keyed. Your service provider may not support keyed services."
        .Where(descriptor => descriptor.ImplementationType != typeof(WarmupService)) 
        .Where(descriptor => descriptor.ServiceType.ContainsGenericParameters == false)
        .Select(descriptor => descriptor.ServiceType)
        .Distinct();
}
benjaminpetit commented 9 months ago

We throw when you try to access non-keyed property on a keyed descriptor, and when you try to access keyed property on a non keyed descriptor. It's not a bug, it's to make sure that if the container doesn't support keyed DI it will not build if there is keyed descriptors.

You need to check the value of the property IsKeyedService before accessing the other properties.

amis92 commented 9 months ago

@benjaminpetit I understand what was the intent - but the result is that it throws not when the container doesn't support Keyed DI, but if any components that attempt registration aren't prepared to test keyed-ness. Which makes it a breaking change for a lot of cases.

What you're saying is completely unhelpful for people who want to use an older component that performs such a registration under the hood.

Can you say why returning a null instead of throwing an exception is not a valid fix?

benjaminpetit commented 9 months ago

It's only a breaking change if you are using keyed registration.

Would it be better to return null instead? Maybe, but that could introduce some weird behavior if a library doesn't support keyed registration, which is what we wanted to avoid by throwing an exception.

I don't think there is a right or a wrong solution here. I think throwing an exception is safer, but I understand that it's painful if you cannot update an older component. Maybe in future version we could make it configurable @steveharter ?

amis92 commented 9 months ago

Right now it's a breaking change if I'm using keyed registration, indeed. But soon, as Orlean's linked issue suggests, other components (external to my own codebase) will use Keyed, and it will collide with me using any other external component which doesn't know a thing about Keyed registrations but still performs the check as I've mentioned in OP. Which, assuming I can't just stop using existing dependencies, means I'm blocked from upgrading at all.

This means that soon, I'll be unable to use new components that opted into Keyed registrations, if older components don't update to check for Keyedness.

And all this is well before actual container is involved. We're having issues with registrations which are keyedness-oblivious, while the container does/would support them.

I can't really imagine how could a container that doesn't support keyed services not throw/fail if all of ServiceDescriptor.Implementation* properties returned null (which is what I'd suggest as a solution that doesn't break old registrations). The default sure would. For others, well I didn't use, but no sensible action can be done anyway, can it?

PS Of course, the error message would be more confusing, as it'd come from the container instead of a registration, but if that's the cost of compatibility... I've seen worse.

thuijer commented 9 months ago

It's only a breaking change if you are using keyed registration.

See https://github.com/AzureAD/microsoft-identity-web/issues/2604. Many packages are not updated to take keyed services into account, causing exceptions based on the order in which you register servcies. Throwing an exception may be the right solution when all packages are taking keyed services into account.

ArpitD93 commented 9 months ago

Another issue I just ran into. Calling JsonConvert.SerializeObject() on the ServiceDescriptor is bound to throw error every time, as one of the two getters (ImplementationType, KeyedImplementationType) is always gonna throw exception. This is breaking our existing code.

thuijer commented 9 months ago

... but I understand that it's painful if you cannot update an older component.

@benjaminpetit : Many packages that are looking up services need to be updated. Until then keyed services are virtually impossible to use with the current behaviour of throwing exceptions.

Ephron-WL commented 8 months ago

We throw when you try to access non-keyed property on a keyed descriptor, and when you try to access keyed property on a non keyed descriptor. It's not a bug, it's to make sure that if the container doesn't support keyed DI it will not build if there is keyed descriptors.

You need to check the value of the property IsKeyedService before accessing the other properties.

An exception when calling a property getter is unusual and it kind of threw me until I saw what was going on in the source code.

I think a more helpful exception message would work wonders. The current message is just too vague. However, you explain it well above. How about something like "You are trying to read a property that is not supported when using a keyed descriptor. Consider accessing the same property name prefixed by "Keyed", instead. You may receive this exception if your service provider does not support keyed services. "

jkonecki commented 8 months ago

Orleans 8 has just been released and it's impossible to use it in the same project as Application Insights due to this issue. @ReubenBond

ReubenBond commented 8 months ago

@jkonecki it sounds like this would be fixed by an update to the Application Insights SDK, can you confirm? If so, let's find out who we need to talk to and see if we can get the fix in, @benjaminpetit

jkonecki commented 8 months ago

@ReubenBond @benjaminpetit I'm using the latest ApplicationInsights 2.22.0. I cannot see any newer pre-release packages.

Actually AppInsights was a red herring. The issue is caused by Microsoft.Identity.Web

2>System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
2> ---> System.InvalidOperationException: This service descriptor is keyed. Your service provider may not support keyed services.
2>   at Microsoft.Extensions.DependencyInjection.ServiceDescriptor.ThrowKeyedDescriptor()
2>   at Microsoft.Extensions.DependencyInjection.ServiceDescriptor.get_ImplementationType()
2>   at Microsoft.Identity.Web.MicrosoftIdentityWebAppAuthenticationBuilderExtensions.<>c.<AddMicrosoftIdentityWebAppInternal>b__5_0(ServiceDescriptor s)
2>   at System.Linq.Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)
2>   at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
2>   at Microsoft.Identity.Web.MicrosoftIdentityWebAppAuthenticationBuilderExtensions.AddMicrosoftIdentityWebAppInternal(AuthenticationBuilder builder, Action`1 configureMicrosoftIdentityOptions, Action`1 configureCookieAuthenticationOptions, String openIdConnectScheme, String cookieScheme, Boolean subscribeToOpenIdConnectMiddlewareDiagnosticsEvents, String displayName)
2>   at Microsoft.Identity.Web.MicrosoftIdentityWebAppAuthenticationBuilderExtensions.AddMicrosoftIdentityWebAppWithConfiguration(AuthenticationBuilder builder, Action`1 configureMicrosoftIdentityOptions, Action`1 configureCookieAuthenticationOptions, String openIdConnectScheme, String cookieScheme, Boolean subscribeToOpenIdConnectMiddlewareDiagnosticsEvents, String displayName, IConfigurationSection configurationSection)
2>   at Microsoft.Identity.Web.MicrosoftIdentityWebAppAuthenticationBuilderExtensions.AddMicrosoftIdentityWebApp(AuthenticationBuilder builder, IConfigurationSection configurationSection, String openIdConnectScheme, String cookieScheme, Boolean subscribeToOpenIdConnectMiddlewareDiagnosticsEvents, String displayName)
2>   at Microsoft.Identity.Web.MicrosoftIdentityWebAppAuthenticationBuilderExtensions.AddMicrosoftIdentityWebApp(AuthenticationBuilder builder, IConfiguration configuration, String configSectionName, String openIdConnectScheme, String cookieScheme, Boolean subscribeToOpenIdConnectMiddlewareDiagnosticsEvents, String displayName)
2>   at Microsoft.Identity.Web.MicrosoftIdentityWebAppServiceCollectionExtensions.AddMicrosoftIdentityWebAppAuthentication(IServiceCollection services, IConfiguration configuration, String configSectionName, String openIdConnectScheme, String cookieScheme, Boolean subscribeToOpenIdConnectMiddlewareDiagnosticsEvents, String displayName)

The workaround for me was to Identity Web registration before Orleans client registration.

thuijer commented 8 months ago

@ReubenBond @benjaminpetit @jkonecki

The workaround for me was to Identity Web registration before Orleans client registration.

Yeah, that is a workaround for now, but a totally counterintuitive one. And often not possible/difficult to implement in bigger applications. Registration of services in a container should not have a required order. For now, keyed services are just impossible for us to use now, given that there are too many packages around that are not compatible with keyed services.

rogereeno commented 8 months ago

I am also experiencing this issue when using Orleans 7, calling services.AddOrleansClient( ). I should also state that I am using builder.Host.UseServiceProviderFactory( new AutofacServiceProviderFactory( ) ). Not sure if this should have any impact, though.

The work-around does not help in my case.

(edit) Never mind. This turned out to be an Autofac issue in my case.

rvhromov commented 7 months ago

@rogereeno would you mind sharing your solution? it seems like i've got exactly the same issue with Autofac.

rogereeno commented 7 months ago

I updated to the latest version of Autofac packages.

tore-hammervoll commented 7 months ago

I just spent a day debugging some changes unrelated to Application Insights that was hit by this bug. Luckily the changes never made it to production, since the application crashed on startup due to this setup in our Serilog config:

WriteTo.ApplicationInsights(services.GetRequiredService<TelemetryConfiguration>(), TelemetryConverter.Traces)

The TelemetryConfiguration was not registered due the exception thrown in AddSingletonIfNotExists.

After hours of debugging to figure out what I changed to cause this issue, I isolated it to adding resilience to an HttpClient with AddStandardResilienceHandler() from Microsoft.Extensions.Http.Resilience.

Either AddSingletonIfNotExists has to be updated to handle keyed services, or the ServiceDescriptor implementation has to be changed to not throw exception on property access.

The workaround of having to register Application Insights (or anything else not updated to handle keyed services) first, before anything using keyed service, is not acceptable in the long run. This will not scale as keyed services usage increases in the future.

cyril265 commented 6 months ago

I'm already on .NET 8. Apparently some MS dependencies decided to use keyed services in one of the minor version updates. And now my app breaks if I update. How do I figure out which of those dependencies started using keyed services? 🥴

image
jkonecki commented 6 months ago

Go through their release notes / code changes.

Or trial and error - downgrade to previous working versions and upgrade one at a time.

On Mon, 18 Mar 2024, 10:52 cyril265, @.***> wrote:

I'm already on .NET 8. Apparently some MS dependencies decided to use keyed services in one of the minor version updates. And now my app breaks if I update. How do I figure out which of those dependencies started using keyed services? 🥴 image.png (view on web) https://github.com/dotnet/runtime/assets/3495565/5508c38f-2948-4e7d-baa9-e6201d407482

— Reply to this email directly, view it on GitHub https://github.com/dotnet/runtime/issues/95789#issuecomment-2003590329, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANRND4UAAEEXPIITYWJ2DLYY3BPTAVCNFSM6AAAAABAMZWDP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBTGU4TAMZSHE . You are receiving this because you were mentioned.Message ID: @.***>

KalleOlaviNiemitalo commented 6 months ago

Or enumerate the IServiceCollection (with a debugger or with code in your application) and search for ServiceDescriptor.IsKeyedService; the assembly name of the implementation type may indicate which package is to blame.

amis92 commented 5 months ago

It really is painful, and the error message more often than not is completely wrong, because it's not the ServiceProvider that's at fault, but some registration-performing extensions from external libraries. Can we at least get a warning or add a note in official documentation that suggests there may be issues due to some registration patterns with the new v8.0 of DependencyInjection?

rjgotten commented 5 months ago

@benjaminpetit Would it be better to return null instead? Maybe, but that could introduce some weird behavior if a library doesn't support keyed registration, which is what we wanted to avoid by throwing an exception.

It'd have been better to go with an entirely different design for keyed services. As it stands what the framework did is violate the basic tenet of the Liskov substitution principle. As well as use exceptions as part of nominal control flow, violating that other well known tenet: exceptions should be exceptional.

So maybe what Microsoft should do, is retract this support. With the amount of discussion around the API surface for keyed services that was still going on, it should never have found its way into stable .NET versions anyway. The only thing pushing it through serves to do, is that now you've made your bed and you get to lay in it. Because it's out there, and it's not going away anymore - unless you do the sane thing and reel it back in and remove it again. RIGHT - NOW. While it's still relatively early days.

mosoftwareenterprises commented 5 months ago

I have just bumped into this today, when upgrading a third party nuget package (Polly). I upgraded from version 8.0.0 to 8.3.1 and now I am getting this error. No other code changes occurred. The app I am building is dotnet6 My issue is that the error although descriptive does not indicate which service registration is the problem, and so it makes this very difficult to identify where the problem is. I was lucky as I had only just changed the nuget package and spotted the issue.

As far as I understand it now, I cannot upgrade Polly anymore due to this issue 😱

a-sink-a-wait commented 4 months ago

Ran into this issue today entirely within third party packages.

Microsoft.Extensions.Http.Resilience packages version 8.2.0 and above use keyed services to register polly dependencies. This results in AddApplicationInsightsTelemetry method in Microsoft.Extensions.DependencyInjection and other packages that use it ( Microsoft.Azure.ApplicationInsights.WorkerService ) to fail silently.

try
{
   // brevity
}
catch (Exception e)
{
    WorkerServiceEventSource.Instance.LogError(e.ToInvariantString()); // This doesn't show up in console logs
    return services;
}

This was manifesting with an OptionValidationException when trying to call ConfigureFunctionsApplicationInsights from within a .NET 8 isolated function because TelemetryClient wasn't registered in DI. Chased my tail for a couple hours before finally realizing the package was failing and then finding this thread. Fixed by downgrading Microsoft.Extensions.Http.Resilience to 8.1.0.

Seems like a huge oversight to throw an exception in this case. Especially in a property getter that seems so innocuous to consumers. I'm sure this is causing a lot of frustration in the community.

CSharpFiasco commented 4 months ago

I have just bumped into this today, when upgrading a third party nuget package (Polly). I upgraded from version 8.0.0 to 8.3.1 and now I am getting this error. No other code changes occurred. The app I am building is dotnet6 My issue is that the error although descriptive does not indicate which service registration is the problem, and so it makes this very difficult to identify where the problem is. I was lucky as I had only just changed the nuget package and spotted the issue.

As far as I understand it now, I cannot upgrade Polly anymore due to this issue 😱

I have also ran into this, I had to rollback my Polly updates to 8.1.0

adrian-crisan625 commented 3 months ago

I also subscribe to the fact that this exception driven approach should not have be included in the .NET version. This breaks so many third party libs. And to make the thing worse, some packages will throw the exception, but for example Microsoft.ApplicationInsights.AspNetCore (https://github.com/microsoft/ApplicationInsights-dotnet/issues/2828) will just silently fail, without any warning and no telemetry will be logged anymore. I really think this is a major issue in the .NET framework. I got suggestions to make our own implementation in the dependency provider mechanism, but this defeat the purpose of having a framework the offers you a dependency injection mechanism which does not need additional tweaking. I hope things will be fixed soon. For now we are postponing any package updates, unless we get a vulnerability report on one of them

gerwim commented 2 months ago

Same issue here. We use the service collection to find services which implement one of our interfaces. Our services are not keyed, so now we need to exclude keyed services.

This change breaks third party integrations and should be reverted IMO.

gallivantor commented 1 month ago

@benjaminpetit what is the plan for this? These issues are blocking us upgrading from .NET 6 to .NET 8 because of nuget packages that we use attempting to iterate through the services and check the ImplementationType which is now causing them to crash.
Is there a way forward or are we going to be forced to stay on an unsupported and potentially insecure .net 6 runtime?

steveharter commented 1 month ago

This should be fixed for v9.

adrian-crisan625 commented 1 month ago

This should be fixed for v9.

Do you mean .NET 9? Will not be included as a fix for .NET 8?

rjgotten commented 1 month ago

This should be fixed for v9.

Do you mean .NET 9? Will not be included as a fix for .NET 8?

Frankly - if this were to only be fixed in .NET 9 and .NET 8 would be ignored, that would be completely unacceptable, considering that .NET 8 is an LTS version and will outlive .NET 9 for many production-use environments.

steveharter commented 1 month ago

I didn't say it wouldn't be fixed in v8; we need to get it into v9 first.

KrzysztofBranicki commented 1 month ago

My team also just stumbled into this problem. Can someone explain why it was necessary in the first place to add KeyedImplementationType, KeyedImplementationInstance and KeyedImplementationFactory instead of just using existing properties in combination with newly added ServiceKey which can be either null or not?

amis92 commented 1 month ago

Thank you so much for this decision and change. Will this thread receive an update when it gets back ported to .NET 8? Or is tracking release notes the only option? 🤗

steveharter commented 1 month ago

Yes this thread will be referenced during a backport.

steveharter commented 1 month ago

Can someone explain why it was necessary in the first place to add KeyedImplementationType, KeyedImplementationInstance and KeyedImplementationFactory

One forcing factor is the factory property has different signatures: https://github.com/dotnet/runtime/blob/bd293233e080d299b577d242f8e147598e10c2ba/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs#L152 vs https://github.com/dotnet/runtime/blob/bd293233e080d299b577d242f8e147598e10c2ba/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs#L157

cc @benjaminpetit

sweatyeti commented 1 month ago

just to add another scenario, starting a brand new ASP.NET Core WebAPI on .NET 8 with both Aspire and Microsoft.Identity.Web bits for EntraID scaffolded via the GUI while creating the solution in VS2022 runs into this exception on the very first run with no code changes.

Mike-E-angelo commented 3 weeks ago

just to add another scenario, starting a brand new ASP.NET Core WebAPI on .NET 8 with both Aspire

I too ran into this issue when creating my first Aspire application and incorporating my existing code. Looks like a fix is on the way. Thank you for all your efforts out there. 🙏