huahaofeng
/
aspnetcore
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix 5594 - ExceptionHandled + Result is broken 

This change ensures that setting ExceptionContext.Result will always
execute if set. The problem with 1.1.0 is that when we had a real short
circuit the wrong set of conditions were checked. I suspect that when you
set ExceptionFilter.Result and didn't short circuit that result filters
were also running (which is a bug).

Added a few tests that verify that the result doesn't trigger result
filters.

I did some general cleanup on this code path to make the state transitions
more clear.

No exception was thrown -> BeginResult
Exception was handled -> ExceptionHandled
Exception was not handled -> gets rethrown
This commit is contained in:
Ryan Nowak 2017-02-09 19:34:04 -08:00
parent 8ee3d45ef1
commit 531c11df2a
2 changed files with 272 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs
View File

@ -577,6 +577,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal
if (exceptionContext.Exception == null || exceptionContext.ExceptionHandled)
{
// We don't need to do anthing to trigger a short circuit. If there's another
// exception filter on the stack it will check the same set of conditions
// and then just skip itself.
_logger.ExceptionFilterShortCircuited(filter);
}
@ -614,6 +617,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal
if (exceptionContext.Exception == null || exceptionContext.ExceptionHandled)
{
// We don't need to do anthing to trigger a short circuit. If there's another
// exception filter on the stack it will check the same set of conditions
// and then just skip itself.
_logger.ExceptionFilterShortCircuited(filter);
}
}
@ -626,11 +632,23 @@ namespace Microsoft.AspNetCore.Mvc.Internal
goto case State.ActionBegin;
}
case State.ExceptionShortCircuit:
case State.ExceptionHandled:
{
// We arrive in this state when an exception happened, but was handled by exception filters
// either by setting ExceptionHandled, or nulling out the Exception or setting a result
// on the ExceptionContext.
//
// We need to execute the result (if any) and then exit gracefully which unwinding Resource
// filters.
Debug.Assert(state != null);
Debug.Assert(_exceptionContext != null);
if (_exceptionContext.Result == null)
{
_exceptionContext.Result = new EmptyResult();
}
Task task;
if (scope == Scope.Resource)
{
@ -657,6 +675,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal
return task;
}
// Found this while investigating #5594. This is not right, but we think it's harmless
// so we're leaving it here for the patch release. This should go to InvokeEnd if the
// scope is not Resource because that can only happen when there are no resource filters.
goto case State.ResourceEnd;
}
@ -672,12 +693,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal
if (exceptionContext != null)
{
if (exceptionContext.Result != null && !exceptionContext.ExceptionHandled)
if (exceptionContext.Result != null ||
exceptionContext.Exception == null ||
exceptionContext.ExceptionHandled)
{
goto case State.ExceptionShortCircuit;
goto case State.ExceptionHandled;
}
Rethrow(exceptionContext);
Debug.Fail("unreachable");
}
goto case State.ResultBegin;
@ -1488,7 +1512,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal
ExceptionSyncBegin,
ExceptionSyncEnd,
ExceptionInside,
ExceptionShortCircuit,
ExceptionHandled,
ExceptionEnd,
ActionBegin,
ActionNext,

249
test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs
View File

@ -166,7 +166,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal
}
[Fact]
public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionNull()
public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionNull_WithoutResult()
{
// Arrange
var filter1 = new Mock<IExceptionFilter>(MockBehavior.Strict);
@ -193,7 +193,47 @@ namespace Microsoft.AspNetCore.Mvc.Internal
}
[Fact]
public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionHandled()
public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionNull_WithResult()
{
// Arrange
var result = new Mock<IActionResult>(MockBehavior.Strict);
result
.Setup(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()))
.Returns(Task.FromResult(true))
.Verifiable();
var filter1 = new Mock<IExceptionFilter>(MockBehavior.Strict);
var filter2 = new Mock<IExceptionFilter>(MockBehavior.Strict);
filter2
.Setup(f => f.OnException(It.IsAny<ExceptionContext>()))
.Callback<ExceptionContext>(context =>
{
context.Result = result.Object;
context.Exception = null;
})
.Verifiable();
// Result filters are never used when an exception bubbles up to exception filters.
var resultFilter = new Mock<IResultFilter>(MockBehavior.Strict);
var invoker = CreateInvoker(
new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object },
actionThrows: true);
// Act
await invoker.InvokeAsync();
// Assert
filter2.Verify(
f => f.OnException(It.IsAny<ExceptionContext>()),
Times.Once());
result.Verify(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()), Times.Once());
}
[Fact]
public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionHandled_WithoutResult()
{
// Arrange
var filter1 = new Mock<IExceptionFilter>(MockBehavior.Strict);
@ -219,7 +259,47 @@ namespace Microsoft.AspNetCore.Mvc.Internal
}
[Fact]
public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionNull()
public async Task InvokeAction_InvokesExceptionFilter_ShortCircuit_ExceptionHandled_WithResult()
{
// Arrange
var result = new Mock<IActionResult>(MockBehavior.Strict);
result
.Setup(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()))
.Returns(Task.FromResult(true))
.Verifiable();
var filter1 = new Mock<IExceptionFilter>(MockBehavior.Strict);
var filter2 = new Mock<IExceptionFilter>(MockBehavior.Strict);
filter2
.Setup(f => f.OnException(It.IsAny<ExceptionContext>()))
.Callback<ExceptionContext>(context =>
{
context.Result = result.Object;
context.ExceptionHandled = true;
})
.Verifiable();
// Result filters are never used when an exception bubbles up to exception filters.
var resultFilter = new Mock<IResultFilter>(MockBehavior.Strict);
var invoker = CreateInvoker(
new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object },
actionThrows: true);
// Act
await invoker.InvokeAsync();
// Assert
filter2.Verify(
f => f.OnException(It.IsAny<ExceptionContext>()),
Times.Once());
result.Verify(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()), Times.Once());
}
[Fact]
public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionNull_WithoutResult()
{
// Arrange
var filter1 = new Mock<IExceptionFilter>(MockBehavior.Strict);
@ -229,7 +309,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal
.Setup(f => f.OnExceptionAsync(It.IsAny<ExceptionContext>()))
.Callback<ExceptionContext>(context =>
{
filter2.ToString();
context.Exception = null;
})
.Returns<ExceptionContext>((context) => Task.FromResult(true))
@ -248,7 +327,48 @@ namespace Microsoft.AspNetCore.Mvc.Internal
}
[Fact]
public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionHandled()
public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionNull_WithResult()
{
// Arrange
var result = new Mock<IActionResult>(MockBehavior.Strict);
result
.Setup(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()))
.Returns(Task.FromResult(true))
.Verifiable();
var filter1 = new Mock<IExceptionFilter>(MockBehavior.Strict);
var filter2 = new Mock<IAsyncExceptionFilter>(MockBehavior.Strict);
filter2
.Setup(f => f.OnExceptionAsync(It.IsAny<ExceptionContext>()))
.Callback<ExceptionContext>(context =>
{
context.Exception = null;
context.Result = result.Object;
})
.Returns<ExceptionContext>((context) => Task.FromResult(true))
.Verifiable();
// Result filters are never used when an exception bubbles up to exception filters.
var resultFilter = new Mock<IResultFilter>(MockBehavior.Strict);
var invoker = CreateInvoker(
new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object },
actionThrows: true);
// Act
await invoker.InvokeAsync();
// Assert
filter2.Verify(
f => f.OnExceptionAsync(It.IsAny<ExceptionContext>()),
Times.Once());
result.Verify(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()), Times.Once());
}
[Fact]
public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionHandled_WithoutResult()
{
// Arrange
var filter1 = new Mock<IExceptionFilter>(MockBehavior.Strict);
@ -274,6 +394,88 @@ namespace Microsoft.AspNetCore.Mvc.Internal
Times.Once());
}
[Fact]
public async Task InvokeAction_InvokesAsyncExceptionFilter_ShortCircuit_ExceptionHandled_WithResult()
{
// Arrange
var result = new Mock<IActionResult>(MockBehavior.Strict);
result
.Setup(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()))
.Returns(Task.FromResult(true))
.Verifiable();
var filter1 = new Mock<IExceptionFilter>(MockBehavior.Strict);
var filter2 = new Mock<IAsyncExceptionFilter>(MockBehavior.Strict);
filter2
.Setup(f => f.OnExceptionAsync(It.IsAny<ExceptionContext>()))
.Callback<ExceptionContext>(context =>
{
context.ExceptionHandled = true;
context.Result = result.Object;
})
.Returns<ExceptionContext>((context) => Task.FromResult(true))
.Verifiable();
// Result filters are never used when an exception bubbles up to exception filters.
var resultFilter = new Mock<IResultFilter>(MockBehavior.Strict);
var invoker = CreateInvoker(
new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object },
actionThrows: true);
// Act
await invoker.InvokeAsync();
// Assert
filter2.Verify(
f => f.OnExceptionAsync(It.IsAny<ExceptionContext>()),
Times.Once());
result.Verify(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()), Times.Once());
}
[Fact]
public async Task InvokeAction_InvokesAsyncExceptionFilter_SettingResultDoesNotShortCircuit()
{
// Arrange
var result = new Mock<IActionResult>(MockBehavior.Strict);
result
.Setup(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()))
.Returns<ActionContext>((context) => Task.FromResult(true))
.Verifiable();
var filter1 = new Mock<IAsyncExceptionFilter>(MockBehavior.Strict);
filter1
.Setup(f => f.OnExceptionAsync(It.IsAny<ExceptionContext>()))
.Callback<ExceptionContext>(context =>
{
context.Result = result.Object;
})
.Returns<ExceptionContext>((context) => Task.FromResult(true))
.Verifiable();
var filter2 = new Mock<IExceptionFilter>(MockBehavior.Strict);
filter2
.Setup(f => f.OnException(It.IsAny<ExceptionContext>()))
.Callback<ExceptionContext>(c => { }) // Does nothing, we just want to verify that it was called.
.Verifiable();
var resultFilter = new Mock<IResultFilter>(MockBehavior.Strict);
var invoker = CreateInvoker(
new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object },
actionThrows: true);
// Act
await invoker.InvokeAsync();
// Assert
filter1.Verify(f => f.OnExceptionAsync(It.IsAny<ExceptionContext>()), Times.Once());
filter2.Verify(f => f.OnException(It.IsAny<ExceptionContext>()), Times.Once());
result.Verify(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()), Times.Once());
}
[Fact]
public async Task InvokeAction_InvokesExceptionFilter_UnhandledExceptionIsThrown()
{
@ -320,6 +522,43 @@ namespace Microsoft.AspNetCore.Mvc.Internal
result.Verify(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()), Times.Once());
}
[Fact]
public async Task InvokeAction_InvokesExceptionFilter_SettingResultDoesNotShortCircuit()
{
// Arrange
var result = new Mock<IActionResult>(MockBehavior.Strict);
result
.Setup(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()))
.Returns<ActionContext>((context) => Task.FromResult(true))
.Verifiable();
var filter1 = new Mock<IExceptionFilter>(MockBehavior.Strict);
filter1
.Setup(f => f.OnException(It.IsAny<ExceptionContext>()))
.Callback<ExceptionContext>(c => c.Result = result.Object)
.Verifiable();
var filter2 = new Mock<IExceptionFilter>(MockBehavior.Strict);
filter2
.Setup(f => f.OnException(It.IsAny<ExceptionContext>()))
.Callback<ExceptionContext>(c => { }) // Does nothing, we just want to verify that it was called.
.Verifiable();
var resultFilter = new Mock<IResultFilter>(MockBehavior.Strict);
var invoker = CreateInvoker(
new IFilterMetadata[] { filter1.Object, filter2.Object, resultFilter.Object },
actionThrows: true);
// Act
await invoker.InvokeAsync();
// Assert
filter1.Verify(f => f.OnException(It.IsAny<ExceptionContext>()), Times.Once());
filter2.Verify(f => f.OnException(It.IsAny<ExceptionContext>()), Times.Once());
result.Verify(r => r.ExecuteResultAsync(It.IsAny<ActionContext>()), Times.Once());
}
[Fact]
public async Task InvokeAction_InvokesAuthorizationFilter()
{