Allow external service providers (#550 and #1309) #1322
Conversation
| @@ -153,7 +153,7 @@ public IWebHost Build() | |||
|
|
|||
| var hostingServices = BuildCommonServices(out var hostingStartupErrors); | |||
| var applicationServices = hostingServices.Clone(); | |||
| var hostingServiceProvider = hostingServices.BuildServiceProvider(); | |||
| var hostingServiceProvider = GetProviderFromFactory(hostingServices) ?? hostingServices.BuildServiceProvider(); | |||
|
|
|||
There was a problem hiding this comment.
At present configured factories are ignored and builder uses hardcoded hostingServices.BuildServiceProvider(). Instead it should check if proper factory is registered and if yes, use it to create provider.
Fall back on hardcoded call if nothing is registered.
There was a problem hiding this comment.
I almost like this change. Lets instead build the service provider another time:
var hostingServiceProvider = hostingServices.BuildServiceProvider();
hostingServiceProvider = GetProviderFromFactory(hostingServiceProvider) ?? hostingServiceProvider;
IServiceProvider GetProviderFromFactory(IServiceProvider provider)
{
var factory = provider.GetService<IServiceProviderFactory<IServiceCollection>>();
return factory?.CreateServiceProvider(factory.CreateBuilder(colection));
}It's a bit more robust.
| .ConfigureTestServices(services => services.AddSingleton(new SimpleService { Message = "OverridesConfigureServices" })) | ||
| .ConfigureServices(s => { | ||
| s.AddSingleton<IServiceProviderFactory<ThirdPartyContainer>, ThirdPartyContainerServiceProviderFactory>(); | ||
| s.AddSingleton<IServiceProvider>((a) => s.BuildServiceProvider()); // Provide outside provider instance |
There was a problem hiding this comment.
No, it is not. But I thought I'd include a demonstration of using these either separately or combined.
There was a problem hiding this comment.
It hasn't been removed yet it seems.
| @@ -202,6 +202,14 @@ public IWebHost Build() | |||
| host.Dispose(); | |||
| throw; | |||
| } | |||
|
|
|||
| IServiceProvider GetProviderFromFactory(IServiceCollection colection) | |||
There was a problem hiding this comment.
nit: spelling colection-> collection
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: Remove extra space new line.
| Assert.Equal("ApplicationServicesEqual:True", response); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: Remove extra space new line.
|
What time zone are you in? Do you work in the middle of the night? |
|
|
||
| IServiceProvider GetProviderFromFactory(IServiceCollection collection) | ||
| { | ||
| var provider = collection.BuildServiceProvider(); |
There was a problem hiding this comment.
Hmm after thinking bit, we might need to dispose the temporary container we created If there's an IServiceProviderFactory<IServiceCollection> so this code gets a bit more complicated:
if (factory != null)
{
using (provider)
{
return factory.CreateServiceProvider(factory.CreateBuilder(collection))
}
}
return provider;There was a problem hiding this comment.
You don't want to do that. In case factory is resolved from a type and had any disposable dependencies this code will discard these but it might still be used in the provider.
The provider should die quietly as any other Gen0 variable during next partial collection.
There was a problem hiding this comment.
Any service provider we create should be disposed so that services get disposed. The provider may not be collected if there’s a singleton relying on dispose to unroot itself (we had an example recently where a timer was causing problems). The reasons it’s ok here is because we’re creating a whole new container anyways so disposing services after creation should be fine.
| @@ -71,6 +71,34 @@ public class ThirdPartyContainerStartup | |||
| $"{ctx.RequestServices.GetRequiredService<SimpleService>().Message}, {ctx.RequestServices.GetRequiredService<TestService>().Message}")); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public async Task ExternalContainerInstanceCanBeUsedForEverything() | |||
There was a problem hiding this comment.
How does this verify the custom container is being used?
There was a problem hiding this comment.
How would you like it to be verified? Can you think of all your comments and do them all at once?
There was a problem hiding this comment.
I am not that familiar with your testing framework, I used one of the tests as example and run it in debugger to verify. Feel free to change it any way you like.
There was a problem hiding this comment.
Heh, sorry, I have a bad habit of re-reviewing things where they are big changes. A good test would be to add a service in the ExternalContainer implementation and make sure it's resolved in the Startup constructor.
I would also not add a test to TestServerTests.cs I would add a test to https://github.com/aspnet/Hosting/blob/dev/test/Microsoft.AspNetCore.Hosting.Tests/WebHostBuilderTests.cs
There was a problem hiding this comment.
Is this it? Think about it, I am catching flight in a few hours. I have time just for one more change.
There was a problem hiding this comment.
I can take over if you want. It's pretty much done.
There was a problem hiding this comment.
I would appreciate if you do. I am getting low on time.
|
Replaced by #1325 |
This proposed solution allows replacement of application as well as framework service providers.
It should satisfy requirements in both #550 and #1309 and might eliminate need for #1060