Open FooRider opened 2 years ago
Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection See info in area-owners.md if you want to be subscribed.
Author: | FooRider |
---|---|
Assignees: | - |
Labels: | `api-suggestion`, `untriaged`, `area-Extensions-DependencyInjection` |
Milestone: | - |
I agree with this proposal but I just want to mention the multiple IDisposable calls should be a nonissue as I think it seems to be the standard pattern that multiple calls to Dispose should do nothing.
https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose (3rd paragraph)
I agree with this proposal but I just want to mention the multiple IDisposable calls should be a nonissue as I think it seems to be the standard pattern that multiple calls to Dispose should do nothing.
Note that the dispose order is also affected, not only the number of calls.
E.g.
class ExampleNameRunner
{
private readonly INameGenerator nameGenerator;
public ExampleNameRunner(INameGenerator nameGenerator)
{
this.nameGenerator = nameGenerator;
}
public string GenerateName()
=> nameGenerator.GenerateName();
}
class ExampleGreetingRunner
{
private readonly IGreetingGenerator greetingGenerator;
public ExampleGreetingRunner(IGreetingGenerator greetingGenerator)
{
this.greetingGenerator = greetingGenerator;
}
public string GenerateGreeting()
=> greetingGenerator.GenerateGreeting();
}
If you instanciate ExampleNameRunner then ExampleGreetingRunner, the creation order will be the following:
Since the services are disposed in the reverse order (as they should), IGreetingGenerator will be disposed before ExampleNameRunner, which means ExampleNameRunner will hold a reference to a disposed singleton without itself having been disposed.
This is a very relatable issue. I sometimes run into it with configuration classes (1 config implementation implementing several config interfaces).
I think the array of service types is not the right way to go. I'd rather have some helper method create multiple servicedescriptors as you now manually do.
As for validation, I recommend adding a unit test just for DI that verifies you can resolve the services you want to expose.
Proposal makes sense to me and I've needed this many times in the past. However, I see zero reason why this is being limited to singletons.
The new API should be implemented to work with all lifetimes.
@wvpm
I think the array of service types is not the right way to go. I'd rather have some helper method create multiple servicedescriptors as you now manually do.
Why would you rather have N service descriptors registered? IMHO, the most "native" approach to this would be to have one descriptor and expand the current ServiceDescriptor.Type
property into a Types
array.
Why would you rather have N service descriptors registered? IMHO, the most "native" approach to this would be to have one descriptor and expand the current
ServiceDescriptor.Type
property into aTypes
array.
The Microsoft DI package is meant to be compatible with all implementations. Expanding the ServiceDescriptor requires all implementations to also expand. Creating multiple ServiceDescriptors is something that already works. It just looks and feels weird. I previously ran into a similar issue, see https://github.com/dotnet/runtime/issues/41050#issuecomment-677785224 .
Background and motivation
When using Microsoft.Extensions.DependencyInjection, there is no way to explicitly bind multiple interfaces to a singleton instance of their implementation. Common workaround is to create ServiceDescriptor with implementationFactory that calls IServiceProvider.GetRequiredService() method.
This approach has two issues with current implementation:
Minimum example of the disposing issue:
Outputs:
Minimum example of the validation issue (assume the same support classes and interfaces):
In this example, ServiceProvider validation doesn't reveal the fact that INameGenerator, nor IGreetingGenerator instances could be created using provided bindings, and doesn't throw an exception from .Build() method, allowing the program to continue and fail after initialization, when requesting ExampleRunner service.
API Proposal
Both of the current issues stem from the fact that ServiceDescriptor class currently doesn't know that multiple different service types are provided by the same instance of implementation type. There are multiple possible ways to encode this information into ServiceDescriptor class.
Possible solution # 1 - modify ServiceDescriptor class to contain multiple types as its ServiceType, in new property ServiceTypes.
API Usage
Usage of possible solution # 1:
Using extension methods:
Another extension method using generics:
The name of this method should be different from "AddSingleton", because it could be confusing. Name of the extension method in the example is just a first proposal, a better name could be surely found.
Alternative Designs
Another solution would require IoC container to check whether implementation type is already bound and use that binding to satisfy request for requested service type.
This could be problematic, since it would change current behavior.
Risks
Proposed solution will have consequences for current implementation of IoC containers. Each implementation would need to be able to work with ServiceDescriptors that have (ServiceType != null && ServiceTypes == null) and (ServiceType == null && ServiceTypes != null) and somehow implement the new scenario.
Proposed alternative solution will mean no changes in API interface, but would specify different behavior for container implementation. The risk for existing projects could be mitigated by new option in ServiceProviderOptions class that would enable this new behavior on request. Problem with this could be perhaps non-intuitive behavior when bindings do not have the same ServiceLifetime set.