autofac / Autofac.Owin

OWIN integration for Autofac
MIT License
23 stars 15 forks source link

Is the dispose of the lifetime made too soon? #13

Closed joaomatossilva closed 7 years ago

joaomatossilva commented 7 years ago

I'm trying to access the AutofacLifetimeScope inside the context.Response.OnSendingHeaders event. This event may only run a bit late on the request execution, but shouldn't it be considered still request execution?

A bit of context: Microsoft.Owin.Security.OAuth only calls the Create providers of the codes and tokens during this event. As of now, my only solution is to create transient services and pass delegates to create the service instances on those providers so I can save the tokens and codes.

Anyway, I'm trying to change that in order to use the "normal" context per request so I don't have to open up multiple connections.

Here's an example of an owin middleware with the issue replication:

public class SimpleMiddleware : OwinMiddleware
{
    private static readonly PathString Path = new PathString("/test");

    public SimpleMiddleware(OwinMiddleware next) : base(next)
    {
    }

    public async override Task Invoke(IOwinContext context)
    {
        if (!context.Request.Path.Equals(Path))
        {
            await Next.Invoke(context);
            return;
        }

        context.Response.OnSendingHeaders(o =>
        {
            //scope here is already disposed
            var scopeOnHeaders = context.GetAutofacLifetimeScope();
            var serviceOnHeaders = scopeOnHeaders.Resolve<PluggableComponent>();
            context.Response.Headers.Add("Echo", new[] { serviceOnHeaders.Echo() });
        }, this);

        //scope here is fine
        var scope = context.GetAutofacLifetimeScope();
        var service = scope.Resolve<PluggableComponent>();

        var responseData = new { Date = DateTime.Now, Echo = service.Echo() };

        context.Response.StatusCode = (int)HttpStatusCode.OK;
        context.Response.ContentType = "application/json";
        context.Response.Write(JsonConvert.SerializeObject(responseData));
    }

}
tillig commented 7 years ago

Is this an OWIN-only app or a mix of OWIN and classic pipelines? For example, ASP.NET MVC has OWIN hooks but doesn't actually run OWIN-only - it still uses the classic ASP.NET pipeline.

joaomatossilva commented 7 years ago

The end scenario is an ASP.NET MVC application. Pretty similar of the default samples of ASP.NET Katana.

But I'm able to reproduce this without any MVC reference and only using OWIN (the inicial sample I posted on the issue).

srogovtsev commented 7 years ago

From what I remember, in ASP.NET host (i.e., IIS), OnSendingRequestHeaders occurs far after the end of OWIN pipeline (even after Request_End). Autofac.OWIN disposes of the lifetime scope at the end of the pipeline (as OWIN doesn't offer an opportunity to dispose something at the end of request). One can add a check whether the call is done in the context of ASP.NET pipeline and bind the disposal to ASP.NET request (this, actually, is possible), but I think this would heavily complicate things (and add unneeded dependencies).

joaomatossilva commented 7 years ago

Indeed.. I also filed an issue on https://github.com/aspnet/AspNetKatana/issues/85 becuase I don't think that's a good place to put that logic in... But I'm not optimistic..

srogovtsev commented 7 years ago

@tillig, actually, I think it's possible to provide a fairly easy-to-use WebHost integration package on top of my #10, which had arisen from, I think, the same problem as @joaomatossilva has - of course, if I am not mistaken and they are running in ASP.NET pipeline.

tillig commented 7 years ago

I'm open to suggestions or PRs, but if it needs a separate WebHost integration package just to support this, I'd be very open to someone else owning and distributing that package. We have quite a few integration packages now and it's pretty difficult to keep on top of everything. It's one of the reasons we don't have time to address some of the longer-running issues in Autofac core.

srogovtsev commented 7 years ago

@joaomatossilva, are you hosting your OWIN pipeline in ASP.NET/IIS? Is that your target production environment?

joaomatossilva commented 7 years ago

It is.

On Jul 10, 2017 21:51, "Serg Rogovtsev" notifications@github.com wrote:

@joaomatossilva https://github.com/joaomatossilva, are you hosting your OWIN pipeline in ASP.NET/IIS? Is that your target production environment?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/autofac/Autofac.Owin/issues/13#issuecomment-314236040, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXTFiJLydZAiHiwVJKYJFeYuKKyS6Qbks5sMo7jgaJpZM4OS4Nm .

srogovtsev commented 7 years ago

Well, then you may try to get advantage of the approach I've vouched in #10 (for similar reasons), and which is now possible with Autofac.Owin 4.1.0:

  1. Create your lifetime scope manually in Application_BeginRequest (it happens earlier that any OWIN pipeline) and save it to HttpContext. In the same event handler register your lifetime scope for disposal with HttpContext.DisposeOnPipelineCompleted
  2. In OWIN pipeline, use UseAutofacLifetimeInjector with factory (instead of container) and in that factory get your lifetime from HttpContext (which is available from IOwinContext with the following: context.Get<HttpContextBase>(typeof(HttpContextBase).FullName), or you can simply use HttpContext.Current, if you don't mind about testability there).

I will migrate my own project to this solution in a few days and get back with exact code samples.

joaomatossilva commented 7 years ago

And I see that 4.1.0 is already out there. I'll give it a try as well

joaomatossilva commented 7 years ago

One question, this will mean that I need to change the lifetime configuration to InstancePerLifetimeScope instead of InstancePerRequest, right?

joaomatossilva commented 7 years ago

This is my working sample so far..

On Global.asax

public class MvcApplication : System.Web.HttpApplication
{
    public const string ScopeKey = "CustomRequestScope";

    protected void Application_Start()
    {
        AreaRegistration.RegisterAllAreas();
        RouteConfig.RegisterRoutes(RouteTable.Routes);
    }

    protected void Application_BeginRequest()
    {
        var context = HttpContext.Current;
        ConfigureAutofac(context);
    }

    private IContainer BuildContainer()
    {
        var builder = new ContainerBuilder();

        //setup the registry of the PluggableComponent
        builder.RegisterType<PluggableComponent>().AsSelf().InstancePerLifetimeScope();

        //setup the middleware
        builder.RegisterType<SimpleMiddleware>().AsSelf();

        //Register the Controllers
        builder.RegisterControllers(typeof(MvcApplication).Assembly);

        var container = builder.Build();
        return container;
    }

    private void ConfigureAutofac(HttpContext context)
    {
        //configure the container and start a lifetimescope
        var container = BuildContainer();
        var scope = container.BeginLifetimeScope(ScopeKey);

        //Save the scope on the context and register it to be disposed on the end of the context
        context.Items[ScopeKey] = scope;
        context.DisposeOnPipelineCompleted(scope);

        DependencyResolver.SetResolver(new AutofacDependencyResolver(container));
    }
}

And the owin startup:

public partial class Startup
{
    public void Configuration(IAppBuilder app)
    {
        app.UseAutofacLifetimeScopeInjector(context =>
        {
            var httpContext = context.Get<HttpContextBase>(typeof(HttpContextBase).FullName);
            var scope = (ILifetimeScope) httpContext.Items[MvcApplication.ScopeKey];
            return scope;
        });

        app.Use<SimpleMiddleware>();
        app.UseAutofacMvc();
    }
}

Full sample here: https://github.com/joaomatossilva/AutofacCustomLifetimeScope

tillig commented 7 years ago

If you use Autofac.Core.Lifetime.MatchingScopeLifetimeTags.RequestLifetimeScopeTag as the request lifetime scope tag instead of a custom tag then you can use InstancePerRequest.

srogovtsev commented 7 years ago

@joaomatossilva, you should create your container in Application_Start, not Application_BeginRequest.

joaomatossilva commented 7 years ago

Ah.. Of course.. it was a copy past from the Owin startup that only runs once..

joaomatossilva commented 7 years ago

I digged a bit further, and found that the scope is not being disposed by the context.DisposeOnPipelineCompleted but from the Autofac.Integration.Mvc.RequestLifetimeHttpModule.OnEndRequest

And that is due to the DependencyResolver.SetResolver(new AutofacDependencyResolver(container));

Does this means that in order to make this work I'll need to create a custom IDependencyResolver?

joaomatossilva commented 7 years ago

I've update the sample now.

I did not write an entire IDependencyResolver, I've created a ILifetimeScopeProvider and passed it on the AutofacDependencyResolver.

here's mine:

public class LifeTimeScopeProvider : ILifetimeScopeProvider
{
    public LifeTimeScopeProvider(ILifetimeScope container)
    {
        ApplicationContainer = container;
    }

    public ILifetimeScope GetLifetimeScope(Action<ContainerBuilder> configurationAction)
    {
        if (HttpContext.Current == null)
        {
            throw new InvalidOperationException("Http Context Not Available");
        }
        return (ILifetimeScope)HttpContext.Current.Items[MvcApplication.ScopeKey];
    }

    public void EndLifetimeScope()
    {
    }

    public ILifetimeScope ApplicationContainer { get; }
}

then I register it like this on Application_Start:

        Container = BuildContainer();
        var lifeTimeScopeProvider = new LifeTimeScopeProvider(Container);
        DependencyResolver.SetResolver(new AutofacDependencyResolver(Container, lifeTimeScopeProvider));
tillig commented 7 years ago

It sounds like with the recent changes and the ability to provide your own lifetime scope/scope provider you've figured out how to get things working for your unique app needs. I'm going to go ahead and close this issue.