4

Closed

Add "Ambiguous Lifestyles" diagnostic warning

description

The following code demonstrates an configuration error that is not picked up by the Torn Lifestyle warning.
public interface IUserService { }
public class SomeMessage { }
public interface IHandler<T> { }
public class UserService : IUserService, IHandler<SomeMessage> { }

var container = new Container();

container.RegisterSingle<IUserService, UserService>();

container.RegisterManyForOpenGeneric(typeof(IHandler<>),
    container.RegisterAll,
    typeof(UserService).Assembly);

container.Verify();

var results = Analyzer.Analyze(container).OfType<AmbiguousLifestylesDiagnosticResult>();

Assert.IsTrue(results.Any());
The results should not be empty, but it is.

Source is this discussion.
Closed Apr 10, 2015 at 2:55 PM by dot_NET_Junkie

comments

dot_NET_Junkie wrote Feb 1, 2015 at 11:28 AM

After investigation, I came to the conclusion that this is not a bug, since this diagnostic warning checks if there are multiple registrations for the same component with the same lifestyle. In this case however there are multiple registrations of different lifestyles, which means we're not talking about a torn lifestyle here. Once the component is registered as singleton in the collection as well, the warning does get triggered.

This type of registration however however, is probably still not what the user meant to do, so we might want to introduce a new warning type for that. The question of course is, what should it exactly check and how should it be called.

TheBigRic wrote Mar 11, 2015 at 8:06 PM

I think a new type of warning is the better approach after thinking about it a little more.

What should it check:
If a single implementation is part of multiple registrations with different lifestyles resulting in the creation of multiple instances with different lifestyles.

Maybe a good name would be, reading what is should check, 'MultipleLifestyleRegistrationWarning'

dot_NET_Junkie wrote Mar 17, 2015 at 6:58 PM

The root problem here is that the user registered a component multiple times, but with different lifestyles. It is very unlikely that it is correct for a single component to have multiple lifestyles. For instance, having both a singleton and scoped registration doesn't make sense, because the component is either thread-safe, or it isn't; so only one of the two lifestyles is correct. If a component is registered as scoped and as transient, this as well is unlikely to be correct, because registering it as scoped means that you care that it is only used once; having an extra transient registration will likely cause problems.

So here are some suggestions for a name for the warning:
  • Irresolute component
  • Irresolute lifestyle
  • Double-minded component
  • Double-minded lifestyle
Any feedback on this?

qujck wrote Mar 17, 2015 at 8:19 PM

indeterminable lifestyle

TheBigRic wrote Mar 17, 2015 at 8:46 PM

After reading this part:
If a component is registered as scoped and as transient, this as well is unlikely to be correct, because registering it as scoped means that you care that it is only used once; having an extra transient registration will likely cause problems.
I'm thinking, is this a warning? Or is this a permanent error in the configuration? Suppose the user makes registrations as mentioned. Normally the container will throw an exception when a PerLifetimeScope instance is requested outside a scope. In this case the container won't be able to throw this exception because the same component could also be retrieved as a transient and because the container warned for this misconfiguration it would be misleading to throw the exception later on no matter what.

Just a thought...

dot_NET_Junkie wrote Mar 17, 2015 at 9:17 PM

I'm thinking, is this a warning?
I think we already had this discussion here.

dot_NET_Junkie wrote Mar 17, 2015 at 9:20 PM

indeterminable lifestyle
I'm unsure whether that will work for anybody who has English as his second language. I have trouble pronouncing this myself :-S

TheBigRic wrote Mar 17, 2015 at 9:39 PM

What about:

MultipleRegistrationsWithConflictingLifestylesWarning

dot_NET_Junkie wrote Mar 17, 2015 at 10:13 PM

The main point here is that those "multiple registrations" are for the same component, so I rather have something like:

Component With Conflicting Lifestyles

TheBigRic wrote Mar 17, 2015 at 10:42 PM

My blessing for:

ComponentWithConflictingLifeStylesWarning

dot_NET_Junkie wrote Mar 18, 2015 at 8:10 AM

Or perhaps we could even broaden this. Both this warning and the Torn lifestyle are about the same thing: the fact that one single component is registered multiple times. Perhaps they are the same thing.

qujck wrote Mar 18, 2015 at 11:23 AM

This situation should only be descibed as the same thing if it is fixed with the same solution. Does it share the solution with the fix for Torn Lifestyles?

If not then it should qualify for its own message.

I also vote for Conflicting Lifestyles Warning

dot_NET_Junkie wrote Mar 18, 2015 at 12:25 PM

That's a good point about the fix. Let's take a look at how to fix the code attached to this work item. The original posted code can be fixed as follows to prevent this problem:
var userServiceRegistration = Lifestyle.Singleton.CreateRegistration<UserService>(contianer);

container.AddRegistration(typeof(IUserService), userServiceRegistration);

container.RegisterManyForOpenGeneric(typeof(IHandler<>),
    container.RegisterAll,
    typeof(UserService).Assembly);

container.Options.AllowOverridingRegistrations = true;

container.AddRegistration(typeof(IHandler<SomeMessage>), userServiceRegistration);

container.Options.AllowOverridingRegistrations = false;
So except for the calls to AllowOverridingRegistrations, the solution is the same as described in the Torn Lifestyle documentation.

And I think the solution will always be the same. Either the user should make the registrations transient, or he should fall back to the use Lifestyle.CreateRegistration in combination with multiple calls to AddRegistration.

TheBigRic wrote Mar 18, 2015 at 3:37 PM

I do not completly agree.

At first (see this discussion) I had the same impression that this should be the same warning. But while the fix is the same, the root cause and possibly resulting effect are slightly different.

For the current implementation and description of the Torn Lifestyle the diagnostic result is that the user has a single component with 2 separate instance producers of the same lifestyle, probably because he is unfamiliar with other components of Simple Injector than the Container object and therefore does not use:
var registration = Lifestyle.Singleton.CreateRegistration<SomeService>(container);
For the type of misconfiguration we're discussing, I think the user is actually trying to do something that he thinks is a good thing to do. E.g.:
  • A component implements 2 interfaces.
  • One interface is only needed in services with transient lifestyles
  • The other interface is only needed in services with a scoped lifestyles
  • He has knowledge of the 'captive dependency' theory
  • He therefore makes a second call to Container.Register()
So from the vantage point of the user the root cause is different and therefore deserves, IMO, another name than 'TornLifestyle', also because the lifestyles are not torn but conflicting.

dot_NET_Junkie wrote Mar 19, 2015 at 8:25 PM

In the scenario that @TheBigRic describes the solution to this problem could actually also be different. Let's recap the situation:
  • The user has one component.
  • This component implements two interfaces.
  • Each interface is registered with a different lifestyle.
The reason for doing it like this might be that the functionality of the component that is exposed to one of the interfaces is less volatile than the other part of the component, allowing the user to define a longer lifestyle for that.

If this is the case, this is a clear indication that this component should actually be split into multiple smaller components, each with their specific lifestyle. For instance:
// Old situation
public class Component : IService1, IService2 {
    private Shared someSharedState;
    void IService1.Action1() { this.HelperMethod(this.someSharedState.Use()); }
    void IService2.Action2() { this.HelperMethod(null); }

    private void HelperMethod(object x) { }
}

container.Register<IService1, Component>(scopedLifestyle);
container.Register<IService2, Component>(Lifestyle.Singleton);

// New situation
public class Component1 : IService1 {
    private Shared someSharedState;
    private readonly Helper helper;
    public Component1(Helper helper) { this.helper = helper; }
    void IService1.Action1() { this.helper.HelperMethod(this.someSharedState.Use()); }
}

public class Component2 : IService2 {
    private readonly Helper helper;
    public Component2(Helper helper) { this.helper = helper; }
    void IService2.Action2() { this.helper.HelperMethod(null); }
}

public class Helper {
    public void HelperMethod(object x) { }
}

container.Register<Helper >(Lifestyle.Singleton);
container.Register<IService1, Component1>(scopedLifestyle);
container.Register<IService2, Component2>(Lifestyle.Singleton);

TheBigRic wrote Mar 20, 2015 at 1:58 PM

With the complete example from @dot_NET_Junkie I think the conclusion must be: It is a different warning and the name should be?

ConflictingLifestyleWarning

or

ComponentWithConflictingLifestylesWarning

I prefer the last longer one as it is more descriptive of the problem.

dot_NET_Junkie wrote Mar 22, 2015 at 3:11 PM

I'm in doubt about the name "Component with conflicting lifestyles". This warning seems to indicate that it would be possible for the component to have non-conflicting lifestyles, but this is not really possible. Each combination of two lifestyles is a problem. The only way to prevent this exception from happening while having multiple registrations is when you let both registrations use the same lifestyle, but that will imediately trigger the Torn Lifestyle warning.

Does this make any sense?

So what about:

Component With Multiple Lifestyles.

Any other ideas?

qujck wrote Mar 22, 2015 at 4:16 PM

What about AmbiguousLifeStyleWarning?

dot_NET_Junkie wrote Mar 22, 2015 at 5:26 PM

Ambiguous? Yes, I like that word.

TheBigRic wrote Mar 22, 2015 at 7:20 PM

Yes, that sounds like something useful!

dot_NET_Junkie wrote Mar 25, 2015 at 9:37 PM

I'm planning on returning the following diagnostic message:
The registration for IFoo (Singleton) maps to the same implementation (FooBar) but different lifestyle as the registration for IBar does. This will cause each registration to resolve to a different instance.
Anything to improve?

TheBigRic wrote Mar 25, 2015 at 11:06 PM

I would say that this is more readible:
The registration for IFoo (Singleton) maps to the same implementation (FooBar) as the registration for IBar (Transient) does. But the registrations map to a different lifestyle. This will cause each registration to resolve to a different instance.

dot_NET_Junkie wrote Mar 26, 2015 at 8:42 PM

Fixed in changeset 57d319029404f2511cdb180fde75444d9851c9c6