ServiceDescriptor.ImplementationType throws exception for Keyed descriptor: System.InvalidOperationException: This service descriptor is keyed. Your service provider may not support keyed services.
amis92 opened this issue · 42 comments
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:
Reproduction Steps
dotnet new consoledotnet add package Microsoft.Extensions.DependencyInjection
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
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:
Problematic source code:
Reproduction Steps
dotnet new consoledotnet add package Microsoft.Extensions.DependencyInjection
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.
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: |
|
| Milestone: | - |
I also encountered it, I think this is a bug, KeyedImplementationType needs to check whether KeyedService is true, but ImplementationType should not be checked
@benjaminpetit PTAL
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
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.
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();
}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.
@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?
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 ?
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.
It's only a breaking change if you are using keyed registration.
See AzureAD/microsoft-identity-web#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.
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.
... 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.
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
IsKeyedServicebefore 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. "
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
@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
@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.
@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.
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.
@rogereeno would you mind sharing your solution? it seems like i've got exactly the same issue with Autofac.
I updated to the latest version of Autofac packages.
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.
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? 🥴
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.
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?
@benjaminpetit
Would it be better to returnnullinstead? 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.
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 😱
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.
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
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 (microsoft/ApplicationInsights-dotnet#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
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.
@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?
This should be fixed for v9.
This should be fixed for v9.
Do you mean .NET 9? Will not be included as a fix for .NET 8?
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.
I didn't say it wouldn't be fixed in v8; we need to get it into v9 first.
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?
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? 🤗
Yes this thread will be referenced during a backport.
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:
vs
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.
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. 🙏