Fix 5594 - ExceptionHandled + Result is broken #5777
Conversation
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
| @@ -657,7 +675,7 @@ private Task Next(ref State next, ref Scope scope, ref object state, ref bool is | |||
| return task; | |||
| } | |||
|
|
|||
| goto case State.ResourceEnd; | |||
| goto case State.InvokeEnd; | |||
There was a problem hiding this comment.
This was a bug as well 😲
There was a problem hiding this comment.
Should we be fixing this in a patch?
There was a problem hiding this comment.
Notice no tests changed to confirm a bug was fixed here. Suggest backing this out as it seems unrelated.
There was a problem hiding this comment.
I can't think of a way that this will manifest in observable bad behavior, but it's part of the code we're fixing and it's just so obviously wrong.
"if the task is completed goto state1; else await the task and then goto state2"
If you still feel strongly then I'm at least going to leave a comment here that this is wrong but we decided not to fix it, and then I'll fix it in dev.
| @@ -657,7 +675,7 @@ private Task Next(ref State next, ref Scope scope, ref object state, ref bool is | |||
| return task; | |||
| } | |||
|
|
|||
| goto case State.ResourceEnd; | |||
| goto case State.InvokeEnd; | |||
There was a problem hiding this comment.
Should we be fixing this in a patch?
| if (exceptionContext.Result != null && !exceptionContext.ExceptionHandled) | ||
| if (exceptionContext.Result != null || | ||
| exceptionContext.Exception == null || | ||
| exceptionContext.ExceptionHandled) |
There was a problem hiding this comment.
It's possible the table we came up with still has a bug. In MVC 5.x, setting the Result alone changes nothing because the Exception will still be rethrown. I believe that was also the case in ASP.NET Core MVC 1.0.0. Should I double-check with 1.0.0?
If I'm wrong, this is fine for the patch release. We should revisit the exceptionContext.Result != null && exceptionContext.Exception != null && !exceptionContext.ExceptionHandled behaviour for 2.0. It doesn't make sense (and confused our customers) to have the Result used when they haven't indicated the Exception should be suppressed.
There was a problem hiding this comment.
Follow up: I was incorrect and the ASP.NET Core MVC 1.0.0 behaviour when an IExceptionFilter sets Result but neither nulls out Exception nor sets ExceptionHandled is not the same as MVC 5.x: ASP.NET Core 1.0.0 and 1.1.0 both execute the Result in this case and neither short-circuits.
There was a problem hiding this comment.
So, no change needed here. I'd like to discuss suppressing the Exception in the "Result-only" case for 2.0.
There was a problem hiding this comment.
Discussed in person and I will add a test that verifies that we don't short circuit when you set result. IE any subsequent exception filters still have a chance to run.
|
@dougbu updated |
| @@ -657,6 +675,9 @@ private Task Next(ref State next, ref Scope scope, ref object state, ref bool is | |||
| 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. | |||
There was a problem hiding this comment.
And, of course, my requested change is to double the number of comments like this.
| Debug.Assert(state != null); | ||
| Debug.Assert(_exceptionContext != null); | ||
|
|
||
| if (_exceptionContext.Result == null) | ||
| { | ||
| _exceptionContext.Result = new EmptyResult(); |
There was a problem hiding this comment.
We can remove the Debug.Assert on Line 655 since this ensures Result is never null.
There was a problem hiding this comment.
Nah, if I don't get to fix my obvious bug I'm not adding more code cleanup.
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