Remove AutofacWebApiDependencyScope from HttpRequestMessage after we leave the handler

[srogovtsev]

In the same vein as autofac/Autofac.Owin#26, we remove created AutofacWebApiDependencyScope from HttpRequestMessage's properties when we leave the handler to allow for earlier garbage collection. Should work better in pair with #16, which also removes AutofacWebApiDependencyScope from the finalizer queue.

[tillig]

LGTM. Thanks!

[russellfoster]

Hi, I ran into an issue caused by this change:

            try
            {
                return base.SendAsync(request, cancellationToken);
            }
            finally
            {
                request.Properties.Remove(HttpPropertyKeys.DependencyScope);
            }

If the base.SendAsync returns a task (from awaiting), then the dependency scope is removed. When the await is resumed, then the dependency scope is no longer present and you get a "An error occurred when trying to create a controller of type 'XXX'".

Need to add an await there to prevent this:

            try
            {
                await base.SendAsync(request, cancellationToken);
            }
            finally
            {
                request.Properties.Remove(HttpPropertyKeys.DependencyScope);
            }

[srogovtsev]

@russellfoster , you're right, this is a rookie mistake on my part.

[tillig]

Let me see if I can get a quick fix out for this.

[tillig]

It turns out you can't have an async/await method marked by [SecurityCritical] or [SecuritySafeCritical] so the option is to either remove that (which... do people use that anymore?) or do something more complex. Looking at the current implementations of DelegatingHandler and HttpMessageHandler it appears they're still marking those assemblies with [assembly: AllowPartiallyTrustedCallers] and [assembly: SecurityRules(SecurityRuleSet.Level2, SkipVerificationInFullTrust = true)] so to comply with that, we need to keep the attributes.

I'll see what I can do using something like .ContinueWith to accomplish the same thing in non-async/await terms.

[tillig]

The fix will be released in 6.2.1 which is going out right now.

[adzhiljano]

Hello,
First of all, thank you very much for Autofac! It has been excellent and a joy to work with!

Sorry for bringing this up again, but the changes in this PR are a breaking change for anyone who is using Autofac in a combination with a global exception handler/logger in WebApi. If someone has the following setup:

config.Services.Add(typeof(IExceptionLogger), new MyExceptionLogger());
config.Services.Replace(typeof(IExceptionHandler), new MyExceptionHandler())

and inside those Services, one is using the Request DependencyScope to resolve dependencies:

public class MyExceptionLogger : ExceptionLogger
{
    public override void Log(ExceptionLoggerContext context)
    {
        var dependencyScope = context.Request.GetDependencyScope(); // this will return an EmptyResolver since the AutofacWebApiDependencyScope has been removed already
        var otherLogger = dependencyScope.GetService(typeof(IOtherLogger)) as IOtherLogger; // this will be null
        var lifetimeScope = dependencyScope.GetRequestLifetimeScope(); // this will be null
        var logger = lifetimeScope.Resolve<ILogger>(); // throws
    }
}

This happens, because the Exception Logger/Handler is called at a later point from the top-most DelegatingHandler (HttpServer), after all the other handlers ran (including the DependencyScopeHandler):
https://github.com/aspnet/AspNetWebStack/blob/1231b77d79956152831b75ad7f094f844251b97f/src/System.Web.Http/HttpServer.cs#L244-L245

I'm not sure what the right approach would be here. Some ideas:

• Don't remove the AutofacWebApiDependencyScope at all (revert to how it used to be)
• Find a later point to remove it (after the HttpServer has fully completed the Send), not sure if this is possible though

Thanks!

[tillig]

We can't revert to the old way because we need to be able to garbage collect, so it'd possibly need to be removed later. We'd be happy to take a PR to fix it, but I won't lie, this isn't something I'll personally be actively working on.

[adzhiljano]

Given the discussion in #15, it doesn't look like there is a simple solution. But since we're in an Owin scenario, there is a nice workaround:

public class MyExceptionLogger : ExceptionLogger
{
    public override void Log(ExceptionLoggerContext context)
    {
        var lifetimeScope = context.Request.GetOwinContext().GetAutofacLifetimeScope(); // get the lifetimeScope from the OwinContext rather than getting it from the DependencyScope
        var logger = lifetimeScope.Resolve<ILogger>(); 
    }
}