structuremap / StructureMap.Microsoft.DependencyInjection

StructureMap integration for ASP.NET Core
MIT License
72 stars 29 forks source link

Can't resolve some controller parameters if scanning in the registry #19

Open mcliment opened 7 years ago

mcliment commented 7 years ago

When scanning for things in the registry, some types can't be resolved in the controller constructor (in the case of the example IOptions but I don't think it's exclusive to this type).

I have pushed some code to reproduce the issue: https://github.com/mcliment/StructureMap.Microsoft.DependencyInjection/tree/issue-demo

To reproduce, run the sample site and go to /Bad. This should throw an exception because the type IOptions<MySettings> can't be resolved. Then go to /Good and should work, loading the same type through a wrapper. Then "/Bad" works perfectly.

The thing it that TryGetInstance can't resolve the type the first time (GetInstance works properly).

If the registry.Scan is removed from ConfigureContainer and the type MySettingsWrapper is added as services.AddTransient<IMySettingsWrapper, MySettingsWrapper>(); then /Bad works the first time.

As well, if the Scan has the clause a.Exclude(t -> true); instead of the Include(...) also works the first time.

khellang commented 7 years ago

Hmm. That's really weird. I'll take a look at it when I get home from work. What happens if you set a breakpoint in ConfigureContainer and inspect the registry? Does it have the IOptions <MySettings> registration?

khellang commented 7 years ago

I'm not sure what you mean by

As well, if the Scan has the clause a.Exclude(t -> true); instead of the Include(...) also works the first time.

How can that work? The wrapper registration won't get registered then.

@jeremydmiller If you call Scan on a Registry, will it lose all its existing registrations?

mcliment commented 7 years ago

@khellang Actually it does not lose the registrations because the MySettingsWrapper in the example wraps the IOptions<MySettings> and resolves properly.

OTOH, calling GetInstance instead of TryGetInstance resolves properly.

jeremydmiller commented 7 years ago

@mcliment Check your WhatDoIHave() output for that type that's not behaving correctly. And no, the Scan() doesn't throw away any of your other registrations.

mcliment commented 7 years ago

@jeremydmiller I don't see any major difference in any case, the results are on this gist: https://gist.github.com/mcliment/c3ae3e08c978996be1f390004fdde971

I also tried to add the MySettingsWrapper in the ConfigureContainer but works properly.

jeremydmiller commented 7 years ago

@mcliment I need a little more context. What exactly isn't working correctly?

mcliment commented 7 years ago

@jeremydmiller If you have some time to check out this repository (https://github.com/mcliment/StructureMap.Microsoft.DependencyInjection/tree/issue-demo) and run the demo site you'll see the error live. Just go to ~/Bad to see the error. The other url ~/Good always works and once it loads then the ~/Bad controller also works. I hope this helps.

khellang commented 7 years ago

Something leads me to believe that this is a StructureMap bug related to TryGetInstance and open generics. The open generic registration is there all along, but it won't "materialize" until it's resolved as a constructor argument or a call to GetInstance. After that, it works fine.

jeremydmiller commented 7 years ago

That's not a bug, that's behavior as designed. To keep the performance within reasonable limits with TryGetInstance, StructureMap uses a very small subset of its auto-resolution rules to magically resolve whether or not it could resolve that service, and the magic open-generic resolution isn't part of that subset. My thinking is that TryGetInstance should only apply to things you've explicitly registered.

I never use TryGetInstance in my own development and constantly recommend against using it, but the clowns on the ASP.Net team insist upon it for their goofy fallback mechanism.

khellang commented 7 years ago

Haha. Right. I think we're getting somewhere... Now we just need to figure out what we can do about this. I'm afraid this will get very hairy.

mcliment commented 7 years ago

@khellang Hmmm, I understand that but I can't see why not scanning things overcomes the problem if the same stuff is registered

jeremydmiller commented 7 years ago

If it's scanning, can you look at the WhatDoIHave() and see if there are multiple registrations for whatever service this is? Because in that case, StructureMap doesn't automatically select the first or last one like Windsor or Autofac does. In the case of the TryGetInstance, SM just punts because there's no explicit default.

jeremydmiller commented 7 years ago

@mcliment @khellang Not doing anything until January, but I'm gonna propose a new AspNetCoreCompatibility switch in 4.5 that can flip the transient disposal and open generic auto-resolution inside of the Container in one fell swoop -- plus whatever other wacky issues like this come up later.

Might add something to the diagnostics to assert when there's multiple registrations, or maybe something that can verify that there is a default for everything expected. Or something......

upta commented 7 years ago

@jeremydmiller Any new thoughts on this, or work-arounds? I'm trying out core 2.0 and deciding on a DI framework and ran into this issue.

We currently use Ninject (which has basically nothing going in the core arena) and rely heavily on being able to instantiate concrete classes without explicit registration.

My experience has been much the same:

1) Setup using .UseStructureMap() 1) Add concrete Dummy class to my Controller's constructor 1) Run as-is with no config: "Unable to resolve service for type 'Dummy'" 1) ConfigureServices -> services.AddTransient<Dummy>(): Works 1) ConfigureContainer -> registry.ForConcreteType<Dummy>(): Works 1) container.GetInstance<Dummy>() to "prime" it (when I had it setup the IServiceProvider ConfigureServices( IServiceCollection services ) way): Works

Thanks for any insight!

jeremydmiller commented 7 years ago

You can thank the ASP.Net http://asp.net/ team for this one. If you were using StructureMap idiomatically, that would have worked just fine. The ASP.Net http://asp.net/ Core adapter is the equivalent to StructureMap’s Container.TryGetInstance(), which will return a null because you don’t have any explicit registration. Container.GetInstance() would work as well.

If necessary, I could show you how to toss in an IFamilyPolicy that would force SM to resolve missing concrete types in the TryGetInstance() logic.

On Oct 4, 2017, at 2:41 PM, Brian notifications@github.com wrote:

@jeremydmiller https://github.com/jeremydmiller Any new thoughts on this, or work-arounds? I'm trying out core 2.0 and deciding on a DI framework and ran into this issue.

We currently use Ninject (which has basically nothing going in the core arena) and rely heavily on being able to instantiate concrete classes without explicit registration.

My experience has been much the same:

Setup using .UseStructureMap() Add concrete Dummy class to my Controller's constructor Run as-is with no config: "Unable to resolve service for type 'Dummy'" ConfigureServices -> services.AddTransient(): Works ConfigureContainer -> registry.ForConcreteType(): Works container.GetInstance() to "prime" it (when I had it setup the IServiceProvider ConfigureServices( IServiceCollection services ) way): Works Thanks for any insight!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/structuremap/StructureMap.Microsoft.DependencyInjection/issues/19#issuecomment-334267324, or mute the thread https://github.com/notifications/unsubscribe-auth/AAK8C7NJ8MnlXmQKXA8cBMoZ1eqdk1mjks5so99sgaJpZM4LTENG.

upta commented 7 years ago

I've seen a lot of similar complaints about how the ASP.Net team handled the DI stuff in Core :( I've been a bit hesitant in general about switching, but the tech marches forward and I figure I should at least keep somewhat abreast of the way things are headed.

I'd be really curious to see how you'd implement it, if it's not too much trouble. Based on your message I started trying my hand at making an IFamilyPolicy and I got it wired in, but my very naive Build method throws exceptions :)

Specifically, for:

public PluginFamily Build( Type type )
{
    if ( !type.IsAbstract )
    {
        var family = new PluginFamily( type );

        return family;
    }

    return null;
}

I get:

Unable to create a build plan for concrete type Microsoft.Extensions.Logging.Console.ConsoleLoggerProvider

new ConsoleLoggerProvider(Func<String, LogLevel, Boolean>, Boolean includeScopes)
  ┗ Func<String, LogLevel, Boolean> = **Default**
                                  Boolean includeScopes = Required primitive dependency is not explicitly defined

1.) Attempting to create a BuildPlan for Instance of Microsoft.Extensions.Logging.ILoggerProvider -- Microsoft.Extensions.Logging.Console.ConsoleLoggerProvider
2.) All registered children for IEnumerable<ILoggerProvider>
3.) Instance of IEnumerable<ILoggerProvider>
4.) new LoggerFactory(*Default of IEnumerable<ILoggerProvider>*, *Default of LoggerFilterOptions*)
5.) Microsoft.Extensions.Logging.LoggerFactory
6.) Instance of Microsoft.Extensions.Logging.ILoggerFactory (Microsoft.Extensions.Logging.LoggerFactory)
7.) new Logger`1(*Default of ILoggerFactory*)
8.) Logger<ApplicationLifetime> ('d8e278d3-9a1f-4586-8663-996eb09433b2')
9.) Instance of ILogger<ApplicationLifetime> ('d8e278d3-9a1f-4586-8663-996eb09433b2')
10.) new ApplicationLifetime(*Default of ILogger<ApplicationLifetime>*)
11.) Microsoft.AspNetCore.Hosting.Internal.ApplicationLifetime
12.) Instance of Microsoft.AspNetCore.Hosting.IApplicationLifetime (Microsoft.AspNetCore.Hosting.Internal.ApplicationLifetime)
13.) new LibuvTransportFactory(*Default of IOptions<LibuvTransportOptions>*, *Default of IApplicationLifetime*, *Default of ILoggerFactory*)
14.) Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.LibuvTransportFactory
15.) Instance of Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions.Internal.ITransportFactory (Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.LibuvTransportFactory)
16.) new KestrelServer(*Default of IOptions<KestrelServerOptions>*, *Default of ITransportFactory*, *Default of ILoggerFactory*)
17.) Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServer
18.) Instance of Microsoft.AspNetCore.Hosting.Server.IServer (Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServer)
19.) Container.GetInstance(Microsoft.AspNetCore.Hosting.Server.IServer)

Thanks for the insight!

mcliment commented 6 years ago

I've created an IFamilyPolicy that deals with the creation of the IOptions<> open generic and seems to work. It's based on the documentation but maybe someone with more expertise in StructureMap internals can check the code:

public class OptionsPolicy : IFamilyPolicy
{
    public PluginFamily Build(Type type)
    {
        if (type.Name == "IOptions`1")
        {
            var family = new PluginFamily(type);
            var instance = BuildInstance(type);

            family.SetDefault(instance);

            return family;
        }

        return null;
    }

    private Instance BuildInstance(Type type)
    {
        var instanceType = typeof(OptionsInstance<,>).MakeGenericType(type, type.GetGenericArguments()[0]);

        return Activator.CreateInstance(instanceType) as Instance;
    }

    public bool AppliesToHasFamilyChecks => true;

    public class OptionsInstance<T, TOptions> : LambdaInstance<T>
        where T : class, IOptions<TOptions>
        where TOptions : class, new()
    {
        public OptionsInstance() : 
            base($"Building {typeof(T).FullName} from options", c => c.GetInstance<OptionsManager<TOptions>>() as T)
        {
        }
    }
}

Then just use it in a Registry like this:

Policies.OnMissingFamily<OptionsPolicy>();
jeremyZX commented 6 years ago

@khellang @jeremydmiller
Why is the SM Service provider choosing to call Container.TryGetInstance vs Container.GetInstance at all? Or why isn't the behavior configurable? I don't think an analogy of GetService/TryGetInstance vs GetRequiredService/GetInstance holds up. As @jeremydmiller mentions, the behavior changes substantially beyond returning null (GetService's behavior) vs. throwing an exception (GetRequiredService's behavior.)

We've historically had to work around this issue with a custom fork of StructureMap.Microsoft.DependencyInjection. Ideally, we'd like to shelve that fork; but short of playing wack-a-mole with missing family policies to cover the gap between GetService vs GetRequiredService when upstream code is insistent on calling the former, I don't see a resolution.

Edit: Now that I've just updated the fork, I understand a little better. Having an option to switch between the ServiceProvider calling Container.TryGetInstance vs Container.GetInstance means that calls to GetRequiredService may be left with suboptimal exception messages -- compared with StructureMap's excellent internal exceptions.