Pass runtime value to constructor using Simple Injector abd WebFormsMVP

Question

I'm trying to combine SimpleInjector with WebFormsMvp.

To facilitate DI WebFormsMvp provides the IPresenterFactory interface.
It contains the Create method which provides the presenter type to resolve and the view instance.
I need to inject the view instance into the constructor of the presenter.
The presenter also has other dependencies that need creating by the container.

This is what I got so far, but it is not ideal.
What's the correct solution for the problem?

Presenter constructor:

public FooPresenter(IFooView view, IClientFactory clientFactory) : base(view)

Factory:

public class SimpleInjectorPresenterFactory : IPresenterFactory
{
    private readonly Container _container;
    private IView _currentView;

    public SimpleInjectorPresenterFactory()
    {
        _container = new Container();

        Func<Type, bool> isIView = 
            type => typeof(IView).IsAssignableFrom(type);

        _container.ResolveUnregisteredType += (s, e) => {
            if (isIView(e.UnregisteredServiceType))
                e.Register(() => _currentView);
        };
    }

    public IPresenter Create(Type presenterType, Type viewType, IView viewInstance)
    {
        lock (_currentView)
        {
            _currentView = viewInstance;
            return _container.GetInstance(presenterType) as IPresenter;
        }
    }
}

Answer 1

WebFormsMvp forces you to take the view in the constructor of the presenter, but this triggers a circular reference. If you look in the factory implementations for the different container's, you'll see that for each container they do a different trick to work around this quirk in the design. For instance, with unity, they create a child container and register that view in the child container, and resolve the presenter using that child container. Quite bizarre and performance heavy.

Instead of taking the view in the constructor of the presenter, the designers of WebFormsMvp should have made the View a writable property on the IPresenter interface. This would make it embarrassingly easy to set the view on the presenter. Something like this:

public IPresenter Create(Type presenterType, IView view)
{
    var presenter = (IPresenter)_container.GetInstance(presenterType);
    presenter.View = view;
    return presenter;
}   

Unfortunately they didn't do this, and it is impossible to extend the design to allow this (without doing really nasty things using reflection).

Simple Injector does not support supplying constructor arguments to the GetInstance() method. For good reason, since this would typically lead to the Service Locator anti-pattern, and you can always go around this by changing the design. In your case, you didn't make that quirky design, so you can't change it.

What you did with the ResolveUnregisteredType is pretty clever. I wouldn't have thought about this myself. And since I'm the lead dev behind Simple Injector, I'm in the position to say that what you did was really clever :-)

Just two points of feedback about your SimpleInjectorPresenterFactory.

First of all, you should supply the Container as constructor argument, since it will be very likely that you need to add other registrations to the container, and you don't want to register the Container inside the SimpleInjectorPresenterFactory.

Secondly, you can improve the code by using a System.Threading.ThreadLocal<IView>. This allows you to get rid of the global lock. The lock prevents any presenter from being made concurrently, and this could slow down your website.

So here's a refactored version:

public class SimpleInjectorPresenterFactory : IPresenterFactory {
    private readonly Container _container;
    private ThreadLocal<IView> _currentView = new ThreadLocal<IView>();

    public SimpleInjectorPresenterFactory(Container container) {
        _container = container;

        _container.ResolveUnregisteredType += (s, e) => {
            if (typeof(IView).IsAssignableFrom(e.UnregisteredServiceType)) {
                e.Register(() => _currentView.Value);
            }
        };
    }

    public IPresenter Create(Type presenterType, Type viewType, 
        IView viewInstance)
    {
        _currentView.Value = viewInstance;

        try {
            return _container.GetInstance(presenterType) as IPresenter;
        } finally {
            // Clear the thread-local value to ensure
            // views can be disposed after the request ends.
            _currentView.Value = null;
        }
    }
}

If you look at the implementation of the UnityPresenterFactory, you see a lot of caching going on in there. I have no idea why they do that, but from a performance perspective, you don't need such thing for Simple Injector at all. Perhaps I'm missing something, but I don't see why there should be a cache.

But even worse, there is a concurrency bug in the UnityPresenterFactory. Take a look at this method:

private Type FindPresenterDescribedViewTypeCached(Type presenter, 
    IView view) 
{
    IntPtr handle = presenter.TypeHandle.Value;
    if (!this.cache.ContainsKey(handle)) 
    {
        lock (this.syncLock)
        {
            if (!this.cache.ContainsKey(handle))
            {
                Type viewType = CreateType(presenter, view);
                this.cache[handle] = viewType;
                return viewType;
            }
        }
    }
    return this.cache[handle];
}

At first sight this code looks okay, since a double checked lock is implemented. Unfortunately, the cache (dictionary) is read from outside the lock, while it is updated inside the lock. This is not thread-safe. Instead, the developer should have either wrapped the whole thing in a lock, use a ConcurrentDictionary (.net 4 only) or consider the cache immutable, which means that you create a copy of the original dictionary, add the new value, and replace the reference to the old dictionary with the new. However, in this case I would probably just have locked the whole thing.

This was a little bit off topic, but just wanted to tell :-)