HttpRequestMessage is disposed after the filters are resolved
Closed this issue · 7 comments
Describe the Bug
HttpRequestMessage is disposed after the filters are resolved in some situation
Steps to Reproduce
My code: AutofacRequestMessageDisposed.zip
public class ReproTest
{
[Fact]
public async Task RegisterHttpRequestMessageNotDisposeAfterScopeAsync()
{
var config = new HttpConfiguration();
var builder = new ContainerBuilder();
config.RegisterHttpRequestMessage(builder);
var container = builder.Build();
Assert.True(container.IsRegistered<HttpRequestMessage>());
var httpRequestMessage = new HttpRequestMessage
{
Content = new StringContent("")
};
HttpRequestMessageProvider.Current = httpRequestMessage;
var result = HttpRequestMessageProvider.Current;
using(var scope = container.BeginLifetimeScope(MatchingScopeLifetimeTags.RequestLifetimeScopeTag))
{
Assert.Same(result, scope.Resolve<HttpRequestMessage>());
}
_ = await result.Content.ReadAsStringAsync();
}
}Expected Behavior
Autofac.WebAPI not dispose it
Exception with Stack Trace
<Error>
<Message>An error has occurred.</Message>
<ExceptionMessage>
Cannot access a disposed object. Object name: 'System.Web.Http.WebHost.HttpControllerHandler+LazyStreamContent'.
</ExceptionMessage>
<ExceptionType>System.ObjectDisposedException</ExceptionType>
<StackTrace>
at System.Net.Http.HttpContent.CheckDisposed() at System.Net.Http.HttpContent.ReadAsStringAsync() at AutofacRequestMessageDisposed.Filters.SomeDependency.<DoSomething>d__2.MoveNext() in C:\Users\RAD17\source\repos\AutofacRequestMessageDisposed\AutofacRequestMessageDisposed\Filters\CheckRequestFilter.cs:line 33 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.TaskAwaiter.GetResult() at AutofacRequestMessageDisposed.Filters.CheckRequestFilter.<ExecuteAuthorizationFilterAsync>d__2.MoveNext() in C:\Users\RAD17\source\repos\AutofacRequestMessageDisposed\AutofacRequestMessageDisposed\Filters\CheckRequestFilter.cs:line 18 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Web.Http.Dispatcher.HttpControllerDispatcher.<SendAsync>d__15.MoveNext()
</StackTrace>
</Error>
Dependency Versions
Autofac: 6.0.0/5.2.0
Autofac.WebAPI: 6.0.0/5.0.0
Thanks for providing the sample solution. It's always appreciated when the effort has been taken to provide a reproduction.
If you remove the RegisterCallback handler method your Application_Start() you should find everything works fine.
It is attempting to resolve the HttpRequestMessage in the Preparing event handler which is causing a circular dependency.
Circular component dependency detected: AutofacRequestMessageDisposed.Controllers.ValuesController -> λ:System.Net.Http.HttpRequestMessage -> λ:System.Net.Http.HttpRequestMessage.
You should note that this callback is invoked for every service that is resolved. That includes the HttpRequestMessage itself and anything the framework resolves during any part of the request lifecycle. Performing the resolve operation in that callback will lead to all sorts of exceptions. I assume someone already discovered this based on the fact there are empty catch blocks around the code. That code may have been added to debug something but having it there will only cause further issues.
@alexmg Thank you, even if I add a if to not do that tracing for HttpRequestMessage the problem continue :(
builder.RegisterCallback(callback =>
{
callback.Registered += (sender, args) =>
{
if (args.ComponentRegistration.Activator.LimitType == typeof(HttpRequestMessage))
{
return;
}
args.ComponentRegistration.Preparing += (sender2, arg2) =>
{
try
{
var message = arg2.Context.Resolve<HttpRequestMessage>();
if (!message.Headers.Contains("Preparing"))
{
message.Headers.Add("Preparing", DateTime.Now.ToString());
}
}
catch (Exception)
{
}
};
args.ComponentRegistration.Activated += (sender2, arg2) =>
{
try
{
var message = arg2.Context.Resolve<HttpRequestMessage>();
if (!message.Headers.Contains("Activated"))
{
message.Headers.Add("Activated", DateTime.Now.ToString());
}
}
catch (Exception)
{
}
};
};
});The controller will be resolved before the HttpRequestMessage dependency so that will not be enough to stop the circular reference. Try commenting out the entire RegisterCallback and it should be fine. That is not the right place to interact with the message instance.
@alexmg Ok, just one more question, if the problem is circular reference why when I mark HttpRequestMessage with ExternallyOwned start to work?
public class WebApiApplication : System.Web.HttpApplication
{
protected void Application_Start()
{
var builder = new ContainerBuilder();
// Get your HttpConfiguration.
var config = GlobalConfiguration.Configuration;
builder.RegisterCallback( ... );
....
// builder.RegisterHttpRequestMessage(config);
// Autofac code
builder.Register(c => HttpRequestMessageProvider.Current)
.InstancePerRequest()
.ExternallyOwned(); // <--- this was added
...
}
}
// Copy from Autofac source code
/// <summary>
/// A delegating handler that updates the current dependency scope
/// with the current <see cref="HttpRequestMessage"/>.
/// </summary>
public class CurrentRequestHandler : DelegatingHandler
{
/// <summary>
/// Sends an HTTP request to the inner handler to send to the server as an asynchronous operation.
/// </summary>
/// <param name="request">The HTTP request message to send to the server.</param>
/// <param name="cancellationToken">A cancellation token to cancel operation.</param>
/// <returns>
/// Returns <see cref="System.Threading.Tasks.Task{T}" />. The task object representing the asynchronous operation.
/// </returns>
protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
UpdateScopeWithHttpRequestMessage(request);
return base.SendAsync(request, cancellationToken);
}
/// <summary>
/// Updates the current dependency scope with current HTTP request message.
/// </summary>
/// <param name="request">The HTTP request message.</param>
internal static void UpdateScopeWithHttpRequestMessage(HttpRequestMessage request)
{
HttpRequestMessageProvider.Current = request;
}
}
public static class HttpRequestMessageProvider
{
private static readonly string Key = Guid.NewGuid().ToString("N", CultureInfo.InvariantCulture).Substring(0, 12);
internal static HttpRequestMessage Current
{
get
{
var wrapper = (HttpRequestMessageWrapper)CallContext.LogicalGetData(Key);
return wrapper?.Message;
}
set
{
var wrapper = value == null ? null : new HttpRequestMessageWrapper(value);
CallContext.LogicalSetData(Key, wrapper);
}
}
[Serializable]
private sealed class HttpRequestMessageWrapper : MarshalByRefObject
{
[NonSerialized]
[SuppressMessage("SA1401", "SA1401", Justification = "Field is only used during testing.")]
internal readonly HttpRequestMessage Message;
internal HttpRequestMessageWrapper(HttpRequestMessage message)
{
Message = message;
}
}
}There were two exception being thrown. The first exception was caused by the circular dependency and the second was a result of resolving the HttpRequestMessage from the root lifetime scope and not the request lifetime scope (it searches up the chain of lifetime scopes). Both exceptions stem from resolving the HttpRequestMessage in the Preparing handler.
A key reason for the request lifetime scoping is to ensure that services are not used outside of the request lifetime. This case is an interesting one though. Because we are not actually responsible for creating the HttpRequestMessage in the first place, it could be said that we should also not take responsibility for its disposal, because doing so could result in it being disposed before the Web API framework intended.
I see you have a PR open to make the HttpRequestMessage service ExternallyOwned. I think that leaving Web API to dispose the HttpRequestMessage is the right thing to do for this service. You should still be careful about what you resolve in those handlers because an exception will still be thrown due to the circular dependency, but that could be considered a separate issue to who is responsible for disposing the HttpRequestMessage.
It's been a long time since I have worked with Web API so it would be good to get a second opinion, but I think the change in your PR makes sense given we did not create the instance in the first place. /cc @tillig
I think marking HttpRequestMessage as ExternallyOwned is reasonable. Neither we nor the developer created it; we're merely including it for convenience and probably shouldn't be taking ownership.
