Register multiple interfaces with the same implementation when Decorator in play #380

Closed
BrettJaner opened this Issue Feb 16, 2017 · 8 comments

Projects

None yet

2 participants

@BrettJaner
BrettJaner commented Feb 16, 2017 edited

The Simple Injector Documentation provides the following example to register multiple interfaces with the same implementation.

// Impl implements IInterface1, IInterface2 and IInterface3.
var registration = Lifestyle.Singleton.CreateRegistration<Impl>(container);

container.AddRegistration(typeof(IInterface1), registration);
container.AddRegistration(typeof(IInterface2), registration);
container.AddRegistration(typeof(IInterface3), registration);

var a = container.GetInstance<IInterface1>();
var b = container.GetInstance<IInterface2>();

// Since Impl is a singleton, both requests return the same instance.
Assert.AreEqual(a, b);

But how do I do this when I want to add the implementation as a decorator? I've gotten this to work as shown below, but the last registration line seems awkward.

public interface IProducer { }
public interface IFlush { }
public class Producer : IProducer { }
public class BufferedProducer : IProducer, IFlush { }

public class Program
{
	public static void Main(string[] args)
	{
		var container = new Container();

		container.Register<IProducer, Producer>(Lifestyle.Singleton);
		container.RegisterDecorator<IProducer, BufferedProducer>(Lifestyle.Scoped);
		container.Register<IFlush>(() => (BufferedProducer)container.GetInstance<IProducer>(), Lifestyle.Scoped);

		container.Verify();
	}
}

Whether I request an IProducer or IFlush, I need it to return the same instance in the same scope.

@dotnetjunkie
Collaborator

I'm not sure I follow you. Could you show a manually built object graph that shows what it is you are trying to accomplish? For instance:

var producer = new BufferedProducer(new Producer());
var flusher = new BufferedProducer(new Flusher(new DbContext("constr")));
@BrettJaner
BrettJaner commented Feb 16, 2017 edited

Something more along these lines:

public interface IProducer { }
public interface IFlush { }
public class Producer : IProducer { }

public class BufferedProducer : IProducer, IFlush
{
    public BufferedProducer(IProducer producer) { }
}

public class WcfService
{
    public WcfService(IProducer producer, IFlush flusher) { }
}

//Object graph composition
var bufferedProducer = new BufferedProducer(new Producer());
var service = new WcfService(bufferedProducer, bufferedProducer);
@dotnetjunkie
Collaborator

What you are trying to achieve can't be done using the RegisterDecorator methods. Instead you will have to take a different approach.

If you don't strictly need auto-wiring, I would advice to build that part of the object graph by hand, since that would produce the most readable code:

var bufferedProducer = new BufferedProducer(new Producer());

container.RegisterSingleton<IProducer>(bufferedProducer);
container.RegisterSingleton<IFlush>(bufferedProducer);

If however, either Producer or BufferedProducer have dependencies of their own, manually building that part of the object graph can become cumbersome. In that case, you will have to create the BufferedProducer using the Lifestyle.CreateRegistration method, just as you already tried. However, instead of using RegisterDecorator, you'll have to revert to RegisterConditional as as described here:

var bufProd = Lifestyle.Singleton.CreateRegistration<BufferedProducer>(container);

container.RegisterConditional<IProducer, Producer>(Lifestyle.Singleton,
    c => c.Consumer.ImplementationType == typeof(BufferedProducer));
container.RegisterConditional(typeof(IProducer), bufProd,
    c => !c.Handled);

container.AddRegistration(typeof(IFlush), bufProd);

If you are using Simple Injector v4 (which is in alpha now; RTM expected within a month or two), the registration can be simplified to the following:

container.RegisterConditional<IProducer, Producer>(Lifestyle.Singleton,
    c => c.Consumer.ImplementationType == typeof(BufferedProducer));
container.RegisterConditional<IProducer, BufferedProducer>(Lifestyle.Singleton,
    c => !c.Handled);

container.RegisterSingleton<IFlush, BufferedProducer>();

Simple Injector v4 will automatically prevent so called torn lifestyles which is the main reason for having to using this construct when registering multiple interfaces with the same implementation. Instead, Simple Injector v4 will detect this and will ensure that only one BuggeredProduct instance will be created, even though there are two separate registrations for it.

@BrettJaner

Can I wire by hand? In my original example, BufferedProducer has a scoped lifestyle. Sorry I left that out of my last post. Option 2 looks good, I'll look at applying that. You mentioned in Simple Injector v4, that this registration would be simplified. In v3.2.2, below is what I initially tried and thought it might work or at least give a diagnostics warning, but it did not.

var container = new Container();

container.Register<IProducer, Producer>(Lifestyle.Singleton);
container.RegisterDecorator<IProducer, BufferedProducer>(Lifestyle.Scoped);
container.Register<IFlush, BufferedProducer>(Lifestyle.Scoped);

container.Verify();

var producer = container.GetInstance<IProducer>();
var flusher = container.GetInstance<IFlush>();

Assert.IsEquals(producer, flusher);

Will the above code work in v4?

@dotnetjunkie
Collaborator

In v3.2.2, below is what I initially tried and thought it might work or at least give a diagnostics warning, but it did not.

You probably don't get this warning in v3.2, because torn lifestyle warnings are partly suppressed for decorators, because they often require decoratees to be wrapped with new instances. So although you won't get a warning, it will most likely cause you to have two BufferedProducer instances during a single scope.

I just tested your code in v4, but against all expectations this actually works :) I expected to see two BufferedProducer instances within a single scope, but there actually is one. I have to dive a bit deeper into this to understand why and whether this actually is correct behavior or not.

@BrettJaner

You probably don't get this warning in v3.2, because torn lifestyle warnings are partly suppressed for decorators, because they often require decoratees to be wrapped with new instances. So although you won't get a warning, it will most likely cause you to have two BufferedProducer instances during a single scope.

Correct, two BufferedProducer instances in a single scope is the behavior I'm seeing in v3.2.

I just tested your code in v4, but against all expectations this actually works :) I expected to see two BufferedProducer instances within a single scope, but there actually is one. I have to dive a bit deeper into this to understand why and whether this actually is correct behavior or not.

I like this feature a lot :), so I hope it is correct. It was the expected result I was hoping for in v3.2.

@dotnetjunkie
Collaborator

My spider senses were correct, this is a bug. I'll explain why this behavior is incorrect.

Simple Injector v4 will prevent Torn Lifestyles by caching `Registration instances for you. This means that the following will hold:

var p1 = Lifestyle.Singleton.CreateRegistration<Producer>(container);
var p2 = Lifestyle.Singleton.CreateRegistration<Producer>(container);
Assert.IsTrue(object.ReferenceEquals(p1, p2));

This ensures that you can do the following without getting any Torn Lifestyles:

// class Service : IReader, IWriter
container.Register<IReader, Service>(Lifestyle.Singleton);
container.Register<IWriter, Service>(Lifestyle.Singleton);

In other words, Simple Injector will ensure that a single instance of Service is created, not one per registration.

This 'detornation' however is suppressed when applying decorators. To explain its importance, let's look at the following example:

var p1 = Lifestyle.Singleton.CreateProducer<IProducer, Producer1>(container);
var p2 = Lifestyle.Singleton.CreateProducer<IProducer, Producer2>(container);
container.RegisterDecorator<IProducer, BufferedProducer>(Lifestyle.Singleton);

Assert.IsFalse(object.ReferenceEquals(p1.GetInstance(), p2.GetInstance());

Here two Singleton IProducer registrations are made; one for Producer1 and one for Producer2. Both producers are wrapped with the BufferedProducer decorator; a Singleton as well.

Each registration however should get its own decorator. If this 'detornation' is applied at decorators, it would cause p1.GetInstance() and p2.GetInstance() to return the same decorator instance, but since such decorator can only wrap one instance, it would either wrap Producer1 or Producer2. This would obviously be incorrect, because the decorator should always wrap its own instance.

There's however a bug in V4-alpha3. If we rewrite the above to the use of Scoped lifestyles, the test will suddenly fail:

var p1 = Lifestyle.Scoped.CreateProducer<IProducer, Producer1>(container);
var p2 = Lifestyle.Scoped.CreateProducer<IProducer, Producer2>(container);
container.RegisterDecorator<IProducer, BufferedProducer>(Lifestyle.Scoped);

using (ThreadScopedLifestyle.BeginScope(container) {
    Assert.IsFalse(object.ReferenceEquals(p1.GetInstance(), p2.GetInstance());
}

This fails, due to a bug.

So this means this should be fixed, and v4 RTM will not allow decorators to be shared across registrations.

@dotnetjunkie dotnetjunkie added a commit that referenced this issue Feb 18, 2017
@dotnetjunkie dotnetjunkie Fixes bug introduced in alpha release of v4
Related to #380.
55b1add
@BrettJaner

Makes sense. Thanks for the detailed explanation!

@BrettJaner BrettJaner closed this Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment