Fix consistency issue in GetKeyedServices with AnyKey #97561

benjaminpetit · 2024-01-26T16:00:29Z

As noted in #95582, @halter73 noticed that there was some inconsistency, because a call to enumerate all services will create some cache.

In this PR, we make sure that when enumerating all services with KeyedService.AnyKey, we create matching callsite that matches the real descriptor identifier.

ghost · 2024-01-26T16:00:35Z

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

Issue Details

As noted in #95582, @halter73 noticed that there was some inconsistency, because a call to enumerate all services will create some cache.

In this PR, we make sure that when enumerating all services with KeyedService.AnyKey, we create matching callsite that matches the real descriptor identifier.

Author:	benjaminpetit
Assignees:	-
Labels:	`area-Extensions-DependencyInjection`
Milestone:	-

steveharter · 2024-01-26T18:49:26Z

Note: the feedback from #95582 (comment) states:

Only include all matching AnyKey-registered services if there are no null-registered services.
- This is equivalent to what we do for keys that are not AnyKey or null.
Always include all matching AnyKey-registered services after null-registered services.
- Then we should update the behavior for keys that are not AnyKey or null to match.
Continue to say the AnyKey only applies to non-null keys
- This is probably more surprising but simpler to explain.

About not including the registration with KeyedService.AnyKey: it would make things simpler (for me at least) but will be very confusing for the user.

I think it's simpler and easier to explain to not return non-keyed services in GetKeyedServices(KeyedService.AnyKey). It's already very confusing, and the developer can call GetKeyedServices and GetServices to get both.

...raries/Microsoft.Extensions.DependencyInjection/Microsoft.Extensions.DependencyInjection.sln

...ns.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs

benjaminpetit · 2024-01-30T19:14:21Z

Note: the feedback from #95582 (comment) states:

Only include all matching AnyKey-registered services if there are no null-registered services.

This is equivalent to what we do for keys that are not AnyKey or null.

Always include all matching AnyKey-registered services after null-registered services.

Then we should update the behavior for keys that are not AnyKey or null to match.

Continue to say the AnyKey only applies to non-null keys

This is probably more surprising but simpler to explain.

About not including the registration with KeyedService.AnyKey: it would make things simpler (for me at least) but will be very confusing for the user.

I think it's simpler and easier to explain to not return non-keyed services in GetKeyedServices(KeyedService.AnyKey). It's already very confusing, and the developer can call GetKeyedServices and GetServices to get both.

Yes I still need to update that part, I can do it in this PR too.

…ey as the lookup key

halter73 · 2024-02-07T22:10:21Z

...ns.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs

-            Assert.Equal(5, allServices.Count);
-            Assert.Equal(new[] { service1, service2, service3, service4 }, allServices.Skip(1));
+            Assert.Equal(4, allServices.Count);
+            Assert.Equal(new[] { service1, service2, service3, service4 }, allServices);


Suggested change

Assert.Equal(new[] { service1, service2, service3, service4 }, allServices);

Assert.Equal(new[] { service1, service2, service3, service4 }, allServices);

var someKeyedServices = provider.GetKeyedServices<IService>("service").ToList();

Assert.Equal(new[] { service2, service3, service4 }, someKeyedServices);

var unkeyedServices = provider.GetServices<IService>().ToList();

Assert.Equal(new[] { service5, service6 }, unkeyedServices);

Nit: I'm assuming this all works, and some of this might even be covered partially by other tests. But we don't have many tests with the combination of AnyKey registrations, unkeyed registrations, and enumerable resolution of both keyed and unkeyed services. And I don't see the harm in extra verification.

halter73 · 2024-02-07T22:31:23Z

...ns.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs

+            // Check twice in different order to check caching
+            var provider2 = CreateServiceProvider(serviceCollection);
+            Assert.Equal(new[] { service }, provider2.GetKeyedServices<IService>(KeyedService.AnyKey));
+            Assert.Same(any, provider2.GetKeyedService<IService>(KeyedService.AnyKey));


Now that the AnyKey has special status for enumerable lookups, I think we should be careful not to imply it's doing anything special for single service resolution. The simplest thing is to simply not allow it, and only allow it for service registration and enumerable lookups.

If you have [ServiceKey] string key in the constructor of your AnyKey service, the above line will already throw, so it's not something a library or framework developer can rely on always working anyway.

System.InvalidOperationException: The type of the key used for lookup doesn't match the type in the constructor parameter with the ServiceKey attribute.

Given this, and that you can just use new object() instead if you really want to get the AnyKey registration for a service and you know it will not blow up on plain object instance, I see no reason not to always throw an InvalidOperationException when someone attempts to resolve a single service using the AnyKey. Then we also don't have to explain why the service returned when using AnyKey for single service resolution isn't included in the enumerable lookup.

Technically this is breaking like any behavior change is, but I would be surprised if anything other than our tests specifically try to resolve a single service with the AnyKey. And if anyone is doing that, they should probably stop and just use something else like new object() or "" which is easier to reason about.

Suggested change

Assert.Same(any, provider2.GetKeyedService<IService>(KeyedService.AnyKey));

Assert.ThrowsAny<InvalidOperationException>(() => provider2.GetKeyedService<IService>(KeyedService.AnyKey));

Assert.Same(any, provider2.GetKeyedService<IService>(new object()));

Less importantly, it also leaves the door open for us retconning what the AnyKey in single service resolution means. Maybe in .NET 20, we decide that it should return a non-keyed service if there are no keyed service registrations for a given type,

I agree that we should throw on GetKeyedService(AnyKey) and also agree on the previously discussed hiding of the AnyKey registration when querying with IEnumerable<>.

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs

ghost · 2024-02-29T03:01:12Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

stephentoub · 2024-03-21T14:19:59Z

Reopened based on #95853 (comment):
"we first need to reopen and finish #97561 (I don't think we have consensus right now on the right approach)"

dotnet-policy-service · 2024-04-04T17:25:33Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

dotnet-policy-service · 2024-04-18T21:50:09Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

robertmclaws · 2024-05-05T22:09:14Z

So is anything happening with this?

steveharter · 2025-03-04T21:12:34Z

So is anything happening with this?

Re-opened via #113137

Fix consistency issue in GetKeyedServices with AnyKey

e96f6f7

benjaminpetit added the area-Extensions-DependencyInjection label Jan 26, 2024

ghost assigned benjaminpetit Jan 26, 2024

build-analysis bot mentioned this pull request Jan 26, 2024

Tests crashing in CI with no dump: exit code 137 means SIGKILL Killed #97049

Closed

steveharter reviewed Jan 26, 2024

View reviewed changes

...raries/Microsoft.Extensions.DependencyInjection/Microsoft.Extensions.DependencyInjection.sln Outdated Show resolved Hide resolved

...ns.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs Show resolved Hide resolved

steveharter requested a review from halter73 January 31, 2024 17:19

benjaminpetit added 2 commits February 2, 2024 15:39

Do not return AnyKey registration when enumerating services with AnyK…

d6bacae

…ey as the lookup key

Revert solution change

7d22771

This was referenced Feb 2, 2024

Tracking issue for CI build timeouts #76454

Closed

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

halter73 reviewed Feb 7, 2024

View reviewed changes

steveharter added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 14, 2024

ghost added the no-recent-activity label Feb 29, 2024

dotnet-policy-service bot closed this Mar 14, 2024

benjaminpetit mentioned this pull request Mar 21, 2024

[release/8.0-staging] Add support for enumerating keyed services with KeyedService.AnyKey #95853

Closed

2 tasks

stephentoub reopened this Mar 21, 2024

dotnet-policy-service bot removed the no-recent-activity label Mar 21, 2024

dotnet-policy-service bot added the no-recent-activity label Apr 4, 2024

dotnet-policy-service bot closed this Apr 18, 2024

github-actions bot locked and limited conversation to collaborators Jun 5, 2024

steveharter self-assigned this Mar 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix consistency issue in GetKeyedServices with AnyKey #97561

Fix consistency issue in GetKeyedServices with AnyKey #97561

benjaminpetit commented Jan 26, 2024

ghost commented Jan 26, 2024

steveharter commented Jan 26, 2024 •

edited

Loading

benjaminpetit commented Jan 30, 2024

halter73 Feb 7, 2024

halter73 Feb 7, 2024

steveharter Mar 6, 2025 •

edited

Loading

ghost commented Feb 29, 2024

stephentoub commented Mar 21, 2024

dotnet-policy-service bot commented Apr 4, 2024

dotnet-policy-service bot commented Apr 18, 2024

robertmclaws commented May 5, 2024

steveharter commented Mar 4, 2025

-            Assert.Equal(new[] { service1, service2, service3, service4 }, allServices);
+            Assert.Equal(new[] { service1, service2, service3, service4 }, allServices);
+            var someKeyedServices = provider.GetKeyedServices<IService>("service").ToList();
+            Assert.Equal(new[] { service2, service3, service4 }, someKeyedServices);
+            var unkeyedServices = provider.GetServices<IService>().ToList();
+            Assert.Equal(new[] { service5, service6 }, unkeyedServices);

	Assert.Same(any, provider2.GetKeyedService<IService>(KeyedService.AnyKey));
	Assert.ThrowsAny<InvalidOperationException>(() => provider2.GetKeyedService<IService>(KeyedService.AnyKey));
	Assert.Same(any, provider2.GetKeyedService<IService>(new object()));

Fix consistency issue in GetKeyedServices with AnyKey #97561

Fix consistency issue in GetKeyedServices with AnyKey #97561

Conversation

benjaminpetit commented Jan 26, 2024

ghost commented Jan 26, 2024

steveharter commented Jan 26, 2024 • edited Loading

benjaminpetit commented Jan 30, 2024

halter73 Feb 7, 2024

Choose a reason for hiding this comment

halter73 Feb 7, 2024

Choose a reason for hiding this comment

steveharter Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

ghost commented Feb 29, 2024

stephentoub commented Mar 21, 2024

dotnet-policy-service bot commented Apr 4, 2024

dotnet-policy-service bot commented Apr 18, 2024

robertmclaws commented May 5, 2024

steveharter commented Mar 4, 2025

steveharter commented Jan 26, 2024 •

edited

Loading

steveharter Mar 6, 2025 •

edited

Loading