New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
robertovanoli opened this Issue Dec 5, 2016 · 31 comments

Comments

Projects
None yet
8 participants
@robertovanoli

robertovanoli commented Dec 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@frankabbruzzese

frankabbruzzese Dec 5, 2016

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.

frankabbruzzese commented Dec 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@robertovanoli

robertovanoli Dec 5, 2016

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

robertovanoli commented Dec 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@frankabbruzzese

frankabbruzzese Dec 5, 2016

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();
?

frankabbruzzese commented Dec 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@robertovanoli

robertovanoli Dec 5, 2016

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

robertovanoli commented Dec 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@frankabbruzzese

frankabbruzzese Dec 5, 2016

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!

frankabbruzzese commented Dec 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@juunas11

juunas11 Dec 7, 2016

Contributor

@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?

Contributor

juunas11 commented Dec 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@frankabbruzzese

frankabbruzzese Dec 7, 2016

@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 :)

frankabbruzzese commented Dec 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@juunas11

juunas11 Dec 7, 2016

Contributor

@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.

Contributor

juunas11 commented Dec 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@juunas11

juunas11 Dec 7, 2016

Contributor

@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.

Contributor

juunas11 commented Dec 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@frankabbruzzese

frankabbruzzese Dec 7, 2016

@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.

frankabbruzzese commented Dec 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@juunas11

juunas11 Dec 7, 2016

Contributor

@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.

Contributor

juunas11 commented Dec 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@frankabbruzzese

frankabbruzzese Dec 7, 2016

@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.

frankabbruzzese commented Dec 7, 2016

@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 comment has been minimized.

Show comment
Hide comment
@rynowak

rynowak Dec 8, 2016

Member

This seems like it should be an or.

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

Member

rynowak commented Dec 8, 2016

This seems like it should be an or.

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

@juunas11

This comment has been minimized.

Show comment
Hide comment
@juunas11

juunas11 Dec 8, 2016

Contributor

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.

Contributor

juunas11 commented Dec 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@frankabbruzzese

frankabbruzzese Dec 8, 2016

@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!

frankabbruzzese commented Dec 8, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@robertovanoli

robertovanoli Dec 14, 2016

@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);
}

robertovanoli commented Dec 14, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Jan 25, 2017

Member

@dougbu @rynowak - any ideas on this?

Member

Eilon commented Jan 25, 2017

@dougbu @rynowak - any ideas on this?

@rynowak

This comment has been minimized.

Show comment
Hide comment
@rynowak

rynowak Jan 25, 2017

Member

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

Member

rynowak commented Jan 25, 2017

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 dougbu added the 2 - Working label Jan 27, 2017

@dougbu dougbu added this to the 1.1.1 milestone Jan 27, 2017

@dougbu

This comment has been minimized.

Show comment
Hide comment
@dougbu

dougbu Jan 28, 2017

Member

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.

Member

dougbu commented Jan 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dougbu

dougbu Jan 31, 2017

Member

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

Member

dougbu commented Jan 31, 2017

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

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Feb 8, 2017

Member

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.

Member

Eilon commented Feb 8, 2017

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.

@Eilon Eilon added patch-approved and removed patch-proposed labels Feb 8, 2017

@Eilon Eilon assigned rynowak and unassigned dougbu Feb 9, 2017

@rynowak

This comment has been minimized.

Show comment
Hide comment
@rynowak

rynowak Feb 10, 2017

Member

@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.

Member

rynowak commented Feb 10, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@rynowak

rynowak Feb 10, 2017

Member

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.

Member

rynowak commented Feb 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dougbu

dougbu Feb 10, 2017

Member

@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.)

Member

dougbu commented Feb 10, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@rynowak

rynowak Feb 10, 2017

Member

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

That's part of the fix.

Member

rynowak commented Feb 10, 2017

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

That's part of the fix.

@rynowak

This comment has been minimized.

Show comment
Hide comment
@rynowak

rynowak Feb 13, 2017

Member

#5777 (comment)

This has been fixed in dev and in rel/1.1.2

Member

rynowak commented Feb 13, 2017

#5777 (comment)

This has been fixed in dev and in rel/1.1.2

@rynowak rynowak closed this Feb 13, 2017

@rynowak rynowak added 3 - Done and removed 1 - Ready labels Feb 13, 2017

@grokky1

This comment has been minimized.

Show comment
Hide comment
@grokky1

grokky1 Feb 20, 2017

@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.?

grokky1 commented Feb 20, 2017

@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.?

@grokky1

This comment has been minimized.

Show comment
Hide comment
@grokky1

grokky1 Feb 20, 2017

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

grokky1 commented Feb 20, 2017

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

@Ciantic

This comment has been minimized.

Show comment
Hide comment
@Ciantic

Ciantic Jun 2, 2017

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 commented Jun 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Ciantic

Ciantic Jun 2, 2017

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" />

Ciantic commented Jun 2, 2017

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" />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment