Prevent Torn Lifestyles by default by caching Registration objects per lifestyle

[dotnetjunkie]

In case a user batch-registers types that implement multiple interfaces of different generic types using a different lifestyle than the Transient lifestyle, the registration will result in Torn Lifestyle errors thrown when Verify() is called.

Example:

interface ICommandHandler<TCommand> { }
interface IEventHandler<TEvent> { }

sealed class ProcessOrderWorkflow :
    ICommandHandler<CreateOrder>,
    IEventHandler<OrderCreated>,
    ICommandHandler<CancelOrder>,
    IEventHandler<OrderCancellationReportGenerated>
}

var assemblies = new[] { typeof(ProcessOrderWorkflow).Assembly };
container.Register(typeof(ICommandHandler<>), assemblies, Lifestyle.Singleton);
container.Register(typeof(IEventHandler<>), assemblies, Lifestyle.Singleton);

container.Verify();

The current version of Simple Injector promotes the use of the Transient lifestyle in this case, while the use of singletons can make your DI configuration much easier (as expressed here). Simple Injector should therefore make it much easier to apply an alternative lifestyle to batch-registered components.

Some possible implementations of such enhancement are described here, while a critical note on auto-merging producers can be found here.

[dotnetjunkie]

Besides solving this problem for batch-registration only, we might take a stab at solving this problem container wide. For instance, it would be fairly easy to implement a solution that let's the container reuse Registration instances for a single implementation + lifestyle (since having 2 Registration classes for the same implementation + lifestyle combination is what causes a Torn Lifestyle).

This is quite easy to implement because the creation of Registration instances almost always goes through one of the Lifestyle.CreateRegistration overloads. This holds both for calls made by the user and the library.

This would almost completely even remove the need for the container to check for Torn Lifestyles, because after applying this change, the only way for the user to create a Torn Registration is when he uses a customly created Lifestyle, because all built-in Registration implementations are internal; there's no way the user can create new ones explicitly by calling the constructor.

When we take this approach, there are still some questions though:

• This makes it impossible to deliberately cause torn lifestyles in your application. Is this a problem? What use cases are there where someone deliberately wants to cause a torn lifestyle?
• What should we do with Ambiguous Lifestyle mismatches. An Ambiguous Lifestyle mismatch means there are two Registrations for the same implementation, but for different lifestyles. Because we will be caching Registration instances by their implementation, it is possible to check during a call to Lifestyle.CreateRegistration if there already is a Registration for that implementation, but for a different Lifestyle. This would make it impossible to create Ambiguous Lifestyle mismatch. But is that a good thing? What are the use cases to needing an Ambiguous Lifestyle. This seems even more unlikely than a Torn Lifestyle.

[qujck]

I'm against blocking the ability to register an implementation with an ambiguous lifestyle. A fabricated example could be a security or a caching component that stores some internal state in a poorly designed way such that it can't act as both a session component and a global (singleton) component at the same time.

The developer may be working through the process of defining specific abstractions that will eventually lead to a clean set of implementations but still needs the ability to rely on the original underlying components for a time.

[TheBigRic]

I don't have any good examples but my gut feeling says this is too much of a breaking change. The fact that we supply diagnostics to warn the user is a good thing, while preventing the user in some rare cases to do this is IMO a bad thing.

The flexibility of the current implementation is perfect. We warn the user but give them a choice whether to follow the 'advice' or not.

So if you implement this, it should be a major version release. And: A user should be able to determine if a registration is reused or not by a switch in the container options.

I can imagine this fairly simple to implement also and gives the option to implement in a minor version by making the option default to the old implementation where registrations aren't reused for normal registrations and for batch registrations these can be reused. Maybe this should also be configurable.

At the major release the option can default, if we want, to reusing registration for all registration types.

[dotnetjunkie]

@qujck, @TheBigRic, you both make good comments about causing breaking changes. Completely blocking both Ambiguous Lifestyles disables functionality that is currently available, which might cause pain for users that currently depend on this or require this in the future. The same holds for auto-merging registrations; it is a breaking change and a configuration switch to enable/disable this should be in place if we add such feature.

But despite the possible breaking change, what is your feeling about having such 'auto-merge' for Registration instances to prevent Torn Lifestyles, opposed to trying to implement this for batch-registration only? I think that the container-wide solution is much easier to implement and maintain for us, and much more consistent and easier to understand from a user perspective.

[TheBigRic]

I think that the container-wide solution is much easier to implement and maintain for us, and much more consistent and easier to understand from a user perspective

Yes I think you're correct. But I don't see how you can implement this without making breaking changes? Does this mean you solve this for batch registrations in container wide solution where normal registrations are not merged due to the configuration switch?

[dotnetjunkie]

But I don't see how you can implement this without making breaking changes?

This can be done without a breaking change by adding a configuration switch and by default leaving the old behavior in tact. I am tempted to change this behavior in 3.2, but at least I would flip the switch in v4.

Does this mean you solve this for batch registrations in container wide solution

Even changing this for batch-registration only is a breaking change, so I rather do a container-wide change completely.

[TheBigRic]

and much more consistent and easier to understand from a user perspective

But what if a user want's to make batch registrations for both ICommandHandler en IEventHandler and on top of that need Ambigious lifestyle. With a single switch this is a scenario that we can't provide and is IMO not easy to understand for a user.

[dotnetjunkie]

But what if a user want's to make batch registrations for both ICommandHandler en IEventHandler and on top of that need Ambigious lifestyle.

Can you give an example? I'm not sure I follow.

[TheBigRic]

Let's say the user deliberately wants to make this config:

interface ISecurityProvider { }
class SecurityProvider : ISecurityProvider { }

interface ICommandHandler<TCommand> { }
interface IEventHandler<TEvent> { }

sealed class ProcessOrderWorkflow :
    ICommandHandler<CreateOrder>,
    IEventHandler<OrderCreated>,
    ICommandHandler<CancelOrder>,
    IEventHandler<OrderCancellationReportGenerated>
}

var assemblies = new[] { typeof(ProcessOrderWorkflow).Assembly };
container.Register(typeof(ICommandHandler<>), assemblies, Lifestyle.Singleton);
container.Register(typeof(IEventHandler<>), assemblies, Lifestyle.Singleton);

container.Register<ISecurityProvider, SecurityProvider>(Lifestyle.Singleton);
container.Register<SecurityProvider>(Lifestyle.Transient);

container.Verify();

With a container wide solution and a single configuration switch what is going to happen is:

1. The switch is set to the old (current) behavior => Torn lifestyles on command- and eventhandlers
2. The switch is set to the discussed new behavior => the registrations for SecurityProvider are merged without the user noticing and thus hard to find bugs at runtime.

I'm not saying this isn't a design problem, ofcourse it is. It is however a use case we support at the moment and with the container wide solution the user has to choose which problem he wants to solve.

[dotnetjunkie]

1. The switch is set to the discussed new behavior => the registrations for SecurityProvider are merged without the user noticing and thus hard to find bugs at runtime.

That's not what I intended to let happen. The idea is to reuse the Registration within the lifestyle. So there will still be a transient Registration for SecurityProvider and a one for Singleton. Of course the container would still warn about the Ambiguous Lifestyle here, but that can be suppressed.

[TheBigRic]

Sorry, I was confused between this:

Because we will be caching Registration instances by their implementation, it is possible to check during a call to Lifestyle.CreateRegistration if there already is a Registration for that implementation, but for a different Lifestyle. This would make it impossible to create Ambiguous Lifestyle mismatch

and this:

The idea is to reuse the Registration within the lifestyle

If I misunderstood the first or in the meantime you threw that point of the table, I'm in.

[dotnetjunkie]

In the fast year, we've seen many questions on stackoverflow and here on Github from developers who are struggling with these torn lifestyles. Just today a new question was posted on stackoverflow with exactly this issue.

I think that a global container change will make the life of many of our users easier, while very few would even notice if we changed this. So even though such change is a breaking change, I am considering to -not only- add this feature to v3.2 or v3.3, but also enable this behavior by default, with the option for users to switch back to the old behavior by explicitly setting a configuration flag.

I think the problem is real enough to not wait for the next major release, while the breaking change is small enough to not impact many users.

The only problem of course with such configuration flag is that it is a container wide switch. We might also need a way to allow users to explicitly force the creation of a new (uncached) Registration object. However, I'm even willing to leave such feature out of the next minor release and wait for feedback from our users first. By waiting we can get experience of the scenarios that our users run into. This prevents us from adding such feature too early. It's even possible that nobody really misses this at all.

[dotnetjunkie]

After doing some further investigation, I have to conclude that the current design doesn't allow caching Registration instances.

Consider the following registrations:

container.Register<IX, A>();
container.Register<IY, A>();

Here we see registration of the same implementation A for two different service types.

The reason we can't automatically use the same Registration instance for both registrations, is because a large part of the public and internal API depends on both the implementation type and service type for building a registration object.

For instance:

• Registration objects are often generic, containing both the given service type and implementation type as generic type arguments.
• During the building of the Expression for the Registration object, IConstructorResolutionBehavior.GetConstructor is called, which requires both a service type and implementation type as its parameters. IConstructorResolutionBehavior can be overridden by the user, and the user is allowed to select the proper constructor based on the registered service type.
• During the building of the Expression, InjectionConsumerInfo instances are created and supplied to the IDependencyInjectionBehavior methods. IDependencyInjectionBehavior allows users to override how dependencies are injected, but even if not overridden, the InjectionConsumerInfo is used for conditional registration. In other words, the registered service type is available for the user to inspect when creating a predicate for a conditional registration. In that case, the InjectionConsumerInfo describes the parent (or consumer) of the conditional dependency which is created at that time.
• During the building of the Expression, IPropertySelectionBehavior.SelectProperty is called and the service type is supplied to this SelectProperty method.
• The ExpressionBuildingEventArgs contains the RegisteredServiceType which contains either IX or IY of our example. When registering an ExpressionBuilding event in the Container, the user is allowed to inspect this property.

What the above all means is that both registrations for A could have theoretically a completely different structure, because:

• Because of their different service types, the overridden IConstructorResolutionBehavior might have selected a different constructor for both registrations.
• Different dependencies might be injected into the same constructor, because of detection of the service type while overriding the IDependencyInjectionBehavior.
• Different dependencies might be injected because the user's RegisterConditional predicate uses the PredicateContext.ServiceType property.
• Both types might have different properties injected, because the overridden IPropertySelectionBehavior uses the supplied service type.

I think that most scenarios are never used by users and probably shouldn't even be used. Fact is however, that the availability of the service type is assumed in almost every part of the pipeline and changing this is a very big breaking change in the public API.

For this reason it is impossible to make this change in v3, and we might not even get this done in v4, because for some parts of the API (i.e. ExpressionBuildingEventArgs.RegisteredServiceType and PredicateContext.ServiceType) actually make a lot of sense for the user to have the ServiceType. In those cases, the removal of ServiceType would still mostly allow the user to do the same checks, but doing the checks on the implementation type is often a bit harder for the user.

[dotnetjunkie]

Here's another use case for this:

container.RegisterSingleton(typeof(IEventListener<>), typeof(EventService<>));
container.RegisterSingleton(typeof(IEventPublisher<>), typeof(EventService<>));

Here we register the same generic EventService<T> class using two seperate generic interfaces. This will result in a torn lifestyle, but preventing the torn lifestyle is in fact not a trivial exercise.

[dotnetjunkie]

The concerns raised in this previous comment are fixed with the changes for #348 (for v4), i.e.:

• The serviceType argument is removed from the IConstructorResolutionBehavior.SelectConstructor method, making it impossible to select a constructor based on the service type.
• The serviceType argument is removed from the IDependencyInjectionBehavior.BuildExpression method, making it impossible to build an expression based on the service type.
• PredicateContext.Consumer.ServiceType has been deprecated, making it impossible to inject a different dependency based on the service type of the consumer.
• The serviceType argument is removed from the IPropertySelectionBehavior.SelectProperty method, making it impossible to select a property based on the service type.
• ExpressionBuildingEventArgs.RegisteredServiceType` is deprecated.

This makes it now feasible to implement this change.

[marcosbrigante]

Hi, I have the exact situation you described:

In case a user batch-registers types that implement multiple interfaces of different generic types using a different lifestyle than the Transient lifestyle, the registration will result in Torn Lifestyle errors thrown when Verify() is called.

and

preventing the torn lifestyle is in fact not a trivial exercise

I cannot change the lifestyle of my instances to transient. Could you advise the best workaround for this situation on v3? Should I register my generic types as closed types one by one?

Thanks

[dotnetjunkie]

Hi @marcosbrigante,

The answer to your question depends a lot on the details. So please post a new question that shows (some of) your types and your registrations. I'm also interested to understand why your batch-registered types can't be transient.

But to be honest, the answer to your question could very well be: register them as Transient -or- switch to Simple Injector v4. v4 is now in beta. Simple Injector's beta's have high quality and we can really use some beta testers :)

[marcosbrigante]

Hi @dotnetjunkie thanks for your response. As I was writing my example I noticed that my batch type registrations could be made transient by tweaking other types. We have a hybrid (Lifetime/WCF) scope strategy, so we used it as the "default" scope for most of the types, and when we need transient we made factories. So I had to change some other classes to avoid registration warnings, but it worked.

I think we can use v4. I'll let you know if we find any bugs.

Thanks a lot anyway.

[dotnetjunkie]

I think we can use v4. I'll let you know if we find any bugs.

Awesome!