Fix dependency injection issue #339

Merged
merged 1 commit into from Aug 15, 2016

Projects

None yet
@acedened
Contributor
acedened commented Jun 13, 2016 edited

I think it's a good idea to try to retrieve single instance when GetAllInstances returns empty collection.
See simpleinjector/SimpleInjector#251 (comment)

@nigel-sampson
Contributor

Thanks for the pull request.

Can you clarify the issue that this pull request solves?

Currently the container will resolve single registrations when GetAllInstances is called so there shouldn't be any result from the latter call that wasn't returned by the former.

@acedened
Contributor

Some containers (like Simple Injector) do not create new instances implicitly when GetAllInstances is called and instead return empty collection. With this fix Caliburn.Micro will also try to get an instance via GetInstance

@TheBigRic

I think the question here is, when you expect a single instance, which you do, hence this call:

var view = IoC.GetAllInstances(viewType).FirstOrDefault() as UIElement; 

why would you call GetAllInstances(). GetAllInstances is supposed to return multiple instances in every DI container I know. Some containers will return an IEnumerable<view> with a single item, others, like Simple Injector, will throw an exception if an 'IEnumerable' isn't registered explicitly and most containers will just return an empty list.

In other words I would recommend that the call is being changed to:

var view = IoC.GetInstance(viewType, null);
@nigel-sampson
Contributor

I'm inclined to agree @TheBigRic , this code predates me and i can't think of a good reason why GetAllInstances is being called over GetInstance.

I'd like to switch this over, but depending on the container a developer is using this could be a breaking change and will be have to held off to a 4.0.0 release.

@acedened
Contributor
acedened commented Jun 15, 2016 edited

I think my solution can be used until 4.0.0 is released. It relies on current behavior, but tries to retrieve an instance the right way if GetAllInstances() returns empty collection. It is unlikely to break backward compatibility

@TheBigRic

I agree with both of you. It is a breaking change an thus should be in a major version release. The fix in this pull request is very unlikely to break anybody's application I guess, although there are probably implementations depending on this exact implementation.

@nigel-sampson nigel-sampson merged commit ebf1b8d into Caliburn-Micro:master Aug 15, 2016
@nigel-sampson nigel-sampson added this to the v3.0.2 milestone Aug 15, 2016
@superware

After adding IoC.GetInstance here, Caliburn.Micro.HelloMef.MefBootstrapper fails since it's GetInstance throws an exception when it can't locate any instances. Thoughts? :)

Contributor

Just updated the 3.0.2 release notes with a bit more about this.

I'd recommend checking if the service type is assignable to UIElement and returning null before calling into MEF.

Apologies, this change had a bigger impact than expected on the containers that throw exceptions on lack of resolution.

Thanks. I missed the notes, sorry. Can you please correct the HelloMef example? simply return null?

Contributor

Done, check out MefBootstrapper.cs, what it does is check if the type is a "view" and short cuts going to MEF and just returns null. This way the framework will fall back to manual instantiation (the old behavior).

Sorry but in my opinion adding the check is nothing less than a hack, had a big headache to find out why the hell GetInstance was called when a previous version of the code worked, If Mef cannot find it with GetAllInstances, then it sure won't find it with GetInstance (unless some other inconsistencies or hack are present on the client side).

If this was needed i would have recommended making a custom func for this delegate instead of introducing this breaking change. What would happen if i need to instantiate a custom UIElement from the IoC? add another hack? or a FrameworkElement which is assignable from UIElement.

Am I missing a valid reason to have this? please help me understand how this came to be.

Contributor

I've reverted this out in 3.0.3, it shouldn't have been included in 3.0.2. It may be included in 4.0.0 with some more thought / discussion.

Apologies to all.

Only calling GetAllInstances was a hack in it's own way to taking advantage that it might not throw an exception in the way GetInstance does.

Being consistent in behavior in view resolution and view model resolution is appropriate. Anywhere where we have to say, for view models it works one way, for views it works another just adds more confusion to things.

If it is included again there will most likely be a feature switch to revert back to the existing behavior.

Thank you :) much apreciated, and sorry if I came on a little strong

@beachwalker
beachwalker commented Dec 8, 2016 edited

Hmm, it seems this might broke a lot more than just MEF. I used Autofac and this also throws an Exception because of trying to resolve an unregistered type now. Can be solved by:

protected override object GetInstance(Type service, string key)
{
     try
     {
           return string.IsNullOrWhiteSpace(key)
                      ? this.container.Resolve(service)
                      : this.container.ResolveNamed(key, service);
     }
     catch (ComponentNotRegisteredException) // this fixes the issue
     {
           return null;
     }
}
@nigel-sampson
Contributor

Yeah, it would affect any container that throws an exception when unable to resolve a dependency (not sure of the full list here).

Apologies again.

@bluerobotch

Same with Ninject. Checking for service type assignable to UIElement did the trick.

@laranjee
laranjee commented Dec 15, 2016 edited

This change also broke the Caliburn.Micro.Start that uses the SimpleContainer and all the apps that based their implementations on this one to integrate another container.

From Caliburn.Micro.Start 3.0.2, AppBootstrapper.cs

   protected override object GetInstance(Type service, string key) {
        var instance = container.GetInstance(service, key);
        if (instance != null)
            return instance;

        throw new InvalidOperationException("Could not locate any instances.");
    }

I've always assumed that if the service is not found it should throw, and from several implementations I've seen out there I'm not alone.

What should be the correct behavior, to throw or return null?

By the way, what is the point of calling IoC.GetInstance after the IoC.GetAllInstances returning an empty collection?

@ZoolWay
ZoolWay commented Dec 20, 2016

Just run unexpectly into this issue when upgrading 3.0.1 to 3.0.2...

For SimpleContainer, this does the trick but is it right?

protected override object GetInstance(Type service, string key)
{
    var instance = container.GetInstance(service, key);
    if (instance != null)
         return instance;
    if (typeof(UIElement).IsAssignableFrom(service)) return null;
    throw new InvalidOperationException("Could not locate any instances.");
}
@nigel-sampson
Contributor

@ZoolWay that's pretty much right, I'm debating reverting this one till 4.0.0 since it's become more of a breaking change than expected.

@wannabeuk

Turns out this one caught me out as well. Moving towards using some external modules with views and viewmodels. Spent the entire day pulling my hair out trying to work out why it just wouldn't find any instances of my views anymore. Turns out at some point I absent mindedly upgraded to 3.0.2 (Because the new DLL used the latest available on NUGET, so i upgraded the main app to match) Thankfully the above posted solutions solved my problems.

@chrisa23
chrisa23 commented Jan 4, 2017

This bit me as well and wasted hours of my time. I started a new project and used my normal bootstrapper code and then kept getting errors saying couldn't find views. Figured I was missing something stupid on my part and ended up putting Export attributes on my view's as a quick fix so I could keep working until I figured out what happened. Finally thought of downgrading the package and then after more digging found this pull request through the release notes. Painful... I would vote for reverting this to help others save their sanity...

@nigel-sampson
Contributor
nigel-sampson commented Jan 9, 2017 edited

I've reverted this out in 3.0.3, it shouldn't have been included in 3.0.2. It may be included in 4.0.0.

Apologies to all.

@godrose
godrose commented Jan 9, 2017
@nigel-sampson
Contributor

It's specific to containers that throw exceptions on failure to resolve the dependency such as Autofac. SimpleContainer included in the framework wasn't affected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment