Add support for enumerating keyed services with KeyedService.AnyKey

[benjaminpetit]

My take on how we should fix #95308

Registering "keyed" service with a null key was supposed to be possible for convenience, however it will be considered as a non-keyed service. That's why I think we should not return them when enumerating services.

[ghost]

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

Issue Details

---

My take on how we should fix #95308

Registering "keyed" service with a null key was supposed to be possible for convenience, however it will be considered as a non-keyed service. That's why I think we should not return them when enumerating services.

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

[stephentoub]

Registering "keyed" service with a null key was supposed to be possible for convenience, however it will be considered as a non-keyed service. That's why I think we should not return them when enumerating services.

FWIW, that still doesn't make sense to me, given how it works today. You can call GetKeyedServices with a null key and it returns all services registered without a key and with a null key. We made null a valid key. It's logical then that AnyKey includes null keys and therefore no keys.

[stephentoub]

Other than the null thing, lgtm. Thanks!

[steveharter]

Some test questions, but otherwise LGTM

[stephentoub]

It still doesn't make sense to me that:

sp.GetKeyedService<T>(null)

can return something that:

sp.GetKeyedServices<T>()

will not include.

[stephentoub]

Regardless, thank you for fixing this; will you handle backporting it as well?

[steveharter]

Regardless, thank you for fixing this; will you handle backporting it as well?

Yes this will be backported.

[benjaminpetit]

/backport to release/8.0-staging

[github-actions]

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7167493983

[benjaminpetit]

It still doesn't make sense to me that:

sp.GetKeyedService<T>(null)

can return something that:

sp.GetKeyedServices<T>()

will not include.

Maybe we shouldn't have permitted sp.GetKeyedService<T>(null)...

[stephentoub]

It still doesn't make sense to me that:

sp.GetKeyedService<T>(null)

can return something that:

sp.GetKeyedServices<T>()

will not include.

Maybe we shouldn't have permitted sp.GetKeyedService<T>(null)...

Maybe... but we did.

[davidfowl]

@benjaminpetit can you update the original issue to make sure other DI container authors see this change

[halter73]

I agree with @benjaminpetit that a null key is equivalent to no key. It's no different from calling GetService without a key at that point. The naming of GetKeyedService is a bit unfortunate. It would probably be better to make the serviceKey non-nullable, or add a nullable IKeyedServiceProvider.GetService "overload" that takes a serviceKey without "Keyed" in the name. It's too late for that now though.

Are there any tests for the non-plural provider.GetKeyedService<IService>(KeyedService.AnyKey)? I think the intention is to always return null in this case unless there was a specific AnyKey registration like before. That might now be more unexpected given that provider.GetKeyedServices<IService>(KeyedService.AnyKey) can be non-empty when provider.GetKeyedService<IService>(KeyedService.AnyKey) returns null. By contrast, if provider.GetServices<IService>() is non-empty provider.GetService<IService>() will only return null given a null-returning factory. The same used to be true for the AnyKey, but isn't after this change.

I'm fine with the singular AnyKey resolution continuing to return null unless there was a specific AnyKey registration despite the change to the plural resolution, but we should definitely add tests. Right now, singular AnyKey resolution changes the results of any subsequent plural AnyKey resolutions and vice versa:

var sc1 = new ServiceCollection();
sc1.AddKeyedSingleton<IService>("first-service", new Service("first-service"));

var sp1 = sc1.BuildServiceProvider();
Console.WriteLine($"sp1 Name: {sp1.GetKeyedService<IService>(KeyedService.AnyKey)?.Name ?? "null"}");
Console.WriteLine($"sp1 Names: {string.Join(", ", sp1.GetKeyedServices<IService>(KeyedService.AnyKey).Select(s => s.Name))}");
Console.WriteLine();

var sp2 = sc1.BuildServiceProvider();
Console.WriteLine($"sp2 Names: {string.Join(", ", sp2.GetKeyedServices<IService>(KeyedService.AnyKey).Select(s => s.Name))}");
Console.WriteLine($"sp2 Name: {sp2.GetKeyedService<IService>(KeyedService.AnyKey)?.Name ?? "null"}");

class Service(string name) : IService
{
    public string Name => name;
}

interface IService
{
    string Name { get; }
}

Output:

sp1 Name: 'null'
sp1 Names: 'first-service'

sp2 Names: 'first-service'
sp2 Name: 'first-service'

And then things get even weirder with an AnyKey registration:

var sc3 = new ServiceCollection();
sc3.AddKeyedSingleton<IService>(KeyedService.AnyKey, (sp, key) => new Service($"Any '{key}'"));
sc3.AddKeyedSingleton<IService>("first-service", new Service("first-service"));

var sp3 = sc3.BuildServiceProvider();
Console.WriteLine($"sp3 Name: {sp3.GetKeyedService<IService>(KeyedService.AnyKey)?.Name ?? "null"}");
Console.WriteLine($"sp3 Names: {string.Join(", ", sp3.GetKeyedServices<IService>(KeyedService.AnyKey).Select(s => s.Name))}");
Console.WriteLine();

var sp4 = sc3.BuildServiceProvider();
Console.WriteLine($"sp4 Names: {string.Join(", ", sp4.GetKeyedServices<IService>(KeyedService.AnyKey).Select(s => s.Name))}");
Console.WriteLine($"sp4 Name: {sp4.GetKeyedService<IService>(KeyedService.AnyKey)?.Name ?? "null"}");

Output:

sp3 Name: Any '*'
sp3 Names: Any '*', Any '*'

sp4 Names: Any '*', first-service
sp4 Name: first-service

Another thing I dislike about this change is that plural resolution for singleton and scoped services registered using the AnyKey will not return the same instances of the service that you would get (or have already gotten) when attempting to resolve a specific key only to have it fall back to the AnyKey registration. This stands in contrast to singleton and scoped services registered using specific keys. I know that we cannot anticipate what keys will be used to resolve AnyKey-registered services, and it would be madness to have what keys have been resolved already impact future plural resolution.

That's why I think it's better not to include the AnyKey registration at all in the plural results. If someone wants an instance of the AnyKey service, they can already easily by get it by calling the non-plural GetKeyedService with AnyKey. That way it's easy to know exactly which service was registered with the AnyKey versus which services were registered with specific keys. This would make it impossible to resolve multiple AnyKey registrations, but I don't think that's a huge loss. It can always be the last AnyKey registration wins and any other is irrelevant. We should add tests for multiple AnyKey registrations too no matter how we chose to handle it.

I'm also not sure it makes sense to return multiple services registered using the same specific key when doing plural AnyKey resolution. In the more common scenario when applications resolve a single instance of a keyed services, the last registered service for the given key wins and the rest are completely ignored. This will cause services to be resolved which otherwise probably never would be and could cause issues if any of the previously ignored service cannot be initialized. It would probably make more sense to return the last registered service for each given key that is not the AnyKey.

In any case where applications are resolving multiple services for the same key, the application/library/framework author should know what keys are relevant and resolve them directly. If the application needs to be able to handle an arbitrary number of keys that are unknown even at resolution time, it's not clear why you couldn't give each service a unique key.

@davidfowl @stephentoub Do you think we should discuss this with the DI council?

[halter73]

After discussing this a bit more with @stephentoub, I can see the reason in including the non/null-keyed service (if any) in the GetKeyedServices(KeyedService.AnyKey) results considering that GetKeyedServices(null) already returns those services. However, if we want to take that line of thinking to its logical conclusion, KeyedService.AnyKey should apply to non-keyed services universally meaning that services registered using KeyedService.AnyKey should be used during normal non-keyed service resolution if there's no matching service descriptor with a null key.

Then there's the question of what happens for IEnumerable non-keyed service resolution. Should that also include the AnyKey-registered services? We could

• 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.

[benjaminpetit]

@halter73 yeah it seems I missed this when implementing this PR. But that's why it wasn't implemented in the first place. I will open a PR fixing the first issue.

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.