Exception Filters returns an empty body in asp.net-core 1.1

[robertovanoli]

I have a very simple controller with an ExceptionFilterAttribute that throws an exception

   [TypeFilter(typeof(MyControllerExceptionFilterAttribute))]
    public class HomeController : Controller
    {
        public IActionResult Index()
        {
            throw new Exception("Hello Exception");
            return View();
        }
   }
}

this is the ExceptionFilterAttribute

   [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, AllowMultiple = false)]
    public class MyControllerExceptionFilterAttribute : ExceptionFilterAttribute
    {
        protected readonly IHostingEnvironment _hostingEnvironment;
        protected readonly IModelMetadataProvider _modelMetadataProvider;

        public MyControllerExceptionFilterAttribute(IHostingEnvironment hostingEnvironment, IModelMetadataProvider modelMetadataProvider)
        {
            _hostingEnvironment = hostingEnvironment;
            _modelMetadataProvider = modelMetadataProvider;
        }

        public override void OnException(ExceptionContext context)
        {
            if (context.ExceptionHandled) return;
            var result = new ViewResult { ViewName = "ApplicationError" };
            context.ExceptionHandled = true; // mark exception as handled
            context.HttpContext.Response.Clear();
            context.Result = result;
        }
    }

this works fine in asp.net core 1.0, while in asp.net core 1.1 the result is not rendered anymore and the returned http response is like this:

HTTP/1.1 200 OK
Server: Kestrel
X-SourceFiles: =?UTF-8?B?QzpcUHJvamVjdHNcaW50ZXJhaC52aXN1YWxzdHVkaW8uY29tXEludmVudGFyaW9cTWFpblxTb3VyY2VcRnJvbnRvZmZpY2Vcc3JjXEZyb250b2ZmaWNl?=
X-Powered-By: ASP.NET
Date: Wed, 30 Nov 2016 14:49:53 GMT
Content-Length: 0

You can reproduce very easily in this way

1. create a new ASP.NET Core Web Application (.NET Framework). by default asp.net core 1.0 is installed. then add the controller the ExceptionFilterAttribute above and a very simple ApplicationError view: the view is rendered
2. update to asp.net core 1.1 then excecute: the view is not rendered and the returned page is blank
3. comment the row 'context.ExceptionHandled = true;' in ExceptionFilterAttribute: at this point the view is rendered as in step 1.

my conclusion is that 'context.ExceptionHandled = true' is not working as expected

I've made this small project to demostrate the behavior https://www.dropbox.com/s/dl1xa7d2nzy48o8/WebApplication3.zip?dl=0

[frankabbruzzese]

Not sure this is a bug. It appears just a change in the default state of context.ExceptionHandled. That value is not intended to inform the developer on the exception state, but on the contrary for the developer to inform the framework it handeld the exception, so the exception must stop propagation..

In a few words you dont need to check context.ExceptionHandled since if another component handles the exception yor handler will not be invoked at all.

What changed in the new version is that this value is pre-set to true, since it is more common developer handles exception. In case developer decide it is not able to recover exception he may set context.ExceptionHandled to false.

[robertovanoli]

Hi @frankabbruzzese, thank you for your explanation.
I removed the check on ExceptionHandled, but I am still a bit confused about when to set it true or false:

the official sample set ExceptionHandled = true, then returns a view as result.

In my case the behavior seems to be very different :-(

• if I set ExceptionHandled = true: a 200 httpstatus is returned (as in the sample), but the body of the response is empty
• if I set ExceptionHandled = false (or if I don't set it at all - which should be equal true): 200 httpstatus is returned and the body of the response contains the rendered view

[frankabbruzzese]

You should set ExceptionHandled = true when you handle exception and false whenyou want exception coninue being thrown up. This should always work also if ExceptionHandled default changed.

Otherwise there is a bug somewhere. Have you tried removing:
context.HttpContext.Response.Clear();
?

[robertovanoli]

Here my ExceptionFilterAttribute now

public override void OnException(ExceptionContext context) { var result = new ViewResult { ViewName = "ApplicationError" }; context.ExceptionHandled = true; // mark exception as handled context.Result = result; }

setting context.ExceptionHandled = true, a blank page is shown (response body empty)
The view is a single row
<h1>Errore Applicativo</h1>

the project is an empty ASP.NET Core Web Application (.NET Framework)
No other code is present on my side

[frankabbruzzese]

I downloaded your project and played with it. Basically context.ExceptionHandled = true,always cause a blank page while context.ExceptionHandled = false, correcly displays the custom error page, that makes absolutely no sense.

You are right there is a bug!

[juunas11]

@frankabbruzzese I thought the point of ExceptionHandled was to signal the framework that "I got it, it's ok". So it returning 200 OK should be expected no?

[frankabbruzzese]

@juunas11 ,
ExceptionHandled = true, says "ok I handled the exception, no need to propagate it further", so in the case of @robertovanoli 2oo ok is expected since he have not explicitely set an error header. What is not expected is the blank page!!! Why the the View passsed in ViewResult is not rendered???

It is strange also what happens when setting ExceptionHandled = false. In this case if no ViewResult is set you get an error header which is correct. However, if you set a ViewResult you still get a 200 ok, but this time View is rendered!

Thus it appears that `ExceptionHandled should alway be set to false, and the response depends if you set or not the ViewResult.

Maybe ExceptionHandled = true must be used only when ExceptionFilter write directly in the response stream?? This would explain everything. However, this behavior is different from what is written in the documentation...So some correction is needed anywau :)

[juunas11]

@frankabbruzzese Ohh, I misread it. The blank page is certainly weird since a view result was set. I'll have a look at this if I have a moment.

[juunas11]

@robertovanoli I got the error page to show just by commenting out context.HttpContext.Response.Clear();. The other thing is that as @frankabbruzzese mentioned the default for ExceptionHandled is now true, so you must set it to false if you don't want to handle the exception.

    public override void OnException(ExceptionContext context)
    {
        if (_hostingEnvironment.IsDevelopment())
        {
            context.ExceptionHandled = false;
            return;
        }
        var result = new ViewResult { ViewName = "ApplicationError" };
        context.Result = result;
    }

This works for me.

[frankabbruzzese]

@juunas11 ,
No actually. context.ExceptionHandled is not true as defaul, but it is false. I supposed it was true to explain the behavior,..but before downloading the project and debugging it.

Your code works simply bacause you dont set context.ExceptionHandled = true. Try setting explicitaly it to true, and you will see error page is not shown anymore.

[juunas11]

@frankabbruzzese You are right! I took a little dive into ControllerActionInvoker's state machine.

This line in particular seems to be the reason the behaviour is the way it is: https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs#L675

if (exceptionContext.Result != null && !exceptionContext.ExceptionHandled)
{
    goto case State.ExceptionShortCircuit;
}

So the short-circuiting is only done if a result is set, and ExceptionHandled is false. The short-circuit state is the one that executes the result. Seems to be this commit that added this part. @rynowak was this intended behaviour? After more testing I can confirm the OP's code worked as expected on 1.0.0, but not on 1.1.0.

[frankabbruzzese]

@juunas11,
Either bug or change in intended meaning of ExceptionHandled = true from "exception handled" to "exception handled completely" by writing response in the response stream.

[rynowak]

This seems like it should be an or.

So, to confirm this is working as expected when you set both .Result and .ExceptionHandled

[juunas11]

When you set a result, and leave ExceptionHandled false, the result gets executed. But if you set a result, and set ExceptionHandled to true, the result does not get executed.

[frankabbruzzese]

@rynowak ,
Not convinced must be an or. In any case it appears that !exceptionContext.ExceptionHandled should be substituted with exceptionContext.ExceptionHandled. IE the main error appears to be the negation.

About the or. It depends if one allows the exception be handled by writing directly in the Response stream instead of returning a result. I dont know if this scenario is properly handled anyway, since
State.ExceptionShortCircuit perform an InvokeAsymc of the Result in any case.

Another strange stuff is:

Since with ExceptionHandled = true at moment the if fails, so according to the state machine the exception should be rethrown...instead a 200 ok is returned with a blank page!

[robertovanoli]

@frankabbruzzese
Francesco, for now I bypassed rendering a HTML response by myself . As you can see setting ExceptionHandled = true works fine. Obviously it's only a temporary workaround...
regards

public override void OnException(ExceptionContext context)
{
   context.ExceptionHandled = true; // mark exception as handled
   context.HttpContext.Response.Clear();
   context.HttpContext.Response.StatusCode = 400;
   context.HttpContext.Response.ContentType = new MediaTypeHeaderValue("text/html").ToString();
   var home = string.Format("{0}://{1}", context.HttpContext.Request.Scheme,context.HttpContext.Request.Host);
   var htmlString ="SI E' VERIFICATO UN ERRORE INATTESO";
   context.HttpContext.Response.WriteAsync(htmlString, Encoding.UTF8);
}

[Eilon]

@dougbu @rynowak - any ideas on this?

[rynowak]

We need to double check the behavior of short circuiting exception filters wrt compatibility with MVC5. This should be a patch candidate if we have a bug here

[dougbu]

Have confirmed IActionFilters in MVC 5.2.3 and ASP.NET Core 1.1.0 behave the same. However, IExceptionFitlers behave differently w.r.t. setting Result but leaving ExceptionHandled==false. Should remove this special case around setting Result.

1.1.0 behaviour is also a regression compared to ASP.NET Core 1.0.x.

Long story about a consistent behaviour for ASP.NET Core:

• Users can short-circuit most IFilterMetadata implementations by setting Result. But, only on the way in e.g. OnActionExecuting() can short-circuit but OnActionExecuted() cannot.
• To short-circuit IExceptionFilter implementations (which are only called on the way out), users must set ExceptionHandled==true.
• Setting ExceptionHandled==true in all IFilterMetadata implementations also ensures an Exception thrown in an action is not rethrown. An overridden Result is used in that case.
• In a small, intentional deviation from MVC 5.2.3, setting Exception==null is handled identically to setting ExceptionHandled==true.

Fix should be a one-line change in ControllerActionInvoker line 675.

[dougbu]

Resetting labels other than Bug and patch-proposed. @Eilon / @danroth27

[Eilon]

This patch bug is approved. Please use the normal code review process w/ a PR and make sure the fix is in the correct branch, then close the bug and mark it as done.

[rynowak]

@dougbu - I could use your help to fact check the following table:

I'm defining short circuit to mean that subsequent exception filters do not run, and no exception is thrown. If it doesn't say short circuit then it means that all exception filters will run.

I'm defining result executed to mean that the action result is executed and the original exception is not rethrown. If it doesn't say result executed then nothing is written to the output by MVC.

---

	MVC 5.2.3	MVC Core 1.0.0	MVC Core 1.1.0	MVC Core 1.1.2 (proposed by me)
set Result	exception thrown	result executed @dougbu confirmed	result executed @dougbu confirmed	result executed
set ExceptionHandled	@rynowak short circuit @dougbu just disables re-throwing	short circuit	short circruit	short circruit
set Exception = null	exception thrown	short circuit	short circuit	short circruit
set Result && Exception Handled	@rynowak short circuit && result executed @dougbu exception handlers never short circuit	short circuit && result executed	short circuit	short circuit && result executed

---

Explanation

This bug is users broken by the change going from MVC Core 1.0.0 -> MVC Core 1.1.0. Namely that we don't run the result when you set exception handled. This seems like an obvious bug.

However, no one is complaining about the behavior changes going from MVC 5.2.3 -> MVC Core 1.X.X. Namely that setting .Result will short circuit and execute the result. Changing this to have the MVC 5.2.3 behavior wouldn't make much sense, why are you setting a result if you don't want it to run? It would also break a lot of people.

[rynowak]

I'm going to move forward with a PR to make the behavior like the table above since that resolves the breaking change we accidentally made in 1.1.0. If there are any other changes we want to make here let's discuss that separately.

[dougbu]

@rynowak I edited your table above with one tweak. But, I agree we should focus on the one thing customers have complained about. That's the unintended Core 1.0.0 to 1.1.0 change you're reverting in the 1.1.2 column.

BTW this could simply be a typo in one line of code. @frankabbruzzese mentioned the oddball at line 675 in ControllerActionInvoker above. None of the other ExceptionContext checks look like that one. The rest check exceptionContext.Exception == null and not exceptionContext.Result != null. (Well, there's another line with the same condition but that's just for logging.)

[rynowak]

BTW this could simply be a typo in one line of code.

That's part of the fix.

[rynowak]

#5777 (comment)

This has been fixed in dev and in rel/1.1.2

[grokky1]

@rynowak Glad this bug was squashed.

However, since 1.1.2 hasn't been released yet, what is the recommended workaround right now?
i.e. Within an exception filter, to perform a redirect / set the Result / etc.?

[robertovanoli]

@grokky1 my workaround was: http://stackoverflow.com/questions/40892538/exception-filters-returning-an-empty-body-in-asp-net-core-1-1

[grokky1]

@robertovanoli Yeah, pity we can't set .Result though, because it is more flexible, e.g. to redirect with arguments

[Ciantic]

I came here from the docs they say at the moment:

In ASP.NET 1.1, the response is not sent if you set ExceptionHandled to true and write a response. In that scenario, ASP.NET Core 1.0 does send the response, and ASP.NET Core 1.1.2 will return to the 1.0 behavior.

Should this work again?

        public void OnException(ExceptionContext context)
        {
            if (context.Exception is ApiError)
            {
                context.Result = (context.Exception as ApiError).GetResult();
                context.Exception = null;
                context.ExceptionHandled = true;
            }

I keep getting empty result back

[Ciantic]

I think I was confused by csproj, I changed:

<RuntimeFrameworkVersion>1.1.1</RuntimeFrameworkVersion>
to
<RuntimeFrameworkVersion>1.1.2</RuntimeFrameworkVersion>

Instead, of course one must update this line:

<PackageReference Include="Microsoft.AspNetCore.Mvc.Core" Version="1.1.1" />
to
<PackageReference Include="Microsoft.AspNetCore.Mvc.Core" Version="1.1.2" />