webadvanced / Structuremap.MVC5

Apache License 2.0
21 stars 9 forks source link

RenderAction #3

Closed pcoulter closed 10 years ago

pcoulter commented 10 years ago

Hi there,

I just started upgrading some projects to MVC5 and discovered your Nuget package. It's been a welcome addition to the toolbox. It has worked without inicident across most of the projects, but then I hit a project where I'm using Html.RenderAction. This causes an exception with this message:

A single instance of controller 'HomeController' cannot be used to handle multiple requests. If a custom controller factory is in use, make sure that it creates a new instance of the controller for each request.

I think that this may have something to do the use of StructureMap's nested containers, but I'm not sure. Have you tried testing this with a [ChildActionOnly] method on the same controller as the page you are calling RenderAction?

Thanks, Pete

pcoulter commented 10 years ago

I just found that it will work if I add this to my IoC.Initialize:

x.For<HomeController>().AlwaysUnique();

However, this does not work:

x.For<IController>().AlwaysUnique();

Obviously, I don't want to have to add every controller by name here.

valmont commented 10 years ago

I have been able to reproduce this and will look into it. Is there a bit of your last post missing? Both examples are the same.

pcoulter commented 10 years ago

Sorry, markdown got me on that code post above. I edited it to show accurately.

pcoulter commented 10 years ago

Confirmed that it is nested containers. I adjusted StructureMapDependencyScope as below, and it worked just fine. What I can't figure out is how to make it work with nested containers.

protected override IEnumerable<object> DoGetAllInstances(Type serviceType) {
        //return (CurrentNestedContainer ?? Container).GetAllInstances(serviceType).Cast<object>();
        return Container.GetAllInstances(serviceType).Cast<object>();
    }

    protected override object DoGetInstance(Type serviceType, string key) {
        //IContainer container = (CurrentNestedContainer ?? Container);
        IContainer container = Container;

        if (string.IsNullOrEmpty(key)) {
            return serviceType.IsAbstract || serviceType.IsInterface
                ? container.TryGetInstance(serviceType)
                : container.GetInstance(serviceType);
        }

        return container.GetInstance(serviceType, key);
    }
valmont commented 10 years ago

Without changing the way RenderAction works, there is no way. I will try to find a work around tomorrow. Thanks for the feedback!

-----Original Message----- From: "pcoulter" notifications@github.com Sent: ‎6/‎2/‎2014 9:09 PM To: "webadvanced/Structuremap.MVC5" Structuremap.MVC5@noreply.github.com Cc: "Paul Barton" paulb33@gmail.com Subject: Re: [Structuremap.MVC5] RenderAction (#3)

Confirmed that it is nested containers. I adjusted StructureMapDependencyScope as below, and it worked just fine. What I can't figure out is how to make it work with nested containers. protected override IEnumerable DoGetAllInstances(Type serviceType) { //return (CurrentNestedContainer ?? Container).GetAllInstances(serviceType).Cast(); return Container.GetAllInstances(serviceType).Cast(); }

protected override object DoGetInstance(Type serviceType, string key) {
    //IContainer container = (CurrentNestedContainer ?? Container);
    IContainer container = Container;

    if (string.IsNullOrEmpty(key)) {
        return serviceType.IsAbstract || serviceType.IsInterface
            ? container.TryGetInstance(serviceType)
            : container.GetInstance(serviceType);
    }

    return container.GetInstance(serviceType, key);
}

— Reply to this email directly or view it on GitHub.

NickLocke commented 10 years ago

I've made the change as above and RenderAction is now working, so thanks for that. However, I now have another problem which, I suspect, is related to the change. I have this code in IoC.cs:

public static class IoC {
    public static IContainer Initialize() {
        ObjectFactory.Initialize(x =>
                    {
                        x.Scan(scan =>
                                {
                                    scan.AssembliesFromApplicationBaseDirectory(); 
                                    scan.WithDefaultConventions();
                                });
                        x.For<IRs485OutgoingMessageQueue>().Singleton();
                        x.For<IRs485SerialPortAdapter>().Singleton();

Where I have asked for a singleton, it doesn't seem to be working the way I'd expect. It looks like I'm getting a new instance (or at least the constructor is executing) every time the class gets injected.

Thoughts welcome!

Thanks, Nick

pcoulter commented 10 years ago

Is there any chance that your Initialize method is being called twice? I ran into a similar issue that I'm going to post as a separate issue when using both Structuremap.MVC5 and StructureMap.WebApi2.

valmont commented 10 years ago

It looks like there is no way to use nested containers with RenderAction unless you use x.For<YourController>().AlwaysUnique(); The other option is to not use the nested containers but that is not advised. Going to close this out.

SonofNun15 commented 10 years ago

Can someone explain the purpose and / or benefit of nested containers? This issue was closed, but it still seems to be very relevant since nested containers do NOT work with RenderAction.

valmont commented 10 years ago

Using nested controllers is recommended as the correct way by the creator of Structuremap. The reasons nested Controllers do not work with RenderAction have to do with issues around the way RenderAction was implemented in ASP.NET MVC.

If you would rather not use nested containers, you can easily comment out the use of nested containers in the StructureMapDependencyScope.cs class.

jeffputz commented 10 years ago

I came across this same problem today. This seems to mostly break MVC, and I'm trying to wrap my head around why the nested containers are necessary.

justinmichaels commented 10 years ago

The best way to understand what this means is to look at Jeremy Miller's original post on this.

valmont commented 10 years ago

I believe I have a resolution to this problem. Will test today and push a new version if it's resolved.

nickalbrecht commented 9 years ago

Ok, so I'm running into a problem with the way StructureMap3, Nested Containers and ASP.NET MVC are working, and it's related to this issue.

  1. By default in StructureMap, all requests are transient, however if you are using NestedContainers, they are only transient to the scope of the Nested Container. That is to say, repeated requests within the INSIDE of a nested container for a type, will yield the same object every time. http://structuremap.github.io/the-container/nested-containers/
  2. MVC requires that Controllers and Views NOT be reused, and a new instance is created for each one.

But here's the problem, The NestedContainer created in this package, is done at the HttpApplication.BeginRequest. Meaning any Controller or View that is requested more than once during the initial request, any child requests and any partial views, will result in the controller or view being reused, and will generate an error. The error for reused Controllers is very explicit in what the problem is when this happens. Errors when a View is reused on the other hand appear to be a bit less so, and appear to be an InvalidOperationException with an exception message of "Stack empty" from what I've experienced.

The ControllerConvention added in this patch appears to override the Lifecycle scope behavior inside of NestedContainers to force them to be using the UniquePerRequestLifecycle, regardless if they are for the root action or child actions. However it does not solve the problem for views since views are compiled at runtime.

I was able to fix the behavior to be more inline with what ASP.NET MVC expects by using ChildContainers. Basically just adding a third possible scope resolution to the StructureMapDependencyScope class, and changing the first line of the two DoGetInstance methods to look something like this. IContainer container = (CurrentChildContainer ?? CurrentNestedContainer ?? Container);

In an ActionFilter inheriting from ActionFilterAttribute I call StructuremapMvc.StructureMapDependencyScope.CreateChildContainer(); as the first line of the OnActionExecuting() handler, and StructuremapMvc.StructureMapDependencyScope.DisposeChildContainer(); as the last line of the OnActionExecuted() event. I then register this ActionFilter in the FilterConfig class for the project. By doing this, it now works without any errors for the initial action, child actions, Views and PartialViews when a Controller or View is used multiple times. Doing this also meant I did not need the ControllerConvention any longer, and was able to comment it out with no change to the desired behavior I just described.

Do we know of any concerns from using it in this fashion? Basically, this setup means any dependencies resolved inside of an action have their own ChildContainer. Dependencies requested within the HttpRequest but outside of an action use the NestedContainer, and any Dependencies that are requested outside of an HttpRequest use the Default Container.

Any thoughts on this?

EDIT: Running into a few issues with containers getting reused on the View for the primary action, using my solution. Trying to sort through them, there's a 'magic' setup here somewhere, just trying to figure out what that is...

medvjed commented 8 years ago

@valmont your ControllerConvention solves the issue of the controller not being unique. but not of the view. If you use recursuve views of the same view multiple times in a foreach loop. You get the same instance for every view and this is an issue.

There is a solution, but I would rather see structuremap solving this issue. on an better level. right before changing your DependencyResolver get the old one. var defaultResolver = DependencyResolver.Current; DependencyResolver.SetResolver(StructureMapDependencyScope);

Reregister your viewengines: SimpleViewPageActivator activator = new SimpleViewPageActivator(defaultResolver); ViewEngines.Engines.Clear(); ViewEngines.Engines.Add(new RazorViewEngine(activator)); ViewEngines.Engines.Add(new WebFormViewEngine(activator)); //I use this order razor before aspx, but this could be an issue default is other way arround You now get a fresh new view for every instance

SimpleViewPageActivator is a custom class: ` public class SimpleViewPageActivator : IViewPageActivator { private IDependencyResolver resolver;

    public SimpleViewPageActivator(IDependencyResolver resolver)
    {
        if (resolver == null)
        {
            throw new ArgumentException("resolver can't be null");
        }
        this.resolver = resolver;
    }

    public object Create(ControllerContext controllerContext, Type type)
    {
        return this.resolver.GetService(type) ?? Activator.CreateInstance(type);
    }
}`
nickalbrecht commented 8 years ago

I solved this issue by using the ControllerConvention, and then creating and using a ViewConvention as well. It's literally a exact copy of the controller convention, swapping the type.CanBeCastTo<Controller>() check to typeof(IView).IsAssignableFrom(type) If you wanted, you could combine the two into an ASP.NET MVC convention, which is probably the better option, since having just the one, is only have the solution. Not having the UniquePerRequest scope on views results in problems with recursive uses.

using System.Web.Mvc;
using StructureMap.TypeRules;

namespace YourNamespace.DependencyResolution {
    public class AspNetMvcConvention : StructureMap.Graph.IRegistrationConvention {
        public void ScanTypes(StructureMap.Graph.Scanning.TypeSet types, StructureMap.Registry registry)
        {
            foreach (var type in types.AllTypes())
            {
                if (!type.IsAbstract && (typeof(IView).IsAssignableFrom(type) || type.CanBeCastTo<Controller>()))
                {
                    registry.For(type).LifecycleIs(new StructureMap.Pipeline.UniquePerRequestLifecycle());
                }
            }
        }
    }
}