Allow external container to resolve IStartup #1321

ENikS · 2018-01-30T00:30:10Z

dnfclas · 2018-01-30T00:30:21Z

All CLA requirements met.

ENikS · 2018-01-30T00:45:15Z

src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs

@@ -188,7 +195,7 @@ private void EnsureStartup()
                return;
            }

-            _startup = _hostingServiceProvider.GetService<IStartup>();
+            _startup = (_applicationServices ?? _hostingServiceProvider).GetService<IStartup>();


I am not sure if resolved IStartup instance should be registered with _hostingServiceProvider as well. If it does, let me know. I'll add it.

Why are you resolving the IStartup from the application services? That's just broken. The hosting service provider is responsible for building the IStartup. There are 2 separate phases.

Because I need to resolve it from Unity when it is provided as instance. Did you run the test I submitted?

At this point in time _applicationServices holds reference to externally supplied IServiceProvider or null in which case normal flow kicks in and IStartup is resolved from _hostingServiceProvider.

davidfowl · 2018-01-30T01:18:42Z

src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs

@@ -176,8 +176,15 @@ private void EnsureApplicationServices()
        {
            if (_applicationServices == null)
            {
+                _applicationServices = (IServiceProvider)_applicationServiceCollection


Doesn't this code just not call _startup.ConfigureServices(...) if an IServiceProvider is specified?

Any code that looks at service registrations is usually assuming too much.

It just skips calling provider factory if instance is provided. The rest is the same as usual.

Doesn't this code grab the IServiceProvider out of the IServiceCollection (which itself is an anti-pattern) and then skips calling _startup.ConfigureServices if it's an instance?

davidfowl · 2018-01-30T01:21:49Z

Generally I don't like the approach and I think #550 cannot be fixed without changing the overall design of the WebHostBuilder. It has to be the owner of the DI container as things are designed today. This PR makes an attempt to make that not the case but it actually doesn't work well as it basically skips calling into ConfigureServices in the startup class.

ENikS · 2018-01-30T01:24:43Z

If you run the test I've provided you will see that with exception of ServiceProviderFactory everything else is called properly. Calling Factory methods in this scenario is pointless when service provider already instantiated.

ENikS · 2018-01-30T01:33:16Z

@davidfowl

This PR makes an attempt to make that not the case but it actually doesn't work well as it basically skips calling into ConfigureServices in the startup class.

Just checked it again and every single configure method is being called.

davidfowl · 2018-01-30T01:41:15Z

I don’t see how any of this can work. If you pass a concrete service provider to the WebHostBuilder, what’s the contract? What service provider is used for each of the scenarios?

Part of the confusion is that you claim this fixes #500, but it really doesn’t.

ENikS · 2018-01-30T02:12:05Z

Part of the confusion is that you claim this fixes #500, but it really doesn’t.

It fixes #550 (Allow acceptance of pre-created IServiceProvider instance). I'll refactor the test so it is more obvious.

ENikS · 2018-01-30T02:42:47Z

src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs

+                _applicationServices = (IServiceProvider)_applicationServiceCollection
+                    .LastOrDefault(d => d.ServiceType == typeof(IServiceProvider) &&
+                                        d.ImplementationInstance != null)?
+                    .ImplementationInstance;


We can not do this: _hostingServiceProvider.GetService<IServiceProvider>(); because it will return _hostingServiceProvider. We need to look it up in the list.

Perhaps you know better way of doing the lookup?

I think the entire feature is undesirable really and this is a hack.

The problem is that there are 3 service providers and the service provider you get when you build is different to the one added to the IServiceCollection. It's using the IServiceCollection like a property bag to shuttle the information to the WebHost

ENikS · 2018-01-30T16:21:52Z

I replaced solution with less 'naive' approach. It should be more robust now.

The good part about this fix, it is non invasive. It does not change behavior or API but solves both of these #1309 #550 .

davidfowl · 2018-01-30T17:11:15Z

I fundamentally have an aversion to this change because IMO it doesn't make much sense to add an IServiceProvider to the IServiceCollection. There are 2 service providers:

The hosting service provider - This activates the IStartup and the IServiceProviderFactory (it also has the LoggerFactory which causes a bunch of problems).
The application service provider - This is a combination of the hosting services + the services added in IStartup.ConfigureServices.

There's no physical way to replace the application service provider because hosting fundamentally needs to call into user code to then build the "final" service provider. Passing in an already constructed one will never work. This is what #550 is asking for.

Replacing the hosting service provider is achievable and it's basically what you have to do to achieve the "use a third party container to resolve IStartup". I believe the cleanest way to achieve this is to use the IServiceProviderFactory outside of any dependency injection (so removing it from the hosting container) and using it on both the hosting services as well as the application services. Today it's only used on the application services.

This PR attempts to shuttle an IServiceProvider implementation through the hosting services (which is a clever "hack") and then resolve the "last" IServiceProvider instance to resolve the IStartup. This feels extremely fragile and can break if the default dependency injection implementation changes to disallow overriding the internal IServiceProvider implementation. On top of that, to make things work well, you really need to preserve the existing hosting services when resolving the IStartup or things may fall apart. It means something like this would just fail:

public static IWebHost BuildWebHost(string[] args)
{
    var hostingServices = new ServiceCollection();
    return WebHost.CreateDefaultBuilder(args)
        .UseStartup<Startup>()
        .ConfigureServices(services =>
        {
            services.AddSingleton<IServiceProvider>(hostingServices.BuildServiceProvider());
        })
        .Build();
}

If you want to go this route, then we need to define a separate interface that hosting can use to replace the hosting services independently from application services. The WebHostBuilder does something like this:

public interface IHostingServiceProviderFactory
{
    IServiceProvider BuildServiceProvider(IServiceCollection services);
}

Internally, hosting would resolve this from the container (we might have to build another container) and call BuildServiceProvider passing in the initial collection it creates internally to get the final hosting IServiceProvider.

ENikS · 2018-01-30T17:32:46Z

Your reservations are based on the notion that DI container has to be built before it could be used. Unity and some other containers do not require Build step. Registrations could be adjusted on the fly. Added, removed, replaced.
So it is possible to use Unity before, during and after WebHost is created.

This feels extremely fragile and can break if the default dependency injection implementation changes to disallow overriding the internal IServiceProvider implementation

The behavior of the DI container this solution is based on is Required by the Microsoft.Extensions.DependencyInjection specification and will not change unless specification itself changes. At which point this whole implementation will stop working anyway.

In normal flow events happen in this sequence:

Internal DI resolves IStartup
ConfigureContainer / Container factory is called
Rest of the execution uses resolved container

In proposed PR flow is as follows:

Builder checks if IServiceProvider is registered
If provider registered IStartup is resolved from it, If not is falls back on default implementation
Since container already created factory call is unnecessary. Instead IStartup.ConfigureServices must be called.
Rest of the execution is default behavior

The only difference is where IStartup is resolved from.

During resolution of IStartup only types required are these used in constructor of implementing type. Nothing from Hosting namespace is required at that moment.
Immediately after type is created framework calls ConfigureServices (again, default behavior) and all required types are introduced into Container/Provider. Returned IServiceProvider will have all required types registered.
So as you can see the flow is not interrupted

If you run the test and check it in debugger you will see that all your concerns are addressed. All required services are registered and resolve properly including server, logging, etc..

ENikS · 2018-01-30T17:53:02Z

On top of that, to make things work well, you really need to preserve the existing hosting services when resolving the IStartup or things may fall apart. It means something like this would just fail:

As long as services are in service collection they will be registered with the container and available during resolution.

Fixed #550

ENikS · 2018-01-30T21:47:15Z

After thinking about it, you are right. It does not fix #550. I was only thinking about IStartup scenario when claiming it does. I've removed reference to #550

davidfowl · 2018-01-30T23:06:34Z

As long as services are in service collection they will be registered with the container and available during resolution.

Only if you use the service collection provided by ConfigureServices which is required.

ENikS · 2018-01-30T23:09:15Z

Yes, but this solution does not require collection replacement. It uses provided by the framework collection. It other words: It always uses collection provided by ConfigureServices

davidfowl · 2018-01-30T23:12:05Z

@ENikS you say "it" but thats not expressed in any contract. The example I show is completely broken. So you can't just provide any service provider and you have to understand this hidden contract to make any of this work.

ENikS · 2018-01-30T23:14:02Z

What are you talking about, what contract? What am I missing?
The test runs, everything initializes, what exactly are you talking about?

ENikS · 2018-01-30T23:15:08Z

I sent you another PR which solves both of these problems.

ENikS · 2018-01-30T23:22:08Z

I am withdrawing this PR because #1322 provides more complete solution

ENikS mentioned this pull request Jan 30, 2018

Allow resolving Startup from external service provider #1309

Closed

ENikS commented Jan 30, 2018

View reviewed changes

davidfowl reviewed Jan 30, 2018

View reviewed changes

ENikS commented Jan 30, 2018

View reviewed changes

ENikS mentioned this pull request Jan 30, 2018

Allow acceptance of pre-created IServiceProvider instance #550

Closed

Fixed #1309

c21239e

Fixed #550

ENikS closed this Jan 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow external container to resolve IStartup #1321

Allow external container to resolve IStartup #1321

ENikS commented Jan 30, 2018 •

edited

Loading

dnfclas commented Jan 30, 2018 •

edited

Loading

ENikS Jan 30, 2018

davidfowl Jan 30, 2018

ENikS Jan 30, 2018

ENikS Jan 30, 2018

davidfowl Jan 30, 2018

ENikS Jan 30, 2018

davidfowl Jan 30, 2018

davidfowl commented Jan 30, 2018

ENikS commented Jan 30, 2018 •

edited

Loading

ENikS commented Jan 30, 2018

davidfowl commented Jan 30, 2018

ENikS commented Jan 30, 2018 •

edited

Loading

ENikS Jan 30, 2018 •

edited

Loading

davidfowl Jan 30, 2018

davidfowl Jan 30, 2018

ENikS commented Jan 30, 2018 •

edited

Loading

davidfowl commented Jan 30, 2018 •

edited

Loading

ENikS commented Jan 30, 2018 •

edited

Loading

ENikS commented Jan 30, 2018

ENikS commented Jan 30, 2018

davidfowl commented Jan 30, 2018

ENikS commented Jan 30, 2018

davidfowl commented Jan 30, 2018

ENikS commented Jan 30, 2018

ENikS commented Jan 30, 2018

ENikS commented Jan 30, 2018

Allow external container to resolve IStartup #1321

Allow external container to resolve IStartup #1321

Conversation

ENikS commented Jan 30, 2018 • edited Loading

dnfclas commented Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Jan 30, 2018

ENikS commented Jan 30, 2018 • edited Loading

ENikS commented Jan 30, 2018

davidfowl commented Jan 30, 2018

ENikS commented Jan 30, 2018 • edited Loading

ENikS Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ENikS commented Jan 30, 2018 • edited Loading

davidfowl commented Jan 30, 2018 • edited Loading

ENikS commented Jan 30, 2018 • edited Loading

The only difference is where IStartup is resolved from.

ENikS commented Jan 30, 2018

ENikS commented Jan 30, 2018

davidfowl commented Jan 30, 2018

ENikS commented Jan 30, 2018

davidfowl commented Jan 30, 2018

ENikS commented Jan 30, 2018

ENikS commented Jan 30, 2018

ENikS commented Jan 30, 2018

ENikS commented Jan 30, 2018 •

edited

Loading

dnfclas commented Jan 30, 2018 •

edited

Loading

ENikS commented Jan 30, 2018 •

edited

Loading

ENikS commented Jan 30, 2018 •

edited

Loading

ENikS Jan 30, 2018 •

edited

Loading

ENikS commented Jan 30, 2018 •

edited

Loading

davidfowl commented Jan 30, 2018 •

edited

Loading

ENikS commented Jan 30, 2018 •

edited

Loading