Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

Allow external service providers (#550 and #1309) #1322

Closed
wants to merge 8 commits into from
Closed

Allow external service providers (#550 and #1309) #1322

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b1ad979
Allow external service providers (#550 and #1309)
ENikS Jan 30, 2018
da37dab
Consolidated it a bit more
ENikS Jan 31, 2018
fc23c07
Removed test
ENikS Jan 31, 2018
207af1d
Spelling
ENikS Jan 31, 2018
8190d24
Update WebHostBuilder.cs
ENikS Jan 31, 2018
75a5b5b
Update TestServerTests.cs
ENikS Jan 31, 2018
a83e93e
Update WebHostBuilder.cs
ENikS Jan 31, 2018
4526219
Update WebHostBuilder.cs
ENikS Jan 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/Microsoft.AspNetCore.Hosting/WebHostBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (!_options.SuppressStatusMessages)
{
Expand Down Expand Up @@ -202,6 +202,14 @@ public IWebHost Build()
host.Dispose();
throw;
}

IServiceProvider GetProviderFromFactory(IServiceCollection collection)
{
var provider = collection.BuildServiceProvider();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand

var factory = provider.GetService<IServiceProviderFactory<IServiceCollection>>();

return factory?.CreateServiceProvider(factory.CreateBuilder(collection)) ?? provider;
}
}

private IServiceCollection BuildCommonServices(out AggregateException hostingStartupErrors)
Expand Down
28 changes: 28 additions & 0 deletions test/Microsoft.AspNetCore.TestHost.Tests/TestServerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,34 @@ public void Configure(IApplicationBuilder app) =>
$"{ctx.RequestServices.GetRequiredService<SimpleService>().Message}, {ctx.RequestServices.GetRequiredService<TestService>().Message}"));
}

[Fact]
public async Task ExternalContainerInstanceCanBeUsedForEverything()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this verify the custom container is being used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you like it to be verified? Can you think of all your comments and do them all at once?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this it? Think about it, I am catching flight in a few hours. I have time just for one more change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can take over if you want. It's pretty much done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would appreciate if you do. I am getting low on time.

{
var builder = new WebHostBuilder()
.UseStartup<CustomContainerStartup>()
.ConfigureServices(s => // Register outside provider instance
s.AddSingleton<IServiceProviderFactory<IServiceCollection>>(new ExternalContainerFactory()));

var host = new TestServer(builder);

var response = await host.CreateClient().GetStringAsync("/");

Assert.Equal("ApplicationServicesEqual:True", response);
}

public class ExternalContainerFactory : IServiceProviderFactory<IServiceCollection>
{
public IServiceCollection CreateBuilder(IServiceCollection services)
{
return services;
}

public IServiceProvider CreateServiceProvider(IServiceCollection containerBuilder)
{
return containerBuilder.BuildServiceProvider();
}
}

public class ThirdPartyContainer
{
public IServiceCollection Services { get; set; }
Expand Down